WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
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
Summary
Web Inspector: [REGRESSION] StepInto (F11) and StepOut (Shift-F11) shortcuts ...
Alexander Pavlov (apavlov)
Reported
2013-03-12 01:08:06 PDT
Caused by
http://trac.webkit.org/changeset/145045
(some shortcut handlers still do not return boolean values).
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Eugene Klyuchnikov
Comment 1
2013-03-12 02:25:25 PDT
Created
attachment 192678
[details]
Patch
Alexander Pavlov (apavlov)
Comment 2
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.
Eugene Klyuchnikov
Comment 3
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.
Eugene Klyuchnikov
Comment 4
2013-03-12 06:12:23 PDT
Created
attachment 192721
[details]
Patch
Alexander Pavlov (apavlov)
Comment 5
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?
Eugene Klyuchnikov
Comment 6
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 =)
Eugene Klyuchnikov
Comment 7
2013-03-12 07:59:26 PDT
Created
attachment 192736
[details]
Patch
WebKit Review Bot
Comment 8
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
>
WebKit Review Bot
Comment 9
2013-03-12 08:21:52 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug