[Webkit-unassigned] [Bug 81786] Initial support fixed position elements in Qt WebKit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=81786


Noam Rosenthal <noam.rosenthal at nokia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #135747|review?                     |review-
               Flag|                            |




--- Comment #37 from Noam Rosenthal <noam.rosenthal at nokia.com>  2012-04-05 07:49:19 PST ---
(From update of attachment 135747)
View in context: https://bugs.webkit.org/attachment.cgi?id=135747&action=review

This looks much better, but it doesn't cover cases where a fixed layer changes style and becomes un-fixed. I think the test should cover that as well.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:514
> +void TextureMapperLayer::setScrollPositionDelta(const IntPoint& delta)

Delta -> Offset

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:518
> +    m_transform.setPosition(m_state.pos + m_scrollPositionDelta);

Add a scrollOffset to LayerTransform instead of making this calculation here.
This would break when the position changes without the delta changing.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:130
> +    bool fixedToViewport() const { return m_state.fixedToViewport; }
> +    void setFixedToViewport(bool isFixed) { m_state.fixedToViewport = isFixed; }

You're not using this for anything.

> Source/WebKit2/UIProcess/LayerTreeHostProxy.cpp:141
> +    m_renderer->didUpdateScrollPosition(position);

This doesn't look thread safe. It should go through
dispatchUpdate(bind(...))

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:78
> +    IntSize size(static_cast<int>(contentSize.width()), static_cast<int>(contentSize.height()));

You don't need the static_casts.

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:187
> +void WebLayerTreeRenderer::adjustPositionForAllFixedLayers()

adjustPositionForFixedLayers

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:189
> +    IntPoint scrollPosition = normalizedScrollPosition(m_visibleContentsRect.location(), m_visibleContentsRect, m_contentsSize);

Return early if m_fixedLayers is empty

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:228
> +        m_fixedLayers.remove(layerInfo.id);

Here you should also reset the delta/offset... I'm not sure you're testing this with a layer that's fixed and becoming un-fixed.

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.h:120
> +    WebCore::IntPoint m_webScrollPosition;
> +    WebCore::IntPoint m_pendingWebScrollPosition;

m_renderedContentsScrollPosition
m_pendingRenderedContentsScrollPosition

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:305
> +    if (!m_webPage->mainFrame()->view()->hasFixedObjects())
> +        return;

You can only return here if this was true in the previous sync; otherwise if you've had one layer that was fixed, and then it became unfixed, you wouldn't sync that change.

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:507
> +        m_scrollPositionOfLastUpdateWasSent = false;

m_shouldSendScrollPositionUpdate = true

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.h:116
> +    bool m_scrollPositionOfLastUpdateWasSent;

m_shouldSendScrollPositionUpdate

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.