Bug 52406 – The pageScaleFactor() should be saved/restored along with the scroll position

• Component: Page Loading
• Status: RESOLVED FIXED

[Mike Thole]

The pageScaleFactor() should be saved and restored at the same time that the scroll point is saved/restored.  HistoryItems should store the pageScaleFactor and the HistoryController should save/restore the scale factor at the appropriate times.

<rdar://problem/8714412>

[Mike Thole]

Created attachment 78872 [details]
Proposed patch

[Darin Adler]

Comment on attachment 78872 [details]
Proposed patch

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.

[Brady Eidson]

Comment on attachment 78872 [details]
Proposed patch

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

r+ with all those changes.

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

I think it would just be good enough to bump the version number in backForwardTreeEncodingVersion at the top of HistoryItem.cpp

> Source/WebCore/history/HistoryItem.cpp:796
> +    float pageScaleFactor;
> +    if (!decoder.decodeFloat(pageScaleFactor))
> +        return 0;
> +    node->m_pageScaleFactor = pageScaleFactor;

This can just be:
if (!decoder.decodeFloat(node->m_pageScaleFactor))
    return 0;

We only use a temporary on others because they're non-primitives and/or need some other special semantics.

[Early Warning System Bot]

Attachment 78872 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7624007

[Mike Thole]

Committed revision 75758.