Summary: | Web Inspector: refactor ConsoleView, Drawer, ConsolePanel trio. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pavel Feldman <pfeldman> | ||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Pavel Feldman <pfeldman> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, caseq, joepeck, keishi, loislo, ossy, pfeldman, pmuellr, rik, timothy, yurys | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 65113 | ||||||||||||
Attachments: |
|
Description
Pavel Feldman
2011-09-15 05:44:15 PDT
Created attachment 107481 [details]
Patch
Created attachment 107487 [details]
[Patch] with fixed drawer status bar style.
Comment on attachment 107487 [details] [Patch] with fixed drawer status bar style. View in context: https://bugs.webkit.org/attachment.cgi?id=107487&action=review > Source/WebCore/inspector/front-end/ConsolePanel.js:57 > + this._view.show(this.element); passing parent element to show() is redundant here. > Source/WebCore/inspector/front-end/Drawer.js:173 > + resize: function() shouldn't this propagate resize to the child view? > Source/WebCore/inspector/front-end/inspector.js:219 > + this._toggleConsoleButton.toggled = !this._toggleConsoleButton.toggled; I think the way we used to maintain console button state previously was better: this gave us somewhat better encapsulation, less logic in inspector.js and a clear assertion that the state of the button matches that of the view. Now we have this logic spread over 3 methods of inspector.js. (In reply to comment #3) > (From update of attachment 107487 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107487&action=review > > > Source/WebCore/inspector/front-end/ConsolePanel.js:57 > > + this._view.show(this.element); > > passing parent element to show() is redundant here. Done. > > > Source/WebCore/inspector/front-end/Drawer.js:173 > > + resize: function() > > shouldn't this propagate resize to the child view? Done. > > > Source/WebCore/inspector/front-end/inspector.js:219 > > + this._toggleConsoleButton.toggled = !this._toggleConsoleButton.toggled; > > I think the way we used to maintain console button state previously was better: this gave us somewhat better encapsulation, less logic in inspector.js and a clear assertion that the state of the button matches that of the view. Now we have this logic spread over 3 methods of inspector.js. ConsoleView should not host a button that toggles drawer. This button is a glue, and our glue lives in the inspector.js. Created attachment 107492 [details]
[Patch] Review comments addressed
Comment on attachment 107492 [details]
[Patch] Review comments addressed
LGTM
Created attachment 107882 [details]
[Patch] rebaselined.
Comment on attachment 107882 [details] [Patch] rebaselined. View in context: https://bugs.webkit.org/attachment.cgi?id=107882&action=review > Source/WebCore/ChangeLog:10 > + screen" console animation. The one line description suggestions this is just a refactor, but it is actually a behavior change. Maybe there is a better one-line description? > Source/WebCore/inspector/front-end/ConsolePanel.js:31 > WebInspector.Panel.call(this, "console"); While you are here, why not add an /**@constructor*/ annotation to this ConsolePanel ctor. Ditto for ConsoleView below. > Source/WebCore/inspector/front-end/Drawer.js:63 > + this.immediatelyFinishAnimation(); Don't quite get why these are still necessary. Should the animation bits be ripped out more completely or do some animations still exist? Comment on attachment 107882 [details] [Patch] rebaselined. View in context: https://bugs.webkit.org/attachment.cgi?id=107882&action=review Thanks for your review! >> Source/WebCore/ChangeLog:10 >> + screen" console animation. > > The one line description suggestions this is just a refactor, but it is actually a behavior change. Maybe there is a better one-line description? Done. >> Source/WebCore/inspector/front-end/ConsolePanel.js:31 >> WebInspector.Panel.call(this, "console"); > > While you are here, why not add an /**@constructor*/ annotation to this ConsolePanel ctor. Ditto for ConsoleView below. I haven't added them to the compilation yet. I do it file by file as a part of the separate effort. Thanks for noticing though! >> Source/WebCore/inspector/front-end/Drawer.js:63 >> + this.immediatelyFinishAnimation(); > > Don't quite get why these are still necessary. Should the animation bits be ripped out more completely or do some animations still exist? Drawer is still animating. It is just that transition between visible drawer and console panel does not animate anymore. Committed r95536: <http://trac.webkit.org/changeset/95536> It broke a test on the SL and the Qt bot: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20%28Tests%29/r95540%20%2833226%29/inspector/debugger/script-formatter-pretty-diff.html Could you check it, please? Looking (In reply to comment #12) > Looking Fix landed as r95557. Sorry for trouble - this test was disabled on chromium :( (In reply to comment #13) > (In reply to comment #12) > > Looking > > Fix landed as r95557. Sorry for trouble - this test was disabled on chromium :( Not problem, thanks for the quick fix. Comment on attachment 107882 [details] [Patch] rebaselined. View in context: https://bugs.webkit.org/attachment.cgi?id=107882&action=review > Source/WebCore/inspector/front-end/ConsolePanel.js:51 > + WebInspector.drawer.hide(true); ConsolePanel should not directly depend on the Drawer. |