[Webkit-unassigned] [Bug 58591] RenderDetailsMarker should belong to shadow element.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=58591


Dimitri Glazkov (Google) <dglazkov at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #89816|review?                     |review-
               Flag|                            |




--- Comment #9 from Dimitri Glazkov (Google) <dglazkov at chromium.org>  2011-04-15 11:40:21 PST ---
(From update of attachment 89816)
View in context: https://bugs.webkit.org/attachment.cgi?id=89816&action=review

overall, looks great. Except I think you need to really simplify the plumbing for rendering of children vs. shadow tree. We are getting rid of this soon anyway.

> Source/WebCore/dom/Node.h:96
> +enum ShadowBoundaryType {
> +    ShadowBoundaryLight,
> +    ShadowBoundaryShadowExclusive,
> +    ShadowBoundaryShadowBeforeLight,
> +    ShadowBoundaryShadowAfterLight
> +};

Why do we have so many types? These are all hacks that we will eventually remove, and we only use two of these types.

Also, this needs better name. The enum describes how shadow nodes are renderered in relation to 
child nodes. So maybe ShadowRenderersPositions { BeforeChildren, AfterChildren }?

> Source/WebCore/dom/Node.h:217
> +    virtual ShadowBoundaryType shadowBoundaryType() const { return ShadowBoundaryLight; }

I think this is backwards. We should just have a virtual method on a host element, saying whether or not children should be rendered (false by default). Then let HTMLSummaryElement set it to true. No need to tweak ShadowRoot at all.

> Source/WebCore/html/HTMLSummaryElement.cpp:60
> +HTMLDetailsElement* HTMLSummaryElement::hostDetails() const

should be detailsElement.

> Source/WebCore/html/shadow/DetailsControlElements.cpp:51
> +    if (result)
> +        result->setAnimatableStyle(style);

Don't need this. Style is set just after createRenderer in Node::createRendererAndStyle.

> Source/WebCore/html/shadow/DetailsControlElements.cpp:66
> +HTMLSummaryElement* DetailsMarkerControl::hostSummary()

summaryElement.

> Source/WebCore/html/shadow/DetailsControlElements.h:31
> +#ifndef DetailsControlElements_h

Should just be DetailsMarkerControl. Don't follow the example of MediaControlElements -- it's an antipattern. All these classes should be split out into separate files.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.