Bug 50661

Summary: Convert <meter> shadow DOM to a DOM-based shadow.
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, dominicc, enne, eric, rolandsteiner, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 54440, 56001, 58149, 59281    
Bug Blocks: 48698, 53417    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
My last minute change was wrong...
none
Patch tkent: review+

Description Hajime Morrita 2010-12-07 18:22:26 PST
<meter> is holding its shadows as a member variables of  RenderMeter. 
We could move them to HTMLMeterElement.
Comment 1 Hajime Morrita 2011-02-07 21:05:00 PST
Created attachment 81576 [details]
Patch
Comment 2 Hajime Morrita 2011-02-07 21:05:47 PST
Hi dimitri, could you take a look?
Comment 3 Kent Tamura 2011-02-07 21:13:56 PST
Comment on attachment 81576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81576&action=review

Some drive-by comments.

> Source/WebCore/ChangeLog:22
> +        Need a short description and bug URL (OOPS!)

oops!

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:22454
> +				0F3DD45012F5EA1B000D9190 /* ShadowBlur.h in Headers */,
> +				BCE4413412F748E2009B84B8 /* RenderCombineText.h in Headers */,
> +				BCE4413612F7490B009B84B8 /* FontWidthVariant.h in Headers */,
> +				14947FFE12F80CD200A0F631 /* DocumentOrderedMap.h in Headers */,
> +				97C471DC12F925BD0086354B /* ContentSecurityPolicy.h in Headers */,
> +				A7FE819C12FA677700850C1E /* ProgressBarValueElement.h in Headers */,

Do you want to add these files?

> Source/WebCore/html/shadow/MeterPartElement.h:53
> +    MeterPartElement(Document* document, PartType type) 

This should be private.

> Source/WebCore/html/shadow/MeterPartElement.h:76
> +inline const AtomicString* MeterPartElement::computeShadowPseudoId()

Do not define such large functions in a header file.
Comment 4 Dimitri Glazkov (Google) 2011-02-07 21:26:36 PST
Comment on attachment 81576 [details]
Patch

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.
Comment 5 Hajime Morrita 2011-02-08 02:02:21 PST
Kent-san, Dimitri, thank you for reviewing quickly!

> 
> > Source/WebCore/css/html.css:662
> > +    position: absolute;
> 
> Absolutely positioning seems really gross here. Why are we doing this?
I found this cause a tricky crash according to custom layout algorithm on related Render classes.
I'm considering to use normal layout code instead of custom one, with just rewriting RenderStyle values.
The patch will come later.
Comment 6 Hajime Morrita 2011-04-04 11:03:32 PDT
Created attachment 88082 [details]
Patch
Comment 7 Dimitri Glazkov (Google) 2011-04-04 11:39:35 PDT
Comment on attachment 88082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88082&action=review

Two concerns:

1) Do we really need filling element? Can't the meter itself act as the filling?
2) creating subtree on attach seems wrong. We'll be adding DOM elements at the time when we're supposed to be populating the render tree.

> LayoutTests/ChangeLog:9
> +        Also updated meter-styles.html pixel result because the render tree is now layouted

layouted -> laid out

> Source/WebCore/ChangeLog:15
> +          -webkit-appeearance is "none" and the platform supports <meter>.

-webkit-appearance

> Source/WebCore/ChangeLog:18
> +          indicators are painted using the platform theme engine as same
> +          old.

as before.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:615
> +bool CSSMutableStyleDeclaration::setProperty(int propertyID, double value, CSSPrimitiveValue::UnitTypes unit, bool important, bool notifyChanged)
> +{
> +    CSSProperty property(propertyID, CSSPrimitiveValue::create(value, unit), important);
> +    setPropertyInternal(property);
> +    if (notifyChanged)
> +        setNeedsStyleRecalc();
> +    return true;
> +}

Do you need this change? Can you not just use

> Source/WebCore/html/shadow/MeterShadowElement.cpp:52
> +    getInlineStyleDecl()->setProperty(CSSPropertyWebkitBoxFlex, flex, CSSPrimitiveValue::CSS_NUMBER);

can be just style()->
Comment 8 Dimitri Glazkov (Google) 2011-04-04 12:43:36 PDT
Here's what I am thinking: http://jsfiddle.net/8hpem/
Comment 9 Hajime Morrita 2011-04-04 13:54:40 PDT
Created attachment 88117 [details]
Patch
Comment 10 Hajime Morrita 2011-04-04 14:01:09 PDT
Hi Dimitri, thank you for lightning feedback!
I updated patch for address yours.


> 1) Do we really need filling element? Can't the meter itself act as the filling?
As you mentioned on the demo, I removed filling and use box-sizing and relative size specification
instead.

> 2) creating subtree on attach seems wrong. We'll be adding DOM elements at the time when we're supposed to be populating the render tree.
Sure. I moved shadow creation timing into HTMLMeterElement::create().

> 
> > LayoutTests/ChangeLog:9
> > +        Also updated meter-styles.html pixel result because the render tree is now layouted
> 
> layouted -> laid out
Fixed.

> 
> > Source/WebCore/ChangeLog:15
> > +          -webkit-appeearance is "none" and the platform supports <meter>.
> 
> -webkit-appearance
Fixed.

> 
> > Source/WebCore/ChangeLog:18
> > +          indicators are painted using the platform theme engine as same
> > +          old.
> 
> as before.
Fixed.

> 
> > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:615
> > +bool CSSMutableStyleDeclaration::setProperty(int propertyID, double value, CSSPrimitiveValue::UnitTypes unit, bool important, bool notifyChanged)
> > +{
> > +    CSSProperty property(propertyID, CSSPrimitiveValue::create(value, unit), important);
> > +    setPropertyInternal(property);
> > +    if (notifyChanged)
> > +        setNeedsStyleRecalc();
> > +    return true;
> > +}
> 
> Do you need this change? Can you not just use
> 
> > Source/WebCore/html/shadow/MeterShadowElement.cpp:52
> > +    getInlineStyleDecl()->setProperty(CSSPropertyWebkitBoxFlex, flex, CSSPrimitiveValue::CSS_NUMBER);
> 
> can be just style()->
Non-mutable CSSStyleDeclaration doesn't have typed property access. It has only string-based API.
Comment 11 Dimitri Glazkov (Google) 2011-04-04 14:07:31 PDT
Comment on attachment 88117 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88117&action=review

This is super-close! Thanks for working on this.

> LayoutTests/ChangeLog:10
> +        as normal flexboxes and its layout result is actually different,

This probably needs an update.

> Source/WebCore/html/HTMLMeterElement.cpp:52
> +    RefPtr<HTMLMeterElement> creating = adoptRef(new HTMLMeterElement(tagName, document, form));

creating -> meter

> Source/WebCore/html/HTMLMeterElement.h:61
> +    bool shouldShadowHaveRenderer() const;

You can now use rendererIsNeeded here.

> Source/WebCore/html/HTMLMeterElement.h:74
> +    void setupShadow();

Can we call it createShadowSubtree, for consistency? That's what I called it in InputType.

> Source/WebCore/html/shadow/MeterShadowElement.h:62
> +    static PassRefPtr<MeterBarElement> create(Document* document) { return adoptRef(new MeterBarElement(document)); }

Move to an inline definition right after the class decl. See http://google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/shadow/SliderThumbElement.h&l=76

> Source/WebCore/html/shadow/MeterShadowElement.h:73
> +    static PassRefPtr<MeterValueElement> create(Document* document) { return adoptRef(new MeterValueElement(document)); }

Ditto.
Comment 12 WebKit Review Bot 2011-04-04 14:31:46 PDT
Attachment 88117 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8332101
Comment 13 Hajime Morrita 2011-04-04 14:35:16 PDT
Created attachment 88132 [details]
Patch
Comment 14 Hajime Morrita 2011-04-04 14:37:26 PDT
Dimitri, thank you for another turn!

(In reply to comment #11)
> 
> > LayoutTests/ChangeLog:10
> > +        as normal flexboxes and its layout result is actually different,
> 
> This probably needs an update.
Updated the ChangeLog description.

> 
> > Source/WebCore/html/HTMLMeterElement.cpp:52
> > +    RefPtr<HTMLMeterElement> creating = adoptRef(new HTMLMeterElement(tagName, document, form));
> 
> creating -> meter
renamed.

> 
> > Source/WebCore/html/HTMLMeterElement.h:61
> > +    bool shouldShadowHaveRenderer() const;
> 
> You can now use rendererIsNeeded here.
Good point. I moved this and related code to MeterShadowElement.

> 
> > Source/WebCore/html/HTMLMeterElement.h:74
> > +    void setupShadow();
> 
> Can we call it createShadowSubtree, for consistency? That's what I called it in InputType.
> 
Sure.

> > Source/WebCore/html/shadow/MeterShadowElement.h:62
> > +    static PassRefPtr<MeterBarElement> create(Document* document) { return adoptRef(new MeterBarElement(document)); }
> 
> Move to an inline definition right after the class decl. See http://google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/shadow/SliderThumbElement.h&l=76
> 
> > Source/WebCore/html/shadow/MeterShadowElement.h:73
> > +    static PassRefPtr<MeterValueElement> create(Document* document) { return adoptRef(new MeterValueElement(document)); }
> 
> Ditto.
Done.
Comment 15 Dimitri Glazkov (Google) 2011-04-04 14:40:59 PDT
Comment on attachment 88132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88132&action=review

lovely, with one more tweak:

> Source/WebCore/html/shadow/MeterShadowElement.cpp:60
> +    return meterRenderer && meterRenderer->shouldHaveParts() && HTMLDivElement::rendererIsNeeded(style);

I think you can kill RenderMeter::shouldHaveParts altogether and call RenderTheme here directly:

return document()->page()->theme()->supportsMeter(style->appearance());
Comment 16 Hajime Morrita 2011-04-04 14:54:46 PDT
> I think you can kill RenderMeter::shouldHaveParts altogether and call RenderTheme here directly:
> 
> return document()->page()->theme()->supportsMeter(style->appearance());
Okay, I'll do it before landing. Thanks for the polish!
Comment 17 Hajime Morrita 2011-04-04 15:16:08 PDT
Committed r82880: <http://trac.webkit.org/changeset/82880>
Comment 18 WebKit Review Bot 2011-04-04 15:30:09 PDT
http://trac.webkit.org/changeset/82880 might have broken GTK Linux 32-bit Release
Comment 19 Adrienne Walker 2011-04-04 16:18:00 PDT
Reverted r82880 for reason:

Meter elements not rendered in Chromium Linux tests

Committed r82889: <http://trac.webkit.org/changeset/82889>
Comment 21 Hajime Morrita 2011-04-04 16:55:22 PDT
Created attachment 88161 [details]
My last minute change was wrong...
Comment 22 Kent Tamura 2011-04-04 17:15:29 PDT
Comment on attachment 88161 [details]
My last minute change was wrong...

View in context: https://bugs.webkit.org/attachment.cgi?id=88161&action=review

> Tools/ChangeLog:11
> +2011-04-04  Tony Chang  <tony@chromium.org>
> +
> +        Reviewed by Ojan Vafai.
> +
> +        [chromium] don't write .checksum files if a fallback platform has an embedded checksum
> +        https://bugs.webkit.org/show_bug.cgi?id=57783
> +
> +        * Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:
> +        * Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests_unittest.py:
> +
>  2011-04-04  Kevin Ollivier  <kevino@theolliviers.com>

What's this?
Comment 23 Hajime Morrita 2011-04-04 17:22:15 PDT
> 
> What's this?
It looks I did a miserably worng git commit --amend...
I'm sorry for that, will fix shortly.
Comment 24 Hajime Morrita 2011-04-04 17:38:30 PDT
Created attachment 88173 [details]
Patch
Comment 25 Kent Tamura 2011-04-04 18:04:17 PDT
Comment on attachment 88173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88173&action=review

The difference from r82880 is MeterShadowElement::rendererIsNeeded(), right?

> Source/WebCore/html/HTMLMeterElement.cpp:219
> +    return (value - min) / (max - min);

Is it possible to return Infinity?
Comment 26 Hajime Morrita 2011-04-04 18:22:11 PDT
Kent-san, thank you for our help!

(In reply to comment #25)
> (From update of attachment 88173 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88173&action=review
>
> The difference from r82880 is MeterShadowElement::rendererIsNeeded(), right?
Yes, it should.

> 
> > Source/WebCore/html/HTMLMeterElement.cpp:219
> > +    return (value - min) / (max - min);
> 
> Is it possible to return Infinity?
Such case is guarded by previous statement.
Comment 27 Kent Tamura 2011-04-04 18:25:02 PDT
Comment on attachment 88173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88173&action=review

>>> Source/WebCore/html/HTMLMeterElement.cpp:219
>>> +    return (value - min) / (max - min);
>> 
>> Is it possible to return Infinity?
> 
> Such case is guarded by previous statement.

Really?
Suppose that max=2.2e-308, min=0, value=1.79e+308
Comment 28 Kent Tamura 2011-04-04 18:32:44 PDT
(In reply to comment #27)
> (From update of attachment 88173 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88173&action=review
> 
> >>> Source/WebCore/html/HTMLMeterElement.cpp:219
> >>> +    return (value - min) / (max - min);
> >> 
> >> Is it possible to return Infinity?
> > 
> > Such case is guarded by previous statement.
> 
> Really?
> Suppose that max=2.2e-308, min=0, value=1.79e+308

Anyway, it's not new code of this patch.  It's ok to commit the patch.
Comment 29 WebKit Review Bot 2011-04-04 18:43:47 PDT
Attachment 88173 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8334143
Comment 30 Hajime Morrita 2011-04-04 18:48:08 PDT
> > Really?
> > Suppose that max=2.2e-308, min=0, value=1.79e+308
> 
value() is clamped by min() and max(). So it should work, in my understanding....

Just in case, I tested against following HTML, and it looks to work.

<html>
  <body>
    <meter min="0" max="2.2e-308" value="1.79e+308">
  <body>
</html>

And build failed... I lost a header inclusion :/
Comment 31 Hajime Morrita 2011-04-04 18:48:54 PDT
> 
> And build failed... I lost a header inclusion :/

I'll fix this and land anyway.  Thank you for your feedback!
Comment 32 Dimitri Glazkov (Google) 2011-04-04 18:49:09 PDT
This patch is cursed!!!
Comment 33 Hajime Morrita 2011-04-04 19:02:08 PDT
Committed r82899: <http://trac.webkit.org/changeset/82899>
Comment 34 Hajime Morrita 2011-04-04 19:02:42 PDT
Pray the commit cures us...
Comment 35 WebKit Review Bot 2011-04-04 21:52:25 PDT
http://trac.webkit.org/changeset/82899 might have broken GTK Linux 32-bit Debug
Comment 37 Kent Tamura 2011-04-04 22:14:38 PDT
(In reply to comment #36)
> Some of them should just be an easy rebaseline, but I am not sure about why the spinbutton tests were failing from this meter change.  So, I wanted somebody else's eye on it instead of just rebaselining the whole lot of them blindly.

A change for CSSSelector.cpp unexpectedly removed a case label :-)
Comment 38 Kent Tamura 2011-04-04 22:27:38 PDT
(In reply to comment #37)
> A change for CSSSelector.cpp unexpectedly removed a case label :-)

Fixed: http://trac.webkit.org/changeset/82904
Comment 39 WebKit Review Bot 2011-04-04 22:53:15 PDT
http://trac.webkit.org/changeset/82904 might have broken Qt Linux Release
The following tests are not passing:
fast/ruby/overhang-horizontal.html
fast/ruby/overhang-vertical.html
Comment 40 Hajime Morrita 2011-04-05 11:00:59 PDT
(In reply to comment #38)
> (In reply to comment #37)
> > A change for CSSSelector.cpp unexpectedly removed a case label :-)
> 
> Fixed: http://trac.webkit.org/changeset/82904
OMG!!! thanks for fixing this....
Comment 42 Hajime Morrita 2011-04-08 09:12:06 PDT
(In reply to comment #41)
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=meter-element.html%2Cmeter-styles.html&showExpectations=true&showLargeExpectations=true&group=%40ToT%20-%20chromium.org
> 
> Chromium results look incorrect.
Woa, I'll take a look. Thank you for noticing this.
Comment 43 Hajime Morrita 2011-04-08 11:18:58 PDT
> > 
> > Chromium results look incorrect.
> Woa, I'll take a look. Thank you for noticing this.
The test is wrong. too-thick borders and margins is hiding content area of meter.
I'll update the test.
Comment 44 Hajime Morrita 2011-04-08 11:56:02 PDT
Will update as Bug 58149.
Comment 45 Dimitri Glazkov (Google) 2011-06-04 13:47:36 PDT
Fixed, right?
Comment 46 Hajime Morrita 2011-06-05 20:17:00 PDT
(In reply to comment #45)
> Fixed, right?
Right, I tend to forget to update metabugs....