This should be straightforward and allow us to remove the AutoRooter variant.

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?

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.

Comment on attachment 787479 [details] [diff] [review] propdesc-patch-xpc Review of attachment 787479 [details] [diff] [review]: ----------------------------------------------------------------- Nice. r=bholley

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

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.

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.

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?

(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.)

(In reply to Daniel Holbert [:dholbert] from comment #16) Great, thanks for sorting this out!