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

[bugzilla-daemon at webkit.org]

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

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

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