[webkit-reviews] review denied: [Bug 36113] Optimize painting for HTMLProgressElement : [Attachment 50900] Patch V3

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Darin Adler <darin at apple.com>
> -    setNeedsLayoutAndPrefWidthsRecalc();
> +    HTMLProgressElement* element =
static_cast<HTMLProgressElement*>(node());
> +    double position = element->position();
> +    int newPosition;
> +    bool canOptimize = theme()->progressSizeForProgressPosition(position,
newPosition);
> +    IntRect deltaRect = IntRect(IntPoint(0, 0), size());

I believe this is the same as borderBoxRect. But you probably want
contentBoxRect instead.

> +    if (!canOptimize) {
> +	   // The platform does not provide interface to query the number of
pixels a progress bar would require.
> +	   if (m_position == newPosition)
> +	       return;
> +	   m_position = newPosition;
> +	   repaintRectangle(deltaRect);
> +	   return;
> +    }
> +
> +    if (newPosition == m_position)
> +	   return;
> +
> +    deltaRect.setX(std::min(m_position, newPosition));
> +    deltaRect.setWidth(std::max(m_position, newPosition) - deltaRect.x());
> +    m_position = newPosition;
> +    repaintRectangle(deltaRect);

You will be able to refactor this function better if you put the result of
progressSizeForProgressPosition directly into m_position and put the old value
into an oldPosition variable instead of the other way around.

If you do it that way, then you can do the early return before checking
canOptimize at all, and you can put the optimization inside an if statement and
won't need an early return at all.

The deltaRect code doesn't seem to handle the special m_position value of -1,
which I imagine means you need to repaint the entire rectangle.

I think deltaRect is not a good name for the rectangle when it's the entire
rectangle of the element.

> +    int position() { return m_position; }

I think it might be better to pass this as an argument to paintProgressBar
rather than exposing it with a getter function. For example, the getter could
be called when m_position is -1, but we can guarantee never to pass -1 to
paintProgressBar. We definitely don't want anyone to use -1 to compute
anything.

> +    virtual bool progressSizeForProgressPosition(double position, int
&progressSize) const;

For functions like this that return values as out parameters we normally "get"
in the name. The "&" should be next to the int "int& progressSize". I don't
think you need to repeat the word "progress" twice in teh function name. I
don't think the term "size" vs. "position" is very clear. This turns an
abstract position into actual rendered coordinates. Both could be called either
"size" or "position" so I don't think that's the naming you can use to
distinguish them.

review- because at least the -1 problem needs to be fixed.