Closed
Bug 864727
Opened 11 years ago
Closed 11 years ago
Root WebIDL binding Wrap() methods
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(8 files)
1.34 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
3.24 KB,
patch
|
Ms2ger
:
review+
terrence
:
review+
|
Details | Diff | Splinter Review |
6.05 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
14.54 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
212.53 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
218.06 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
4.15 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
1.08 KB,
text/plain
|
Details |
No description provided.
Assignee | ||
Updated•11 years ago
|
Blocks: ExactRootingBrowser
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #740968 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → bzbarsky
Assignee | ||
Comment 2•11 years ago
|
||
Terrence, just want you for a sanity-check on the fromMarkedLocation bit
Attachment #740969 -
Flags: review?(terrence)
Attachment #740969 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #740971 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #740972 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #740973 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #740975 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #740978 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 8•11 years ago
|
||
Assignee | ||
Comment 9•11 years ago
|
||
Try push at https://tbpl.mozilla.org/?tree=Try&rev=864ba4134be8 is showing just the expected orange from the patch for bug 860841.
Updated•11 years ago
|
Attachment #740969 -
Flags: review?(terrence) → review+
Comment 10•11 years ago
|
||
Comment on attachment 740968 [details] [diff] [review] part 1. Root the global before calling WrapObject in XPCConvert. sort of wish GetGlobalJSObject just returned a handle. Review of attachment 740968 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCConvert.cpp @@ +838,5 @@ > if (!ccx.IsValid()) > return false; > > if (!flat) { > + JS::Rooted<JSObject*> global(lccx.GetJSContext(), Just pass ccx here and below. We've already unlazified it, and it'll auto-convert to JSContext*. @@ +840,5 @@ > > if (!flat) { > + JS::Rooted<JSObject*> global(lccx.GetJSContext(), > + xpcscope->GetGlobalJSObject()); > + flat = cache->WrapObject(lccx.GetJSContext(), global); Unrelated to your changes but when the heck do we have something where cache->IsDOMBinding() is true, but we don't want have anything in the cache? And why do we want to return the global in that case? Is this just belt-and-suspenders? If so, seems like we should just assert instead...
Attachment #740968 -
Flags: review?(bobbyholley+bmo) → review+
Comment 11•11 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #10) > Unrelated to your changes but when the heck do we have something where > cache->IsDOMBinding() is true, but we don't want have anything in the cache? > And why do we want to return the global in that case? Is this just > belt-and-suspenders? If so, seems like we should just assert instead... Per IRC, I was just confused here. WrapObject isn't wrapping the global. It's wrapping the binding object in the scope of the global. Nevermind me.
Assignee | ||
Comment 12•11 years ago
|
||
> Just pass ccx here and below.
Looks like we have a "cx" around as well; I'll just pass that. ;)
Comment 13•11 years ago
|
||
Comment on attachment 740969 [details] [diff] [review] part 2. Pass a handle for the scope object to union conversions. Review of attachment 740969 [details] [diff] [review]: ----------------------------------------------------------------- I just noticed that the review combobox says "The patch has passed review by a module owner or peer." on hover... I guess my review will do, though.
Attachment #740969 -
Flags: review?(Ms2ger) → review+
Comment 14•11 years ago
|
||
Comment on attachment 740971 [details] [diff] [review] part 3. Pass a handle for the parent object to WebIDL dictionary ToObject. Review of attachment 740971 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +3643,5 @@ > sequenceWrapLevel -= 1 > innerTemplate = CGIndenter(CGGeneric(innerTemplate), 6).define() > return ((""" > uint32_t length = %s.Length(); > +JS::Rooted<JSObject*> returnArray(cx,JS_NewArrayObject(cx, length, NULL)); Space after comma. Feel free to make it nullptr, too.
Attachment #740971 -
Flags: review?(Ms2ger) → review+
Assignee | ||
Comment 15•11 years ago
|
||
> Space after comma. Feel free to make it nullptr, too.
Done.
Comment 16•11 years ago
|
||
Comment on attachment 740972 [details] [diff] [review] part 4. Pass a handle for the scope object to all the various Wrap*Object stuff in BindingUtils. The JS::Rooted in CGWrapWithCacheMethod is just there until we start passing a handle to Wrap(). Review of attachment 740972 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContextUtils.h @@ +25,2 @@ > JSAutoCompartment ac(cx, wrapper); > + if (!dom::WrapNewBindingObject(cx, wrapper, const_cast<WebGLObjectType*>(object), v.address())) { The vp argument to WrapNewBindingObject should probably turn into a MutableHandle<Value> at some point. Followup? ::: dom/bindings/Codegen.py @@ +2064,5 @@ > else: > assertISupportsInheritance = "" > return """%s > %s > + JS::Rooted<JSObject*> aScope(aCx, aScopeArg); // Temporary! Oh, good, I would have complained about the name otherwise :) @@ +8566,5 @@ > > bodyWithThis = string.Template( > setupCall+ > + "JSObject* thisObjJS =\n" > + " WrapCallThisObject(s.GetContext(), JS::Handle<JSObject*>::fromMarkedLocation(&mCallback), thisObj);\n" Is this the same mCallback as a few patches back? If so, please add a getter instead of doing this inline everywhere.
Attachment #740972 -
Flags: review?(Ms2ger) → review+
Comment 17•11 years ago
|
||
Comment on attachment 740973 [details] [diff] [review] part 5. Make all the WrapObject methods take a handle for the scope object. Review of attachment 740973 [details] [diff] [review]: ----------------------------------------------------------------- Only skimmed, didn't see anything.
Attachment #740973 -
Flags: review?(Ms2ger) → review+
Comment 18•11 years ago
|
||
Comment on attachment 740975 [details] [diff] [review] part 6. Make all the WrapNode methods take a handle for the scope object. Review of attachment 740975 [details] [diff] [review]: ----------------------------------------------------------------- Same
Attachment #740975 -
Flags: review?(Ms2ger) → review+
Updated•11 years ago
|
Attachment #740978 -
Flags: review?(Ms2ger) → review+
Assignee | ||
Comment 19•11 years ago
|
||
> The vp argument to WrapNewBindingObject should probably turn into a MutableHandle<Value> > at some point. Followup? It probably should, but it's a somewhat low priority for now, especially because it would be blocked on CallArgs-ification of the specialized natives. > Is this the same mCallback as a few patches back? Yes, it is. I guess I can change CallbackPreserveColor to return a Handle. Let me do that.
Assignee | ||
Comment 20•11 years ago
|
||
Actually, those consumers should be using Callback(), not CallbackPreserveColor() anyway. I made both return handles in any case.
Assignee | ||
Comment 21•11 years ago
|
||
> Actually, those consumers should be using Callback()
And actually actually, they should not, since CallSetup handles the unmark-gray.
Assignee | ||
Comment 22•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a67479723670 https://hg.mozilla.org/integration/mozilla-inbound/rev/27c89b2200ad https://hg.mozilla.org/integration/mozilla-inbound/rev/d13e71853e55 https://hg.mozilla.org/integration/mozilla-inbound/rev/3011f288bfe7 https://hg.mozilla.org/integration/mozilla-inbound/rev/1b1381894c44 https://hg.mozilla.org/integration/mozilla-inbound/rev/e98c80465618 https://hg.mozilla.org/integration/mozilla-inbound/rev/74bd92b1c033
Flags: in-testsuite-
Target Milestone: --- → mozilla23
Assignee | ||
Comment 23•11 years ago
|
||
And http://hg.mozilla.org/integration/mozilla-inbound/rev/ac39efa583f7 to fix build bustage.
Comment 24•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a67479723670 https://hg.mozilla.org/mozilla-central/rev/27c89b2200ad https://hg.mozilla.org/mozilla-central/rev/d13e71853e55 https://hg.mozilla.org/mozilla-central/rev/3011f288bfe7 https://hg.mozilla.org/mozilla-central/rev/1b1381894c44 https://hg.mozilla.org/mozilla-central/rev/e98c80465618 https://hg.mozilla.org/mozilla-central/rev/74bd92b1c033 https://hg.mozilla.org/mozilla-central/rev/ac39efa583f7
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
•