Bug 75576

Created attachment 121157 [details] Patch

Created attachment 121163 [details] Patch

Updated based on conversation with aelias. Thanks! :)

Looks good to me, thanks.

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?

(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.

Created attachment 121330 [details] Patch

(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.

(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.

Created attachment 122688 [details] Patch

Comment on attachment 122688 [details] Patch Clearing flags on attachment: 122688 Committed r105528: <http://trac.webkit.org/changeset/105528>

All reviewed patches have been landed. Closing bug.

*** Bug 74155 has been marked as a duplicate of this bug. ***