767929 - Use Maybe instead of Optional for the holder object in new DOM binding code

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Boris Zbarsky [:bzbarsky]]

I can't remember what this is for...  Is this still relevant?

Comment 2

[Peter Van der Beken [:peterv]]

I think this was in response to bug 756258 comment 10:

> The optional/nullable handling here is really complicated, but I don't see
> an obvious way to make it simpler.  I'll think about it a bit.  For one
> thing, the "use Maybe for the holder" code pattern is a good one to hoist up
> into the main place we handle this stuff; I'd used Optional for the holder
> when the decl is Optional, but Maybe is a better fit.  Maybe a followup bug
> on this?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, ok, so in instantiateJSToNativeConversionTemplate.  OK.

Comment 4

[Glenna]

I'd like to work on this.  I should change the Optional to Maybe only for the mutableHolderType and mutableDeclType?

Comment 5

[Boris Zbarsky [:bzbarsky]]

You don't want to change the decl type.  Just the holder type.

Comment 6

[Glenna]

Attachment: prop

Comment 7

[Glenna]

Attachment: proposed patch

Does this have any tests?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 709993 [details] [diff] [review]
proposed patch

There are some tests to make sure things compile and whatnot; make -C $objdir/dom/bindings/test will run those.

But this patch doesn't even compile for the normal bindings codegen...  The way to get the value out of a Maybe is ref(), not Value(), so you need to change more code than that.

Comment 9

[Max Li [:maxli]]

Attachment: Patch

Comment 10

[Boris Zbarsky [:bzbarsky]]

Max, what does the resulting generated code look like?

Comment 11

[Max Li [:maxli]]

(In reply to Boris Zbarsky (:bz) from comment #10)
> Max, what does the resulting generated code look like?

Something like this (I took this from one of the test files):

>  Maybe<nsRefPtr<mozilla::dom::IndirectlyImplementedInterface> > arg0_holder;
>  Optional<mozilla::dom::IndirectlyImplementedInterface* > arg0;
>  if (0 < argc) {
>    arg0.Construct();
>    arg0_holder.construct();
>    if (argv[0].isObject()) {
>      JS::Rooted<JS::Value> tmpVal(cx, argv[0]);
>      mozilla::dom::IndirectlyImplementedInterface* tmp;
>      if (NS_FAILED(xpc_qsUnwrapArg<mozilla::dom::IndirectlyImplementedInterface>(cx, argv[0], &tmp, static_cast<mozilla::dom::IndirectlyImplementedInterface**>(getter_AddRefs(arg0_holder.ref())), tmpVal.address()))) {
>        ThrowErrorMessage(cx, MSG_DOES_NOT_IMPLEMENT_INTERFACE, "IndirectlyImplementedInterface");return false;
>      }
>      MOZ_ASSERT(tmp);
>      if (tmpVal != argv[0] && !arg0_holder.ref()) {
>        // We have to have a strong ref, because we got this off
>        // some random object that might get GCed
>        arg0_holder.ref() = tmp;
>      }
>      arg0.Value() = tmp;

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 748735 [details] [diff] [review]
Patch

Great, thanks.  r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

Do you have push access, or would you like me to check this in for you?

Comment 14

[Max Li [:maxli]]

(In reply to Boris Zbarsky (:bz) from comment #13)
> Do you have push access, or would you like me to check this in for you?

I don't have access, so it'd be great if you could do it for me.

Comment 15

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/662d99ac18fb

Max, thank you for fixing this!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/662d99ac18fb