Added support for HTML5 progress element.

Added support for HTML5 progress element. 
WebKit side of this change is on http://webkit.org/b/37308 .

continued from http://codereview.chromium.org/1596018

[gmorrita, 2010-05-12 10:02:48]

from  http://codereview.chromium.org/1596018

[tkent, 2010-05-12 10:06:02]

Add pkasting as a reviewer.

[Peter Kasting, 2010-05-12 18:44:25]

http://codereview.chromium.org/1988012/diff/1/2
File gfx/native_theme_win.cc (right):

http://codereview.chromium.org/1988012/diff/1/2#newcode1
gfx/native_theme_win.cc:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
Nit: 2010

http://codereview.chromium.org/1988012/diff/1/2#newcode492
gfx/native_theme_win.cc:492: // for indeterminate overflows bar rectangle.
Nit: Grammar.  I'm not actually sure what this means.  Do you mean that if the
value is indeterminate, draw_theme_(..., value_rect, NULL); ignores the provided
rect?

http://codereview.chromium.org/1988012/diff/1/2#newcode493
gfx/native_theme_win.cc:493: HRGN clip = GetClipHandle();
Do you really need to cache the clip region?  Can't you just create the HRGN
here?  I can't imagine it's slow to create.

http://codereview.chromium.org/1988012/diff/1/2#newcode708
gfx/native_theme_win.cc:708: dummy_rect.left = dummy_rect.right = 0;
Nit: Instead of these just do
RECT dummy_rect = {0};

http://codereview.chromium.org/1988012/diff/1/3
File gfx/native_theme_win.h (right):

http://codereview.chromium.org/1988012/diff/1/3#newcode1
gfx/native_theme_win.h:1: // Copyright (c) 2009 The Chromium Authors. All rights
reserved.
Nit: 2010

http://codereview.chromium.org/1988012/diff/1/3#newcode282
gfx/native_theme_win.h:282: HRGN GetClipHandle() const;
Nit: Separate from above function, add comment.  What is a clipping handle? 
Should this be GetClipRegion() instead?

http://codereview.chromium.org/1988012/diff/1/3#newcode298
gfx/native_theme_win.h:298: // A cache of original clipping handle
Nit: Add period at end

http://codereview.chromium.org/1988012/diff/1/3#newcode299
gfx/native_theme_win.h:299: mutable HRGN clip_;
Nit: Should this be clip_region_?

http://codereview.chromium.org/1988012/diff/1/4
File webkit/glue/webthemeengine_impl_win.cc (right):

http://codereview.chromium.org/1988012/diff/1/4#newcode1
webkit/glue/webthemeengine_impl_win.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.  Use of this
Nit: 2010

http://codereview.chromium.org/1988012/diff/1/4#newcode130
webkit/glue/webthemeengine_impl_win.cc:130: 
Nit: Extra newline

http://codereview.chromium.org/1988012/diff/1/4#newcode133
webkit/glue/webthemeengine_impl_win.cc:133: 
Nit: Extra newline

http://codereview.chromium.org/1988012/diff/1/6
File webkit/tools/test_shell/test_shell_webthemecontrol.cc (right):

http://codereview.chromium.org/1988012/diff/1/6#newcode1
webkit/tools/test_shell/test_shell_webthemecontrol.cc:1: // Copyright (c) 2009
The Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/1988012/diff/1/6#newcode34
webkit/tools/test_shell/test_shell_webthemecontrol.cc:34: SkColorSetRGB(0xcc,
0xcc, 0xcc)   // Indetermiante
Nit: Indetermiante -> Indeterminate

http://codereview.chromium.org/1988012/diff/1/6#newcode420
webkit/tools/test_shell/test_shell_webthemecontrol.cc:420: 
Nit: Unnecessary newline

http://codereview.chromium.org/1988012/diff/1/7
File webkit/tools/test_shell/test_shell_webthemecontrol.h (right):

http://codereview.chromium.org/1988012/diff/1/7#newcode1
webkit/tools/test_shell/test_shell_webthemecontrol.h:1: // Copyright (c) 2009
The Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/1988012/diff/1/7#newcode46
webkit/tools/test_shell/test_shell_webthemecontrol.h:46: //   Indetermiante -
when set to indetermiante (only for progress bar)
Nit: Indetermiante -> Indeterminate

http://codereview.chromium.org/1988012/diff/1/7#newcode110
webkit/tools/test_shell/test_shell_webthemecontrol.h:110: // Like
drawTextField(), drawing a progress bar also requires extra states
Nit: I think this would be clearer as:

// Use this for drawing ProgressBar controls instead, since we
// need to know the rect to fill inside the bar.

http://codereview.chromium.org/1988012/diff/1/7#newcode113
webkit/tools/test_shell/test_shell_webthemecontrol.h:113: void
drawProgressBar(const SkIRect &fill_rect);
Nit: The placement of & and * in this file (and the associated C++ file)
violates google and webkit style.  Could you fix these while you're here?

http://codereview.chromium.org/1988012/diff/1/7#newcode114
webkit/tools/test_shell/test_shell_webthemecontrol.h:114: private:
Nit: Add newline above

http://codereview.chromium.org/1988012/diff/1/8
File webkit/tools/test_shell/test_shell_webthemeengine.cc (right):

http://codereview.chromium.org/1988012/diff/1/8#newcode63
webkit/tools/test_shell/test_shell_webthemeengine.cc:63: void
drawProgressBar(WebCanvas *canvas,
Nit: Placement of & and * in this file violates google and webkit style.  Could
you fix these while you're here?

http://codereview.chromium.org/1988012/diff/1/8#newcode549
webkit/tools/test_shell/test_shell_webthemeengine.cc:549: Control::kNormal_State
:
Nit: Just collapse these onto one line:

Control::State cstate = (valuePart == PP_FILL) ?
    Control::kNormal_State : Control::kIndeterminate_State;

http://codereview.chromium.org/1988012/diff/1/9
File webkit/tools/test_shell/test_shell_webthemeengine.h (right):

http://codereview.chromium.org/1988012/diff/1/9#newcode1
webkit/tools/test_shell/test_shell_webthemeengine.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/1988012/diff/1/9#newcode56
webkit/tools/test_shell/test_shell_webthemeengine.h:56: 
Nit: Unnecessary newline

[gmorrita, 2010-05-13 03:12:47]

Hi Peter,
thank you for reviewing!
I've updated the patch for deal with all points above.

[Peter Kasting, 2010-05-13 18:52:22]

LGTM.  Can you send me a note with a testcase when this gets checked in?  I want
to see how it looks, especially the indeterminate progress bar.

http://codereview.chromium.org/1988012/diff/13001/14001
File gfx/native_theme_win.cc (right):

http://codereview.chromium.org/1988012/diff/13001/14001#newcode487
gfx/native_theme_win.cc:487: // For indeterminate progres bar, its moving
highlight need to be clip
Nit: Grammar/spelling.

I'm still trying to figure out what this means.  Let me see if I can write a
comment based on studying the WebKit patch and MSDN docs.  Tell me if this is
correct:

// For an indeterminate progress bar, we draw a moving highlight that's animated
// (by the caller) across the width of the bar.  The highlight part is always
// drawn as being as wide as the bar, to scale it properly.  Therefore, we need
// to clip it against the bar rect, so that as it moves, it doesn't extend past
// the ends of the bar.  For a normal progress bar, we won't try to draw past
// the bar ends, so this clipping is useless, but harmless.

http://codereview.chromium.org/1988012/diff/13001/14001#newcode490
gfx/native_theme_win.cc:490: HRGN clip = CreateRectRgnIndirect(&dummy_rect);
Use ScopedRegion in base/scoped_handle_win.h to avoid the DeleteObject() calls
on each exit arm.

http://codereview.chromium.org/1988012/diff/13001/14006
File webkit/tools/test_shell/test_shell_webthemecontrol.h (right):

http://codereview.chromium.org/1988012/diff/13001/14006#newcode1
webkit/tools/test_shell/test_shell_webthemecontrol.h:1: // Copyright (c) 2009
The Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/1988012/diff/13001/14007
File webkit/tools/test_shell/test_shell_webthemeengine.cc (right):

http://codereview.chromium.org/1988012/diff/13001/14007#newcode549
webkit/tools/test_shell/test_shell_webthemeengine.cc:549: Control::kNormal_State
: Control::kIndeterminate_State;
Nit: Indent 4

[gmorrita, 2010-05-14 02:44:51]

On 2010/05/13 18:52:22, Peter Kasting wrote:
> LGTM.  Can you send me a note with a testcase when this gets checked in?  I
want
> to see how it looks, especially the indeterminate progress bar.
Test cases are already in the webkit tree.
You can find them at
third_party/WebKit/LayoutTests/fast/dom/HTMLProgressElement/
... but I noticed that there is no test for indeterminate progress bar.
So we should have one.

[gmorrita, 2010-05-14 02:49:25]

http://codereview.chromium.org/1988012/diff/13001/14001
File gfx/native_theme_win.cc (right):

http://codereview.chromium.org/1988012/diff/13001/14001#newcode487
gfx/native_theme_win.cc:487: // For indeterminate progres bar, its moving
highlight need to be clip
> comment based on studying the WebKit patch and MSDN docs.  Tell me if this is
> correct:
> (snip)
Thank you so much! This is exactly what I meant to say
(and further)!

http://codereview.chromium.org/1988012/diff/13001/14001#newcode490
gfx/native_theme_win.cc:490: HRGN clip = CreateRectRgnIndirect(&dummy_rect);
>Use ScopedRegion in base/scoped_handle_win.h to avoid the DeleteObject() calls
on each exit arm.
Fixed introducing ScopedRegionClipping to invoke 
SelectClipRgn also.

http://codereview.chromium.org/1988012/diff/13001/14006
File webkit/tools/test_shell/test_shell_webthemecontrol.h (right):

http://codereview.chromium.org/1988012/diff/13001/14006#newcode1
webkit/tools/test_shell/test_shell_webthemecontrol.h:1: // Copyright (c) 2009
The Chromium Authors. All rights reserved.
On 2010/05/13 18:52:22, Peter Kasting wrote:
> Nit: 2010
Fixed.

http://codereview.chromium.org/1988012/diff/13001/14007
File webkit/tools/test_shell/test_shell_webthemeengine.cc (right):

http://codereview.chromium.org/1988012/diff/13001/14007#newcode549
webkit/tools/test_shell/test_shell_webthemeengine.cc:549: Control::kNormal_State
: Control::kIndeterminate_State;
>Nit: Indent 4
Fixed.

[tkent, 2010-05-14 03:11:10]

http://codereview.chromium.org/1988012/diff/23002/24001
File base/scoped_handle_win.h (right):

http://codereview.chromium.org/1988012/diff/23002/24001#newcode237
base/scoped_handle_win.h:237: class ScopedRegionClipping {
I'm not sure whether this class should be in this file.
I couldn't find similar usage of regions in Chromium code.

http://codereview.chromium.org/1988012/diff/23002/24001#newcode238
base/scoped_handle_win.h:238: public:
Scope label should be indented by 1 space.

http://codereview.chromium.org/1988012/diff/23002/24001#newcode250
base/scoped_handle_win.h:250: private:
ditto.

[gmorrita, 2010-05-14 03:54:20]

http://codereview.chromium.org/1988012/diff/23002/24001
File base/scoped_handle_win.h (right):

http://codereview.chromium.org/1988012/diff/23002/24001#newcode237
base/scoped_handle_win.h:237: class ScopedRegionClipping {
On 2010/05/14 03:11:10, Kent Tamura wrote:
> I'm not sure whether this class should be in this file.
> I couldn't find similar usage of regions in Chromium code.
Agreed. So I moved this native_theme_win.cc at this time.

http://codereview.chromium.org/1988012/diff/23002/24001#newcode238
base/scoped_handle_win.h:238: public:
On 2010/05/14 03:11:10, Kent Tamura wrote:
> Scope label should be indented by 1 space.
> 
Fixed.

http://codereview.chromium.org/1988012/diff/23002/24001#newcode250
base/scoped_handle_win.h:250: private:
On 2010/05/14 03:11:10, Kent Tamura wrote:
> ditto.
Fixed.

[tkent, 2010-05-14 04:43:31]

LGTM.
I'll commit this change.

[tkent, 2010-05-14 06:48:23]

Committed as r47247.

[Peter Kasting, 2010-05-14 18:08:11]

I know you've already committed this, but the 80 column bit at least should
probably be fixed.

http://codereview.chromium.org/1988012/diff/30001/31001
File gfx/native_theme_win.cc (right):

http://codereview.chromium.org/1988012/diff/30001/31001#newcode482
gfx/native_theme_win.cc:482: class ScopedRegionClipping {
Nit: This would be clearer as "ScopedClipRegion"

http://codereview.chromium.org/1988012/diff/30001/31001#newcode506
gfx/native_theme_win.cc:506: // For an indeterminate progress bar, we draw a
moving highlight that's animated
Nit: Sorry, I gave you a set of comments that were > 80 characters.