[webkit-reviews] review requested: [Bug 38140] Initial support for HTMLMeterElement : [Attachment 55016] Patch addressing comment #19

[bugzilla-daemon at webkit.org]

Yael <yael.aharon at nokia.com> has asked	for review:
Bug 38140: Initial support for HTMLMeterElement
https://bugs.webkit.org/show_bug.cgi?id=38140

Attachment 55016: Patch addressing comment #19
https://bugs.webkit.org/attachment.cgi?id=55016&action=review

------- Additional Comments from Yael <yael.aharon at nokia.com>
(In reply to comment #19)
> (From update of attachment 54921 [details])
> Why do we need a derived class, RenderMeter, instead of just using a
> RenderBlock? For the custom layout function?
RenderMeter does a custom layout and also triggers a repaint when an attribute
of the meter element changes. As the implementation of the meter element
becomes more complete, we would need more support from this renderer. 

> > +	 return std::min(std::max(value, min()), max());
> 
> When I write this I like to order the values like this:
> 
>     std::max(min(), std::min(value, max()))
> 
> It seems to make it more obvious that we are putting the value between the
min
> and max values, but also my ordering makes the min win if the min and max
> conflict, which is typically what we want. I guess that you instead made the
> max() function call the min() function which is OK if that's what the
standard
> requires.
> 
> As long as we have enough test cases I think we're OK either way.

>From the user agent requirements for the meter element:

User agents must then use all these numbers to obtain values for six points on
the gauge, as follows. (The order in which these are evaluated is important, as
some of the values refer to earlier ones.)

I wanted to follow the order as defined by the spec. I added 4 tests to test
that the calculation is correct. Please see the tests marked "Set attributes to
improper values - 1 through 4".

> What does the return value of this function mean? Is it right for the theme
to
> always paint? Shouldn't the non-theme-specific painting be in the renderer
> class, not the theme at all? I think the theme should be returning true like
> the other functions such as paintProgressBar.

The return value is used by RenderBox::paintBoxDecorationsWithSize to determine
if the RenderTheme painted or not. In case of the progress element, RenderTheme
does not paint, thus returns true. Each platform's RenderTheme would return
false. In case of paintMeter, we have a default implementation that does paint,
so it should return false.
>From comment #11 I understand that some platforms will want to overwrite
Rendertheme::paintMeter, and that is why this method is implemented in
RenderTheme and not in RenderMeter.