Bug 52385 - [GTK] Move progress bar painting out of gtk2drawing.c
Summary: [GTK] Move progress bar painting out of gtk2drawing.c
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 52258
Blocks: 51155
  Show dependency tree / graph
 
Reported: 2011-01-13 11:44 PST by Martin Robinson
Modified: 2011-01-19 18:52 PST (History)
3 users (show)

See Also:

Attachments
Patch (17.17 KB, patch)
2011-01-18 13:59 PST, Martin Robinson 		no flags Details | Formatted Diff | Diff
Patch with extra comments and Carlos' fix (17.72 KB, patch)
2011-01-19 10:37 PST, Martin Robinson 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2011-01-13 11:44:27 PST 
This bug tracks removing the progress bar rendering out of gtk2drawing.c and sharing some animation code between the GTK+ 2.x and GTK+ 3.x port.
Comment 1 Martin Robinson 2011-01-18 13:59:45 PST 
Created attachment 79324 [details]
Patch
Comment 2 Daniel Bates 2011-01-18 21:32:54 PST 
Comment on attachment 79324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79324&action=review

This won't compile when PROGRESS_TAG is disabled/undefined since calculateProgressRect() is only declared in RenderThemeGtk.h when PROGRESS_TAG is defined.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:565
> +void RenderThemeGtk::calculateProgressRect(RenderObject* renderObject, IntRect& rect)

Given the name of this function, it seems a bit weird that this function adjusts the rectangle of the progress bar instead of returning a rectangle.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:577
> +    int newWidth = max(2, rect.width() / progressActivityBlocks);

Where does the 2 come from?

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:581
> +        rect.setX(rect.x() + (animationProgress * 2 * movableWidth));

Where does the 2 come from? The parentheses are not needed for the second addend since multiplication has a higher precedence that addition.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:583
> +        rect.setX(rect.x() + ((1.0 - animationProgress) * 2 * movableWidth));

Ditto.
Comment 3 Daniel Bates 2011-01-18 21:46:06 PST 
(In reply to comment #2)
> (From update of attachment 79324 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79324&action=review
> 
> This won't compile when PROGRESS_TAG is disabled/undefined since calculateProgressRect() is only declared in RenderThemeGtk.h when PROGRESS_TAG is defined.

Disregard this comment. Additional context reveals that RenderThemeGtk::calculateProgressRect() is enclosed within an ENABLE(PROGRESS_TAG).
Comment 4 Daniel Bates 2011-01-18 21:48:39 PST 
Comment on attachment 79324 [details]
Patch

This patch look sane to me. I am curious about where the 2's come from in lines 576 thru 583 in RenderThemeGtk.cpp.
Comment 5 Carlos Garcia Campos 2011-01-19 00:47:35 PST 
Comment on attachment 79324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79324&action=review

> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:491
> +        widgetContext.gtkPaintBox(progressRect, widget, GTK_STATE_NORMAL, GTK_SHADOW_IN, "bar");

This should be GTK_STATE_PRELIGHT, GTK_SHADOW_OUT
Comment 6 Martin Robinson 2011-01-19 10:37:52 PST 
Created attachment 79447 [details]
Patch with extra comments and Carlos' fix
Comment 7 Martin Robinson 2011-01-19 18:51:46 PST 
Committed r76192: <http://trac.webkit.org/changeset/76192>