Closed
Bug 993026
Opened 10 years ago
Closed 10 years ago
Merge JSResolveOp and JSNewResolveOp; remove JSCLASS_NEW_RESOLVE
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: jorendorff, Assigned: evilpie)
References
Details
Attachments
(4 files, 2 obsolete files)
47.53 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
31.34 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
21.31 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
33.40 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Current signatures: typedef bool (* JSResolveOp)(JSContext *cx, JS::HandleObject obj, JS::HandleId id); typedef bool (* JSNewResolveOp)(JSContext *cx, JS::HandleObject obj, JS::HandleId id, unsigned flags, JS::MutableHandleObject objp); After bug 547140, the 'flags' parameter will be gone from the latter, and I think the only proper use of 'objp' is to set it to 'obj' (or leave it null). Thus we can merge the two and have only: typedef bool (* JSResolveOp)(JSContext *cx, JS::HandleObject obj, JS::HandleId id, bool *resolvedp);
Updated•10 years ago
|
Assignee: general → nobody
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → evilpies
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8515243 -
Attachment is obsolete: true
Attachment #8515451 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8515452 -
Flags: review?(jorendorff)
Assignee | ||
Updated•10 years ago
|
Attachment #8515244 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8515453 -
Flags: review?(bobbyholley)
Comment 6•10 years ago
|
||
Comment on attachment 8515453 [details] [diff] [review] v1 - Change resolve hook in browser/XPC Review of attachment 8515453 [details] [diff] [review]: ----------------------------------------------------------------- \o/
Attachment #8515453 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8515451 [details] [diff] [review] v1 - Change XPIDLScriptable NewResolve to simpler Resolve Review of attachment 8515451 [details] [diff] [review]: ----------------------------------------------------------------- Bobby thank you for that super quick review, can you take a look at this one as well? Bz seems to have tons of review requests at the moment.
Attachment #8515451 -
Flags: review?(bzbarsky) → review?(bobbyholley)
![]() |
||
Comment 8•10 years ago
|
||
Comment on attachment 8515453 [details] [diff] [review] v1 - Change resolve hook in browser/XPC Could you pleas fix the naming, probably in a separate patch? So s/NEWRESOLVE_HOOK_NAME/RESOLVE_HOOK_NAME/, s/newResolveHook/resolveHook/, s/CGNewResolveHook/CGResolveHook/, s/NeedNewResolve/NeedResolve/ (including in the documentation), s/NPObjWrapper_NewResolve/NPObjWrapper_Resolve/.
Comment 9•10 years ago
|
||
Comment on attachment 8515451 [details] [diff] [review] v1 - Change XPIDLScriptable NewResolve to simpler Resolve Review of attachment 8515451 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/idl/nsIXPCScriptable.idl @@ +114,5 @@ > boolean newEnumerate(in nsIXPConnectWrappedNative wrapper, > in JSContextPtr cx, in JSObjectPtr obj, > in uint32_t enum_op, in JSValPtr statep, out jsid idp); > > + boolean resolve(in nsIXPConnectWrappedNative wrapper, Please rev the IID of this interface.
Attachment #8515451 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8517499 -
Flags: review?(bzbarsky)
![]() |
||
Comment 11•10 years ago
|
||
Comment on attachment 8517499 [details] [diff] [review] v1 - Rename NewResolve to Resolve in browser >+ Resolve hook for objects with custom hooks. "for objects that have the NeedResolve extended attribute". r=me, but please update https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#NeedNewResolve when you land
Attachment #8517499 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8515452 [details] [diff] [review] v1 - Change resolve hook in JS Review of attachment 8515452 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/shell/js.cpp @@ +4537,5 @@ > JS_FN_HELP("shapeOf", ShapeOf, 1, 0, > "shapeOf(obj)", > " Get the shape of obj (an implementation detail)."), > > +// XXX this actually seems unused? Well this code is gone now :)
Reporter | ||
Comment 13•10 years ago
|
||
Comment on attachment 8515452 [details] [diff] [review] v1 - Change resolve hook in JS Review of attachment 8515452 [details] [diff] [review]: ----------------------------------------------------------------- Very nice simplification. Love it. ::: js/public/Class.h @@ +118,5 @@ > // Lazy properties are those reflected from some peer native property space > // (e.g., the DOM attributes for a given node reflected as obj) on demand. > // > // JS looks for a property in an object, and if not found, tries to resolve > +// the given id. *resolvedp should be set to true iff the propertie was spelling: "property" ::: js/src/jsapi-tests/testFreshGlobalEvalRedefinition.cpp @@ +18,2 @@ > { > + return JS_ResolveStandardClass(cx, obj, id, resolvedp); Hmm. Can this function be eliminated altogether, then? ::: js/src/shell/js.cpp @@ +2520,1 @@ > return false; The if statement could be changed to just: return JS_ResolveStandardClass(cx, obj, id, resolvedp); ::: js/src/vm/ArgumentsObject.cpp @@ -363,2 @@ > { > - objp.set(nullptr); OK, either this has to be retained, as `*resolvedp = false;`, because there are a bunch of `return true;` exits in the body of the function --- or the contract of resolve hooks has to change to promise that *resolvedp is set to false on entry. I prefer the former. @@ -479,2 @@ > { > - objp.set(nullptr); Same here. ::: js/src/vm/NativeObject-inl.h @@ +511,2 @@ > > + // ToDo: See if we can assert that the property exists? Bah! Either give it a shot or don't bother with the comment! Inversely, if (!resolved), you could assert above that the property does not exist. That's more work. Still, it's worth a comment here if for some reason either assertion doesn't hold. @@ +516,5 @@ > return true; > } > > Shape *shape; > + if (!obj->empty() && (shape = obj->lookup(cx, id))) Is that obj->empty() necessary? Surely not: if (Shape *shape = obj->lookup(cx, id)) propp.set(shape); else ...
Attachment #8515452 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #13) > ::: js/src/vm/ArgumentsObject.cpp > @@ -363,2 @@ > > { > > - objp.set(nullptr); > > OK, either this has to be retained, as `*resolvedp = false;`, because there > are a bunch of `return true;` exits in the body of the function --- or the > contract of resolve hooks has to change to promise that *resolvedp is set to > false on entry. I prefer the former. So the bindings already made use of obj2 is nullptr on entry and I just changed those to resolvedp as well. So I would prefer sticking to what I have got now.
Assignee | ||
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b22c5e82467b https://hg.mozilla.org/integration/mozilla-inbound/rev/2c6e370c940c https://hg.mozilla.org/integration/mozilla-inbound/rev/466732e6be01 https://hg.mozilla.org/integration/mozilla-inbound/rev/3c8f81efb9a7 https://hg.mozilla.org/integration/mozilla-inbound/rev/08c816590369
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b22c5e82467b https://hg.mozilla.org/mozilla-central/rev/2c6e370c940c https://hg.mozilla.org/mozilla-central/rev/466732e6be01 https://hg.mozilla.org/mozilla-central/rev/3c8f81efb9a7 https://hg.mozilla.org/mozilla-central/rev/08c816590369
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•