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.
<rdar://problem/19998033>
Created attachment 247622 [details] Patch
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.
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.
Created attachment 247715 [details] Patch
(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.
Comment on attachment 247715 [details] Patch Clearing flags on attachment: 247715 Committed r180941: <http://trac.webkit.org/changeset/180941>
All reviewed patches have been landed. Closing bug.