Once bug 854480 lands, we should improve the naming of the unwrapping functions. Right now the unsafe version is called js::UnwrapObject and the safe version is js::UnwrapObjectChecked, which isn't ideal. I'd like both names to be explicit about what they do. I'm thinking js::CheckedUnwrap and js::UncheckedUnwrap. How do people feel about this? I could see an argument for the world 'Object' being in there, but then the function names start to get kind of long.

CheckUnwrap/UncheckedUnwrap sound good.

How does this look?

Comment on attachment 733005 [details] [diff] [review] Patch to replace method names Was this just a regexp? It looks like it changes other functions, like mozilla::dom::UnwrapObject, which is quite different. I would suggest doing a mass-replace only for the explicitly-namedspaced versions (js::UnwrapObject and js::UnwrapObjectChecked). The only places that do |using namespace js| are js/src and js/xpconnect, and those can be fixed on a second pass. :-)

Shoot, you're right, missed that when doing an initial mxr search and made a bad assumption. Here's version two.

Noticed the patch doesn't build, since I screwed up replacing JS_UnwrapObject -- but here's a question, should I rename it, or leave it as it is (seeing that it's a public API)?

Builds now and doesn't touch the JS API.

Comment on attachment 733246 [details] [diff] [review] A more aware mass replace that actually builds reviewer got lost

Comment on attachment 733246 [details] [diff] [review] A more aware mass replace that actually builds Review of attachment 733246 [details] [diff] [review]: ----------------------------------------------------------------- Nice. r=bholley ::: js/src/jswrapper.h @@ +232,5 @@ > > // Given a JSObject, returns that object stripped of wrappers. At each stage, > // the security wrapper has the opportunity to veto the unwrap. Since checked > // code should never be unwrapping outer window wrappers, we always stop at > // outer windows. Whoops, this last sentence is totally out of date. My fault. :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/a85d21e394c0 Thanks for the patch, Jacek!

Backed out for build bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/c677dd4b1aba https://tbpl.mozilla.org/php/getParsedLog.php?id=21510975&tree=Mozilla-Inbound GeneratedEvents.cpp /usr/bin/ccache /tools/gcc-4.5-0moz3/bin/g++ -o GeneratedEvents.o -c -I../../../dist/stl_wrappers -I../../../dist/system_wrappers -include /builds/slave/m-in-lx-0000000000000000000000/build/config/gcc_hidden.h -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -DJSFILE -DJS_THREADSAFE -DEXPORT_XPC_API -DMOZ_JSDEBUGGER -D_IMPL_NS_LAYOUT -I/builds/slave/m-in-lx-0000000000000000000000/build/js/xpconnect/src/../wrappers -I/builds/slave/m-in-lx-0000000000000000000000/build/js/xpconnect/src/../loader -I/builds/slave/m-in-lx-0000000000000000000000/build/caps/include -I/builds/slave/m-in-lx-0000000000000000000000/build/content/base/src -I/builds/slave/m-in-lx-0000000000000000000000/build/content/base/public -I/builds/slave/m-in-lx-0000000000000000000000/build/content/events/src -I/builds/slave/m-in-lx-0000000000000000000000/build/content/html/content/src -I/builds/slave/m-in-lx-0000000000000000000000/build/content/html/document/src -I/builds/slave/m-in-lx-0000000000000000000000/build/content/svg/content/src -I/builds/slave/m-in-lx-0000000000000000000000/build/layout/style -I/builds/slave/m-in-lx-0000000000000000000000/build/layout/base -I/builds/slave/m-in-lx-0000000000000000000000/build/dom/base -I/builds/slave/m-in-lx-0000000000000000000000/build/xpcom/ds -I/builds/slave/m-in-lx-0000000000000000000000/build/js/xpconnect/src -I. -I../../../dist/include -I/builds/slave/m-in-lx-0000000000000000000000/build/obj-firefox/dist/include/nspr -I/builds/slave/m-in-lx-0000000000000000000000/build/obj-firefox/dist/include/nss -fPIC -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wcast-align -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -pipe -DNDEBUG -DTRIMMED -g -Os -freorder-blocks -fno-omit-frame-pointer -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/GeneratedEvents.o.pp /builds/slave/m-in-lx-0000000000000000000000/build/obj-firefox/js/xpconnect/src/GeneratedEvents.cpp ../../../../js/xpconnect/src/xpcprivate.h: In constructor 'XPCJSRuntime::XPCJSRuntime(nsXPConnect*)': ../../../../js/xpconnect/src/xpcprivate.h:968:10: warning: 'XPCJSRuntime::mExceptionManagerNotAvailable' will be initialized after ../../../../js/xpconnect/src/xpcprivate.h:964:15: warning: 'JSObject* XPCJSRuntime::mJunkScope' ../../../../js/xpconnect/src/XPCJSRuntime.cpp:2637:1: warning: when initialized here ../../../../js/xpconnect/src/XPCJSRuntime.cpp: In member function 'JSObject* XPCJSRuntime::GetJunkScope()': ../../../../js/xpconnect/src/XPCJSRuntime.cpp:3033:22: error: 'UnwrapObject' is not a member of 'js' make[6]: *** [XPCJSRuntime.o] Error 1

Seems like a revision that happened after I wrote the patch added a new call js::UnwrapObject in /js/xpconnect/src/XPCJSRuntime.cpp (here? https://hg.mozilla.org/integration/mozilla-inbound/rev/36fcd3edc6b8) -- can someone confirm if I'm right? Fix shouldn't be too much of a problem if that's the case.

(In reply to maligree from comment #11) > Seems like a revision that happened after I wrote the patch added a new call > js::UnwrapObject in > /js/xpconnect/src/XPCJSRuntime.cpp (here? > https://hg.mozilla.org/integration/mozilla-inbound/rev/36fcd3edc6b8) -- can > someone confirm if I'm right? Fix shouldn't be too much of a problem if > that's the case. That sounds right. Rebase your patch forward and reflag for review?

I rebased, and then rebased again over inbound, and then pushed. Hopefully this patch sticks: https://hg.mozilla.org/integration/mozilla-inbound/rev/4add88d3db69

Thanks for rebasing this for me and sorry for disappearing like that. Hope to contribute something else soon. See you around!