[webkit-reviews] review granted: [Bug 52406] The pageScaleFactor() should be saved/restored along with the scroll position : [Attachment 78872] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Mike Thole <mthole at mikethole.com>'s
request for review:
Bug 52406: The pageScaleFactor() should be saved/restored along with the scroll
position
https://bugs.webkit.org/show_bug.cgi?id=52406

Attachment 78872: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=78872&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=78872&action=review

HistoryItem.h says that you should contact the folks working on the Qt port any
time you add a data member to HistoryItem class. Please do so.

> Source/WebCore/history/HistoryItem.cpp:140
> +    , m_pageScaleFactor(item.m_pageScaleFactor)

The other HistoryItem constructors need to initialize the page scale factor to
something so it doesn’t contain random bits. Look for all the places
m_lastVisitWasHTTPNonGet is initialized. I suppose in theory it doesn’t matter
what the value is, but since the class itself does not guarantee this, I
suggest you put some value in there. You could even consider initializing to
NAN and then asserting the value is not NAN in the setter and the getter.

> Source/WebCore/history/HistoryItem.cpp:383
> +const float HistoryItem::pageScaleFactor() const

This should be "float", not "const float".

> Source/WebCore/history/HistoryItem.cpp:682
> +    encoder.encodeFloat(m_pageScaleFactor);

Adding this to back/forward state may require bumping file format version
number in the WebKit2 code that uses this function. Check with Brady Eidson.