[Webkit-unassigned] [Bug 36113] Optimize painting for HTMLProgressElement

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 17 08:39:40 PDT 2010


Darin Adler <darin at apple.com> changed:

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

--- Comment #8 from Darin Adler <darin at apple.com>  2010-03-17 08:39:40 PST ---
(From update of attachment 50900)
> -    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

> +    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.

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