Bug 122062 - Web Inspector: content view in back/forward list multiple times won't restore earlier positions
Summary: Web Inspector: content view in back/forward list multiple times won't restore...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-09-28 08:10 PDT by Brian Burg
Modified: 2013-10-02 16:32 PDT (History)
5 users (show)

See Also:


Attachments
Patch (12.42 KB, patch)
2013-09-28 08:18 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Take 2 (31.75 KB, patch)
2013-09-29 23:30 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Take 2.1 (27.76 KB, patch)
2013-10-01 12:15 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Take 2.1 (rebased) (27.90 KB, patch)
2013-10-02 14:55 PDT, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2013-09-28 08:10:21 PDT
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.
Comment 1 Radar WebKit Bug Importer 2013-09-28 08:10:47 PDT
<rdar://problem/15105797>
Comment 2 Brian Burg 2013-09-28 08:18:18 PDT
Created attachment 212900 [details]
Patch
Comment 3 Brian Burg 2013-09-28 08:24:17 PDT
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.
Comment 4 Timothy Hatcher 2013-09-28 08:35:26 PDT
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.
Comment 5 Timothy Hatcher 2013-09-28 08:52:10 PDT
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.
Comment 6 Timothy Hatcher 2013-09-28 09:00:26 PDT
(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.
Comment 7 Brian Burg 2013-09-28 10:09:35 PDT
(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
Comment 8 Brian Burg 2013-09-28 10:10:29 PDT
(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
Comment 9 Brian Burg 2013-09-28 10:16:36 PDT
> > 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.
Comment 10 Brian Burg 2013-09-29 23:30:32 PDT
Created attachment 212954 [details]
Take 2
Comment 11 Timothy Hatcher 2013-10-01 10:42:56 PDT
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.
Comment 12 WebKit Commit Bot 2013-10-01 10:44:01 PDT
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
Comment 13 Brian Burg 2013-10-01 12:15:04 PDT
Created attachment 213108 [details]
Take 2.1
Comment 14 WebKit Commit Bot 2013-10-01 12:54:11 PDT
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
Comment 15 Brian Burg 2013-10-01 18:14:59 PDT
I'm not sure why commit-queue is angry as I just rebased, I'll look at it tomorrow.
Comment 16 Brian Burg 2013-10-02 14:55:05 PDT
Created attachment 213203 [details]
Take 2.1 (rebased)
Comment 17 WebKit Commit Bot 2013-10-02 16:31:58 PDT
Comment on attachment 213203 [details]
Take 2.1 (rebased)

Clearing flags on attachment: 213203

Committed r156809: <http://trac.webkit.org/changeset/156809>
Comment 18 WebKit Commit Bot 2013-10-02 16:32:00 PDT
All reviewed patches have been landed.  Closing bug.