[webkit-reviews] review granted: [Bug 85312] Web Inspector: cmd-[ shortcut navigates page and is fr-keyboard incompatible : [Attachment 144040] Patch

[bugzilla-daemon at webkit.org]

Vsevolod Vlasov <vsevik at chromium.org> has granted Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 85312: Web Inspector: cmd-[ shortcut navigates page and is fr-keyboard
incompatible
https://bugs.webkit.org/show_bug.cgi?id=85312

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

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144040&action=review


> Source/WebCore/inspector/front-end/InspectorView.js:46
> +    this._filteredKeyIdentifiers = ["U+005B", "U+00DB", "U+005D",
"U+00DD"].keySet();

Let's create this._leftBracketKeyIdentifiers and
this._rightBracketKeyIdentifiers fields and use them here and in
_keyDownInternal to avoid duplicating this confusing unicode magic.

> Source/WebCore/inspector/front-end/InspectorView.js:95
> +	   if (event.charCode === 91 || event.charCode === 93) {

Please add a comment saying this is checking for brackets or use some function
to convert character to ascii code.

> Source/WebCore/inspector/front-end/InspectorView.js:103
> +	   if (!WebInspector.isWin() ||
!this._filteredKeyIdentifiers.hasOwnProperty(event.keyIdentifier)) {

Please add a comment explaining this workaround (and reasoning) here.