Bug 142141 – Web Inspector: Refactoring: separate ConsoleSession from ConsoleGroup

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Currently, ConsoleGroup is used for both sessions and console groups (created via console.group()), even though they look quite different. Console sessions can’t be collapsed and they don’t have a title.

[Radar WebKit Bug Importer]

<rdar://problem/19998033>

[Nikita Vasilyev]

Created attachment 247622 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 247622 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247622&action=review

> Source/WebInspectorUI/UserInterface/Views/ConsoleGroup.js:-63
> -            this.messagesElement.parentNode.insertBefore(element, this.messagesElement);

This line really had kept me puzzled for some time. Since we don’t use any templates, our views are quite verbose and fairly hard to read. We should avoid using parentNode and insertBefore to keep our views easier to understand.

[Timothy Hatcher]

Comment on attachment 247622 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247622&action=review

It is good to split these, but there really needs to be a model and view split as well. Right now these objects are both the model and the view and that complicates things and makes them untestable.

> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptLogViewController.js:-115
> -        // Reuse the top group if it has no messages.
> -        if (this._topConsoleGroup && !this._topConsoleGroup.hasMessages()) {
> -            // Make sure the session is visible.
> -            this._topConsoleGroup.element.scrollIntoView();
> -            return;
> -        }

Seems good to do still. I remember visually this was causing multiple dividers to stack. We clear on reload by default now, so you might need to turn that off.

> Source/WebInspectorUI/UserInterface/Views/ConsoleGroup.js:42
> +    render: function(msg)

msg should be spelled out. Is this a WebInspector.ConsoleMessage?

ConsoleGroup should really be a model object and we should have a ConsoleGroupView that encapsulates the DOM element handling.

> Source/WebInspectorUI/UserInterface/Views/ConsoleGroup.js:44
> +        var group = document.createElement("div");

groupElement would be a better name.

> Source/WebInspectorUI/UserInterface/Views/ConsoleSession.js:29
> +WebInspector.ConsoleSession = function()

We will likely want a ConsoleSession model object that holds ConsoleMessage model objects. And corresponding view objects — ConsoleSessionView and ConsoleMessageView. This new object current mixes the two — like ConsoleGroup did.

[Nikita Vasilyev]

Created attachment 247715 [details]
Patch

[Nikita Vasilyev]

(In reply to comment #4)
> Comment on attachment 247622 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247622&action=review
> 
> It is good to split these, but there really needs to be a model and view
> split as well. Right now these objects are both the model and the view and
> that complicates things and makes them untestable.

Yes, that’s what I’m going to do next. This patch is just about simplifying Session view.

> 
> > Source/WebInspectorUI/UserInterface/Controllers/JavaScriptLogViewController.js:-115
> > -        // Reuse the top group if it has no messages.
> > -        if (this._topConsoleGroup && !this._topConsoleGroup.hasMessages()) {
> > -            // Make sure the session is visible.
> > -            this._topConsoleGroup.element.scrollIntoView();
> > -            return;
> > -        }
> 
> Seems good to do still. I remember visually this was causing multiple
> dividers to stack. We clear on reload by default now, so you might need to
> turn that off.
> 
> > Source/WebInspectorUI/UserInterface/Views/ConsoleGroup.js:42
> > +    render: function(msg)
> 
> msg should be spelled out. Is this a WebInspector.ConsoleMessage?

Yes, it is.

> 
> ConsoleGroup should really be a model object and we should have a
> ConsoleGroupView that encapsulates the DOM element handling.
> 
> > Source/WebInspectorUI/UserInterface/Views/ConsoleGroup.js:44
> > +        var group = document.createElement("div");
> 
> groupElement would be a better name.
> 
> > Source/WebInspectorUI/UserInterface/Views/ConsoleSession.js:29
> > +WebInspector.ConsoleSession = function()
> 
> We will likely want a ConsoleSession model object that holds ConsoleMessage
> model objects. And corresponding view objects — ConsoleSessionView and
> ConsoleMessageView. This new object current mixes the two — like
> ConsoleGroup did.

Separating models from the views of ConsoleMessage, ConsoleGroup, ConsoleSession is what I’m going to do next.

[WebKit Commit Bot]

Comment on attachment 247715 [details]
Patch

Clearing flags on attachment: 247715

Committed r180941: <http://trac.webkit.org/changeset/180941>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.