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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=35937


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #50543|review?                     |review-
               Flag|                            |




--- Comment #20 from Darin Adler <darin at apple.com>  2010-03-11 16:58:22 PST ---
(From update of attachment 50543)
> +    if (attr->name() == valueAttr) {
> +        // do nothing

If there is no code here, what the progress bar to repaint?

> +    } else if (attr->name() == maxAttr) {
> +        // do nothing

Same question here.

> +    if (hasAttribute(valueAttr)) {
> +        bool ok;
> +        double value = getAttribute(valueAttr).toDouble(&ok);
> +        if (!ok || value < 0)
> +            return 0;
> +        return (value > max()) ? max() : value;
> +    }
> +    return 1;
> +}

It's unnecessarily inefficient to always check hasAttribute(valueAttr). There
are two things you could do to tighten this up.

    1) Put the hasAttribute check inside the error handling code, just before
the "return 0".

    2) Put the result of getAttribute(valueAttr) into a local variable, and use
it so you don't do a second hash table lookup.

If you did both, the code would look like this:

    const AtomicString& valueString = getAttribute(valueAttr);
    bool ok;
    double value = valueString.toDouble(&ok);
    if (!ok || value < 0)
        return valueString.isNull() ? 1 : 0;
    return (value > max()) ? max() : value;

> +double HTMLProgressElement::max() const
> +{
> +    if (hasAttribute(maxAttr)) {
> +        bool ok;
> +        double max = getAttribute(maxAttr).toDouble(&ok);
> +        if (!ok || max <= 0)
> +            return 1;
> +        return max;
> +    }
> +    return 1;
> +}

No need for the hasAttribute check here. The error handling code will already
kick in correctly and return 1.

> +double HTMLProgressElement::position() const
> +{
> +    if (hasAttribute(valueAttr))
> +        return value() / max();
> +    return -1;
> +}

We normally handle errors with early return, rather than having the success
case be the early return.

> +    HTMLProgressElement(const QualifiedName&, Document*, HTMLFormElement* = 0);

No need for the default value here for HTMLFormElement*.

> +        // readonly attribute NodeList labels;

Normally we don't check in commented-out code.

> +RenderProgress::RenderProgress(Element* element)

The constructor should specifically take a HTMLProgressElement* rather than
just an Element*.

> +    : RenderBlock(element)

Should this instead inherit from RenderReplaced? What other render class were
you basing this one on?

I'm going to mark this review- because I don't see how this triggers a repaint
when the progress element changes.

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