Bug 37310 - [Chromium] Support HTML5 <progress> element on Linux.
Summary: [Chromium] Support HTML5 <progress> element on Linux.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 37307 40045
  Show dependency treegraph
 
Reported: 2010-04-08 23:15 PDT by Hajime Morrita
Modified: 2010-06-02 21:46 PDT (History)
1 user (show)

See Also:


Attachments
patch v0 (27.27 KB, patch)
2010-06-01 18:42 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v1 (10.75 KB, patch)
2010-06-02 00:02 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v2 (11.03 KB, patch)
2010-06-02 17:53 PDT, Hajime Morrita
tkent: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2010-04-08 23:15:35 PDT
Porting <progress> to Chromium Linux.
Note that chromium has platform-specific rendering code.
So we need bugs for each platform.
Comment 1 Hajime Morrita 2010-06-01 18:42:22 PDT
Created attachment 57613 [details]
patch v0
Comment 2 Hajime Morrita 2010-06-01 18:48:26 PDT
A change on Chromium side is here: http://codereview.chromium.org/2478002
Comment 3 Kent Tamura 2010-06-01 19:12:29 PDT
Comment on attachment 57613 [details]
patch v0

Please update WebKit source and regenerate the patch.

Could you split the patch into two parts? graphics/skia change and RenderThemeChromium* change.

WebCore/ChangeLog:12
 +          an image resampling mode. across multiple paintSkBitmap() calls.
You probably want to remove "." after "mode"?


WebCore/platform/graphics/skia/PlatformContextSkia.cpp:648
 +      const float kFractionalChangeThreshold = 0.025f;
We don't prepend "k" to constant values though I know you didn't write this code.


r- because of svn-apply failure.
Comment 4 Hajime Morrita 2010-06-02 00:02:40 PDT
Created attachment 57630 [details]
patch v1
Comment 5 Hajime Morrita 2010-06-02 00:05:05 PDT
Hi, Kent-san thank you for reviewing.
I updated and rebased the patch

> Could you split the patch into two parts? graphics/skia change and RenderThemeChromium* change.
OK. I split skia change out to Bug 40045, whose patch will come shortly.
Other feedback will be taken on that patch.
Comment 6 Kent Tamura 2010-06-02 06:58:55 PDT
Comment on attachment 57630 [details]
patch v1

WebCore/rendering/RenderThemeChromiumSkia.cpp:775
 +  static const int progressDeltaPixelsPerSecond = 100;
Please add comments about how did you determine these values.


WebCore/rendering/RenderThemeChromiumSkia.cpp:810
 +      return progressAnimationInterval*progressAnimationFrmaes*2; // "2" for back and forth
Add spaces around *.


WebCore/rendering/RenderThemeChromiumSkia.cpp:826
 +      paintInfo.context->drawTiledImage(barImage, renderObject->style()->colorSpace(), rect, IntPoint(0, 0), barTileSize);
We had better assign renderObject->style()->colorSpace() to a variable because it is used multiple times.


WebCore/rendering/RenderThemeChromiumSkia.h:126
 +          IntRect determinateProgressValueRectFor(RenderProgress*, const IntRect&) const;
We should make these internal functions to private or protected.
Comment 7 Hajime Morrita 2010-06-02 17:53:25 PDT
Created attachment 57720 [details]
patch v2
Comment 8 Hajime Morrita 2010-06-02 17:55:40 PDT
Hi Kent-san, thank you for reviewing!
I updated the patch.

(In reply to comment #6)
> (From update of attachment 57630 [details])
> WebCore/rendering/RenderThemeChromiumSkia.cpp:775
>  +  static const int progressDeltaPixelsPerSecond = 100;
> Please add comments about how did you determine these values.
Added comment.

> WebCore/rendering/RenderThemeChromiumSkia.cpp:810
>  +      return progressAnimationInterval*progressAnimationFrmaes*2; // "2" for back and forth
> Add spaces around *.
Done.

> 
> 
> WebCore/rendering/RenderThemeChromiumSkia.cpp:826
>  +      paintInfo.context->drawTiledImage(barImage, renderObject->style()->colorSpace(), rect, IntPoint(0, 0), barTileSize);
> We had better assign renderObject->style()->colorSpace() to a variable because it is used multiple times.
Done.

> 
> 
> WebCore/rendering/RenderThemeChromiumSkia.h:126
>  +          IntRect determinateProgressValueRectFor(RenderProgress*, const IntRect&) const;
> We should make these internal functions to private or protected.
Moved to protected.
Comment 9 Hajime Morrita 2010-06-02 21:46:54 PDT
Committed r60604: <http://trac.webkit.org/changeset/60604>