Bug 68155 - Web Inspector: refactor ConsoleView, Drawer, ConsolePanel trio.
Summary: Web Inspector: refactor ConsoleView, Drawer, ConsolePanel trio.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords:
Depends on:
Blocks: 65113
  Show dependency treegraph
 
Reported: 2011-09-15 05:44 PDT by Pavel Feldman
Modified: 2011-09-21 05:06 PDT (History)
12 users (show)

See Also:


Attachments
Patch (27.28 KB, patch)
2011-09-15 05:57 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
[Patch] with fixed drawer status bar style. (27.38 KB, patch)
2011-09-15 06:26 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
[Patch] Review comments addressed (27.35 KB, patch)
2011-09-15 07:22 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
[Patch] rebaselined. (27.29 KB, patch)
2011-09-19 10:47 PDT, Pavel Feldman
tonyg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2011-09-15 05:44:15 PDT
1. ConsoleView, ConsolePanel and Drawer are all Views with custom show/hide implementations and weird DOM element ownership.
2. We have a number of cases when switching console from full panel to drawer mode and back result in UI glitches.
For the sake of code clarity and no glitches, I'd like to sacrifice the "grow into full screen" console animation.
Comment 1 Pavel Feldman 2011-09-15 05:57:08 PDT
Created attachment 107481 [details]
Patch
Comment 2 Pavel Feldman 2011-09-15 06:26:04 PDT
Created attachment 107487 [details]
[Patch] with fixed drawer status bar style.
Comment 3 Andrey Kosyakov 2011-09-15 07:02:37 PDT
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.
Comment 4 Pavel Feldman 2011-09-15 07:18:13 PDT
(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.
Comment 5 Pavel Feldman 2011-09-15 07:22:27 PDT
Created attachment 107492 [details]
[Patch] Review comments addressed
Comment 6 Andrey Kosyakov 2011-09-15 07:26:33 PDT
Comment on attachment 107492 [details]
[Patch] Review comments addressed

LGTM
Comment 7 Pavel Feldman 2011-09-19 10:47:45 PDT
Created attachment 107882 [details]
[Patch] rebaselined.
Comment 8 Tony Gentilcore 2011-09-20 05:55:42 PDT
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 9 Pavel Feldman 2011-09-20 06:46:45 PDT
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.
Comment 10 Pavel Feldman 2011-09-20 06:50:16 PDT
Committed r95536: <http://trac.webkit.org/changeset/95536>
Comment 11 Csaba Osztrogonác 2011-09-20 10:24:11 PDT
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?
Comment 12 Pavel Feldman 2011-09-20 10:56:16 PDT
Looking
Comment 13 Pavel Feldman 2011-09-20 11:16:47 PDT
(In reply to comment #12)
> Looking

Fix landed as r95557. Sorry for trouble - this test was disabled on chromium :(
Comment 14 Csaba Osztrogonác 2011-09-20 11:18:20 PDT
(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 15 Yury Semikhatsky 2011-09-21 05:00:39 PDT
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.