1288515 - Rename GetScriptedProxyHandlerObject to ScriptedProxyHandler::handlerObject and make it public

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Oriol Brufau [:Oriol]]

Attachment: Rename GetScriptedProxyHandlerObject to ScriptedProxyHandler::handlerObject and make it public

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

The reasoning for the renaming is in bug 1282257 comment 37.

Comment 1

[Jim Blandy :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?

Comment 2

[Oriol Brufau [:Oriol]]

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

Comment 3

[Jim Blandy :jimb]

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

This one.

Comment 4

[Oriol Brufau [:Oriol]]

Attachment: Rename GetScriptedProxyHandlerObject to ScriptedProxyHandler::handlerObject and make it public

Adding the const.

Comment 5

[Jim Blandy :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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/304a57bfb0d4
Rename GetScriptedProxyHandlerObject to ScriptedProxyHandler::handlerObject and make it public. r=jimb

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/304a57bfb0d4