Bug 103428 – [WK2] TiledBackingStore: Frame view re-layouts with wrong Fixed Visible Content Rect.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Mikhail Pozdnyakov]

WebPage re-layouts its view with wrong Fixed Visible Content Rect. This happens because Fixed Visible Content Rect is set with a delay by LayerTreeCoordinator when layout is already done. As a result have failing css3/device-adapt/opera/constrain-006(7).xhtml tests and maybe others.

[Thiago Marcos P. Santos]

*** Bug 102922 has been marked as a duplicate of this bug. ***

[Mikhail Pozdnyakov]

Created attachment 176489 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 176489 [details]
patch

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

> Source/WebKit2/UIProcess/DrawingAreaProxy.h:91
> +    virtual void setFixedLayoutSize(const WebCore::IntSize& /*layoutSize*/, const WebCore::FloatRect& /*visibleContentsRect*/, float /*scale*/) { }

space after /* and before */

> Source/WebKit2/UIProcess/PageViewportController.cpp:223
> -    drawingArea->setVisibleContentsRect(visibleContentsRect, m_effectiveScale, trajectoryVector);
> +    if (trajectoryVector == FloatPoint::zero()) {
> +        IntSize fixedLayoutSize = IntSize(static_cast<int>(m_rawAttributes.layoutSize.width()), static_cast<int>(m_rawAttributes.layoutSize.height()));
> +        drawingArea->setFixedLayoutSize(fixedLayoutSize, visibleContentsRect, m_effectiveScale);
> +    } else
> +        drawingArea->setVisibleContentsRect(visibleContentsRect, m_effectiveScale, trajectoryVector);
>  

It is zero in other cases than when setting new fixed layout size. For instance when panning ends.

Why not replace syncVisibleContents() inside didChangeViewportAttributes instead?

IntSize fixedLayoutSize = IntSize(static_cast<int>(m_rawAttributes.layoutSize.width()), static_cast<int>(m_rawAttributes.layoutSize.height()));
drawingArea->setFixedLayoutSize(fixedLayoutSize, visibleContentsRect, m_effectiveScale);

Also why not

IntSize fixedLayoutSize = roundedIntRect(m_rawAttributes.layoutSize);

Or

drawingArea->setFixedLayoutSize(roundedIntRect(m_rawAttributes.layoutSize), visibleContentsRect, m_effectiveScale);


Notice that in the code above m_contentsSize cannot be trusted, it is from the old content (old layout sizes)

I guess you should use:

FloatRect visibleContentsRect(FloatPoint(), viewportSizeInContentsCoordinates());
drawingArea->setFixedLayoutSize(roundedIntRect(m_rawAttributes.layoutSize), visibleContentsRect, m_effectiveScale);

[Noam Rosenthal]

Comment on attachment 176489 [details]
patch

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

> Source/WebKit2/UIProcess/PageViewportController.cpp:218
> +    if (trajectoryVector == FloatPoint::zero()) {

Can you explain this in a comment?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:729
> +void LayerTreeCoordinator::setFixedLayoutSize(const IntSize& layoutSize, const IntRect& rect, float scale)
> +{
> +    setVisibleContentsRect(rect, scale, FloatPoint());
> +    m_webPage->setFixedLayoutSize(layoutSize);
> +}

This is a bit messy. Why not have this message in WebPage, and have WebPage call layerTreeCoordinator->setVisibleContentsRect?
I prefer it if LayerTreeCoordinator dealt with viewport/graphics only and not with layout magic.

[Noam Rosenthal]

(In reply to comment #4)
> (From update of attachment 176489 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176489&action=review
> 
> > Source/WebKit2/UIProcess/PageViewportController.cpp:218
> > +    if (trajectoryVector == FloatPoint::zero()) {
> 
> Can you explain this in a comment?
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:729
> > +void LayerTreeCoordinator::setFixedLayoutSize(const IntSize& layoutSize, const IntRect& rect, float scale)
> > +{
> > +    setVisibleContentsRect(rect, scale, FloatPoint());
> > +    m_webPage->setFixedLayoutSize(layoutSize);
> > +}
> 
> This is a bit messy. Why not have this message in WebPage, and have WebPage call layerTreeCoordinator->setVisibleContentsRect?
> I prefer it if LayerTreeCoordinator dealt with viewport/graphics only and not with layout magic.

but seems like Kenneth's comments and mine might collide... his are better :)

[Mikhail Pozdnyakov]

(In reply to comment #3)
> (From update of attachment 176489 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176489&action=review
> 
> Why not replace syncVisibleContents() inside didChangeViewportAttributes instead?
> 
Ah indeed! this is much much better, thanks!

[Kenneth Rohde Christiansen]

Comment on attachment 176489 [details]
patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:-1029
> -    // This also takes care of the relayout.
> -    setFixedLayoutSize(IntSize(static_cast<int>(attr.layoutSize.width()), static_cast<int>(attr.layoutSize.height())));

So this is wrong; we need to do the layout immediate when we get the viewport info, because otherwise it is going to layout using old info.

Say it parses the document and finds the viewport meta tag, then calls then and then continues to layout.

Instead we should make sure here that we update the visibleContentsRect as well.

You know the scale from the viewport info (will require a few changes) and you know the viewport size m_viewportSize. You also have the current position from the previous visibleContentsRect, but it should be set to (0, 0) if page->layoutMilestones() == 0; (no layout was done yet)

[Mikhail Pozdnyakov]

Created attachment 176936 [details]
patch v2

New approach is applied, set contents fixed rect before the layout and still layout immediately after viewport attrs are changed.

[Kenneth Rohde Christiansen]

Comment on attachment 176936 [details]
patch v2

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

> Source/WebKit2/UIProcess/PageViewportController.cpp:237
>      if (updateMinimumScaleToFit(true))
>          m_client->didChangeViewportAttributes();
> -
> -    syncVisibleContents();
>  }

Maybe a comment here some how? that this is not needed

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1032
> +    // Keep the current position, update size only.
> +    FrameView* view = m_page->mainFrame()->view();
> +    IntPoint contentFixedOrigin = view->fixedVisibleContentRect().location();

It should be mentioned here that this is indeed (0, 0) for new loads and not the position for the previous page

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1041
> -    setFixedLayoutSize(IntSize(static_cast<int>(attr.layoutSize.width()), static_cast<int>(attr.layoutSize.height())));
> +    setFixedLayoutSize(roundedIntSize(attr.layoutSize));

much nicer

[Kenneth Rohde Christiansen]

Comment on attachment 176936 [details]
patch v2

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1036
> +    IntSize contentFixedSize = m_viewportSize;
> +    contentFixedSize.scale(1 / m_page->deviceScaleFactor());

This should further more be at the current scale!
1 / (deviceScaleFactor() * attr.initialScale)

[Mikhail Pozdnyakov]

Created attachment 176945 [details]
patch v3

[WebKit Review Bot]

Comment on attachment 176945 [details]
patch v3

Clearing flags on attachment: 176945

Committed r136231: <http://trac.webkit.org/changeset/136231>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Thiago Marcos P. Santos]

This patch made css3/device-adapt/viewport-width-not-affecting-next-page.html flaky.