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

RESOLVED FIXED in mozilla24

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: peterv, Assigned: maxli)

Tracking

(Blocks: 1 bug)

Trunk
mozilla24
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=bz][lang=python])

Attachments

(1 attachment, 2 obsolete attachments)

1.99 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
I can't remember what this is for...  Is this still relevant?
(Reporter)

Comment 2

6 years ago
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?
Ah, ok, so in instantiateJSToNativeConversionTemplate.  OK.
Whiteboard: [mentor=bz] → [mentor=bz][lang=python]

Comment 4

5 years ago
I'd like to work on this.  I should change the Optional to Maybe only for the mutableHolderType and mutableDeclType?
You don't want to change the decl type.  Just the holder type.

Comment 6

5 years ago
Created attachment 709992 [details]
prop

Comment 7

5 years ago
Created attachment 709993 [details] [diff] [review]
proposed patch

Does this have any tests?
Attachment #709992 - Attachment is obsolete: true
Attachment #709993 - Flags: review?(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.
Attachment #709993 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 9

5 years ago
Created attachment 748735 [details] [diff] [review]
Patch
Assignee: nobody → maxli
Attachment #709993 - Attachment is obsolete: true
Attachment #748735 - Flags: review?(bzbarsky)
Max, what does the resulting generated code look like?
Flags: needinfo?(maxli)
(Assignee)

Comment 11

5 years ago
(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;
Flags: needinfo?(maxli)
Comment on attachment 748735 [details] [diff] [review]
Patch

Great, thanks.  r=me
Attachment #748735 - Flags: review?(bzbarsky) → review+
Do you have push access, or would you like me to check this in for you?
(Assignee)

Comment 14

5 years ago
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/662d99ac18fb

Max, thank you for fixing this!
Flags: in-testsuite-
Target Milestone: --- → mozilla24
https://hg.mozilla.org/mozilla-central/rev/662d99ac18fb
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.