[webkit-reviews] review denied: [Bug 84048] ShadowRoot needs resetStyleInheritance : [Attachment 144435] Patch

[bugzilla-daemon at webkit.org]

Hajime Morrita <morrita at google.com> has denied Takashi Sakamoto
<tasak at google.com>'s request for review:
Bug 84048: ShadowRoot needs resetStyleInheritance
https://bugs.webkit.org/show_bug.cgi?id=84048

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

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


The attempt looks right. Let's iterate once to factor code a bit.

> Source/WebCore/css/StyleResolver.cpp:1173
> +    if (resetStyleInheritance)

This can be inside if (e) block above.

> Source/WebCore/css/StyleResolver.cpp:1660
> +	   if (m_checker.document()->settings()) {

I guess you can check frame() or page() here. Checking settings() is cryptic.
Is it possible to extract these if/else to a function or method?
Having many conditionals here in the same function looks too complicated.

My feeling is that it's worth to have a method for creating an initial style
instead of making it up inline here.