[webkit-reviews] review denied: [Bug 50661] Convert <meter> shadow DOM to a DOM-based shadow. : [Attachment 88117] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied MORITA Hajime
<morrita at google.com>'s request for review:
Bug 50661: Convert <meter> shadow DOM to a DOM-based shadow.
https://bugs.webkit.org/show_bug.cgi?id=50661

Attachment 88117: Patch
https://bugs.webkit.org/attachment.cgi?id=88117&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
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/WebCor
e/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.