Bug 145490 – Make RangeInputType into a three-part flexible box, replacing crufty layout code

• Component: New Bugs
• Status: NEW

[Jer Noble]

Make RangeInputType into a three-part flexible box, replacing crufty layout code

[Jer Noble]

Created attachment 253952 [details]
Patch

[Jer Noble]

Will fix up the efl-wk2 error in a subsequent patch.

[Build Bot]

Comment on attachment 253952 [details]
Patch

Attachment 253952 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4720096303906816

New failing tests:
fast/forms/box-shadow-override.html
fast/forms/input-appearance-height.html
fast/forms/range/range-change-min-max.html

[Build Bot]

Created attachment 253954 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 253952 [details]
Patch

Attachment 253952 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5103627219763200

New failing tests:
fast/forms/box-shadow-override.html
fast/forms/input-appearance-height.html
fast/forms/range/range-change-min-max.html

[Build Bot]

Created attachment 253957 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Jer Noble]

Created attachment 254003 [details]
Patch

[Jer Noble]

Added a couple of mac-mavericks rebaselines, and fixed an issue where the tracks children weren't being marked as needing layout.

[Jer Noble]

Created attachment 254007 [details]
Patch

[Darin Adler]

Comment on attachment 254007 [details]
Patch

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

> Source/WebCore/html/HTMLInputElement.h:154
>      HTMLElement* sliderThumbElement() const;
>      HTMLElement* sliderTrackElement() const;
> +    HTMLElement* sliderMinTrackElement() const;
> +    HTMLElement* sliderMaxTrackElement() const;

Exposing all of these elements in public functions seems like a particularly bad way to create a relationship between type-specific renderer classes and HTMLInputElement. I see that you are participating in a grand tradition of questionable design here, but I still don’t like it.

> Source/WebCore/rendering/RenderSlider.cpp:121
> +    HTMLElement* sliderTrackElement = element().sliderTrackElement();
> +    HTMLElement* minTrackElement = element().sliderMinTrackElement();
> +    HTMLElement* maxTrackElement = element().sliderMaxTrackElement();
> +
> +    ASSERT(sliderTrackElement);
> +    ASSERT(minTrackElement);
> +    ASSERT(maxTrackElement);

Can we get at these some other way than making them public functions on HTMLInputElement? Let me put this another way. This function clearly only needs to make style changes to these elements. So could we expose functions that return RenderStyle& instead of functions that return HTMLElement? Or we could add functions that expose the renderers. And could we make RenderSlider a friend of HTMLInputElement so these could be private functions?

If we were going to keep the ones that return elements I would use a return type of HTMLElement& and have the assertion inside the function rather than at the call site.

[Jer Noble]

(In reply to comment #10)
> Comment on attachment 254007 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254007&action=review
> 
> > Source/WebCore/html/HTMLInputElement.h:154
> >      HTMLElement* sliderThumbElement() const;
> >      HTMLElement* sliderTrackElement() const;
> > +    HTMLElement* sliderMinTrackElement() const;
> > +    HTMLElement* sliderMaxTrackElement() const;
> 
> Exposing all of these elements in public functions seems like a particularly
> bad way to create a relationship between type-specific renderer classes and
> HTMLInputElement. I see that you are participating in a grand tradition of
> questionable design here, but I still don’t like it.

Yes, but I'm trying to make it less questionable, so this would be in the new tradition of less questionable design. :)

Another option would be to put these methods on RangeInputType, and make RangeInput downcast-able from InputType.

> > Source/WebCore/rendering/RenderSlider.cpp:121
> > +    HTMLElement* sliderTrackElement = element().sliderTrackElement();
> > +    HTMLElement* minTrackElement = element().sliderMinTrackElement();
> > +    HTMLElement* maxTrackElement = element().sliderMaxTrackElement();
> > +
> > +    ASSERT(sliderTrackElement);
> > +    ASSERT(minTrackElement);
> > +    ASSERT(maxTrackElement);
> 
> Can we get at these some other way than making them public functions on
> HTMLInputElement?

Yes, let me try the option I mention above.

> Let me put this another way. This function clearly only
> needs to make style changes to these elements. So could we expose functions
> that return RenderStyle& instead of functions that return HTMLElement? Or we
> could add functions that expose the renderers. And could we make
> RenderSlider a friend of HTMLInputElement so these could be private
> functions?

I'll keep that in mind if I can't move these methods to RangeInputType.

> If we were going to keep the ones that return elements I would use a return
> type of HTMLElement& and have the assertion inside the function rather than
> at the call site.

I think this is a great idea regardless.

[Jer Noble]

Created attachment 254022 [details]
Patch for landing

[Jer Noble]

Created attachment 254025 [details]
Patch for landing