Bug 37308 - [Chromium] Support HTML5 <progress> element on Windows.
Summary: [Chromium] Support HTML5 <progress> element on Windows.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 37307
  Show dependency treegraph
 
Reported: 2010-04-08 23:10 PDT by Hajime Morrita
Modified: 2010-05-12 15:46 PDT (History)
3 users (show)

See Also:


Attachments
patch v0 (7.23 KB, patch)
2010-04-08 23:40 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
v1; fix style violation (7.24 KB, patch)
2010-04-09 00:12 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
v2; fix to add rtl handling (7.30 KB, patch)
2010-04-20 16:48 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
v3; took feedback (9.11 KB, patch)
2010-04-21 10:14 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
v4; took the feedback, rebased (8.82 KB, patch)
2010-04-21 18:23 PDT, Hajime Morrita
no flags 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:10:40 PDT
Porting <progress> to Chromium windows.
Note that chromium has platform-specific rendering code.
So we need bugs for each platform.
Comment 1 Hajime Morrita 2010-04-08 23:40:33 PDT
Created attachment 52939 [details]
patch v0
Comment 2 WebKit Review Bot 2010-04-08 23:42:38 PDT
Attachment 52939 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderThemeChromiumWin.h:91:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/rendering/RenderThemeChromiumWin.h:92:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/rendering/RenderThemeChromiumWin.h:93:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/rendering/RenderThemeChromiumWin.h:94:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/rendering/RenderThemeChromiumWin.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/rendering/RenderThemeChromiumWin.cpp:669:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/rendering/RenderThemeChromiumWin.cpp:687:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 7 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Hajime Morrita 2010-04-09 00:12:44 PDT
Created attachment 52941 [details]
v1; fix style violation
Comment 4 Hajime Morrita 2010-04-09 00:13:30 PDT
Change on Chromium side is: http://codereview.chromium.org/1596018
Comment 5 Kent Tamura 2010-04-09 00:28:34 PDT
Comment on attachment 52941 [details]
v1; fix style violation

> --- a/WebCore/rendering/RenderThemeChromiumWin.cpp
> +++ b/WebCore/rendering/RenderThemeChromiumWin.cpp
> @@ -38,6 +38,7 @@
>  #include "HTMLNames.h"
>  #include "MediaControlElements.h"
>  #include "RenderBox.h"
> +#include "RenderProgress.h"
>  #include "RenderSlider.h"
>  #include "ScrollbarTheme.h"
>  #include "TransparencyWin.h"
> @@ -653,4 +654,55 @@ bool RenderThemeChromiumWin::paintTextFieldInternal(RenderObject* o,
>      return false;
>  }
>  
> +#if ENABLE(PROGRESS_TAG)
> +
> +static const double progressAnimationFrameRate = 0.033;
> +static const double progressAnimationNumFrames = 60;

How did you decide these values?  Do they match to Windows' behavior?

> --- a/WebKit/chromium/ChangeLog
> +++ b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,19 @@
> +2010-04-08  MORITA Hajime  <morrita@google.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Chromium] Support HTML5 <progress> element on Windows.
> +        https://bugs.webkit.org/show_bug.cgi?id=37308
> +
> +        Extended ChromiumBridge and WebThemeEngine interface
> +        to handle progress bar painting. implementation will 
> +        shit on Chromium tree.

I don't understand the last sentence. shit?
Comment 6 Hajime Morrita 2010-04-20 16:48:34 PDT
Created attachment 53905 [details]
v2; fix to add rtl handling
Comment 7 Hajime Morrita 2010-04-20 16:53:28 PDT
Hi, thank you for reviewing. I'm sorry for my slow response.

> > +#if ENABLE(PROGRESS_TAG)
> > +
> > +static const double progressAnimationFrameRate = 0.033;
> > +static const double progressAnimationNumFrames = 60;
> 
> How did you decide these values?  Do they match to Windows' behavior?
I couldn't found "standard" behaviour for windows.
The guideline doesn't mention marquee speed,
(http://msdn.microsoft.com/en-us/library/aa511258.aspx)
Nor default value of ProgressBar class on .NET framework looks unrealistic. 


> > +        [Chromium] Support HTML5 <progress> element on Windows.
> > +        https://bugs.webkit.org/show_bug.cgi?id=37308
> > +
> > +        Extended ChromiumBridge and WebThemeEngine interface
> > +        to handle progress bar painting. implementation will 
> > +        shit on Chromium tree.
> 
> I don't understand the last sentence. shit?
Ah, this didn't make sense... Fixed.
Comment 8 Kent Tamura 2010-04-20 18:24:44 PDT
(In reply to comment #7)
> Hi, thank you for reviewing. I'm sorry for my slow response.
> 
> > > +#if ENABLE(PROGRESS_TAG)
> > > +
> > > +static const double progressAnimationFrameRate = 0.033;
> > > +static const double progressAnimationNumFrames = 60;
> > 
> > How did you decide these values?  Do they match to Windows' behavior?
> I couldn't found "standard" behaviour for windows.
> The guideline doesn't mention marquee speed,
> (http://msdn.microsoft.com/en-us/library/aa511258.aspx)
> Nor default value of ProgressBar class on .NET framework looks unrealistic. 

So, please add a comment about why you chose 0.033 and 60, and add FIXME if you think we need to improve them.

BTW,
http://msdn.microsoft.com/en-us/library/bb760842(v=VS.85).aspx
It seems the default interval is 30 msec.  So progressAnimationFrameRate=0.033 is reasonable.
Comment 9 Shinichiro Hamaji 2010-04-20 19:03:17 PDT
Comment on attachment 53905 [details]
v2; fix to add rtl handling

> +double RenderThemeChromiumWin::animationDurationForProgressBar(RenderProgress* o) const

I'm not sure if the name 'o' is good for an object of RenderProgress. I would say renderProgress or something like this.

> +    if (o->position() < 0)

I couldn't understand why this is necessary. If this is necessary, don't we need to modfify RenderThemeMac as well?

Also, it would be nice to add a function which returns position()<0 in RenderProgress (maybe isIndeterminate or something like this?). Anyway, I guess we want some refactoring around this patch, but in a separated patch.

> +    int fillPart;
> +    IntRect fillRect;
> +
> +

Unnecessary blank line(s). I guess we need no blanklines here.

> +        int dx = r.width() * renderProgress->animationProgress() * 2.0 - r.width();

Maybe we should use 2 instead of 2.0. (See Floating point literals in http://webkit.org/coding/coding-style.html)

r- for Tamura-san's comment and style issues.
Comment 10 Hajime Morrita 2010-04-21 10:14:25 PDT
Created attachment 53968 [details]
v3; took feedback
Comment 11 Hajime Morrita 2010-04-21 10:20:04 PDT
Hamaji-san, Tamura-san, thank you for reviewing.

(In reply to comment #8)
> So, please add a comment about why you chose 0.033 and 60, and add FIXME if you
> think we need to improve them.
Added a comment.

> 
> BTW,
> http://msdn.microsoft.com/en-us/library/bb760842(v=VS.85).aspx
> It seems the default interval is 30 msec.  So progressAnimationFrameRate=0.033
> is reasonable.
Thank you for your investigation! I included this link in the comment.

(In reply to comment #9)
> > +double RenderThemeChromiumWin::animationDurationForProgressBar(RenderProgress* o) const
> I'm not sure if the name 'o' is good for an object of RenderProgress. I would
> say renderProgress or something like this.
Fixed.

> 
> > +    if (o->position() < 0)
> 
> I couldn't understand why this is necessary. If this is necessary, don't we
> need to modfify RenderThemeMac as well?
Progress bars on windows bar has an animation only when it is indeterminate.
Progress bars on mac has animations both for determinate and for indeterminate. 
So we need this only for Windows port.

> > +    int fillPart;
> > +    IntRect fillRect;
> > +
> > +
> 
> Unnecessary blank line(s). I guess we need no blanklines here.
Fixed.

> 
> > +        int dx = r.width() * renderProgress->animationProgress() * 2.0 - r.width();
> 
> Maybe we should use 2 instead of 2.0. (See Floating point literals in
> http://webkit.org/coding/coding-style.html)
Fixed.
Comment 12 Hajime Morrita 2010-04-21 11:21:01 PDT
The patch seems outdated. I'm rebasing it now.
Comment 13 Darin Fisher (:fishd, Google) 2010-04-21 14:51:47 PDT
Comment on attachment 53968 [details]
v3; took feedback

> +++ b/WebCore/rendering/RenderThemeChromiumWin.cpp
...
> +bool RenderThemeChromiumWin::paintProgressBar(RenderObject* o, const RenderObject::PaintInfo& i, const IntRect& r)
> +{
> +    RenderProgress* renderProgress = toRenderProgress(o);
> +
> +    int fillPart;
> +    IntRect fillRect;
> +    if (renderProgress->isDeterminate()) {
> +        fillPart = PP_FILL;
> +        int dx = r.width() * renderProgress->position();
> +        if (renderProgress->style()->direction() == RTL)
> +            fillRect = IntRect(r.x() + r.width() - dx, r.y(), dx, r.height());
> +        else
> +            fillRect = IntRect(r.x(), r.y(), dx, r.height());
> +    } else {
> +        fillPart = PP_MOVEOVERLAY;
> +        int dx = r.width() * renderProgress->animationProgress() * 2 - r.width();
> +        fillRect = IntRect(r.x() + dx, r.y(), r.width(), r.height());
> +    }
> +
> +    WebCore::ThemePainter painter(i.context, r);

nit: no need for the WebCore:: prefix when you are in the WebCore namespace.


> +    ChromiumBridge::paintProgressBar(painter.context(),
> +                                     PP_BAR,
> +                                     0,
> +                                     r, 
> +                                     fillPart,
> +                                     0,
> +                                     fillRect);

It looks like barState and fillState are not used.  Should those
parameters be excluded?  Also, is there any reason to pass PP_BAR
given that it is a constant?  It seems like the callee would know
to pass PP_BAR on to Windows.


> +++ b/WebKit/chromium/public/win/WebThemeEngine.h
...
> @@ -73,6 +73,10 @@ public:
>      virtual void paintTrackbar(
>          WebCanvas*, int part, int state, int classicState,
>          const WebRect&) = 0;
> +
> +    virtual void paintProgressBar(
> +        WebCanvas*, int barPart, int barState, const WebRect& barRect, 
> +        int fillPart, int fillState, const WebRect& fillRect) {} // FIXME: make pure virtual after implementation landed.
>  };

^^^ we should actually make any embedder implemented methods non-pure.
the old way was for them to be pure, but that makes it harder to change
the API.  the new way is to make methods like these non-pure.
Comment 14 Hajime Morrita 2010-04-21 18:23:12 PDT
Created attachment 54011 [details]
v4; took the feedback, rebased
Comment 15 Hajime Morrita 2010-04-21 18:32:12 PDT
Darin, thank you for reviewing!
I updated the patch.

> > +    WebCore::ThemePainter painter(i.context, r);
> 
> nit: no need for the WebCore:: prefix when you are in the WebCore namespace.
Fixed.

> > +    ChromiumBridge::paintProgressBar(painter.context(),
> > +                                     PP_BAR,
> > +                                     0,
> > +                                     r, 
> > +                                     fillPart,
> > +                                     0,
> > +                                     fillRect);
> 
> It looks like barState and fillState are not used.  Should those
> parameters be excluded?  Also, is there any reason to pass PP_BAR
> given that it is a constant?  It seems like the callee would know
> to pass PP_BAR on to Windows.
Agreed and fixed.
I try to align args to other APIs. But it didn't make sense in this case.

> > +    virtual void paintProgressBar(
> > +        WebCanvas*, int barPart, int barState, const WebRect& barRect, 
> > +        int fillPart, int fillState, const WebRect& fillRect) {} // FIXME: make pure virtual after implementation landed.
> >  };
> 
> ^^^ we should actually make any embedder implemented methods non-pure.
> the old way was for them to be pure, but that makes it harder to change
> the API.  the new way is to make methods like these non-pure.
I see. So I removed the FIXME.
We might need to export notImplemented() for such case.
Comment 16 Kent Tamura 2010-04-22 04:58:03 PDT
(In reply to comment #11)
> > > +    if (o->position() < 0)
> > 
> > I couldn't understand why this is necessary. If this is necessary, don't we
> > need to modfify RenderThemeMac as well?
> Progress bars on windows bar has an animation only when it is indeterminate.
> Progress bars on mac has animations both for determinate and for indeterminate. 
> So we need this only for Windows port.

I remember determinate progress bar on Windows is also animated. A white gloss run from left to right.  Can you implement it?

(In reply to comment #15)
> > > +    virtual void paintProgressBar(
> > > +        WebCanvas*, int barPart, int barState, const WebRect& barRect, 
> > > +        int fillPart, int fillState, const WebRect& fillRect) {} // FIXME: make pure virtual after implementation landed.
> > >  };
> > 
> > ^^^ we should actually make any embedder implemented methods non-pure.
> > the old way was for them to be pure, but that makes it harder to change
> > the API.  the new way is to make methods like these non-pure.
> I see. So I removed the FIXME.
> We might need to export notImplemented() for such case.

Removing FIXME is worse.
In this case, you had better commit the Chromium part first, then commit this change.
Comment 17 Kent Tamura 2010-04-22 05:07:51 PDT
(In reply to comment #16)
> Removing FIXME is worse.
> In this case, you had better commit the Chromium part first, then commit this
> change.

This comment was wrong.  Please ignore it.
Comment 18 Hajime Morrita 2010-04-22 11:49:48 PDT
Hi, thank you for reviewing!

> I remember determinate progress bar on Windows is also animated. A white gloss
> run from left to right.  Can you implement it?
It seems that animating-determinate-progress-bar is available only on Windows 7, not on Vista.
So I'll keep it not animated at this time,  and will implement animation when I get Windows 7.
Comment 19 Kent Tamura 2010-04-22 18:15:29 PDT
(In reply to comment #18)
> It seems that animating-determinate-progress-bar is available only on Windows
> 7, not on Vista.
> So I'll keep it not animated at this time,  and will implement animation when I
> get Windows 7.

It is available on Vista too.
Anyway it's ok to handle it later. It's good to file a bug entry for it now.

The code looks ok.
Comment 20 Kent Tamura 2010-05-10 17:53:27 PDT
Comment on attachment 54011 [details]
v4; took the feedback, rebased

OK.
Comment 21 Hajime Morrita 2010-05-12 01:52:28 PDT
Committed r59217: <http://trac.webkit.org/changeset/59217>
Comment 22 Darin Adler 2010-05-12 10:26:30 PDT
Why is progress bar painting being done with calls to the Chrome bridge? Is there some reason this needs to be done outside WebKit?
Comment 23 Kent Tamura 2010-05-12 15:46:55 PDT
(In reply to comment #22)
> Why is progress bar painting being done with calls to the Chrome bridge? Is there some reason this needs to be done outside WebKit?

Progress bar appearance depends to a desktop theme.
In Chromium port, we can't access desktop themes in WebKit.

This is a Chromium-specific issue.  So RenderThemeWin.cpp can have rendering code for native progress bar.