Bug 75576 - [Chromium] Do not recompute viewport on same page navigation
Summary: [Chromium] Do not recompute viewport on same page navigation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fady Samuel
URL:
Keywords:
: 74155 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-01-04 14:29 PST by Fady Samuel
Modified: 2012-01-24 14:53 PST (History)
4 users (show)

See Also:


Attachments
Patch (3.65 KB, patch)
2012-01-04 14:31 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (3.54 KB, patch)
2012-01-04 15:00 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (3.70 KB, patch)
2012-01-05 14:28 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (4.38 KB, patch)
2012-01-16 15:18 PST, Fady Samuel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2012-01-04 14:29:08 PST
[Chromium] Do not recompute viewport on same page navigation
Comment 1 Fady Samuel 2012-01-04 14:31:36 PST
Created attachment 121157 [details]
Patch
Comment 2 Fady Samuel 2012-01-04 15:00:08 PST
Created attachment 121163 [details]
Patch
Comment 3 Fady Samuel 2012-01-04 15:01:58 PST
Updated based on conversation with aelias. Thanks! :)
Comment 4 Alexandre Elias 2012-01-04 20:17:08 PST
Looks good to me, thanks.
Comment 5 Darin Fisher (:fishd, Google) 2012-01-05 12:36:02 PST
Comment on attachment 121163 [details]
Patch

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

> Source/WebKit/chromium/src/BackForwardListChromium.cpp:62
> +    if ((item && !m_currentItem) || (item && m_currentItem && !equalIgnoringFragmentIdentifier(item->url(), m_currentItem->url())))

Given history.pushState, which can change more than just the reference fragment of
the URL but keep the document the same, performing equalIgnoringFragmentIdentifier
checks like this is almost always wrong.  You probably want to compare
HistoryItem::documentSequenceNumber instead.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2693
> +    // Only reset this flag one on a new page navigation.

typo?  "flag one on a new"?

> Source/WebKit/chromium/src/WebViewImpl.cpp:2696
> +    m_pageScaleFactorIsSet = false;

you can't just put this code in WebViewImpl::didCommitLoad ?

> Source/WebKit/chromium/src/WebViewImpl.h:303
> +    void observeNewPageNavigation();

observeNewPageNavigation sounds way too much like observeNewNavigation.  what do you really mean?
Comment 6 Alexandre Elias 2012-01-05 12:58:00 PST
(In reply to comment #5)
> (From update of attachment 121163 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121163&action=review
> 
> > Source/WebKit/chromium/src/BackForwardListChromium.cpp:62
> > +    if ((item && !m_currentItem) || (item && m_currentItem && !equalIgnoringFragmentIdentifier(item->url(), m_currentItem->url())))
> 
> Given history.pushState, which can change more than just the reference fragment of
> the URL but keep the document the same, performing equalIgnoringFragmentIdentifier
> checks like this is almost always wrong.  You probably want to compare
> HistoryItem::documentSequenceNumber instead.

That sounds good to me.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:2696
> > +    m_pageScaleFactorIsSet = false;
> 
> you can't just put this code in WebViewImpl::didCommitLoad ?

didCommitLoad lacks information about whether the navigation went to a different document.  If that information were pushed there, then didCommitLoad would work.

> 
> > Source/WebKit/chromium/src/WebViewImpl.h:303
> > +    void observeNewPageNavigation();
> 
> observeNewPageNavigation sounds way too much like observeNewNavigation.  what do you really mean?

"observeDifferentDocumentNavigation" might be a more descriptive name.  The intent of this change is to initialize the page scale factor to minimum on initial visit of any document without a viewport tag (and never again thereafter).  We clear this boolean to false when navigating to a new document as a note that we need to initialize the page scale as soon as we learn the content width.  But we don't want to touch the boolean if it's a same-page navigation.
Comment 7 Fady Samuel 2012-01-05 14:28:39 PST
Created attachment 121330 [details]
Patch
Comment 8 Fady Samuel 2012-01-05 14:29:50 PST
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 121163 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=121163&action=review
> > 
> > > Source/WebKit/chromium/src/BackForwardListChromium.cpp:62
> > > +    if ((item && !m_currentItem) || (item && m_currentItem && !equalIgnoringFragmentIdentifier(item->url(), m_currentItem->url())))
> > 
> > Given history.pushState, which can change more than just the reference fragment of
> > the URL but keep the document the same, performing equalIgnoringFragmentIdentifier
> > checks like this is almost always wrong.  You probably want to compare
> > HistoryItem::documentSequenceNumber instead.
> 
> That sounds good to me.
> 
> > > Source/WebKit/chromium/src/WebViewImpl.cpp:2696
> > > +    m_pageScaleFactorIsSet = false;
> > 
> > you can't just put this code in WebViewImpl::didCommitLoad ?
> 
> didCommitLoad lacks information about whether the navigation went to a different document.  If that information were pushed there, then didCommitLoad would work.
> 
> > 
> > > Source/WebKit/chromium/src/WebViewImpl.h:303
> > > +    void observeNewPageNavigation();
> > 
> > observeNewPageNavigation sounds way too much like observeNewNavigation.  what do you really mean?
> 
> "observeDifferentDocumentNavigation" might be a more descriptive name.  The intent of this change is to initialize the page scale factor to minimum on initial visit of any document without a viewport tag (and never again thereafter).  We clear this boolean to false when navigating to a new document as a note that we need to initialize the page scale as soon as we learn the content width.  But we don't want to touch the boolean if it's a same-page navigation.

Updated patch according to comments above.
Comment 9 Darin Fisher (:fishd, Google) 2012-01-12 11:04:09 PST
(In reply to comment #6)
> > > Source/WebKit/chromium/src/WebViewImpl.cpp:2696
> > > +    m_pageScaleFactorIsSet = false;
> > 
> > you can't just put this code in WebViewImpl::didCommitLoad ?
> 
> didCommitLoad lacks information about whether the navigation went to a different document.  If that information were pushed there, then didCommitLoad would work.

It seems trivial to add such a boolean.  I would do that.  See the two callsites in FrameLoaderClientImpl.cpp.  Name the boolean isNavigationWithinPage.
Comment 10 Fady Samuel 2012-01-16 15:18:41 PST
Created attachment 122688 [details]
Patch
Comment 11 WebKit Review Bot 2012-01-20 11:08:53 PST
Comment on attachment 122688 [details]
Patch

Clearing flags on attachment: 122688

Committed r105528: <http://trac.webkit.org/changeset/105528>
Comment 12 WebKit Review Bot 2012-01-20 11:09:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Fady Samuel 2012-01-24 14:53:12 PST
*** Bug 74155 has been marked as a duplicate of this bug. ***