[Webkit-unassigned] [Bug 50661] Convert <meter> shadow DOM to a DOM-based shadow.

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Dimitri Glazkov (Google) <dglazkov at chromium.org>  2011-02-07 21:26:36 PST ---
(From update of attachment 81576)
View in context: https://bugs.webkit.org/attachment.cgi?id=81576&action=review

I am really worried about the direction this patch has taken. You are introducing more custom layout, style calculation, and even new offsprings of methods we need to get rid of (updateFromElement). Please consider this as a plea to return back to basics. First, we should use attach/detach machinery. Second, I would love to get rid of custom positioning using position:absolute. It's almost like you're working _around_ the RenderBox/RenderBlock layout code, instead of using it to the fullest extent. Third, the MPD-stricken MeterPartElement needs to admit the truth and get split up into the multiple classes, each for one of the many entities it represents. You can do much better -- and I am confident you can do this! :)

> Source/WebCore/ChangeLog:10
> +        Introduced two shadow element class, and eliminte ShadowElement dependency.

"Introduces" and "eliminates".

> Source/WebCore/ChangeLog:11
> +        With these classes, the shadow tree has reshaped from the four-flat-children to four-children-with-a-parent.

is reshaped

> Source/WebCore/css/html.css:662
> +    position: absolute;

Absolutely positioning seems really gross here. Why are we doing this?

> Source/WebCore/dom/Node.h:571
> +    virtual void updateFromHost() {}

Noooooooo!!!

> Source/WebCore/html/HTMLMeterElement.cpp:75
> +    didElementStateChange();

You should not need this call. attach propagates down to the shadow DOM.

> Source/WebCore/html/HTMLMeterElement.cpp:215
> +void HTMLMeterElement::didElementStateChange()
> +{
> +    if (renderer())
> +        renderer()->updateFromElement();
> +    if (Node* root = shadowRoot()) {
> +        for (Node* node = root; node; node = node->traverseNextNode(root))
> +            node->updateFromHost();
> +    }
> +}

This should be completely unnecessary now.

> Source/WebCore/html/shadow/MeterPartElement.h:85
> +    DEFINE_STATIC_LOCAL(AtomicString, pseudoIdHorizontalValueSuboptimal, ("-webkit-meter-horizontal-suboptimal-value"));
> +    DEFINE_STATIC_LOCAL(AtomicString, pseudoIdHorizontalValueEvenLessGood, ("-webkit-meter-horizontal-even-less-good-value"));
> +    DEFINE_STATIC_LOCAL(AtomicString, pseudoIdVerticalBar, ("-webkit-meter-vertical-bar"));
> +    DEFINE_STATIC_LOCAL(AtomicString, pseudoIdVerticalValueOptimal, ("-webkit-meter-vertical-optimum-value"));
> +    DEFINE_STATIC_LOCAL(AtomicString, pseudoIdVerticalValueSuboptimal, ("-webkit-meter-vertical-suboptimal-value"));
> +    DEFINE_STATIC_LOCAL(AtomicString, pseudoIdVerticalValueEvenLessGood, ("-webkit-meter-vertical-even-less-good-value"));

This is gross too. If you want polymorphism, that's what subclassing is for.

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