Rename JS unwrapping functions

RESOLVED FIXED in mozilla23

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Unassigned)

Tracking

unspecified
mozilla23
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=bholley][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

90.48 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Diff | Splinter Review
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.

Comment 1

5 years ago
CheckUnwrap/UncheckedUnwrap sound good.
Whiteboard: [mentor=bholley][lang=c++]

Comment 2

5 years ago
Created attachment 733005 [details] [diff] [review]
Patch to replace method names

How does this look?
Attachment #733005 - Flags: review?(bobbyholley+bmo)
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. :-)
Attachment #733005 - Flags: review?(bobbyholley+bmo) → review-

Comment 4

5 years ago
Created attachment 733222 [details] [diff] [review]
A more aware mass replace

Shoot, you're right, missed that when doing an initial mxr search and made a bad assumption. Here's version two.
Attachment #733005 - Attachment is obsolete: true
Attachment #733222 - Flags: review?(bobbyholley+bmo)

Comment 5

5 years ago
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)?

Comment 6

5 years ago
Created attachment 733246 [details] [diff] [review]
A more aware mass replace that actually builds

Builds now and doesn't touch the JS API.
Attachment #733222 - Attachment is obsolete: true
Attachment #733222 - Flags: review?(bobbyholley+bmo)
Attachment #733246 - Flags: review?
Comment on attachment 733246 [details] [diff] [review]
A more aware mass replace that actually builds

reviewer got lost
Attachment #733246 - Flags: review? → review?(bobbyholley+bmo)
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. :-)
Attachment #733246 - Flags: review?(bobbyholley+bmo) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/a85d21e394c0

Thanks for the patch, Jacek!
Keywords: checkin-needed
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

Comment 11

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/4add88d3db69
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Comment 15

5 years ago
Thanks for rebasing this for me and sorry for disappearing like that. Hope to contribute something else soon. See you around!
You need to log in before you can comment on or make changes to this bug.