[webkit-reviews] review denied: [Bug 106634] Distribution state becomes inconsistent with content/shadow reprojection : [Attachment 183121] Patch

[bugzilla-daemon at webkit.org]

Hajime Morrita <morrita at google.com> has denied Shinya Kawanaka
<shinyak at chromium.org>'s request for review:
Bug 106634: Distribution state becomes inconsistent with content/shadow
reprojection
https://bugs.webkit.org/show_bug.cgi?id=106634

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

------- Additional Comments from Hajime Morrita <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=183121&action=review


> Source/WebCore/html/shadow/ContentDistributor.cpp:349
> +void ContentDistributor::ensureDistributionFromAncestor(Node* source)

It looks we can now simply call this ensureDistribution() since the "ensure"
should be always drom the ancestor.

> Source/WebCore/html/shadow/ContentDistributor.cpp:365
> +	   elementShadows[i - 1]->ensureDistribution();

It looks we no longer need ElementShadow::ensureDistribution(). No other place
uses it. Is that right?

> Source/WebCore/html/shadow/ContentDistributor.cpp:388
> +		   }

Could you clarify or elaborate what is happening here?

> Source/WebCore/html/shadow/ContentDistributor.cpp:390
> +	   }

I guess this new code should be orthogonal to node's attached() state.

> Source/WebCore/html/shadow/ContentDistributor.h:137
> +    static void ensureDistributionFromAncestor(Node* source);

Nit: could be container node. Or, we have an overload for shadow root.

> Source/WebCore/html/shadow/ContentDistributor.h:149
> +    bool distributionIsValid() const { return m_validity == Valid; }

Nit: It's fine to just call this isValid(). This is "distributor".