[webkit-reviews] review granted: [Bug 40500] Web Inspector: Should not expose window.console._inspectorCommandLineAPI to the web : [Attachment 58544] [PATCH] Proposed fix.

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 40500: Web Inspector: Should not expose
window.console._inspectorCommandLineAPI to the web
https://bugs.webkit.org/show_bug.cgi?id=40500

Attachment 58544: [PATCH] Proposed fix.
https://bugs.webkit.org/attachment.cgi?id=58544&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
> WebCore/inspector/front-end/InjectedScript.js
>  InjectedScript._evaluateOn = function(evalFunction, object, expression,
dontUseCommandLineAPI)

I know you are just leaving this alone, but I think this would
read much better if the last argument was just "useCommandLineAPI".
Since you are making changes to this part specifically, if you
agree it would be nice to update it! I try to avoid double
negatives "! dontDoSomething".



> +    if (!dontUseCommandLineAPI)
> +	   delete inspectedWindow.console._commandLineAPI;

We discussed this. Its not strong enough to plug the hole,
but it removes its face from generic web scripts.


> +	       while (node = results.iterateNext()) nodes.push(node);

NIT: Style, move nodes.push to the next line.


> +	   inspectedWindow.console.log(object);

Can this just be console.log(object)?


> +	   if (InjectedScript._type(object) === "node") {
> +	       InjectedScriptHost.pushNodePathToFrontend(object, false, true);
> +	   } else {

NIT: Style, braces.


> +    get $0()
> +    get $1()
> +    get $2()
> +    get $3()
> +    get $4()

I think these are much clearer when they are on oneline,
like they used to be. Sometimes style rules are meant
to be broken!