[webkit-reviews] review denied: [Bug 112113] Web Inspector: [REGRESSION] StepInto (F11) and StepOut (Shift-F11) shortcuts toggle Inspector window full-screen state : [Attachment 192678] Patch

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has denied Eugene Klyuchnikov
<eustas at chromium.org>'s request for review:
Bug 112113: Web Inspector: [REGRESSION] StepInto (F11) and StepOut (Shift-F11)
shortcuts toggle Inspector window full-screen state
https://bugs.webkit.org/show_bug.cgi?id=112113

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

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


> Source/WebCore/inspector/front-end/CallStackSidebarPane.js:134
> +	* @param {function(!Array.<!WebInspector.KeyboardShortcut.Descriptor>,
function(?Event=):boolean)} registerShortcutDelegate

Do we really use both null and undefined here? Sounds bad. There's a bunch of
similar annotations. Can it be fixed?

> Source/WebCore/inspector/front-end/Panel.js:264
> +	   console.trace();

Remove this

> Source/WebCore/inspector/front-end/ScriptsPanel.js:722
> +	       return false;

vsevik and I have discussed this. Do we really want to toggle fullscreen when
NOT on a breakpoint?

> Source/WebCore/inspector/front-end/ScriptsPanel.js:740
> +	       return false;

Ditto.

> Source/WebCore/inspector/front-end/TimelinePanel.js:369
> +	   contextMenu.appendItem(WebInspector.UIString("Load Timeline
data\u2026"), this._selectFileToLoad.bind(this), this._operationInProgress);

This change is in fact not necessary but ok for the consistency.

> Source/WebCore/inspector/front-end/TimelinePanel.js:579
> +	* @param {?Event|WebInspector.Event=} event

Looks nasty. Please remove the "event" argument altogether.