Web Inspector: ContentViewContainer can have redundant back-forward entries after ContentView close

[Matt Baker]

Summary:
ContentViewContainer can have redundant back-forward entries content view close. The container guards against adding a BackForwardEntry with the same ContentView/cookie as the current entry, but doesn't check that consecutive entries are unique after replacing/splicing an entry in the list.

Note:
This doesn't seem to cause any bad behavior, but it can make ContentViewContainer debugging unnecessarily complicated and would be nice to fix.

Steps to Reproduce:
1. Open Inspector
2. Switch back and forth between two tabs (Debugger/Elements) multiple times.
3. Open Inspector²
4. Console prompt: `WebInspector.tabBrowser._contentViewContainer._backForwardList`
=> Outputs something like:
    BackForwardEntry {_listeners: null, _contentView: ElementsTabContentView, _tombstone: false, _cookie: {}, _scrollPositions: []}
    BackForwardEntry {_listeners: null, _contentView: DebuggerTabContentView, _tombstone: false, _cookie: {}, _scrollPositions: []}
    BackForwardEntry {_listeners: null, _contentView: ElementsTabContentView, _tombstone: false, _cookie: {}, _scrollPositions: []}
    BackForwardEntry {_listeners: null, _contentView: DebuggerTabContentView, _tombstone: false, _cookie: {}, _scrollPositions: []}
    BackForwardEntry {_listeners: null, _contentView: ElementsTabContentView, _tombstone: false, _cookie: {}, _scrollPositions: []}
    BackForwardEntry {_listeners: null, _contentView: DebuggerTabContentView, _tombstone: false, _cookie: {}, _scrollPositions: []}
    ...

5. Close Elements tab
6. Re-run script in Inspector²
=> Expected:
    BackForwardEntry {_listeners: null, _contentView: DebuggerTabContentView, _tombstone: false, _cookie: {}, _scrollPositions: []}
=> Actual:
    BackForwardEntry {_listeners: null, _contentView: DebuggerTabContentView, _tombstone: false, _cookie: {}, _scrollPositions: []}
    BackForwardEntry {_listeners: null, _contentView: DebuggerTabContentView, _tombstone: false, _cookie: {}, _scrollPositions: []}
    BackForwardEntry {_listeners: null, _contentView: DebuggerTabContentView, _tombstone: false, _cookie: {}, _scrollPositions: []}
    ...

[Matt Baker]

Created attachment 301132 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 301132 [details]
Patch

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

r=me

> Source/WebInspectorUI/ChangeLog:3
> +        Web Inspector: ContentViewContainer can have redundant back-forward entries content view close

Please fix the grammar of the bug title. (I see you just did)

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:470
> +    _removeDuplicateBackForwardEntires()

Typo: Entires => Entries

Note that this is only removing duplicate adjacent entries, right?

[Matt Baker]

Comment on attachment 301132 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:470
>> +    _removeDuplicateBackForwardEntires()
> 
> Typo: Entires => Entries
> 
> Note that this is only removing duplicate adjacent entries, right?

Correct. Will rename to _removeAdjacentIdenticalBackForwardEntries() to better express the intent. The term "identical" is used in an existing comment around line 108.

[Matt Baker]

Created attachment 301209 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 301209 [details]
Patch for landing

Clearing flags on attachment: 301209

Committed r212166: <http://trac.webkit.org/changeset/212166>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.