Bug 168622 – Web Inspector: Session dividers are not added when Console tab is not visible

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

[Devin Rousso]

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

[Devin Rousso]

Created attachment 302197 [details]
[Image] Screenshot of Issue

[Devin Rousso]

Created attachment 335909 [details]
Patch

[Matt Baker]

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.

[Matt Baker]

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.

[Devin Rousso]

(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.

[Devin Rousso]

Created attachment 336172 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 336172 [details]
Patch

Clearing flags on attachment: 336172

Committed r229785: <https://trac.webkit.org/changeset/229785>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38693002>

[Matt Baker]

(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.