Bug 168622 - Web Inspector: Session dividers are not added when Console tab is not visible
Summary: Web Inspector: Session dividers are not added when Console tab is not visible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-02-20 16:21 PST by Devin Rousso
Modified: 2018-03-20 20:42 PDT (History)
4 users (show)

See Also:


Attachments
[HTML] Reduction (30 bytes, text/html)
2017-02-20 16:21 PST, Devin Rousso
no flags Details
[Image] Screenshot of Issue (241.23 KB, image/png)
2017-02-20 16:22 PST, Devin Rousso
no flags Details
Patch (6.46 KB, patch)
2018-03-15 16:43 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (9.68 KB, patch)
2018-03-20 19:40 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2017-02-20 16:21:40 PST
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
Comment 1 Devin Rousso 2017-02-20 16:22:37 PST
Created attachment 302197 [details]
[Image] Screenshot of Issue
Comment 2 Devin Rousso 2018-03-15 16:43:57 PDT
Created attachment 335909 [details]
Patch
Comment 3 Matt Baker 2018-03-20 18:07:50 PDT
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 4 Matt Baker 2018-03-20 18:16:00 PDT
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.
Comment 5 Devin Rousso 2018-03-20 19:30:58 PDT
(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.
Comment 6 Devin Rousso 2018-03-20 19:40:39 PDT
Created attachment 336172 [details]
Patch
Comment 7 WebKit Commit Bot 2018-03-20 20:40:21 PDT
Comment on attachment 336172 [details]
Patch

Clearing flags on attachment: 336172

Committed r229785: <https://trac.webkit.org/changeset/229785>
Comment 8 WebKit Commit Bot 2018-03-20 20:40:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-03-20 20:41:31 PDT
<rdar://problem/38693002>
Comment 10 Matt Baker 2018-03-20 20:42:52 PDT
(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.