[webkit-reviews] review denied: [Bug 61114] [Refactoring] Pass NodeRendererFactory to Node::rendererIsNeeded() : [Attachment 94192] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied MORITA Hajime
<morrita at google.com>'s request for review:
Bug 61114: [Refactoring] Pass NodeRendererFactory to Node::rendererIsNeeded()
https://bugs.webkit.org/show_bug.cgi?id=61114

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

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=94192&action=review

Just one more pass. This is looks awesome. Thanks for working on this!

> Source/WebCore/dom/CharacterData.cpp:185
> +    if ((!renderer() || !rendererIsNeeded(NodeRenderingContext(this,
renderer()->style(), NodeRenderingContext::ResolveNone))) && attached()) {

Looks like this is the only time you use both ResolveNone and pass style in.
Maybe just have a different constructor, instead of adding another enum?

> Source/WebCore/dom/NodeRenderingContext.cpp:76
> +    if (resolve == ResolveNone)
> +	   return;
> +
> +    ContainerNode* parent = m_node->parentOrHostNode();
> +    if (!parent)
> +	   return;
> +
> +    if (parent->isShadowBoundary()) {
> +	   m_location = LocationShadowChild;
> +	   m_parentNodeForRenderingAndStyle = parent->shadowHost();
> +	   return;
> +    }
> +
> +    m_location = LocationLightChild;
> +
> +    if (parent->isElementNode()) {
> +	   m_visualParentShadowRoot = toElement(parent)->shadowRoot();
> +
> +	   if (m_visualParentShadowRoot) {
> +	       if (ContainerNode* contentContainer =
m_visualParentShadowRoot->activeContentContainer()) {
> +		   m_phase = AttachContentForwarded;
> +		   m_parentNodeForRenderingAndStyle =
NodeRenderingContext(contentContainer).parentNodeForRenderingAndStyle();
> +		   return;
> +	       } 
> +		   
> +	       m_phase = AttachContentLight;
> +	       m_parentNodeForRenderingAndStyle = parent;
> +	       return;
> +	   }
> +    }
> +
> +    m_parentNodeForRenderingAndStyle = parent;

I expected that Context would be a dumb storage of the data, and factory would
be stuffing it with the right values, but I guess we can do this in a separate
pass.

> Source/WebCore/dom/NodeRenderingContext.h:29
> +#include "Node.h"

Do we need this include?

> Source/WebCore/dom/NodeRenderingContext.h:50
> +    Node* node() const { return m_node; }

Darin told me once before that having a method definition and declaration in
the same line is bad for readability. I can't find his original comment now,
but basically:

* keep the declarations free of definitions
* if you need a function inlined, put the definition right after of the class
declaration, like
http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/So
urce/WebCore/dom/Node.h&l=727&exact_package=chromium