Bug 133364 - [iOS][WK2] When a page does not finish rotation before the end of the animation, synchronize explicitly
Summary: [iOS][WK2] When a page does not finish rotation before the end of the animati...
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-05-28 16:43 PDT by Benjamin Poulain
Modified: 2014-05-30 17:57 PDT (History)
2 users (show)

See Also:


Attachments
Patch (12.65 KB, patch)
2014-05-28 16:50 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (11.61 KB, patch)
2014-05-29 15:35 PDT, Benjamin Poulain
sam: review+
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-05-28 16:43:39 PDT
[iOS][WK2] When a page does not finish rotation before the end of the animation, synchronize explicitely
Comment 1 Benjamin Poulain 2014-05-28 16:50:54 PDT
Created attachment 232221 [details]
Patch
Comment 2 Benjamin Poulain 2014-05-28 16:54:11 PDT
<rdar://problem/17026333>
Comment 3 Tim Horton 2014-05-28 17:09:44 PDT
Comment on attachment 232221 [details]
Patch

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

> Source/WebKit2/ChangeLog:9
> +        When a page that relayout on rotation does not respond before the end of the animation, we end up with

maybe "does layout" instead of "relayout"? there's a mismatch here.

> Source/WebKit2/ChangeLog:10
> +        a completely inconistent state on the UIProcess (because it is unware of the new states).

typo inconsistent

> Source/WebKit2/ChangeLog:15
> +        This patch force a synchronous synchronization whenever we finish the animation before we heard back

redundant "synchronous synchronization" :D

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:249
> +        WebCore::FloatPoint newScrollPosition;

no WebCore::

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:250
> +        if (m_process->sendSync(Messages::WebPage::SynchronizeDynamicViewportUpdate(), Messages::WebPage::SynchronizeDynamicViewportUpdate::Reply(newScale, newScrollPosition), m_pageID))

please check this with Sam

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2149
> +void WebPage::synchronizeDynamicViewportUpdate(double& newTargetScale, WebCore::FloatPoint& newScrollPosition)

no WebCore here either
Comment 4 Benjamin Poulain 2014-05-28 23:03:30 PDT
Comment on attachment 232221 [details]
Patch

I'll fix from the comments and see how ugly it would be to sync the update with the incoming frame.
Comment 5 Benjamin Poulain 2014-05-29 15:35:35 PDT
Created attachment 232268 [details]
Patch
Comment 6 Sam Weinig 2014-05-30 15:16:18 PDT
Comment on attachment 232268 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:257
> +    if (m_dynamicViewportSizeUpdateInProgress) {
> +        m_dynamicViewportSizeUpdateInProgress = false;
> +        double newScale;
> +        FloatPoint newScrollPosition;
> +        if (m_process->sendSync(Messages::WebPage::SynchronizeDynamicViewportUpdate(), Messages::WebPage::SynchronizeDynamicViewportUpdate::Reply(newScale, newScrollPosition), m_pageID, std::chrono::seconds(2))) {
> +            m_pageClient.dynamicViewportUpdateChangedTarget(newScale, newScrollPosition);
> +
> +            m_process->connection()->waitForAndDispatchImmediately<Messages::RemoteLayerTreeDrawingAreaProxy::CommitLayerTree>(m_pageID, std::chrono::seconds(1));
> +        }
> +    }
> +}

I would add a comment (and bugzilla link?) about how we could do this without a sync message in the future using transactions.
Comment 7 Benjamin Poulain 2014-05-30 17:57:48 PDT
Committed r169501: <http://trac.webkit.org/changeset/169501>