Bug 36113

Created attachment 50699 [details] Patch v1

Comment on attachment 50699 [details] Patch v1 Thanks for tackling this. > + if (attribute->name() == valueAttr) { > + if (renderer()) > + renderer()->updateFromElement(); > + } else if (attribute->name() == maxAttr) { > + if (renderer()) > + renderer()->updateFromElement(); > + } else Looks good. > + int position = (100 * element->position()); Where does this 100 come from? I don't think it makes sense to store a value between 0 and 100. The progress indicator could be more than 100 pixels wide or less than 100 pixels wide. I'm not sure exactly how to handle this, but what we want to store is the actual number of pixels used in the theme's progress bar drawing, which might mean some kind of cooperation with theme theme so you can know how much of the progress indicator is devoted to the progress display. In the mean time, the RenderProgress could instead just store the progress value from the progress element as-is. Multiplying by 100 and converting to an integer is unnecessary if that value, 100, is just an arbitrarily chosen constant. It seems that the Qt progress bar rendering code does require that the use of integers to render the progress element. So the theme should multiply the position value by something, but I suggest a value greater than 100, perhaps numeric_limits<int>::max() to make best use of the precision available in an integer. I don't think the rule "repaint the entire indicator any time the value changes" is optimal for the long run. If the indicator moves a very short distance it would be best to repaint only the affected area. This may not be possible for some platforms, such as Qt, but it would be good to design the mechanism so we can do it.

Created attachment 50826 [details] Patch v2 Thank you for your review. I am not sure I can store the number of pixels to draw in a platform independent way. For example, in the Qt port on different operating systems, we have different border widths and different paddings for the different progress bars. In order to not lose precision, I replaced the previous 100 with 65536, I hope this is good enough precision for all platforms. This patch adds a method for getting the size of the dirty rect from the platform, if available.

(In reply to comment #3) > I am not sure I can store the number of pixels to draw in a platform > independent way. For example, in the Qt port on different operating systems, we > have different border widths and different paddings for the different progress > bars. I think this is actually a Qt-specific problem. Qt only gives you a way to paint the progress indicator, and doesn't let you find out how much of a progress change is needed to see the difference. So I agree that this is indeed impossible for Qt. > In order to not lose precision, I replaced the previous 100 with 65536, I hope > this is good enough precision for all platforms. I don't see any reason to convert the floating point number to an integer in the cross platform code. You'll need to do that in the Qt theme layer because integers is how Qt interfaces to the progress indicator painting code, but that may not be true on other platforms.

(In reply to comment #4) > I don't see any reason to convert the floating point number to an integer in > the cross platform code. You'll need to do that in the Qt theme layer because > integers is how Qt interfaces to the progress indicator painting code, but that > may not be true on other platforms. I am sorry, I thought this is what you were asking me to do. :-)

(In reply to comment #5) > I am sorry, I thought this is what you were asking me to do. :-) I was, but it can’t be just any integer. It has to be an integer that actually corresponds to the pixels being painted to provide the benefit we are looking for.

Created attachment 50900 [details] Patch V3 Save the number of pixels to draw, for platforms that can support it.

Comment on attachment 50900 [details] Patch V3 > - 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.

(In reply to comment #8) > review- because at least the -1 problem needs to be fixed. I was planning to add handling for -1 when I implement the indeterminate progress bar. I will add a FIXME in the next patch. Also RTL is not handled in this patch, I am planning to add RTL support in https://bugs.webkit.org/show_bug.cgi?id=36206 .

Created attachment 50921 [details] Patch v4 Updates based on comment #8.

Created attachment 50931 [details] Patch v5.

Comment on attachment 50931 [details] Patch v5. > + 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.

Landed in 56126.

(In reply to comment #12) > I think the mistakes here can be fixed in the > future, but I think there's a lot of room for improvement here. Darin, thank you for your time and effort of reviewing my patches. As I am learning the rendering code, I hope it will be easier:-) I think, that since on the mac, we would want to animate the progress bar, just like NSProgressIndicator does, perhaps we should always paint the whole progress bar each time after all.