[webkit-reviews] review denied: [Bug 35937] Support for HTMLProgressElement : [Attachment 50543] Patch v7

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 50543: Patch v7
https://bugs.webkit.org/attachment.cgi?id=50543&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    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.