Bug 50661

Created attachment 81576 [details] Patch

Hi dimitri, could you take a look?

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

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.

Created attachment 88082 [details] Patch

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()->

Here's what I am thinking: http://jsfiddle.net/8hpem/

Created attachment 88117 [details] Patch

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

Attachment 88117 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8332101

Created attachment 88132 [details] Patch

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 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());

> 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!

Committed r82880: <http://trac.webkit.org/changeset/82880>

http://trac.webkit.org/changeset/82880 might have broken GTK Linux 32-bit Release

Reverted r82880 for reason: Meter elements not rendered in Chromium Linux tests Committed r82889: <http://trac.webkit.org/changeset/82889>

(In reply to comment #19) > Reverted r82880 for reason: > > Meter elements not rendered in Chromium Linux tests > > Committed r82889: <http://trac.webkit.org/changeset/82889> I saw your IRC message that you planned to rebaseline other tests, but the Linux tests looked truly incorrect, so I just rolled this out. It also looks like the Windows test are wrong as well. Here are the tests that were failing: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#master=ChromiumWebkit&tests=fast/dom/HTMLMeterElement/meter-appearances-capacity.html,fast/dom/HTMLMeterElement/meter-appearances-rating-relevancy.html,fast/dom/HTMLMeterElement/meter-boundary-values.html,fast/dom/HTMLMeterElement/meter-element.html,fast/dom/HTMLMeterElement/meter-optimums.html,fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html,fast/dom/HTMLMeterElement/meter-styles.html,fast/forms/input-appearance-spinbutton-disabled-readonly.html,fast/forms/input-appearance-spinbutton-layer.html,fast/forms/input-appearance-spinbutton-visibility.html,fast/speech/input-appearance-numberandspeech.html

Created attachment 88161 [details] My last minute change was wrong...

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?

> > What's this? It looks I did a miserably worng git commit --amend... I'm sorry for that, will fix shortly.

Created attachment 88173 [details] Patch

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?

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

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

Attachment 88173 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8334143

> > 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 :/

> > And build failed... I lost a header inclusion :/ I'll fix this and land anyway. Thank you for your feedback!

This patch is cursed!!!

Committed r82899: <http://trac.webkit.org/changeset/82899>

Pray the commit cures us...

http://trac.webkit.org/changeset/82899 might have broken GTK Linux 32-bit Debug

(In reply to comment #35) > http://trac.webkit.org/changeset/82899 might have broken GTK Linux 32-bit Debug This patch on its own broke a number of layout tests: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#master=ChromiumWebkit&tests=fast/dom/HTMLMeterElement/meter-appearances-capacity.html,fast/dom/HTMLMeterElement/meter-appearances-rating-relevancy.html,fast/dom/HTMLMeterElement/meter-boundary-values.html,fast/dom/HTMLMeterElement/meter-element.html,fast/dom/HTMLMeterElement/meter-optimums.html,fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html,fast/dom/HTMLMeterElement/meter-styles.html,fast/forms/input-appearance-spinbutton-disabled-readonly.html,fast/forms/input-appearance-spinbutton-layer.html,fast/forms/input-appearance-spinbutton-visibility.html,fast/speech/input-appearance-numberandspeech.html I marked all of these as failing here: http://trac.webkit.org/changeset/82902/trunk/LayoutTests/platform/chromium/test_expectations.txt 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.

(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 :-)

(In reply to comment #37) > A change for CSSSelector.cpp unexpectedly removed a case label :-) Fixed: http://trac.webkit.org/changeset/82904

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

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

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.

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

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

Will update as Bug 58149.

Fixed, right?

(In reply to comment #45) > Fixed, right? Right, I tend to forget to update metabugs....