[Webkit-unassigned] [Bug 35937] Support for HTMLProgressElement

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Mar 14 14:25:50 PDT 2010


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
  Attachment #50575|review?                     |review+
               Flag|                            |

--- Comment #24 from Darin Adler <darin at apple.com>  2010-03-14 14:25:49 PST ---
(From update of attachment 50575)
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.

Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

More information about the webkit-unassigned mailing list