Created attachment 302196 [details] [HTML] Reduction * STEPS TO REPRODUCE: 1. Open the attached <index.html> 2. Open WebInspector to Console tab 3. Refresh the page 4. An error should appear 5. Refresh the page 6. A session divider and another error should appear 7. Switch to the Elements tab 8. Refresh the page ** Expected: A session divider is shown between the last two errors ** Actual: No session divider is shown between the last two errors
Created attachment 302197 [details] [Image] Screenshot of Issue
Created attachment 335909 [details] Patch
Comment on attachment 335909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335909&action=review Works great, r=me with some minor style comments: it feel wrong to use the name `this._currentConsoleGroup`, when the value is always a ConsoleSession object. I know it was already like this, but I think this would be a good time to change the name. Also, the public properties `prompt` and `currentConsoleGroup` are not referenced anywhere, and can be removed. > Source/WebInspectorUI/UserInterface/Controllers/JavaScriptLogViewController.js:62 > + this._pendingMessages = new Map; I think `this._pendingMessageViewsForSession` would be more self-explanatory.
Comment on attachment 335909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335909&action=review >> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptLogViewController.js:62 >> + this._pendingMessages = new Map; > > I think `this._pendingMessageViewsForSession` would be more self-explanatory. Did a quick search and we aren't using For/To in our map names as much as I thought. this._pendingMessageViews is probably fine.
(In reply to Matt Baker from comment #3) > Comment on attachment 335909 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=335909&action=review > > Works great, r=me with some minor style comments: > > it feel wrong to use the name `this._currentConsoleGroup`, when the value is > always a ConsoleSession object. I know it was already like this, but I think > this would be a good time to change the name. So `_currentConsoleGroup` can actually be a `WI.ConsoleGroup` that is created in `_didRenderConsoleMessageView`. I do think we can use a different name tho. > Also, the public properties `prompt` and `currentConsoleGroup` are not > referenced anywhere, and can be removed. Will do. > > Source/WebInspectorUI/UserInterface/Controllers/JavaScriptLogViewController.js:62 > > + this._pendingMessages = new Map; > > I think `this._pendingMessageViewsForSession` would be more self-explanatory. Agreed.
Created attachment 336172 [details] Patch
Comment on attachment 336172 [details] Patch Clearing flags on attachment: 336172 Committed r229785: <https://trac.webkit.org/changeset/229785>
All reviewed patches have been landed. Closing bug.
<rdar://problem/38693002>
(In reply to Devin Rousso from comment #5) > (In reply to Matt Baker from comment #3) > > Comment on attachment 335909 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=335909&action=review > > > > Works great, r=me with some minor style comments: > > > > it feel wrong to use the name `this._currentConsoleGroup`, when the value is > > always a ConsoleSession object. I know it was already like this, but I think > > this would be a good time to change the name. > So `_currentConsoleGroup` can actually be a `WI.ConsoleGroup` that is > created in `_didRenderConsoleMessageView`. I do think we can use a > different name tho. I missed that, good catch. I think `_currentSessionOrGroup` is an improvement over what we had before.