Closed
Bug 767929
Opened 12 years ago
Closed 11 years ago
Use Maybe instead of Optional for the holder object in new DOM binding code
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: peterv, Assigned: maxli)
References
(Blocks 1 open bug)
Details
(Whiteboard: [mentor=bz][lang=python])
Attachments
(1 file, 2 obsolete files)
1.99 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•12 years ago
|
Blocks: ParisBindings
Comment 1•11 years ago
|
||
I can't remember what this is for... Is this still relevant?
Reporter | ||
Comment 2•11 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?
Comment 3•11 years ago
|
||
Ah, ok, so in instantiateJSToNativeConversionTemplate. OK.
Updated•11 years ago
|
Whiteboard: [mentor=bz]
Updated•11 years ago
|
Whiteboard: [mentor=bz] → [mentor=bz][lang=python]
I'd like to work on this. I should change the Optional to Maybe only for the mutableHolderType and mutableDeclType?
Comment 5•11 years ago
|
||
You don't want to change the decl type. Just the holder type.
Does this have any tests?
Attachment #709992 -
Attachment is obsolete: true
Attachment #709993 -
Flags: review?(bzbarsky)
Comment 8•11 years ago
|
||
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•11 years ago
|
||
Assignee: nobody → maxli
Attachment #709993 -
Attachment is obsolete: true
Attachment #748735 -
Flags: review?(bzbarsky)
Comment 10•11 years ago
|
||
Max, what does the resulting generated code look like?
Flags: needinfo?(maxli)
Assignee | ||
Comment 11•11 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 12•11 years ago
|
||
Comment on attachment 748735 [details] [diff] [review] Patch Great, thanks. r=me
Attachment #748735 -
Flags: review?(bzbarsky) → review+
Comment 13•11 years ago
|
||
Do you have push access, or would you like me to check this in for you?
Assignee | ||
Comment 14•11 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.
Comment 15•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/662d99ac18fb Max, thank you for fixing this!
Flags: in-testsuite-
Target Milestone: --- → mozilla24
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/662d99ac18fb
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•