Bug 112113 - Web Inspector: [REGRESSION] StepInto (F11) and StepOut (Shift-F11) shortcuts toggle Inspector window full-screen state
Summary: Web Inspector: [REGRESSION] StepInto (F11) and StepOut (Shift-F11) shortcuts ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Eugene Klyuchnikov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-12 01:08 PDT by Alexander Pavlov (apavlov)
Modified: 2013-03-12 08:21 PDT (History)
9 users (show)

See Also:


Attachments
Patch (11.25 KB, patch)
2013-03-12 02:25 PDT, Eugene Klyuchnikov
no flags Details | Formatted Diff | Diff
Patch (11.15 KB, patch)
2013-03-12 06:12 PDT, Eugene Klyuchnikov
no flags Details | Formatted Diff | Diff
Patch (11.10 KB, patch)
2013-03-12 07:59 PDT, Eugene Klyuchnikov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2013-03-12 01:08:06 PDT
Caused by http://trac.webkit.org/changeset/145045 (some shortcut handlers still do not return boolean values).
Comment 1 Eugene Klyuchnikov 2013-03-12 02:25:25 PDT
Created attachment 192678 [details]
Patch
Comment 2 Alexander Pavlov (apavlov) 2013-03-12 02:37:30 PDT
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.
Comment 3 Eugene Klyuchnikov 2013-03-12 06:11:25 PDT
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.
Comment 4 Eugene Klyuchnikov 2013-03-12 06:12:23 PDT
Created attachment 192721 [details]
Patch
Comment 5 Alexander Pavlov (apavlov) 2013-03-12 07:13:19 PDT
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?
Comment 6 Eugene Klyuchnikov 2013-03-12 07:57:03 PDT
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 =)
Comment 7 Eugene Klyuchnikov 2013-03-12 07:59:26 PDT
Created attachment 192736 [details]
Patch
Comment 8 WebKit Review Bot 2013-03-12 08:21:48 PDT
Comment on attachment 192736 [details]
Patch

Clearing flags on attachment: 192736

Committed r145548: <http://trac.webkit.org/changeset/145548>
Comment 9 WebKit Review Bot 2013-03-12 08:21:52 PDT
All reviewed patches have been landed.  Closing bug.