Bug 52406 - The pageScaleFactor() should be saved/restored along with the scroll position
Summary: The pageScaleFactor() should be saved/restored along with the scroll position
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.6
: P2 Normal
Assignee: Mike Thole
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-01-13 15:57 PST by Mike Thole
Modified: 2011-01-13 17:17 PST (History)
1 user (show)

See Also:


Attachments
Proposed patch (4.85 KB, patch)
2011-01-13 16:17 PST, Mike Thole
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Thole 2011-01-13 15:57:50 PST
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>
Comment 1 Mike Thole 2011-01-13 16:17:34 PST
Created attachment 78872 [details]
Proposed patch
Comment 2 Darin Adler 2011-01-13 16:32:14 PST
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.
Comment 3 Brady Eidson 2011-01-13 16:38:44 PST
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.
Comment 4 Early Warning System Bot 2011-01-13 16:49:50 PST
Attachment 78872 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7624007
Comment 5 Mike Thole 2011-01-13 17:17:08 PST
Committed revision 75758.