[Webkit-unassigned] [Bug 122062] Web Inspector: content view in back/forward list multiple times won't restore earlier positions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=122062





--- Comment #7 from Brian Burg <burg at cs.washington.edu>  2013-09-28 10:09:35 PST ---
(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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.