Bug 112113 – Web Inspector: [REGRESSION] StepInto (F11) and StepOut (Shift-F11) shortcuts toggle Inspector window full-screen state

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

Caused by http://trac.webkit.org/changeset/145045 (some shortcut handlers still do not return boolean values).

[Eugene Klyuchnikov]

Created attachment 192678 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 192678 [details]
Patch

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.

[Eugene Klyuchnikov]

Comment on attachment 192678 [details]
Patch

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?

Done. Removed "?".

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

oops. Fixed.

>> 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?

Fixed.

[Eugene Klyuchnikov]

Created attachment 192721 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 192721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192721&action=review

Good, only a few JSDoc nits.

> Source/WebCore/inspector/front-end/CallStackSidebarPane.js:73
> +     */

@return {boolean}

> Source/WebCore/inspector/front-end/CallStackSidebarPane.js:85
> +     */

Ditto.

> Source/WebCore/inspector/front-end/ScriptsPanel.js:772
> +     * @param {Event=} event

Is it here only for the frontend compilability?

[Eugene Klyuchnikov]

Comment on attachment 192721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192721&action=review

>> Source/WebCore/inspector/front-end/CallStackSidebarPane.js:73
>> +     */
> 
> @return {boolean}

Fixed.

>> Source/WebCore/inspector/front-end/CallStackSidebarPane.js:85
>> +     */
> 
> Ditto.

Fixed.

>> Source/WebCore/inspector/front-end/ScriptsPanel.js:772
>> +     * @param {Event=} event
> 
> Is it here only for the frontend compilability?

Well, if we remove parameter description, it won't hurt compilability =)

[Eugene Klyuchnikov]

Created attachment 192736 [details]
Patch

[WebKit Review Bot]

Comment on attachment 192736 [details]
Patch

Clearing flags on attachment: 192736

Committed r145548: <http://trac.webkit.org/changeset/145548>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.