Closed
Bug 862115
Opened 11 years ago
Closed 11 years ago
Use Rooted<JSPropertyDescriptor> in favor of JSPropertyDescriptor::AutoRooter
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: terrence, Assigned: jonco)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 3 obsolete files)
95.95 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
51.96 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
25.22 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
15.17 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
This should be straightforward and allow us to remove the AutoRooter variant.
Reporter | ||
Updated•11 years ago
|
Reporter | ||
Updated•11 years ago
|
Assignee: general → terrence
Reporter | ||
Comment 1•11 years ago
|
||
Attachment #743773 -
Flags: review?(jcoppeard)
Attachment #743773 -
Flags: feedback?(jwalden+bmo)
Assignee | ||
Comment 2•11 years ago
|
||
Comment on attachment 743773 [details] [diff] [review] v0 Review of attachment 743773 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: js/src/jsapi.h @@ +3358,5 @@ > bool hasAttributes(unsigned attrs) const { return desc()->attrs & attrs; } > > JS::MutableHandleObject object() { return JS::MutableHandleObject::fromMarkedLocation(&desc()->obj); } > unsigned attributes() const { return desc()->attrs; } > + unsigned &attributesRef() { return desc()->attrs; } Should this just be attributes() like the versions of getter() and setter() that return a reference below?
Attachment #743773 -
Flags: review?(jcoppeard) → review+
Comment 3•11 years ago
|
||
Comment on attachment 743773 [details] [diff] [review] v0 Review of attachment 743773 [details] [diff] [review]: ----------------------------------------------------------------- Nothing too goofy sticks out here. Although I was a bit saddened to see that PropertyDescriptor has isPermanent(), isReadonly(), etc. as the field names, rather than adopting the standard-sanctioned inverted names (isConfigurable(), isWritable()). But I guess that's pre-existing, so meh. Oh, and exposing refs to the internal fields feels a bit weird. Especially attributesRef -- you could just as easily fill in a local and then do a setAttributes() after that, which seems easier to reason about/understand, to me.
Attachment #743773 -
Flags: feedback?(jwalden+bmo) → feedback+
Reporter | ||
Updated•11 years ago
|
Reporter | ||
Comment 4•11 years ago
|
||
Reporter | ||
Comment 5•11 years ago
|
||
Reporter | ||
Updated•11 years ago
|
Assignee | ||
Updated•11 years ago
|
Assignee: terrence → jcoppeard
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #743773 -
Attachment is obsolete: true
Attachment #787478 -
Flags: review?(terrence)
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #786357 -
Attachment is obsolete: true
Attachment #787479 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #786359 -
Attachment is obsolete: true
Attachment #787480 -
Flags: review?(bugs)
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #787482 -
Flags: review?(bugs)
Comment 10•11 years ago
|
||
Comment on attachment 787479 [details] [diff] [review] propdesc-patch-xpc Review of attachment 787479 [details] [diff] [review]: ----------------------------------------------------------------- Nice. r=bholley
Attachment #787479 -
Flags: review?(bobbyholley+bmo) → review+
Comment 11•11 years ago
|
||
Comment on attachment 787480 [details] [diff] [review] propdesc-patch-browser > if (!proto) { >- desc->obj = NULL; >+ desc.object().set(NULL); s/NULL/nullptr/ >-FillPropertyDescriptor(JSPropertyDescriptor* desc, JSObject* obj, bool readonly) >+FillPropertyDescriptor(JS::MutableHandle<JSPropertyDescriptor> desc, JSObject* obj, bool readonly) > { >- desc->obj = obj; >- desc->attrs = (readonly ? JSPROP_READONLY : 0) | JSPROP_ENUMERATE; >- desc->getter = NULL; >- desc->setter = NULL; >- desc->shortid = 0; >+ desc.object().set(obj); >+ desc.setAttributes((readonly ? JSPROP_READONLY : 0) | JSPROP_ENUMERATE); >+ desc.setGetter(NULL); >+ desc.setSetter(NULL); >+ desc.setShortId(0); while you're here, nullptr please
Attachment #787480 -
Flags: review?(bugs) → review+
Comment 12•11 years ago
|
||
Comment on attachment 787482 [details] [diff] [review] propdesc-patch-ipc > if (!in.setter()) { >- out->setter = NULL; >+ out.setSetter(NULL); Want to change NULL to nullptr while you're here.
Attachment #787482 -
Flags: review?(bugs) → review+
Reporter | ||
Comment 13•11 years ago
|
||
Comment on attachment 787478 [details] [diff] [review] propdesc-patch-js Review of attachment 787478 [details] [diff] [review]: ----------------------------------------------------------------- Great! r=me ::: js/src/jsproxy.cpp @@ +126,5 @@ > vp.setUndefined(); > return true; > } > + if (!desc.getter() || > + (!desc.hasGetterObject() && desc.getter() == JS_PropertyStub)) { { on new line, while you're here.
Attachment #787478 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 14•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/db34065a8666
Comment 15•11 years ago
|
||
Comment on attachment 787479 [details] [diff] [review] propdesc-patch-xpc > nsGlobalWindow *win; >- if (!desc->obj && Traits::Type == XrayForWrappedNative && JSID_IS_STRING(id) && >+ if (!desc.object() && Traits::Type == XrayForWrappedNative && JSID_IS_STRING(id) && > (win = static_cast<nsGlobalWindow*>(As<nsPIDOMWindow>(wrapper)))) > { > nsDependentJSString name(id); > nsCOMPtr<nsIDOMWindow> childDOMWin = win->GetChildWindow(name); For some reason, this s/->obj/.object()/ replacement makes GCC 4.8 complain about 'win' being maybe unitialized: js/xpconnect/wrappers/XrayWrapper.cpp:1419:70: error: 'win' may be used uninitialized in this function [-Werror=maybe-uninitialized] nsCOMPtr<nsIDOMWindow> childDOMWin = win->GetChildWindow(name); ^ That's a build warning, but it breaks the build for people building w/ 4.8 and --enable-warnings-as-errors, because this directory is FAIL_ON_WARNINGS. I don't see how that change could make 'win' maybe-uninitialized, but maybe I'm missing something... Maybe it's a compiler bug?
Comment 16•11 years ago
|
||
(After a bit more digging, I think we should probably just prevent the warning in comment 15 from being tracked by FAIL_ON_WARNINGS, since it's false-positive-ish. That'll prevent it from causing build bustage. I filed bug 903513 on that.)
Assignee | ||
Comment 17•11 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #16) Great, thanks for sorting this out!
Comment 18•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/db34065a8666
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in
before you can comment on or make changes to this bug.
Description
•