[webkit-reviews] review denied: [Bug 92648] Web Inspector: Update $ to alias to querySelector rather than getElementById : [Attachment 159703] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Addy Osmani
<addyo at chromium.org>'s request for review:
Bug 92648: Web Inspector: Update $ to alias to querySelector rather than
getElementById
https://bugs.webkit.org/show_bug.cgi?id=92648

Attachment 159703: Patch
https://bugs.webkit.org/attachment.cgi?id=159703&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=159703&action=review


> Source/WebCore/inspector/InjectedScriptSource.js:932
> +    $: function (selector, start) {

{ should be on the next line.

> Source/WebCore/inspector/InjectedScriptSource.js:935
> +	       result = start.querySelector(selector);

WebKit suggests that you do return start.querySelector(selector); here and
remove else from below.

> Source/WebCore/inspector/InjectedScriptSource.js:939
> +		   result = document.getElementById(selector);

This branch requires a separate case in command-line-api.html.

> Source/WebCore/inspector/InjectedScriptSource.js:941
> +		       console.warn("The console function $() has changed from
$=getElementById(id) to $=querySelector(selector). You might try $(\"#" +
selector + "\")");

You could use message format:
console.warn("The console function $() has changed from $=getElementById(id) to
$=querySelector(selector). You might try $(\"#%s\")", selector);

> Source/WebCore/inspector/InjectedScriptSource.js:942
> +		       result = null;

return null;

> Source/WebCore/inspector/InjectedScriptSource.js:949
> +    $$: function (selector, start) {

{ on the next line.

> Source/WebCore/inspector/InjectedScriptSource.js:952
> +	       result = start.querySelectorAll(selector);

return start.querySelectorAll(selector);