Bug 81786 – Initial support fixed position elements in Qt WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Yael]

Currently, fixed position elements scroll with the page and get adjusted when scrolling is finished.
Their layout does not take the scale into account, resulting in wrong positioning.
A patch is coming soon.

[Yael]

Created attachment 133057 [details]
Patch

[WebKit Review Bot]

Attachment 133057 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/GraphicsLayer.h:423:  Extra space between PositionInformationMap& and fixedPositionInformation  [whitespace/declaration] [3]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yael]

Created attachment 133058 [details]
Patch

Remove extra space (style error).

[Gyuyoung Kim]

Comment on attachment 133058 [details]
Patch

Attachment 133058 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12034534

[Kenneth Rohde Christiansen]

Comment on attachment 133058 [details]
Patch

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

Any good tests? even manual ones?

> Source/WebCore/platform/graphics/GraphicsLayer.h:421
> +    void setFixedPosition(bool fixed) { m_fixedPosition = fixed; }
> +    bool fixedPosition() const { return m_fixedPosition; }

setUseFixedPosition? setIsFixedPositioned() ?

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:433
> +    PositionInformationMap::iterator end = graphicsLayer->fixedPositionInformation().end();
> +    for (PositionInformationMap::iterator it = graphicsLayer->fixedPositionInformation().begin(); it != end; ++it)
> +        m_state.m_fixedPositionInformation.add(it->first, it->second);

hmmmm, why do you need to copy this?

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:186
>  
> +void WebLayerTreeRenderer::adjustPositionForFixedPositionLayer(WebCore::GraphicsLayer* layer)
> +{
> +    ASSERT(layer->fixedPosition());

This should not need to be called unless we are doing overflow

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:197
> +            adjustedX = visibleEdgeLeft + left->second.getFloatValue() * m_visibleContentsRect.width() / 100;
> +        else // it->second.isFixed()
> +            adjustedX = visibleEdgeLeft + left->second.getFloatValue() * m_contentsScale;

IF you are zooming in the fixed elements will take over teh whole screen. iOS has a limit for this (say 2.0).

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:205
> +        if (right!= map.end()) {
> +            if  (right->second.isPercent())
> +                adjustedX = visibleEdgeRight - right->second.getFloatValue() * m_visibleContentsRect.width() / 100 - layer->size().width();
> +            else // it->second.isFixed()
> +                adjustedX = visibleEdgeRight - right->second.getFloatValue() * m_contentsScale - layer->size().width();

This should be using valueForLength (LengthFunctions.h), I believe

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:213
> +            adjustedY = visibleEdgeTop + top->second.getFloatValue() * m_visibleContentsRect.height() / 100;

top->second is pretty unreadable.

> Source/WebKit2/UIProcess/API/C/WKPreferencesPrivate.h:80
>  // Defaults to false.
> +WK_EXPORT void WKPreferencesSetAcceleratedCompositingForFixedPositionEnabled(WKPreferencesRef, bool);
> +WK_EXPORT bool WKPreferencesGetAcceleratedCompositingForFixedPositionEnabled(WKPreferencesRef);

Why are we exposing this to C? Are Apple guys interesting in using it? I would leave this out of this patch

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:124
> +    webPageProxy->pageGroup()->preferences()->setAcceleratedCompositingForFixedPositionEnabled(true);

Cant we just tie this to the resizeToContents and avoid the preference?

> Source/WebKit2/ChangeLog:10
> +        This is not done for pinching. Only when the pinch gesture is finished, the position is adjusted.

huh? why?

[Yael]

(In reply to comment #5)
Thank you for your review :)

> (From update of attachment 133058 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133058&action=review
> 
> Any good tests? even manual ones?
I will add my test page as a manual test.

> 
> > Source/WebCore/platform/graphics/GraphicsLayer.h:421
> > +    void setFixedPosition(bool fixed) { m_fixedPosition = fixed; }
> > +    bool fixedPosition() const { return m_fixedPosition; }
> 
> setUseFixedPosition? setIsFixedPositioned() ?
> 
setUseFixedPosition() sound good to me :)

> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:433
> > +    PositionInformationMap::iterator end = graphicsLayer->fixedPositionInformation().end();
> > +    for (PositionInformationMap::iterator it = graphicsLayer->fixedPositionInformation().begin(); it != end; ++it)
> > +        m_state.m_fixedPositionInformation.add(it->first, it->second);
> 
> hmmmm, why do you need to copy this?
At the moment, it is not really used. I'll remove that.

> 
> > Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:186
> >  
> > +void WebLayerTreeRenderer::adjustPositionForFixedPositionLayer(WebCore::GraphicsLayer* layer)
> > +{
> > +    ASSERT(layer->fixedPosition());
> 
> This should not need to be called unless we are doing overflow
We need to make sure we are in sync during flick scroll too. 

> 
> > Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:197
> > +            adjustedX = visibleEdgeLeft + left->second.getFloatValue() * m_visibleContentsRect.width() / 100;
> > +        else // it->second.isFixed()
> > +            adjustedX = visibleEdgeLeft + left->second.getFloatValue() * m_contentsScale;
> 
> IF you are zooming in the fixed elements will take over teh whole screen. iOS has a limit for this (say 2.0).
I did not observe a limit with ios 5.1 and my test page. We can add a limit later, if we think it is needed, though :)

> 
> > Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:205
> > +        if (right!= map.end()) {
> > +            if  (right->second.isPercent())
> > +                adjustedX = visibleEdgeRight - right->second.getFloatValue() * m_visibleContentsRect.width() / 100 - layer->size().width();
> > +            else // it->second.isFixed()
> > +                adjustedX = visibleEdgeRight - right->second.getFloatValue() * m_contentsScale - layer->size().width();
> 
> This should be using valueForLength (LengthFunctions.h), I believe
> 
ok

> > Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:213
> > +            adjustedY = visibleEdgeTop + top->second.getFloatValue() * m_visibleContentsRect.height() / 100;
> 
> top->second is pretty unreadable.
> 
I will assign it to a temporary variable with a better name.

> > Source/WebKit2/UIProcess/API/C/WKPreferencesPrivate.h:80
> >  // Defaults to false.
> > +WK_EXPORT void WKPreferencesSetAcceleratedCompositingForFixedPositionEnabled(WKPreferencesRef, bool);
> > +WK_EXPORT bool WKPreferencesGetAcceleratedCompositingForFixedPositionEnabled(WKPreferencesRef);
> 
> Why are we exposing this to C? Are Apple guys interesting in using it? I would leave this out of this patch
> 
ok

> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:124
> > +    webPageProxy->pageGroup()->preferences()->setAcceleratedCompositingForFixedPositionEnabled(true);
> 
> Cant we just tie this to the resizeToContents and avoid the preference?
> 
ok

> > Source/WebKit2/ChangeLog:10
> > +        This is not done for pinching. Only when the pinch gesture is finished, the position is adjusted.
> 
> huh? why?
I tested chrome beta on ICS and ios 5.1. Both do the same. During the pinch, the position is not updates, only after the pinch is done.

[Simon Fraser (smfr)]

Comment on attachment 133058 [details]
Patch

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

>>> Source/WebCore/platform/graphics/GraphicsLayer.h:421
>>> +    bool fixedPosition() const { return m_fixedPosition; }
>> 
>> setUseFixedPosition? setIsFixedPositioned() ?
> 
> setUseFixedPosition() sound good to me :)

GraphicsLayer should not know about any CSS concepts like fixed position. This does not belong here.

[Sam Weinig]

As this change GraphicsLayer pretty fundamentally, Simon Fraser needs to review this before it goes in.  Putting a CSS concept on on GraphicsLayer is probably the wrong way to go.

[Noam Rosenthal]

(In reply to comment #7)
> (From update of attachment 133058 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133058&action=review
> 
> >>> Source/WebCore/platform/graphics/GraphicsLayer.h:421
> >>> +    bool fixedPosition() const { return m_fixedPosition; }
> >> 
> >> setUseFixedPosition? setIsFixedPositioned() ?
> > 
> > setUseFixedPosition() sound good to me :)
> 
> GraphicsLayer should not know about any CSS concepts like fixed position. This does not belong here.

Any other suggestion on how the same effect could be achieved? Or is this just a semantic issue.

[Simon Fraser (smfr)]

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 133058 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=133058&action=review
> > 
> > >>> Source/WebCore/platform/graphics/GraphicsLayer.h:421
> > >>> +    bool fixedPosition() const { return m_fixedPosition; }
> > >> 
> > >> setUseFixedPosition? setIsFixedPositioned() ?
> > > 
> > > setUseFixedPosition() sound good to me :)
> > 
> > GraphicsLayer should not know about any CSS concepts like fixed position. This does not belong here.
> 
> Any other suggestion on how the same effect could be achieved? Or is this just a semantic issue.

It's not semantic. We don't want GraphicsLayer becoming a RenderLayer look-alike. GraphicsLayer is about backing stores, not what you do with them. 

You should communicate the information about which layers have special scrolling behavior via some other mechanism.

[Sam Weinig]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > (From update of attachment 133058 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=133058&action=review
> > > 
> > > >>> Source/WebCore/platform/graphics/GraphicsLayer.h:421
> > > >>> +    bool fixedPosition() const { return m_fixedPosition; }
> > > >> 
> > > >> setUseFixedPosition? setIsFixedPositioned() ?
> > > > 
> > > > setUseFixedPosition() sound good to me :)
> > > 
> > > GraphicsLayer should not know about any CSS concepts like fixed position. This does not belong here.
> > 
> > Any other suggestion on how the same effect could be achieved? Or is this just a semantic issue.
> 
> It's not semantic. We don't want GraphicsLayer becoming a RenderLayer look-alike. GraphicsLayer is about backing stores, not what you do with them. 
> 
> You should communicate the information about which layers have special scrolling behavior via some other mechanism.

It is the long term goal for the ScrollingCoordinator and ScrollingTree to contain this information.

[Noam Rosenthal]

> It is the long term goal for the ScrollingCoordinator and ScrollingTree to contain this information.

OK, understood. Yael, please make sure we're architecturally aligned with this.

[Yael]

Thanks for your comments.
I'll look into using ScrollingCoordinator.

[Yael]

Created attachment 133414 [details]
Patch

Adjusted the previous patch in response to Kenneth's comments.
Also removed all changes from GraphicsLayer.h .

The approach now is that in LayerTreeHostQt, just before syncing the layer tree, we walk the RenderLayers and collect the information needed.

[Yael]

(In reply to comment #13)
> I'll look into using ScrollingCoordinator.

I think it is a bit pre-mature to start using ScrollingCoordinator. I will come back to it later, if that's ok.

[Simon Fraser (smfr)]

Comment on attachment 133414 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:265
> +            if (renderLayer->renderer()->isPositioned() && renderLayer->renderer()->style()->position() == FixedPosition) {

Checking the style's position isn't sufficient to know if it's really fixed position, because transformed elements act as fixed position containers.

[Noam Rosenthal]

Comment on attachment 133414 [details]
Patch

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

[Yael]

(In reply to comment #16)
> (From update of attachment 133414 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133414&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:265
> > +            if (renderLayer->renderer()->isPositioned() && renderLayer->renderer()->style()->position() == FixedPosition) {
> 
> Checking the style's position isn't sufficient to know if it's really fixed position, because transformed elements act as fixed position containers.

Thanks for pointing this out. I'll add a check that renderer()->container() is a RenderView, like in RenderLayerCompositor. Would that be sufficient?

[Simon Fraser (smfr)]

(In reply to comment #18)
> Thanks for pointing this out. I'll add a check that renderer()->container() is a RenderView, like in RenderLayerCompositor. Would that be sufficient?

Yes

[Yael]

(In reply to comment #17)
thanks for the review,
> (From update of attachment 133414 [details])
> 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.

Please keep in mind that not all fixed position layers are moved by the same offset. If the position is specified in pixels, and the page is zoomed, that affects the offset differently than percentage.

> > Source/WebCore/page/FrameView.cpp:1717
> > +        if (m_frame->page()->settings()->acceleratedCompositingForFixedPositionEnabled()) {
> > +            updateFixedElementsAfterScrolling();
> > +            repaintFixedElementsAfterScrolling();
> > +        }
> 
> This probably needs a comment.
I might not need this change after all

> 
> > 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.
ok, I'll change that as we talked on irc
 
> > 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?
We need to sync during scrolling. Otherwise, the fixed layer scrolls and jumps back when scrolling is finished. I'll see if I can avoid the jumps in a different way.

> > Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:167
> > +    m_contentsSize = contentsSize;
> 
> how is this different from DrawingArea::contentsRect()?
WebLayerTreeRenderer does not have access to DrawingArea and I did not want to change that. We already have WebLayerTreeRenderer::setVisibleContentsRect .

> > Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:196
> > +            adjustedX = visibleEdgeLeft + left.getFloatValue() * m_visibleContentsRect.width() / 100;
> 
> You're doing manually what WebCore::Length can do automatically.
Not sure how much I can reuse LengthFunctions. Length does not know about the scale, and I need to take that into account too.

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

[Yael]

Created attachment 133667 [details]
Patch

Address previous comments, and add a manual test.
Fixed a bug related to scale. Apparently, we should not multiply the offset by the scale.

[Noam Rosenthal]

Comment on attachment 133667 [details]
Patch

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

See comments.

> Source/WebCore/platform/graphics/texmap/GraphicsLayerOffsetFromViewport.h:49
> +    typedef int LayerSides;
> +    bool hasTop() { return layerSide & TopSide; }
> +    bool hasBottom() { return layerSide & BottomSide; }
> +    bool hasLeft() { return layerSide & LeftSide; }
> +    bool hasRight() { return layerSide & RightSide; }
> +
> +    LayerSides layerSide;
> +    Length horizontalOffset;
> +    Length verticalOffset;

This is still kind of awkward :)
Instead, we could just have 4 Lengths, and check if they have a value when we adjust. It would make half the code in this patch redundant :)
Also, if all we have is 4 Lengths, we don't need this extra class that doesn't really do much.
We can simply have:
TextureMapperLayer::setViewportOffsets(top, right, bottom, left);
TextureMapperLayer::clearViewportOffsets();
TextureMapperLayer::hasViewportOffsets();

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:418
> +        int visibleEdgeRight = std::min(visibleContentsRect.maxX(), (int)(contentsSize.width()));

No c-style casts please :)

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:26
> +#include "GraphicsLayerOffsetFromViewport.h"

This doesn't feel right here. It should be in TextureMapperLayer. GraphicsLayerTextureMapper is a glue between GraphicsLayer and TextureMapperLayer, we shouldn't put calculations in there.

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:261
> +    return length.isPercent() || length.isFixed();

you should just call length.isSpecified(), you don't need this function.
Also, we should add a comment that our solution doesn't work with calculated values. I'm not sure whether calc() and fixed are

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:270
> +    RenderLayerBacking* backing = renderLayer->backing();
> +    if (backing) {
> +        RenderStyle* style = renderLayer->renderer()->style();
> +        if (style) {
> +            if (renderLayer->renderer()->isPositioned() && renderLayer->renderer()->style()->position() == FixedPosition && renderLayer->renderer()->container()->isRenderView()) {

Use multiple functions with early returns instead of this indentation inflation

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:271
> +                WebGraphicsLayer* graphicsLayer = static_cast<WebGraphicsLayer*>(backing->graphicsLayer());

There's a toWebGraphicsLayer function.

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:307
> +void LayerTreeHostQt::syncFixedLayers()
> +{

You should return early if frameView returns false from hasFixedObjects(), and cancel all existing layers.

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:308
> +    RenderLayer* rootRenderLayer = m_webPage->mainFrame()->contentRenderer()->compositor()->rootRenderLayer();

Can any element in this chain be null?
Maybe adding some asserts would be prudent, that way we can detect that in debug mode.

> Tools/MiniBrowser/qt/qml/FilePicker.qml:26
> +/*
> +import Qt.labs.folderlistmodel 1.0

Unrelated leftover :)

[Yael]

Created attachment 134995 [details]
Patch

This patch has a somewhat different approach. It does not pass Length information to the UI, and relies on the web process to calculate the position. This patch still passes information about which edges have fixed position.

TextureMapperLayer::adjustedScrollOffset does 2 things now:
1. During scroll overshoot, we want the fixed layer to move with the edge only if it is fixed to that edge. so it checks and adjusts for that.(Same behavior as iOS5).
2. Due to async nature of the messages, during scrolling, the UI and the web process might use different scroll offsets, so it checks and compensates for that too.

Currently, when we zoom, fixed position elements are not positioned correctly, and this patch does _not_ address that behavior.

[Yael]

Agreed with Noam on IRC that the initial patch will not handle overshoot, I'll upload an update soon.

[Yael]

Created attachment 135260 [details]
Patch.

Removed special handling for overshoot, will add it later.

[Yael]

Created attachment 135263 [details]
Patch.

Reduce the amount of offset calculation for each paint.

[Noam Rosenthal]

Comment on attachment 135263 [details]
Patch.

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

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:105
> +    IntPoint scrollOffset() const { return m_scrollOffset; }
> +    void setScrollOffset(const IntPoint& offset) { m_scrollOffset = offset; }
> +
> +    IntRect visibleContentsRect() const { return m_visibleContentsRect; }
> +    void setVisibleContentsRect(const IntRect& rect) { m_visibleContentsRect = rect; }
> +
> +    FloatSize contentsSize() const { return m_contentsSize; }
> +    void setContentsSize(const FloatSize& size) { m_contentsSize = size; }
> +

Do you really need all of this for all the fixed layers?
Sounds like you need all those variables once for the whole tree, and add it to the layer position when you calculate the transform.

[Kenneth Rohde Christiansen]

Comment on attachment 135263 [details]
Patch.

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

> Source/WebCore/ChangeLog:8
> +        When the setting acceleratedCompositingForFixedPositionEnabled is true, we need to upadte

update

Couldn't the method be called something like userLayersForFixedElements or similar.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:517
> +FloatPoint TextureMapperLayer::normalizedScrollOffset(const IntPoint& webScrollOffset)

web? This doesn't tell me whethre this is in CSS coords or not.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:520
> +    float webScrollPositionX = std::max(webScrollOffset.x(), 0);
> +    webScrollPositionX = std::min(webScrollPositionX, m_contentsSize.width() - m_visibleContentsRect.width());

isn't there something like qBound?

This is offset from what? current position? because if so, then it should be possible to be negative. Or is this just scroll position?

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:538
> +    float uiScrollPositionX = std::max(m_visibleContentsRect.x(), 0);

What value does ui add here?

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:539
> +    uiScrollPositionX = std::min(uiScrollPositionX, m_contentsSize.width() - m_visibleContentsRect.width());

I have seen similar code elsewhere

> Source/WebKit2/Shared/WebLayerTreeInfo.cpp:38
> +    encoder->encode(CoreIPC::In(scrollOffset));

scrollPosition?

> Source/WebKit2/Shared/WebLayerTreeInfo.h:115
> +    WebCore::IntPoint scrollOffset;

an offset would be IntSize where as a position would be IntPoint

[Yael]

(In reply to comment #28)
> (From update of attachment 135263 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135263&action=review
> 
Thank you for the review :)
> > Source/WebCore/ChangeLog:8
> > +        When the setting acceleratedCompositingForFixedPositionEnabled is true, we need to upadte
> 
> update
> 
oops :)

> Couldn't the method be called something like userLayersForFixedElements or similar.
> 
acceleratedCompositingForFixedPositionEnabled is consistent with other settings for accelerated composition. If we choose to rename it, it probably should not go with this patch.

> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:517
> > +FloatPoint TextureMapperLayer::normalizedScrollOffset(const IntPoint& webScrollOffset)
> 
> web? This doesn't tell me whethre this is in CSS coords or not.
> 
I a not passing CSS information anymore. This is strictly about the scroll offset that the web process uses. You are right, position is better than offset here.

> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:520
> > +    float webScrollPositionX = std::max(webScrollOffset.x(), 0);
> > +    webScrollPositionX = std::min(webScrollPositionX, m_contentsSize.width() - m_visibleContentsRect.width());
> 
> isn't there something like qBound?
> 
This file is used by GTK folks too.

> This is offset from what? current position? because if so, then it should be possible to be negative. Or is this just scroll position?
> 
yes, this should be called position, not offset.

> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:538
> > +    float uiScrollPositionX = std::max(m_visibleContentsRect.x(), 0);
> 
> What value does ui add here?
> 
The purpose of this method is to compensate for the difference in scroll position between the UI process and the web process. The difference is due to the async nature of the IPC messages. I need to distinguish between the position in the UI side, and the position in the web side.

> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:539
> > +    uiScrollPositionX = std::min(uiScrollPositionX, m_contentsSize.width() - m_visibleContentsRect.width());
> 
> I have seen similar code elsewhere
> 
> > Source/WebKit2/Shared/WebLayerTreeInfo.cpp:38
> > +    encoder->encode(CoreIPC::In(scrollOffset));
> 
> scrollPosition?
> 
ok
> > Source/WebKit2/Shared/WebLayerTreeInfo.h:115
> > +    WebCore::IntPoint scrollOffset;
> 
> an offset would be IntSize where as a position would be IntPoint
ok

[Antonio Gomes]

(In reply to comment #27)
> (From update of attachment 135263 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135263&action=review
> 
> > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:105
> > +    IntPoint scrollOffset() const { return m_scrollOffset; }
> > +    void setScrollOffset(const IntPoint& offset) { m_scrollOffset = offset; }
> > +
> > +    IntRect visibleContentsRect() const { return m_visibleContentsRect; }
> > +    void setVisibleContentsRect(const IntRect& rect) { m_visibleContentsRect = rect; }
> > +
> > +    FloatSize contentsSize() const { return m_contentsSize; }
> > +    void setContentsSize(const FloatSize& size) { m_contentsSize = size; }
> > +
> 
> Do you really need all of this for all the fixed layers?
> Sounds like you need all those variables once for the whole tree, and add it to the layer position when you calculate the transform.

Agreed. This is how we do on the blackberry port. See http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/blackberry/LayerRenderer.cpp#L512 , and @ppk and friends say it is the "best" fixed position handling in mobile webkit-based browsers (don't get me wrong, just for you to try on different devices :)

[Yael]

Created attachment 135479 [details]
Patch.

Let WebLayerTreeRenderer do the scrollPosition calculation.

[Yael]

(In reply to comment #30)
> (In reply to comment #27)

> Agreed. This is how we do on the blackberry port. See http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/blackberry/LayerRenderer.cpp#L512 , and @ppk and friends say it is the "best" fixed position handling in mobile webkit-based browsers (don't get me wrong, just for you to try on different devices :)

I just tested http://waplabdc.nokia-boston.com/browser/users/yaharon/pos.html on my husband's blackberry torch. As I started scrolling, the fixed layers were thrown off and then came back to place. The reason I want to account for the delta between web process and ui process is to avoid that side effect.

[Antonio Gomes]

> I just tested http://waplabdc.nokia-boston.com/browser/users/yaharon/pos.html on my husband's blackberry torch. As I started scrolling, the fixed layers were thrown off and then came back to place. The reason I want to account for the delta between web process and ui process is to avoid that side effect.

Depending on what torch we are talking here, it does not have fixed position handled at all. Will test tomorrow and let you know.

[Noam Rosenthal]

Comment on attachment 135479 [details]
Patch.

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

OK, it's going in the right direction, but still too much going on per-layer.
The only thing needed to be serialized per-layer is a flag saying whether or not it's fixed to the viewport. In addition, TextureMapperLayer should have a scrollOffset which is calculated once for all fixed layers.

I feel like you're making some of the choices here based on not wanting to add new IPC messages... That doesn't feel right; We should add an IPC message for when the scroll offset changes, and keep the layer-specific info to the data that's actually layer-specific.

> Source/WebKit2/Shared/WebLayerTreeInfo.h:115
> +    WebCore::IntPoint scrollPosition;

I don't see why you need to pass this for each layer. Pass it once in its own message (something like didChangeFixedObjectScrollOffset), and use the same offset for all layers.

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:78
> +    float scrollPositionX = std::max(scrollPosition.x(), 0.0f);

0.0f -> 0

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:81
> +    float scrollPositionY = std::max(scrollPosition.y(), 0.0f);

ditto

[Antonio Gomes]

(In reply to comment #32)
> (In reply to comment #30)
> > (In reply to comment #27)
> 
> > Agreed. This is how we do on the blackberry port. See http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/blackberry/LayerRenderer.cpp#L512 , and @ppk and friends say it is the "best" fixed position handling in mobile webkit-based browsers (don't get me wrong, just for you to try on different devices :)
> 
> I just tested http://waplabdc.nokia-boston.com/browser/users/yaharon/pos.html on my husband's blackberry torch. As I started scrolling, the fixed layers were thrown off and then came back to place. The reason I want to account for the delta between web process and ui process is to avoid that side effect.

Ok, just tested on a Torch 2 device I have. It does not work perfect, as it does not run the latest code. Playbook does instead, and works perfectly. This runs the code that is upstreamed

[Yael]

Created attachment 135747 [details]
Patch.

Address comment #34.

[Noam Rosenthal]

Comment on attachment 135747 [details]
Patch.

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

[Yael]

(In reply to comment #37)
> (From update of attachment 135747 [details])
> 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.
> 
As soon as a fixed layer becomes un-fixed it is deleted. I don't think I need to handle that case other than removing it from m_fixedLayers.

[Noam Rosenthal]

(In reply to comment #38)
> (In reply to comment #37)
> > (From update of attachment 135747 [details] [details])
> > 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.
> > 
> As soon as a fixed layer becomes un-fixed it is deleted. I don't think I need to handle that case other than removing it from m_fixedLayers.

OK. This deserves a comment :)

[Yael]

(In reply to comment #37)
> (From update of attachment 135747 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135747&action=review
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:514
> > +void TextureMapperLayer::setScrollPositionDelta(const IntPoint& delta)
> 
> Delta -> Offset
This is actually a delta and not an offset. Since you asked me to do the calculation only once, and set to all layers, I calculated the delta between scroll offset of the web process and scroll offset of the ui process and add that to the position of the transform.

> 
> > 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.
> 
If the position changes, we will sync the layers again and will re-calculate. I will add a test that changes the position dynamically, to show that this is not breaking.

[Noam Rosenthal]

> > Delta -> Offset
> This is actually a delta and not an offset. Since you asked me to do the calculation only once, and set to all layers, I calculated the delta between scroll offset of the web process and scroll offset of the ui process and add that to the position of the transform.

Fair enough. Let's keep as is.


> If the position changes, we will sync the layers again and will re-calculate. I will add a test that changes the position dynamically, to show that this is not breaking.
You're right :)

Let's see how this look likes with my other comments, I think this is looking better and better.

[Yael]

Created attachment 135972 [details]
Patch.

Changing variable names and using bind.

[Kenneth Rohde Christiansen]

Comment on attachment 135972 [details]
Patch.

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

> ChangeLog:3
> +        Initial support for fixed position elements in Qt WebKit2

As you call it initial you should state what is supported and what is pending.

> Source/WebCore/page/FrameView.cpp:1707
> +        if (m_frame->page()->settings()->acceleratedCompositingForFixedPositionEnabled())
> +            updateFixedElementsAfterScrolling();

Maybe we should do the check inside the method instead?

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:516
> +void TextureMapperLayer::setScrollPositionDelta(const IntPoint& delta)
> +{
> +    m_scrollPositionDelta = delta;

Maybe this method deserves a comment?

> Source/WebKit2/UIProcess/LayerTreeHostProxy.messages.in:31
> +    DidUpdateScrollPosition(WebCore::IntPoint position)

I believe WK2 mostly uses Change... like DidChangeScrollPosition

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:76
> +static IntPoint normalizedScrollPosition(const IntPoint& scrollPosition, const IntRect& visibleContentRect, const FloatSize& contentSize)

boundedScrollPosition?

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:267
> +    // These conditions must match the condisions in RenderLayerCompositor::requiresCompositingForPosition.

conditions*

[Yael]

Created attachment 136157 [details]
Patch

Address comment #43, except for the change in FrameView.cpp.

> > Source/WebCore/page/FrameView.cpp:1707
> > +        if (m_frame->page()->settings()->acceleratedCompositingForFixedPositionEnabled())
> > +            updateFixedElementsAfterScrolling();
> 
> Maybe we should do the check inside the method instead?

The default in chromium is to not create a graphics layer for fixed elements and moving the check will effectively revert http://trac.webkit.org/changeset/111139 .

[Noam Rosenthal]

Comment on attachment 136157 [details]
Patch

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

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:112
> +    adjustPositionForFixedLayers();
>      syncRemoteContent();

syncRemoteContent might change the position of layers, and also the renderedContentsScrollPosition, both of which adjustPositionForFixedLayers relies on. Sounds like things can go out of sync there.
Shouldn't the order be swapped?

[Yael]

(In reply to comment #45)
> (From update of attachment 136157 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136157&action=review
> 
> > Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:112
> > +    adjustPositionForFixedLayers();
> >      syncRemoteContent();
> 
> syncRemoteContent might change the position of layers, and also the renderedContentsScrollPosition, both of which adjustPositionForFixedLayers relies on. Sounds like things can go out of sync there.
> Shouldn't the order be swapped?

syncRemoteContent is also called from updatePaintNode, so I did not observe this issue :) I will make the change in the next version of the patch.

[Noam Rosenthal]

Comment on attachment 136157 [details]
Patch

Apart from my comment, this patch looks good to me. Kenneth, would you look at it as well to make sure I didn't miss something?

[Allan Sandfeld Jensen]

Comment on attachment 136157 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1707
> +        if (m_frame->page()->settings()->acceleratedCompositingForFixedPositionEnabled())
> +            updateFixedElementsAfterScrolling();

Is this setting really necessary? If we always enable it when using fixed-layout, it seems redundant. Would it be possible to instead check if accelerated compositing is enabled and the frameview has delegates-scrolling and fixed layout size set?

[Yael]

(In reply to comment #48)
> (From update of attachment 136157 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136157&action=review
> 
> > Source/WebCore/page/FrameView.cpp:1707
> > +        if (m_frame->page()->settings()->acceleratedCompositingForFixedPositionEnabled())
> > +            updateFixedElementsAfterScrolling();
> 
> Is this setting really necessary? If we always enable it when using fixed-layout, it seems redundant. Would it be possible to instead check if accelerated compositing is enabled and the frameview has delegates-scrolling and fixed layout size set?

Sure, I can do that. Can you please explain the benefits of the change you are proposing? IMO, it is clearer as it is now, and if someone downstream decides at some point to not enable the setting, there is only one place that they need to change (in WebPage.cpp).

[Noam Rosenthal]

> > Source/WebCore/page/FrameView.cpp:1707
> > +        if (m_frame->page()->settings()->acceleratedCompositingForFixedPositionEnabled())
> > +            updateFixedElementsAfterScrolling();
> 
> Is this setting really necessary? If we always enable it when using fixed-layout, it seems redundant. Would it be possible to instead check if accelerated compositing is enabled and the frameview has delegates-scrolling and fixed layout size set?

Not for this patch, please. We can do this at a later patch.

[Allan Sandfeld Jensen]

(In reply to comment #49)
> (In reply to comment #48)
> > (From update of attachment 136157 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=136157&action=review
> > 
> > > Source/WebCore/page/FrameView.cpp:1707
> > > +        if (m_frame->page()->settings()->acceleratedCompositingForFixedPositionEnabled())
> > > +            updateFixedElementsAfterScrolling();
> > 
> > Is this setting really necessary? If we always enable it when using fixed-layout, it seems redundant. Would it be possible to instead check if accelerated compositing is enabled and the frameview has delegates-scrolling and fixed layout size set?
> 
> Sure, I can do that. Can you please explain the benefits of the change you are proposing? IMO, it is clearer as it is now, and if someone downstream decides at some point to not enable the setting, there is only one place that they need to change (in WebPage.cpp).

It is only because I know from recent experience of trying to make sense of all the settings in frameview, that there are too many that all indicates the same thing (fixed layout, resize-to-content, delegates scrolling). This is not only redundant but very confusing to new comers. But feel free to keep it for now and remove it later when the feature is more stable and better tested.

[Kenneth Rohde Christiansen]

(In reply to comment #49)
> (In reply to comment #48)
> > (From update of attachment 136157 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=136157&action=review
> > 
> > > Source/WebCore/page/FrameView.cpp:1707
> > > +        if (m_frame->page()->settings()->acceleratedCompositingForFixedPositionEnabled())
> > > +            updateFixedElementsAfterScrolling();
> > 
> > Is this setting really necessary? If we always enable it when using fixed-layout, it seems redundant. Would it be possible to instead check if accelerated compositing is enabled and the frameview has delegates-scrolling and fixed layout size set?
> 
> Sure, I can do that. Can you please explain the benefits of the change you are proposing? IMO, it is clearer as it is now, and if someone downstream decides at some point to not enable the setting, there is only one place that they need to change (in WebPage.cpp).

I believe that Chrome for Android uses fixed layouting but does not delegate scrolling. This mostly relates to the delegation.

[Kenneth Rohde Christiansen]

Comment on attachment 136157 [details]
Patch

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

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

how can it be pending and rendered?

[Noam Rosenthal]

(In reply to comment #53)
> (From update of attachment 136157 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136157&action=review
> 
> > Source/WebKit2/UIProcess/WebLayerTreeRenderer.h:120
> > +    WebCore::IntPoint m_pendingRenderedContentsScrollPosition;
> 
> how can it be pending and rendered?

It applies to the content rendered in the web process, and is pending flush in the ui  process.

[Kenneth Rohde Christiansen]

(In reply to comment #54)
> (In reply to comment #53)
> > (From update of attachment 136157 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=136157&action=review
> > 
> > > Source/WebKit2/UIProcess/WebLayerTreeRenderer.h:120
> > > +    WebCore::IntPoint m_pendingRenderedContentsScrollPosition;
> > 
> > how can it be pending and rendered?
> 
> It applies to the content rendered in the web process, and is pending flush in the ui  process.

Yeah I guessed that, but wanted to point out that the names are not that clear.

[Yael]

hey, can we agree to keep the variable name as Noam suggested and if Kenneth finds a more suitable name, he can propose that in a separate patch?

[Noam Rosenthal]

Comment on attachment 136157 [details]
Patch

This is fine with me for now. If we have additional improvement, we can make them on top of this.

[Antonio Gomes]

> I believe that Chrome for Android uses fixed layouting but does not delegate scrolling. This mostly relates to the delegation.

Same for blackberry port

[Yael]

Created attachment 136441 [details]
Patch for landing.

[WebKit Review Bot]

Comment on attachment 136441 [details]
Patch for landing.

Rejecting attachment 136441 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ne).
Hunk #4 succeeded at 260 (offset 1 line).
Hunk #5 succeeded at 499 (offset 1 line).
1 out of 5 hunks FAILED -- saving rejects to file Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp.rej
patching file Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.h
Hunk #1 succeeded at 76 with fuzz 2 (offset 1 line).
Hunk #2 succeeded at 120 (offset 6 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12379597

[Yael]

Committed r113791: <http://trac.webkit.org/changeset/113791>