[webkit-reviews] review denied: [Bug 75576] [Chromium] Do not recompute viewport on same page navigation : [Attachment 121163] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Fady Samuel
<fsamuel at chromium.org>'s request for review:
Bug 75576: [Chromium] Do not recompute viewport on same page navigation
https://bugs.webkit.org/show_bug.cgi?id=75576

Attachment 121163: Patch
https://bugs.webkit.org/attachment.cgi?id=121163&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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?