[webkit-reviews] review canceled: [Bug 101591] Web Inspector: Ctrl+a in the network panel should select resource content, not the entire panel : [Attachment 173019] Patch

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has canceled Andrey Lushnikov
<lushnikov at chromium.org>'s request for review:
Bug 101591: Web Inspector: Ctrl+a in the network panel should select resource
content, not the entire panel
https://bugs.webkit.org/show_bug.cgi?id=101591

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

------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=173019&action=review


> Source/WebCore/ChangeLog:3
> +	   Web Inspector: Ctrl+a in the network panel should select resource
content, not the entire panel

Capital 'A'

> Source/WebCore/ChangeLog:8
> +	   Intercepts Ctrl-a event in DefaultTextEditor to select resource
content

Intercepts -> Intercept (i.e. "what we do by this patch").

Also, 'a' should be consistently capitalized across the patch, as is usual with
the shortcuts notation (see the Shortcuts help screen).

> Source/WebCore/ChangeLog:12
> +	   (WebInspector.DefaultTextEditor.prototype._handleKeyDown): intercept
Ctrl-A even for readonly fields

Comments should be sentence-capitalized ("Intercept...")

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1492
> +	   window.getSelection().addRange(range);

You should probably invoke
window.getSelection().removeAllRanges();
before this line, so that an existing selection will not interfere with your
override.

> Source/WebCore/inspector/front-end/KeyboardShortcut.js:179
> +WebInspector.KeyboardShortcut.SELECT_ALL =
WebInspector.KeyboardShortcut.makeKey("a",
WebInspector.KeyboardShortcut.Modifiers.CtrlOrMeta);

Constants are camel-cased in WebKit: "...SelectAll"