Closed Bug 1288515 Opened 3 years ago Closed 3 years ago

Rename GetScriptedProxyHandlerObject to ScriptedProxyHandler::handlerObject and make it public

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

Attachments

(1 file, 1 obsolete file)

This way it can be used e.g. by the debugger.

The reasoning for the renaming is in bug 1282257 comment 37.
Attachment #8773448 - Flags: review?(jimb)
Comment on attachment 8773448 [details] [diff] [review]
Rename GetScriptedProxyHandlerObject to ScriptedProxyHandler::handlerObject and make it public

Review of attachment 8773448 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. I made some suggestions, but those are up to you; this is fine as-is. If you revise, please re-post, so I can r+ and flag as checkin-needed.

::: js/src/proxy/ScriptedProxyHandler.cpp
@@ +170,5 @@
>  ScriptedProxyHandler::getPrototype(JSContext* cx, HandleObject proxy,
>                                     MutableHandleObject protop) const
>  {
>      // Steps 1-3.
> +    RootedObject handler(cx, ScriptedProxyHandler::handlerObject(proxy));

I think you don't need to include "ScriptedProxyHandler::" here, since we're already in a method of that class. I think this is true even if you're calling a static method from a static method...

::: js/src/proxy/ScriptedProxyHandler.h
@@ +92,5 @@
>      // The "function extended" slot index in which the revocation object is stored. Per spec, this
>      // is to be cleared during the first revocation.
>      static const int REVOKE_SLOT = 0;
> +
> +    static JSObject* handlerObject(JSObject* proxy);

Suggestion: would `const JSObject *` work here?
Attachment #8773448 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #1)
> I think you don't need to include "ScriptedProxyHandler::" here, since we're
> already in a method of that class. I think this is true even if you're
> calling a static method from a static method...

"ScriptedProxyHandler::" is not needed but I added it to be coherent with things like "ScriptedProxyHandler::HANDLER_EXTRA".

> Suggestion: would `const JSObject *` work here?

What do you mean?

    const JSObject* handlerObject(JSObject* proxy);
    static const JSObject* handlerObject(JSObject* proxy);
    static JSObject* handlerObject(const JSObject* proxy);
    static const JSObject* handlerObject(const JSObject* proxy);
(In reply to Oriol from comment #2)
>     static JSObject* handlerObject(const JSObject* proxy);

This one.
Adding the const.
Attachment #8773448 - Attachment is obsolete: true
Attachment #8773865 - Flags: review?(jimb)
Comment on attachment 8773865 [details] [diff] [review]
Rename GetScriptedProxyHandlerObject to ScriptedProxyHandler::handlerObject and make it public

Review of attachment 8773865 [details] [diff] [review]:
-----------------------------------------------------------------

Looks perfect.
Attachment #8773865 - Flags: review?(jimb) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/304a57bfb0d4
Rename GetScriptedProxyHandlerObject to ScriptedProxyHandler::handlerObject and make it public. r=jimb
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/304a57bfb0d4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.