Bug 103428

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

Created attachment 176489 [details] patch

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);

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.

(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 :)

(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!

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)

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.

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

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)

Created attachment 176945 [details] patch v3

Comment on attachment 176945 [details] patch v3 Clearing flags on attachment: 176945 Committed r136231: <http://trac.webkit.org/changeset/136231>

All reviewed patches have been landed. Closing bug.

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