[webkit-reviews] review denied: [Bug 81786] Support fixed position elements in Qt WebKit2 : [Attachment 133414] 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: Support fixed position elements in Qt WebKit2
https://bugs.webkit.org/show_bug.cgi?id=81786

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

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=133414&action=review


Discussed this on IRC;
I'd rather we don't pass around all this fixed-position information, and
instead do something more aligned with how GraphicsLayer are offseted in
WebCore, by setting a scrollOffset on all of them.

> Source/WebCore/page/FrameView.cpp:1717
> +	   if
(m_frame->page()->settings()->acceleratedCompositingForFixedPositionEnabled())
{
> +	       updateFixedElementsAfterScrolling();
> +	       repaintFixedElementsAfterScrolling();
> +	   }

This probably needs a comment.

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:109
> +    HashMap<int, WebCore::Length> m_fixedPositionInformation;

I don't like using a map for 4 items. Please see previous comment.

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:126
> +    if (m_fixedLayers.size()) {
> +	   LayerMap::iterator end = m_fixedLayers.end();
> +	   for (LayerMap::iterator it = m_fixedLayers.begin(); it != end; ++it)

> +	       it->second->syncCompositingStateForThisLayerOnly();
> +    }

Why do you need to sync these again?

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:167
> +    m_contentsSize = contentsSize;

how is this different from DrawingArea::contentsRect()?

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:196
> +	       adjustedX = visibleEdgeLeft + left.getFloatValue() *
m_visibleContentsRect.width() / 100;

You're doing manually what WebCore::Length can do automatically.

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:120
> +    WebKit::PositionInformationMap& fixedPositionInformation() { return
m_fixedPositionInformation; }
> +    bool hasFixedPosition() const { return m_hasFixedPosition; }
> +    void setHasFixedPosition(bool isFixed) { m_hasFixedPosition = isFixed; }

> +

I'd rather call this
fixedToViewport
and offsetFromViewport