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

[bugzilla-daemon at webkit.org]

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


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

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




--- Comment #22 from Noam Rosenthal <noam.rosenthal at nokia.com>  2012-03-24 22:28:35 PST ---
(From update of attachment 133667)
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 :)

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