[webkit-reviews] review granted: [Bug 35937] Support for HTMLProgressElement : [Attachment 50575] Patch v8

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yael <yael.aharon at nokia.com>'s
request for review:
Bug 35937: Support for HTMLProgressElement
https://bugs.webkit.org/show_bug.cgi?id=35937

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

------- Additional Comments from Darin Adler <darin at apple.com>
We need to get the looks for progress bars implemented on all platforms as soon
as possible. It's not good to have all these conditionals. Generally speaking,
though we have many of them, conditional features are bad for the WebKit
project and the web platform!

> +void HTMLProgressElement::parseMappedAttribute(MappedAttribute* attr)
> +{
> +    if (attr->name() == valueAttr) {
> +	   setNeedsStyleRecalc();
> +    } else if (attr->name() == maxAttr) {
> +	   setNeedsStyleRecalc();
> +    } else
> +	   HTMLFormControlElement::parseMappedAttribute(attr);
> +}

Please use "attribute" rather than "attr".

There should not be braces around these single-line if statement bodies. Not
sure why the style checking script doesn't know that.

I don't understand why setNeedsStyleRecalc is the right thing to do here. How
did you decide that was the right call? Do these attributes really change
style? Is there a comparable call site elsewhere showing this is correct?

It's important to do precisely the minimum amount of calculation redrawing when
the value or max of a progress element changes, because this is the use case
for progress elements and we want good efficiency!

> +void RenderProgress::updateFromElement()
> +{
> +    setNeedsLayoutAndPrefWidthsRecalc();
> +}

Is this correct? How would a change to the element affect the layout and
preferred widths?

Some of the strangest rules in this class are the rules for maximums of less
than 1. For example, a maximum of 0.5 sets maximum to 0.5 but a maximum of 0
sets maximum to 1. I'd like to see those test cases covered.

I'm going to say r=me, because I think this is OK in its current form.

I have doubts that the way the DOM is connected to the rendering tree and the
way that repainting is triggered is optimal here. When the progress element
gets progressively larger numbers we should be able to see the minimum
repainting work -- this will affect things like performance battery life.