[webkit-reviews] review granted: [Bug 52317] Flip input[type=range] to use the new shadow DOM model. : [Attachment 78720] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 52317: Flip input[type=range] to use the new shadow DOM model.
https://bugs.webkit.org/show_bug.cgi?id=52317

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=78720&action=review

Looks good.

> Source/WebCore/html/InputType.h:181
>      virtual bool canBeSuccessfulSubmitButton();
>  
> +    // Shadow tree handling
> +    virtual void createShadowSubtree();
> +    void destroyShadowSubtree();
> +
>      // Miscellaneous functions
>  
>      virtual bool rendererIsNeeded();

Formatting here is inconsistent. Number of blank lines (I prefer to never use
more than one). Whether there is a blank line after a comment before the
paragraph of functions it describes.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:44
> +// FIXME: Find a way to cascade appearance and get rid of this class.

I think this would be clearer if you were a bit more explicit and specific.

> Source/WebCore/rendering/MediaControlElements.cpp:474
> +    // FIXME: Remove this once all media controls are transitioned to use
the regular
> +    // style calculation.

Seems a bit cryptic, but maybe someone more expert than I would understand.

> Source/WebCore/rendering/RenderSlider.h:49
> +	   // FIXME: Eventually, accessing sliderThumbElement should not be
necessary in this class.

It would be good to say why it would not be necessary. What is it currently
used for? Why would that no longer be needed?