[webkit-reviews] review denied: [Bug 38140] Initial support for HTMLMeterElement : [Attachment 54921] Patch adding more tests and removing redundant isnan calls

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Yael <yael.aharon at nokia.com>'s request
for review:
Bug 38140: Initial support for HTMLMeterElement
https://bugs.webkit.org/show_bug.cgi?id=38140

Attachment 54921: Patch adding more tests and removing redundant isnan calls
https://bugs.webkit.org/attachment.cgi?id=54921&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
The tests that expect exception should use shouldThrow instead of being
hand-written.

Why do we need a derived class, RenderMeter, instead of just using a
RenderBlock? For the custom layout function?

> +	   case MeterPart:
> +#if ENABLE(METER_TAG)
> +	       m_value.ident = CSSValueMeter;
> +#endif
> +	       break;

This doesn't seem right. With the ifdef written like this won't m_value.ident
be uninitialized in this case?

> +HTMLMeterElement::HTMLMeterElement(const QualifiedName& tagName, Document*
document, HTMLFormElement* form)
> +    : HTMLFormControlElement(tagName, document, form, CreateElement)
> +{
> +    ASSERT(hasTagName(meterTag));
> +}

Does this form element really need the HTMLFormElement argument to the
constructor? If so, I'd like to see tests showing that it does. Did you add any
tests that would fail if we passed 0 for the form?

> +void HTMLMeterElement::parseMappedAttribute(MappedAttribute* attr)

In new code, please use words instead of abbreviations whenever possible, as
they are typically easier to read even if longer. This should be "attribute",
not "attr".

> +    if (attr->name() == valueAttr) {
> +	   if (renderer())
> +	       renderer()->updateFromElement();
> +    } else if (attr->name() == minAttr) {
> +	   if (renderer())
> +	       renderer()->updateFromElement();
> +    } else if (attr->name() == maxAttr) {
> +	   if (renderer())
> +	       renderer()->updateFromElement();
> +    } else if (attr->name() == lowAttr) {
> +	   if (renderer())
> +	       renderer()->updateFromElement();
> +    } else if (attr->name() == highAttr) {
> +	   if (renderer())
> +	       renderer()->updateFromElement();
> +    } else if (attr->name() == optimumAttr) {
> +	   if (renderer())
> +	       renderer()->updateFromElement();

Can we find a less repetitive way of writing this?

> +    double value;
> +    bool ok = parseToDoubleForNumberType(getAttribute(valueAttr), &value);
> +    if (!ok)
> +	   value = 0;

Can't you write that like this?

    double value = 0;
    parseToDoubleForNumberType(getAttribute(valueAttr), &value);

I think that's simpler.

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

> +    // Some platforms do not have a native gauge widget, so we draw here a
default implementation.

It's strange to have a comment here partway through the function saying this.
Isn't this comment the entire reason for the whole function?

> +    FloatRect valueRect;
> +    FloatRect backgroundRect;
> +    if (min >= max) {
> +	   paintInfo.context->fillRect(innerRect, Color::black,
DeviceColorSpace);
> +	   return false;
> +    }

The definitions of the two rectangles should go further down, after this early
return.

> +    if (rect.width() < rect.height()) {
> +	   // Vertical gauge
> +	   double scale = innerRect.height() / (max - min);
> +	   valueRect.setLocation(FloatPoint(innerRect.x(), innerRect.y() + (max
- value) * scale));
> +	   valueRect.setSize(FloatSize(innerRect.width(), (value - min) *
scale));
> +	   backgroundRect.setLocation(innerRect.location());
> +	   backgroundRect.setSize(FloatSize(innerRect.width(), (max - value) *
scale));
> +    } else if (renderMeter->style()->direction() == RTL) {
> +	   // right to left horizontal gauge
> +	   double scale = innerRect.width() / (max - min);
> +	   valueRect.setLocation(FloatPoint(innerRect.x() + (max - value) *
scale, innerRect.y()));
> +	   valueRect.setSize(FloatSize((value - min) * scale,
innerRect.height()));
> +	   backgroundRect.setLocation(innerRect.location());
> +	   backgroundRect.setSize(FloatSize((max - value) * scale,
innerRect.height()));
> +    } else {
> +	   // left to right horizontal gauge
> +	   double scale = innerRect.width() / (max - min);
> +	   valueRect.setLocation(innerRect.location());
> +	   valueRect.setSize(FloatSize((value - min) * scale,
innerRect.height()));
> +	   backgroundRect.setLocation(FloatPoint(innerRect.x() +
valueRect.width(), innerRect.y()));
> +	   backgroundRect.setSize(FloatSize((max - value) * scale,
innerRect.height()));
> +    }
> +    if (!valueRect.isEmpty())
> +	   paintInfo.context->fillRect(valueRect, Color::black,
DeviceColorSpace);
> +
> +    if (!backgroundRect.isEmpty())
> +	   paintInfo.context->fillRect(backgroundRect, Color::lightGray,
DeviceColorSpace);

I am not sure this basic approach is solid enough. I suggest painting the
entire rect with the background and then have the value rect painted on top of
it. Generally speaking I don't know how the rectangles computed here will end
up stitching together when the edges are not on pixel boundaries.

Also, to compute the rectangles you should only multiple by the scale factor
once. The rectangle on the right or bottom should be sized to fill the rest of
the space rather than separately doing "max - value" computation.

It's not correct to pass DeviceColorSpace for all these colors. Instead you
should be passing style()->colorSpace().

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.