[webkit-reviews] review granted: [Bug 58591] RenderDetailsMarker should belong to shadow element. : [Attachment 89850] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has granted MORITA Hajime
<morrita at google.com>'s request for review:
Bug 58591: RenderDetailsMarker should belong to shadow element.
https://bugs.webkit.org/show_bug.cgi?id=58591

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

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

Ok with nits.

> Source/WebCore/ChangeLog:10
> +	     Note thtat marker size is given via style for
-webkit-details-marker pseudo class.

thtat -> that

> Source/WebCore/html/HTMLDetailsElement.cpp:93
> +    if (oldSummary && oldSummary->parentNodeForRenderingAndStyle()) {
> +	   oldSummary->detach();
> +	   oldSummary->attach();
> +    }
> +	   
> +    if (refreshRenderer == RefreshRendererAllowed) {
> +	   m_mainSummary->detach();
> +	   m_mainSummary->attach();
> +    }

I cringe seeing this code, but I think we can tackle this later.

> Source/WebCore/html/HTMLDetailsElement.cpp:145
> +    setAttribute(openAttr, m_isOpen ? String() : String(""));

use nullAtom and emptyAtom

> Source/WebCore/html/HTMLSummaryElement.cpp:56
> +    // Since HTMLDetailsElement::mainSummary() is possibly not ready at this
time, we should make attach() lazy.

We should always attach lazily. No need for this comment.