Last Comment Bug 761462 - Implement JS_CallNonGenericMethodOnProxy
: Implement JS_CallNonGenericMethodOnProxy
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on: 761439
Blocks: 758912
  Show dependency treegraph
 
Reported: 2012-06-04 18:09 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-06-15 14:20 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch with JSAPI test (7.24 KB, patch)
2012-06-04 18:09 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-04 18:09:32 PDT
Created attachment 630022 [details] [diff] [review]
Patch with JSAPI test

It's currently not possible for a JSAPI client to implement a method which works both on objects of a given class, and on proxies/wrappers for objects of that class (most often in the cross-compartment case, but also plausibly in the same-compartment-but-wrapped case).  Internally we have HandleNonGenericMethodClassMismatch for this; it's just a matter of exposing it.

The name is kinda fugly, but it's the best Luke and I could come up with.  The functionality offered is also hairy, but hopefully the jsapi.h comment should sufficiently elucidate things.  The method's primarily relevant to embeddings that have multiple interacting globals with similar functionality in each, where functionality must treat an object from one global the same as it'd treat an object from another global.  That's probably mostly the browser embedding, but I could almost imagine Wes or someone hitting this stuff as I know he uses multiple globals at least somewhat.

I need this to implement bug 758912.  I also strongly suspect that the DOM bindings folks will need this as well at some point so that things like this will work (I'd kind of like to know how they're doing it now -- I'm guessing with bad old hacked-up friend APIs at best):

  var desc = Object.getOwnPropertyDescriptor(otherWindow.Node.prototype, "nodeType");
  assertEq(desc.get.call(document.documentElement), Node.ELEMENT_TYPE);
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-04 18:15:36 PDT
Without bug 761439 fixed, this patch works in that its test passes, but it won't properly work in cases where |this| is a same-compartment but wrapped object -- curiously, a case which I hit in my patch for bug 758912.  XBL: killing babies as always.
Comment 2 Luke Wagner [:luke] 2012-06-04 20:44:31 PDT
Why couldn't we expose NonGenericMethodGuard instead?  That way JSAPI clients wouldn't have to write the RequireMyClassThis you showed in the jsapi comment.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-05 08:55:29 PDT
NonGenericMethodGuard isn't sufficient to meet my need for bug 758912, because in the words of HandleNonGenericMethodClassMismatch, "more than one class is acceptable".  Here's the condition that |this| must satisfy for that code:

http://hg.mozilla.org/mozilla-central/annotate/c76497029f0d/content/xbl/src/nsXBLBinding.cpp#l119

DOM bindings will have the same requirements, actually.  Consider the comment 0 example: the nodeType getter has to work on element nodes, document nodes, comment nodes, and so on, each having a different class.
Comment 4 Luke Wagner [:luke] 2012-06-05 09:27:28 PDT
Comment on attachment 630022 [details] [diff] [review]
Patch with JSAPI test

Very well, then.
Comment 5 Ed Morley [:emorley] 2012-06-06 08:20:47 PDT
https://hg.mozilla.org/mozilla-central/rev/5a289ad94bfe
Comment 6 David Mandelin [:dmandelin] 2012-06-15 14:20:11 PDT
Comment on attachment 630022 [details] [diff] [review]
Patch with JSAPI test

Review of attachment 630022 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like you got the review you needed already.

Note You need to log in before you can comment on or make changes to this bug.