Bug 81786 - Initial support fixed position elements in Qt WebKit2
Summary: Initial support fixed position elements in Qt WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-21 08:40 PDT by Yael
Modified: 2012-04-10 17:44 PDT (History)
14 users (show)

See Also:


Attachments
Patch (26.29 KB, patch)
2012-03-21 09:17 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (26.29 KB, patch)
2012-03-21 09:21 PDT, Yael
simon.fraser: review-
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Patch (25.08 KB, patch)
2012-03-22 19:29 PDT, Yael
noam: review-
Details | Formatted Diff | Diff
Patch (32.01 KB, patch)
2012-03-24 18:32 PDT, Yael
noam: review-
Details | Formatted Diff | Diff
Patch (30.98 KB, patch)
2012-04-01 05:41 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (27.13 KB, patch)
2012-04-02 19:27 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (27.60 KB, patch)
2012-04-02 20:10 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (25.13 KB, patch)
2012-04-03 18:37 PDT, Yael
noam: review-
Details | Formatted Diff | Diff
Patch. (26.03 KB, patch)
2012-04-04 19:30 PDT, Yael
noam: review-
Details | Formatted Diff | Diff
Patch. (25.53 KB, patch)
2012-04-05 20:13 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (25.94 KB, patch)
2012-04-08 13:34 PDT, Yael
noam: review+
Details | Formatted Diff | Diff
Patch for landing. (25.94 KB, patch)
2012-04-10 05:56 PDT, Yael
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2012-03-21 08:40:57 PDT
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.
Comment 1 Yael 2012-03-21 09:17:58 PDT
Created attachment 133057 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-21 09:19:39 PDT
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.
Comment 3 Yael 2012-03-21 09:21:17 PDT
Created attachment 133058 [details]
Patch

Remove extra space (style error).
Comment 4 Gyuyoung Kim 2012-03-21 09:31:06 PDT
Comment on attachment 133058 [details]
Patch

Attachment 133058 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12034534
Comment 5 Kenneth Rohde Christiansen 2012-03-21 09:51:53 PDT
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?
Comment 6 Yael 2012-03-21 10:24:14 PDT
(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.
Comment 7 Simon Fraser (smfr) 2012-03-21 13:18:28 PDT
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.
Comment 8 Sam Weinig 2012-03-21 13:18:34 PDT
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.
Comment 9 Noam Rosenthal 2012-03-21 13:20:48 PDT
(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.
Comment 10 Simon Fraser (smfr) 2012-03-21 13:22:37 PDT
(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.
Comment 11 Sam Weinig 2012-03-21 13:37:27 PDT
(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.
Comment 12 Noam Rosenthal 2012-03-21 13:44:00 PDT
> 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.
Comment 13 Yael 2012-03-21 13:46:41 PDT
Thanks for your comments.
I'll look into using ScrollingCoordinator.
Comment 14 Yael 2012-03-22 19:29:47 PDT
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.
Comment 15 Yael 2012-03-22 19:31:34 PDT
(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.
Comment 16 Simon Fraser (smfr) 2012-03-22 20:05:11 PDT
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.
Comment 17 Noam Rosenthal 2012-03-23 06:18:22 PDT
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
Comment 18 Yael 2012-03-23 14:03:16 PDT
(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?
Comment 19 Simon Fraser (smfr) 2012-03-23 14:09:23 PDT
(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
Comment 20 Yael 2012-03-23 20:44:14 PDT
(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
Comment 21 Yael 2012-03-24 18:32:36 PDT
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.
Comment 22 Noam Rosenthal 2012-03-24 22:28:35 PDT
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 :)
Comment 23 Yael 2012-04-01 05:41:11 PDT
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.
Comment 24 Yael 2012-04-02 17:53:35 PDT
Agreed with Noam on IRC that the initial patch will not handle overshoot, I'll upload an update soon.
Comment 25 Yael 2012-04-02 19:27:38 PDT
Created attachment 135260 [details]
Patch.

Removed special handling for overshoot, will add it later.
Comment 26 Yael 2012-04-02 20:10:26 PDT
Created attachment 135263 [details]
Patch.

Reduce the amount of offset calculation for each paint.
Comment 27 Noam Rosenthal 2012-04-02 20:28:02 PDT
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.
Comment 28 Kenneth Rohde Christiansen 2012-04-03 04:11:13 PDT
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
Comment 29 Yael 2012-04-03 05:13:20 PDT
(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
Comment 30 Antonio Gomes 2012-04-03 07:10:53 PDT
(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 :)
Comment 31 Yael 2012-04-03 18:37:26 PDT
Created attachment 135479 [details]
Patch.

Let WebLayerTreeRenderer do the scrollPosition calculation.
Comment 32 Yael 2012-04-03 18:38:18 PDT
(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.
Comment 33 Antonio Gomes 2012-04-03 20:13:07 PDT
> 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.
Comment 34 Noam Rosenthal 2012-04-03 20:30:58 PDT
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
Comment 35 Antonio Gomes 2012-04-04 08:29:47 PDT
(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
Comment 36 Yael 2012-04-04 19:30:23 PDT
Created attachment 135747 [details]
Patch.

Address comment #34.
Comment 37 Noam Rosenthal 2012-04-05 07:49:19 PDT
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
Comment 38 Yael 2012-04-05 12:20:34 PDT
(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.
Comment 39 Noam Rosenthal 2012-04-05 12:58:13 PDT
(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 :)
Comment 40 Yael 2012-04-05 15:11:11 PDT
(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.
Comment 41 Noam Rosenthal 2012-04-05 15:55:29 PDT
> > 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.
Comment 42 Yael 2012-04-05 20:13:24 PDT
Created attachment 135972 [details]
Patch.

Changing variable names and using bind.
Comment 43 Kenneth Rohde Christiansen 2012-04-06 05:46:53 PDT
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*
Comment 44 Yael 2012-04-08 13:34:31 PDT
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 .
Comment 45 Noam Rosenthal 2012-04-08 17:21:08 PDT
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?
Comment 46 Yael 2012-04-08 19:03:19 PDT
(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.
Comment 47 Noam Rosenthal 2012-04-08 20:58:57 PDT
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?
Comment 48 Allan Sandfeld Jensen 2012-04-09 03:39:35 PDT
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?
Comment 49 Yael 2012-04-09 05:23:52 PDT
(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).
Comment 50 Noam Rosenthal 2012-04-09 05:54:02 PDT
> > 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.
Comment 51 Allan Sandfeld Jensen 2012-04-09 06:05:43 PDT
(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.
Comment 52 Kenneth Rohde Christiansen 2012-04-09 06:59:15 PDT
(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.
Comment 53 Kenneth Rohde Christiansen 2012-04-09 07:03:41 PDT
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?
Comment 54 Noam Rosenthal 2012-04-09 07:26:44 PDT
(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.
Comment 55 Kenneth Rohde Christiansen 2012-04-09 07:29:29 PDT
(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.
Comment 56 Yael 2012-04-09 17:22:36 PDT
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?
Comment 57 Noam Rosenthal 2012-04-09 17:26:30 PDT
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.
Comment 58 Antonio Gomes 2012-04-09 18:35:18 PDT
> I believe that Chrome for Android uses fixed layouting but does not delegate scrolling. This mostly relates to the delegation.

Same for blackberry port
Comment 59 Yael 2012-04-10 05:56:29 PDT
Created attachment 136441 [details]
Patch for landing.
Comment 60 WebKit Review Bot 2012-04-10 12:47:26 PDT
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
Comment 61 Yael 2012-04-10 17:44:35 PDT
Committed r113791: <http://trac.webkit.org/changeset/113791>