Closed Bug 864727 Opened 11 years ago Closed 11 years ago

Root WebIDL binding Wrap() methods

Categories

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

x86
macOS
defect
Not set
normal

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: nobody → bzbarsky
Terrence, just want you for a sanity-check on the fromMarkedLocation bit
Attachment #740969 - Flags: review?(terrence)
Attachment #740969 - Flags: review?(Ms2ger)
Try push at https://tbpl.mozilla.org/?tree=Try&rev=864ba4134be8 is showing just the expected orange from the patch for bug 860841.
Attachment #740969 - Flags: review?(terrence) → review+
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+
(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.
> Just pass ccx here and below.

Looks like we have a "cx" around as well; I'll just pass that.  ;)
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 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+
> Space after comma. Feel free to make it nullptr, too.

Done.
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 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 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+
Attachment #740978 - Flags: review?(Ms2ger) → review+
> 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.
Actually, those consumers should be using Callback(), not CallbackPreserveColor() anyway.  I made both return handles in any case.
> Actually, those consumers should be using Callback()

And actually actually, they should not, since CallSetup handles the unmark-gray.
Depends on: 866203
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.