[webkit-reviews] review denied: [Bug 7555] :hover style not applied on hover if its display property is different from original style's : [Attachment 203597] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Radu Stavila <stavila at adobe.com>'s
request for review:
Bug 7555: :hover style not applied on hover if its display property is
different from original style's
https://bugs.webkit.org/show_bug.cgi?id=7555

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

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=203597&action=review


> Source/WebCore/ChangeLog:12
> +	       - prevent the element from being removed from the list of
hovered/active elements upon detach when a reattach is in progress
> +	       - prevent the style from being incorrectly computed (due to the
previous point)
> +	       - prevent the style from being computed twice (the attach()
method used to recompute it)

It would be better to do these in pieces. At least the last one should be a
separate patch. Let's just fix the hover bug here.

> Source/WebCore/dom/Element.cpp:1420
> -void Element::createRendererIfNeeded()
> +void Element::createRendererIfNeeded(RenderStyle* computedStyle)
>  {
> -    NodeRenderingContext(this).createRendererForElementIfNeeded();
> +    NodeRenderingContext(this,
computedStyle).createRendererForElementIfNeeded();
>  }

I think these changes are only needed for the double style computation
optimisation.

> Source/WebCore/dom/Element.cpp:1422
> +void Element::attach(const Node::AttachDetachContext* context)

There shouldn't be need to qualify this with Node:: (anywhere).

This should be a mandatory parameter and so use reference instead of pointer,
const AttachContext& context.

> Source/WebCore/dom/Element.cpp:1428
> +    createRendererIfNeeded(context ? context->computedStyle.get() : 0);

Then you can get rid of all null testing.

> Source/WebCore/dom/Node.cpp:978
> +Node::AttachDetachContext::AttachDetachContext()
> +: computedStyle(0)
> +, performingReattach(false)

Indentation.

Please inline the constructor.

> Source/WebCore/dom/Node.cpp:1029
> +void Node::detach(const Node::AttachDetachContext* /*context*/)

Unnecessary comment.

> Source/WebCore/dom/Node.h:170
> +    struct AttachDetachContext {
> +	   RefPtr<RenderStyle> computedStyle;
> +	   bool performingReattach;
> +
> +	   AttachDetachContext();
> +	   ~AttachDetachContext();
> +    };

AttachDetachContext is a clumsy name. AttachContext should be fine for both
cases.

Please move this next to attach()/detach() function declarations in the the
class.

Remove the empty destructor.

> Source/WebCore/dom/Node.h:488
> +    virtual void attach(const AttachDetachContext* = 0);

I believe we have a pretty limited number of call sites. There shouldn't be a
default value.

> Source/WebCore/dom/NodeRenderingContext.cpp:69
>  NodeRenderingContext::NodeRenderingContext(Node* node, RenderStyle* style)
>      : m_node(node)
> -    , m_renderingParent(0)
>      , m_style(style)
>      , m_parentFlowRenderer(0)
>  {
> +    m_renderingParent = NodeRenderingTraversal::parent(node,
&m_parentDetails);
>  }

What is this change about?