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

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from MORITA Hajime <morrita at google.com>  2011-04-15 14:11:04 PST ---
Hi, Dimitri, thank you for reviewing!
I update the patch.

> > 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.
> 
Removed these and introduced Node::canHaveLightChildRendererWithShadow() for a replacement.


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

> 
> > 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.
Removed.

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

> 
> > 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.
Renamed the file.

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