Bug 122062 – Web Inspector: content view in back/forward list multiple times won't restore earlier positions

• Component: Web Inspector
• Status: RESOLVED FIXED

[Brian Burg]

The inspector implicitly saves content view's last-used scroll positions, but doesn't save multiple positions per content view. If the same content view has multiple entries in the back/forward list, such entries further back in the history which were associated with a specific line number/position will not have that position restored.

[Radar WebKit Bug Importer]

<rdar://problem/15105797>

[Brian Burg]

Created attachment 212900 [details]
Patch

[Brian Burg]

I extracted BackForwardEntry in the hopes that more functionality would be moved into there, such as taking different restore actions based on ContentView subclass. I'm not sure if that would make this less coupled than the current version which stores an arbitrary callback.

In the next few patches, I want to merge BackForwardEntry with the cookies used for restoring across inspector close/opens.

[Timothy Hatcher]

Comment on attachment 212900 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/ContentViewContainer.js:152
> +        if (currentEntry && currentEntry.contentView === contentView && Object.shallowEqual(currentEntry.cookie, newEntry.cookie))

Object.shallowEqual will not compare the positionToReveal or textRangeToSelect objects. So a navigation to the same location will push another entry. Perhaps we need a more advanced Object.shallowEqual that does an a.equals(b) if the objects are the same type/constructor and has an equals function.

> Source/WebInspectorUI/UserInterface/ContentViewContainer.js:440
> +        entry.restorePositions();
>          this._restoreScrollPositionsForContentView(contentView);

Should these lines flip, or do they not interfere in practice?

> Source/WebInspectorUI/UserInterface/ResourceSidebarPanel.js:266
> +        var cookie = {};
> +        cookie.positionToReveal = positionToReveal;
> +        cookie.textRangeToSelect = textRangeToSelect;
> +        cookie.forceUnformatted = forceUnformatted;

This could be one line. I'm not sure textRangeToSelect should be in the cookie. That doesn't get restored in Xcode when going back/forward. Also forceUnformatted seems weird to trigger. I would only expect position.

Also the position will highlight/flash when revealed. That might be weird on back/forward vs a direct navigation.

> Source/WebInspectorUI/UserInterface/ResourceSidebarPanel.js:267
> +        var restoreCallback = function(contentView, cookie) {

I'd put a newline before this line to separate out the function so it isn't as hidden in a large block.

[Timothy Hatcher]

Comment on attachment 212900 [details]
Patch

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

Looks good. The Object.shallowEqual issue is the only big issue.

> Source/WebInspectorUI/UserInterface/BackForwardEntry.js:36
> +    restorePositions: function()

I think restoreCookie would be a better name. There is nothing about "positions" that this explicitly restores.

[Timothy Hatcher]

(In reply to comment #3)
> I extracted BackForwardEntry in the hopes that more functionality would be moved into there, such as taking different restore actions based on ContentView subclass. I'm not sure if that would make this less coupled than the current version which stores an arbitrary callback.
> 
> In the next few patches, I want to merge BackForwardEntry with the cookies used for restoring across inspector close/opens.

Where is the BackForwardEntry patch?

Making ContentViewController aware of specific ContentView subclasses would be a layering violation. The callback makes sense with how it is used by the sidebar. I do worry there are places where a resource content view will be shown that does not go through the sidebar and it will miss out of restore functionality. Adding a generic content view interface that each view can implement to provide a cookie and restore would be another approach. But getting the cookie from the contentView up-front to compare before it is shown would be a challenge.

[Brian Burg]

(In reply to comment #4)
> (From update of attachment 212900 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212900&action=review
> 
> > Source/WebInspectorUI/UserInterface/ContentViewContainer.js:152
> > +        if (currentEntry && currentEntry.contentView === contentView && Object.shallowEqual(currentEntry.cookie, newEntry.cookie))
> 
> Object.shallowEqual will not compare the positionToReveal or textRangeToSelect objects. So a navigation to the same location will push another entry. Perhaps we need a more advanced Object.shallowEqual that does an a.equals(b) if the objects are the same type/constructor and has an equals function.

Oh, good catch. Sometimes position is just a line number integer, other times a SourceCodePosition instance.

> 
> > Source/WebInspectorUI/UserInterface/ContentViewContainer.js:440
> > +        entry.restorePositions();
> >          this._restoreScrollPositionsForContentView(contentView);
> 
> Should these lines flip, or do they not interfere in practice?

It's not clear that we should restore scroll positions when moving through entries that were created by clicking on links to specific lines. If you imagine the case when the back and forward history can be visualized, it would be confusing to see script.js:999, jump to it, but instead see some other position. I think Xcode does not save scroll positions for this reason.

> > Source/WebInspectorUI/UserInterface/ResourceSidebarPanel.js:266
> > +        var cookie = {};
> > +        cookie.positionToReveal = positionToReveal;
> > +        cookie.textRangeToSelect = textRangeToSelect;
> > +        cookie.forceUnformatted = forceUnformatted;
> 
> This could be one line. I'm not sure textRangeToSelect should be in the cookie. That doesn't get restored in Xcode when going back/forward. Also forceUnformatted seems weird to trigger. I would only expect position.

OK

> Also the position will highlight/flash when revealed. That might be weird on back/forward vs a direct navigation.

I didn't find it overly distracting. In fact it might be desired since the navigated-to line is not always revealed at the same vertical position within the editor.

> > Source/WebInspectorUI/UserInterface/ResourceSidebarPanel.js:267
> > +        var restoreCallback = function(contentView, cookie) {
> 
> I'd put a newline before this line to separate out the function so it isn't as hidden in a large block.

OK

[Brian Burg]

(In reply to comment #5)
> (From update of attachment 212900 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212900&action=review
>
> > Source/WebInspectorUI/UserInterface/BackForwardEntry.js:36
> > +    restorePositions: function()
> 
> I think restoreCookie would be a better name. There is nothing about "positions" that this explicitly restores.

OK

[Brian Burg]

> > In the next few patches, I want to merge BackForwardEntry with the cookies used for restoring across inspector close/opens.
> 
> Where is the BackForwardEntry patch?

Oops, I meant to say "bug(s)". See https://bugs.webkit.org/show_bug.cgi?id=122063
 
> Making ContentViewController aware of specific ContentView subclasses would be a layering violation. The callback makes sense with how it is used by the sidebar. I do worry there are places where a resource content view will be shown that does not go through the sidebar and it will miss out of restore functionality. Adding a generic content view interface that each view can implement to provide a cookie and restore would be another approach. But getting the cookie from the contentView up-front to compare before it is shown would be a challenge.

So I'll keep the callback for now, but move to the cookie interface used for preserving content views across inspector close/open.

[Brian Burg]

Created attachment 212954 [details]
Take 2

[Timothy Hatcher]

Comment on attachment 212954 [details]
Take 2

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

> Source/WebInspectorUI/ChangeLog:60
> +        Need a short description (OOPS!).
> +        Need the bug URL (OOPS!).

OOPS.

> Source/WebInspectorUI/UserInterface/BackForwardEntry.js:37
> +WebInspector.BackForwardEntry.prototype = {
> +    constructor: WebInspector.BackForwardEntry,

Even if it isn't useful now, this should still inherit from WebInspector.Object.

> Source/WebInspectorUI/UserInterface/ResourceSidebarPanel.js:264
> +        var cookie = { lineNumber: positionToReveal.lineNumber, columnNumber: positionToReveal.columnNumber};

Nit, space after { should be removed.

[WebKit Commit Bot]

Comment on attachment 212954 [details]
Take 2

Rejecting attachment 212954 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 212954, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebInspectorUI/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/2956007

[Brian Burg]

Created attachment 213108 [details]
Take 2.1

[WebKit Commit Bot]

Comment on attachment 213108 [details]
Take 2.1

Rejecting attachment 213108 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 213108, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ucceeded at 393 (offset 2 lines).
1 out of 13 hunks FAILED -- saving rejects to file Source/WebInspectorUI/UserInterface/ContentViewContainer.js.rej
patching file Source/WebInspectorUI/UserInterface/Main.html
patching file Source/WebInspectorUI/UserInterface/Main.js
patching file Source/WebInspectorUI/UserInterface/ResourceSidebarPanel.js

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Timothy Hatcher']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/2832120

[Brian Burg]

I'm not sure why commit-queue is angry as I just rebased, I'll look at it tomorrow.

[Brian Burg]

Created attachment 213203 [details]
Take 2.1 (rebased)

[WebKit Commit Bot]

Comment on attachment 213203 [details]
Take 2.1 (rebased)

Clearing flags on attachment: 213203

Committed r156809: <http://trac.webkit.org/changeset/156809>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.