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

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #50931|review?                     |review+
               Flag|                            |




--- Comment #12 from Darin Adler <darin at apple.com>  2010-03-17 13:44:14 PST ---
(From update of attachment 50931)
> +    IntRect paintRect = contentBoxRect();
> +    if (canOptimize) {
> +        // FIXME: Need to handle indeterminate progress bar and RTL
> +        int adjustedPosition = (m_position >= 0) ? m_position : 0;
> +        int adjustedOldPosition = (oldPosition >= 0) ? oldPosition : 0;
> +        paintRect.setX(std::min(adjustedPosition, adjustedOldPosition));
> +        paintRect.setWidth(std::max(adjustedPosition, adjustedOldPosition) - paintRect.x());
> +    }
> +    repaintRectangle(paintRect);

Since m_position can never be negative at this point, I don't think you need
code to adjust for that case.

The handling of oldPosition of -1 here seems wrong. If the old position was -1
I presume we need to repaint the entire rectangle, not act as if the old
position was 0.

WebKit style is to use a using directive for std at the top of the file and
call functions as min and max rather than using std::min and std::max in a cpp
file.

>  public:
>      RenderProgress(HTMLProgressElement*);
> +    int position() { return m_position; }

Given the use of "-1" to mean "no position", I do not think it's a good idea to
have a public function to return the position. Nothing here makes it clear that
there's a special -1 value, nor what the range of values for position are. I
think the position value should be explicitly passed in the theme API instead
as I suggested before.

> +bool RenderTheme::getNumberOfPixelsForProgressPosition(double , int& progressSize) const

Extra space here before the comma.

I really don't think this theme API is going in the right direction. Progress
indicators in general don't have a certain number of pixels from the left. I
think the theme should be involved in the decision of what needs to be dirtied,
not just return a number that's used.

I'm going to say r=me because I think the mistakes here can be fixed in the
future, but I think there's a lot of room for improvement here.

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