[webkit-reviews] review denied: [Bug 101001] Pages with position:fixed elements should still be able to scroll on the scrolling thread : [Attachment 171959] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 101001: Pages with position:fixed elements should still be able to scroll
on the scrolling thread
https://bugs.webkit.org/show_bug.cgi?id=101001

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=171959&action=review


Looks really great. I found one or two showstopper issues, so review- because
of those. Most of the other comments are just minor style ones.

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:54
>  enum ScrollingNodeType {
> -    ScrollingNode
> -    // FIXME: This will soon contain other types such as FixedNode.
> +    ScrollingNode,
> +    FixedNode
>  };

Could this just be defined on one line?

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:122
> +    virtual void updateViewportConstrainedNode(ScrollingNodeID,
PassOwnPtr<ViewportConstraints>, GraphicsLayer*) { }

It’s the empty function body that creates the only need to include PassOwnPtr.h
here. Could we put that empty body into a cpp file instead? I don’t think it
gets inlined.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp:43
> +ScrollingStateFixedNode::ScrollingStateFixedNode(ScrollingStateTree*
stateTree, ScrollingNodeID nodeID)

I’d just name this “tree”.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp:49
> +ScrollingStateFixedNode::ScrollingStateFixedNode(const
ScrollingStateFixedNode& stateNode)

I’d just name this “node”.

>> Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp:100
>> +	if (m_constraints->anchorEdges() != 0) {
> 
> Tests for true/false, null/non-null, and zero/non-zero should all be done
without equality comparisons.  [readability/comparison_to_zero] [5]

You should obey the style bot here.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp:114
> +    if (m_constraints->alignmentOffset() != FloatSize()) {

I think !m_constraints->alignmentOffset() .isEmpty() would be better, if it
generates the correct code. Or maybe
!m_constraints->alignmentOffset().isZero()?

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp:119
> +    if (m_constraints->viewportRectAtLastLayout() != FloatRect()) {

Ditto.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp:125
> +    if (m_constraints->layerPositionAtLastLayout() != FloatPoint()) {

Ditto.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:31
> +#include "IntRect.h"

This include doesn’t seem to be needed.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:32
> +#include "Region.h"

This include doesn’t seem to be needed.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:36
> +#include <wtf/PassOwnPtr.h>

I think this could include Forward.h instead of PassOwnPtr.h.

Please remove any other unneeded includes.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:60
> +    virtual bool isScrollingStateFixedNode() OVERRIDE { return true; }
> +
> +    virtual bool hasChangedProperties() const OVERRIDE { return
m_changedProperties; }
> +    virtual unsigned changedProperties() const OVERRIDE { return
m_changedProperties; }
> +    virtual void resetChangedProperties() OVERRIDE { m_changedProperties =
0; }

Can these overrides be private instead of public?

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:64
> +    FixedPositionViewportConstraints* viewportConstraints() const { return
m_constraints.get(); }

Can this return a const FixedPositionViewportConstraints* instead of a
non-const?

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:66
> +    virtual void dumpProperties(TextStream&, int indent) const;

Please add OVERRIDE.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:72
> +    unsigned m_changedProperties;

The name here seems wrong. Since this variable does not contain properties, it
should not be named “changed properties”. It should have the word “number” or
“count” in its name.

Same for the getter function.

> Source/WebCore/page/scrolling/ScrollingStateNode.cpp:70
> +    if (isScrollingStateScrollingNode())
> +	   clone = adoptPtr(new
ScrollingStateScrollingNode(*toScrollingStateScrollingNode(this)));
> +    else if (isScrollingStateFixedNode())
> +	   clone = adoptPtr(new
ScrollingStateFixedNode(*toScrollingStateFixedNode(this)));

This seems like something that’s best done with a private virtual member
function rather than a set of if statements. The node could clone itself.

Also, what does this function do if the node is neither type? It seems like it
will just crash later. So the second if could just be an assertion instead of
an if.

> Source/WebCore/page/scrolling/ScrollingStateNode.cpp:106
> -    if (size_t index = m_children->find(node)) {
> +    size_t index = m_children->find(node);
> +    if (index != notFound) {

Why this change?

> Source/WebCore/page/scrolling/ScrollingStateNode.h:53
>      virtual bool isScrollingStateScrollingNode() { return false; }
> +    virtual bool isScrollingStateFixedNode() { return false; }

These virtual functions should be const. But also, we should remove them if we
can. Such functions typically mean we have a design mistake.

> Source/WebCore/page/scrolling/ScrollingStateNode.h:63
> +    GraphicsLayer* graphicsLayer() const { return m_graphicsLayer; }

Can this return a const GraphicsLayer* or be a non-const member function?

> Source/WebCore/page/scrolling/ScrollingTree.cpp:172
> +	       OwnPtr<ScrollingTreeNode> newNode;
> +	       if (stateNode->isScrollingStateScrollingNode())
> +		   newNode = ScrollingTreeScrollingNode::create(this);
> +	       else if (stateNode->isScrollingStateFixedNode())
> +		   newNode = ScrollingTreeFixedNode::create(this);
> +	       else
> +		   ASSERT_NOT_REACHED();

This seems like something best done by a virtual function rather than a set of
if statements.

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:53
> +    // Return whether this scrolling coordinator can keep fixed position
layers fixed to their
> +    // containers while scrolling.
> +    virtual bool supportsFixedPositionLayers() const OVERRIDE { return true;
}

Can this be private instead of public?

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:85
> +    // This function will update the ScrollingStateNode for the given
viewport constrained object.
> +    virtual void updateViewportConstrainedNode(ScrollingNodeID,
PassOwnPtr<ViewportConstraints>, GraphicsLayer*) OVERRIDE;
> +
> +    // Called to synch the GraphicsLayer positions for child layers when
their CALayers have been moved by the scrolling thread.
> +    virtual void syncChildPositions(const IntRect& viewportRect) OVERRIDE;

Can these be private instead of public?

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm:258
> +	       PassOwnPtr<ScrollingStateFixedNode> fixedNode =
ScrollingStateFixedNode::create(m_scrollingStateTree.get(), newNodeID);

This should be OwnPtr. We don’t use PassOwnPtr for local variables.

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm:260
> +	       parent->appendChild(fixedNode);

Since we’ll be using OwnPtr, this will need to say fixedNode.release().

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm:390
> +    ScrollingStateScrollingNode* rootNode =
m_scrollingStateTree->rootStateNode();
> +
> +    Vector<OwnPtr<ScrollingStateNode> >* children = rootNode->children();

Not sure we need the rootNode local variable.

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm:397
> +	   FloatPoint newChildPos =
child->viewportConstraints()->layerPositionForViewportRect(viewportRect);

Not a big fan of “pos” here. Could be “position”, in fact, could just be
“position” or “childPosition” instead of “newChildPos”. I don’t think a longer
name makes the super-local-scope variable meaning any clearer.

> Source/WebCore/page/scrolling/mac/ScrollingTreeFixedNode.h:47
> +    virtual void update(ScrollingStateNode*) OVERRIDE;
> +    virtual void parentScrollPositionDidChange(const IntRect& viewportRect)
OVERRIDE;

Can we make these virtual function overrides private?

> Source/WebCore/page/scrolling/mac/ScrollingTreeFixedNode.mm:77
> +    CGPoint newPosition = CGPointMake(layerPosition.x() -
m_constraints->alignmentOffset().width() + anchorPoint.x *
layerBounds.size.width,
> +				       layerPosition.y() -
m_constraints->alignmentOffset().height() + anchorPoint.y *
layerBounds.size.height);

The second line could just be indented by two. Or maybe we can find a way to
write this so that the math is done on both dimensions at once rather than
having to write the expression twice. I wish this just worked:

    layerPosition - m_constraints->alignmentOffset() + anchorPoint *
layerBounds.size

It seems like we could make it work with C++ operator overloading, even if the
overloaded operators were just in this file.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2523
> +    // Also walk our our stacking contexts looking for a composited layer
which is itself fixed.

This comment says what the code does, but not why. I think I understand why
given the function name, but comment should say why rather than what.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:2562
>> +	if (layer->renderer()->isStickyPositioned()) {
> 
> An else statement can be removed when the prior "if" concludes with a return,
break, continue or goto statement.  [readability/control_flow] [4]

stylebot is right, although the code will look a little strange. Maybe factor
this into two functions since there is almost no shared code.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:2565
>> +	    RenderBoxModelObject *renderer =
toRenderBoxModelObject(layer->renderer());
> 
> Declaration has space between type name and * in RenderBoxModelObject
*renderer  [whitespace/declaration] [3]

stylebot is right

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2606
> +    return nullptr;

This may cause a compilation error on some platforms (Windows?) because it’s
unreachable code.