Summary: | Web Inspector: [REGRESSION] StepInto (F11) and StepOut (Shift-F11) shortcuts toggle Inspector window full-screen state | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Pavlov (apavlov) <apavlov> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Eugene Klyuchnikov <eustas> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | apavlov, keishi, loislo, pfeldman, pmuellr, vsevik, web-inspector-bugs, webkit.review.bot, yurys | ||||||||
Priority: | P1 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Alexander Pavlov (apavlov)
2013-03-12 01:08:06 PDT
Created attachment 192678 [details]
Patch
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 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. Created attachment 192721 [details]
Patch
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 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 =) Created attachment 192736 [details]
Patch
Comment on attachment 192736 [details] Patch Clearing flags on attachment: 192736 Committed r145548: <http://trac.webkit.org/changeset/145548> All reviewed patches have been landed. Closing bug. |