Bug 127886 - [iOS] Synchronize the WKContentView and UIScrollView updates with the tiles being commited
Summary: [iOS] Synchronize the WKContentView and UIScrollView updates with the tiles b...
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:
Depends on:
Blocks:
 
Reported: 2014-01-29 20:20 PST by Benjamin Poulain
Modified: 2014-02-05 22:40 PST (History)
3 users (show)

See Also:


Attachments
Patch (33.59 KB, patch)
2014-01-29 21:09 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (33.62 KB, patch)
2014-01-30 14:33 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (68.79 KB, patch)
2014-02-05 15:43 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (69.43 KB, patch)
2014-02-05 20:15 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (69.38 KB, patch)
2014-02-05 21:53 PST, 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-01-29 20:20:57 PST
[iOS] Synchronize the WKContentView and UIScrollView updates with the tiles being commited
Comment 1 Benjamin Poulain 2014-01-29 21:09:20 PST
Created attachment 222625 [details]
Patch
Comment 2 Benjamin Poulain 2014-01-30 14:33:56 PST
Created attachment 222738 [details]
Patch
Comment 3 Benjamin Poulain 2014-01-30 15:53:42 PST
Comment on attachment 222738 [details]
Patch

Let's put this one ice. I'll look into changing the viewport handling first.
Comment 4 Tim Horton 2014-01-30 16:20:12 PST
Comment on attachment 222738 [details]
Patch

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

r+ from smfr and I if you do choose to go this route

> Source/WebKit2/ChangeLog:24
> +        1) A source (the page or the user) change parameters of the geometry.

s/change/changes/

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:158
> +    const WebCore::IntSize& renderedPageSize() const { return m_renderedPageSize; }

anders would tell you not to return a const& because otherwise it will fit in a register. or something.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:70
> +    BOOL _hasPendingPageReset;

I don't think reset is the word you want (we're not sure what it means). viewportUpdatePending? smfr says "the terminology should match whatever's called when whatever's pending is no longer pending" :D

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:341
> +        // Do not mess with UIScrollView during using interaction, that causes undefined behaviors.

s/using/user/?

> Source/WebKit2/UIProcess/API/ios/WKContentView.mm:315
>      if ([_delegate respondsToSelector:@selector(contentView:contentsSizeDidChange:)])
>          [_delegate contentView:self contentsSizeDidChange:contentsSize];

we're a bit curious what this is about, and why setBounds is conditional

> Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:278
> +    // It is common to have the view initialized with a null frame. The resulting WebKit viewport configuration

This is really weird.

> Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:286
> +            size = [[UIScreen mainScreen] bounds].size;

Why the screen size? Why is this common? Why does it matter if it does happen? Shouldn't we just adjust the viewport when we get a valid frame?

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:325
> +    layerTransaction.setRenderedPageSize(m_webPage->corePage()->mainFrame().view()->contentsSize());
> +    layerTransaction.setRenderedPageScale(m_webPage->corePage()->pageScaleFactor());

Can we call renderedPageSize mainFrameContentsSize or something? and use the whole name for pageScaleFactor. and I don't think we need the "rendered" part. it's inherently the size and scale that go along with the transaction.
Comment 5 Benjamin Poulain 2014-02-05 15:43:38 PST
Created attachment 223274 [details]
Patch
Comment 6 Simon Fraser (smfr) 2014-02-05 17:10:11 PST
Comment on attachment 223274 [details]
Patch

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

> Source/WebCore/page/VirtualViewport.cpp:51
> +    m_defaultConfiguration.width = 980;
> +    m_defaultConfiguration.widthIsSet = true;
> +    m_defaultConfiguration.allowsUserScaling = true;
> +    m_defaultConfiguration.minimumScale = 0.25;
> +    m_defaultConfiguration.maximumScale = 5;

The nicer way to do this would be to give VirtualViewportConfiguration a member function that returns a pre-populated VirtualViewportConfiguration with these values.

> Source/WebCore/page/VirtualViewport.cpp:60
> +    ASSERT(virtualViewportConfiguration.maximumScale >= virtualViewportConfiguration.minimumScale);

Why isn't this assertion inside of VirtualViewportConfiguration?

> Source/WebCore/page/VirtualViewport.cpp:151
> +double VirtualViewport::maximumScale() const
> +{
> +    return m_configuration.maximumScale;
> +}
> +
> +bool VirtualViewport::allowsUserScaling() const
> +{
> +    return m_configuration.allowsUserScaling;
> +}

Could be inline.

> Source/WebCore/page/VirtualViewport.h:38
> +        bzero(this, sizeof(VirtualViewportConfiguration));

We normally initialize the members explicitly.

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:159
> +    WebCore::IntSize mainFrameContentsSize() const { return m_mainFrameContentsSize; }
> +    void setMainFrameContentsSize(const WebCore::IntSize& size) { m_mainFrameContentsSize = size; };

Not sure you need "main frame" in the name. Just contentsSize would be fine.

> Source/WebKit2/UIProcess/API/ios/WKContentView.h:49
> +- (void)contentView:(WKContentView *)contentView didCommitLayerTree:(const WebKit::RemoteLayerTreeTransaction&)layerTreeTransaction;

It's a bit gross to see an internal detail of layer tree committing (RemoteLayerTreeTransaction) here. Can you just pass the values you care about?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3994
> +    defaultConfiguration.width = 980;
> +    defaultConfiguration.widthIsSet = true;
> +    defaultConfiguration.allowsUserScaling = true;
> +    defaultConfiguration.minimumScale = 0.25;
> +    defaultConfiguration.maximumScale = 5;

This is the same as the VirtualViewport::VirtualViewport() one!
Comment 7 Benjamin Poulain 2014-02-05 20:15:37 PST
Created attachment 223295 [details]
Patch
Comment 8 Benjamin Poulain 2014-02-05 21:53:02 PST
Created attachment 223304 [details]
Patch
Comment 9 Benjamin Poulain 2014-02-05 22:40:54 PST
Committed r163515: <http://trac.webkit.org/changeset/163515>