Summary: | [Chromium] Support HTML5 <progress> element on Linux. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | tkent | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 37307, 40045 | ||||||||||
Attachments: |
|
Description
Hajime Morrita
2010-04-08 23:15:35 PDT
Created attachment 57613 [details]
patch v0
A change on Chromium side is here: http://codereview.chromium.org/2478002 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.
Created attachment 57630 [details]
patch v1
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 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.
Created attachment 57720 [details]
patch v2
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. Committed r60604: <http://trac.webkit.org/changeset/60604> |