Bug 132307 - [iOS][WK2] Restore the scroll position and scale from the HistoryItem (mostly)
Summary: [iOS][WK2] Restore the scroll position and scale from the HistoryItem (mostly)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-04-28 15:30 PDT by Benjamin Poulain
Modified: 2014-07-24 11:50 PDT (History)
1 user (show)

See Also:


Attachments
Patch (12.39 KB, patch)
2014-04-28 15:37 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (11.68 KB, patch)
2014-04-28 18:20 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2014-04-28 15:30:49 PDT
[iOS][WK2] Restore the scroll position and scale from the HistoryItem (mostly)
Comment 1 Benjamin Poulain 2014-04-28 15:37:56 PDT
Created attachment 230328 [details]
Patch
Comment 2 Simon Fraser (smfr) 2014-04-28 16:20:16 PDT
Comment on attachment 230328 [details]
Patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/ios/WebFrameLoaderClientIOS.mm:90
> +            if (page && m_frame->isMainFrame() && currentItem->pageScaleFactor())
> +                m_frame->page()->restorePageState(currentItem->pageScaleFactor(), currentItem->scrollPoint());

Is the currentItem->pageScaleFactor() test useful, or just trying to catch odd uninitialized cases?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:144
> +    FrameView& frameView = *m_page->mainFrame().view();
> +    frameView.setScrollPosition(scrollPosition);

Odd to put FrameView in a local when you only use it once.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:145
> +    m_userHasChangedPageScaleFactor = true;

Why? Add a comment?
Comment 3 Benjamin Poulain 2014-04-28 17:49:26 PDT
(In reply to comment #2)
> (From update of attachment 230328 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230328&action=review
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/ios/WebFrameLoaderClientIOS.mm:90
> > +            if (page && m_frame->isMainFrame() && currentItem->pageScaleFactor())
> > +                m_frame->page()->restorePageState(currentItem->pageScaleFactor(), currentItem->scrollPoint());
> 
> Is the currentItem->pageScaleFactor() test useful, or just trying to catch odd uninitialized cases?

Zero is the default value of HistoryItem.

It is a weird API, I would prefer if HistoryItem would keep a separate state to know if values have been set. Relying on the default pageScaleFactor is odd.
Comment 4 Benjamin Poulain 2014-04-28 18:20:27 PDT
Created attachment 230345 [details]
Patch
Comment 5 Benjamin Poulain 2014-04-28 18:31:12 PDT
Committed r167916: <http://trac.webkit.org/changeset/167916>
Comment 6 Benjamin Poulain 2014-04-28 18:31:57 PDT
Comment on attachment 230345 [details]
Patch

Uploaded this again by accident.
Comment 7 Simon Fraser (smfr) 2014-07-24 11:50:05 PDT
<rdar://problem/16031704>