[webkit-reviews] review denied: [Bug 81786] Initial support fixed position elements in Qt WebKit2 : [Attachment 135747] Patch.

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Yael
<yael.aharon at nokia.com>'s request for review:
Bug 81786: Initial support fixed position elements in Qt WebKit2
https://bugs.webkit.org/show_bug.cgi?id=81786

Attachment 135747: Patch.
https://bugs.webkit.org/attachment.cgi?id=135747&action=review

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
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