Move more web widgets painting from webkit to chrome.

Move more web widgets painting from webkit to chrome.

- Move linux web widgets painting code from webkit;
- Move dependent resources from webkit_resources into gfx_resources and
  follow the pattern in net package to add resource loading support to
  gfx package;
- Update ChromeOS theme engine to follow mocks in chromium-os:9256.

BUG=chromium-os:9256
TEST=Verify default web widgets has desired look on ChromeOS.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71969

[xiyuan, 2011-01-14 18:08:46]

This is the step 2 change after WebThemeEngine API change 
(https://bugs.webkit.org/show_bug.cgi?id=52413) is landed and rolled in.

[tony, 2011-01-14 22:06:04]

http://codereview.chromium.org/6254004/diff/1/chrome/browser/chromeos/native_...
File chrome/browser/chromeos/native_theme_chromeos.h (right):

http://codereview.chromium.org/6254004/diff/1/chrome/browser/chromeos/native_...
chrome/browser/chromeos/native_theme_chromeos.h:33: const ButtonExtraParams&
button);
Can you add OVERRIDE to the end of these methods?  See base/compiler_specific.h.

Also, I think the Chromeos bits of this change should be a separate follow up
change to make this diff smaller.  Dave can help review those parts.

http://codereview.chromium.org/6254004/diff/1/gfx/color_utils.cc
File gfx/color_utils.cc (right):

http://codereview.chromium.org/6254004/diff/1/gfx/color_utils.cc#newcode148
gfx/color_utils.cc:148: SkColor BrightenColor(const HSL& hsl, SkAlpha alpha,
double lightness_amount) {
Can we just use HSLShift for this?  It's a little different, so maybe it'll
force us to rebaseline a lot of tests.  If that's a lot of tests, I would just
move this method into native_theme_linux.*.  This method doesn't seem like it'll
be used outside that file.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc
File gfx/native_theme_linux.cc (right):

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:50: data.size(), &bitmap);
Should we DCHECK the return value of Decode?  Maybe also DCHECK(data.length() >
0) << " Unable to load " << key; or something.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:84: return gfx::Size(11, 21);
Can we convert 11 and 21 into constants?

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:390: image = state == kDisabled ?
image_disabled_indeterminate :
Nit: I normally line up the : below the ?

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:432: const int right = rect.right();
Local consts should still be of the form kRight.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:451: const int border_alpha = state == kHover ? 0x80 :
0x55;
const naming

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:459: SkPoint p[2];
Please use a variable name other than p.  Maybe gradient_bounds?

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:461: const int dark_end = !light_end;
const naming

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:511: const SkColor kLightColor = text.is_listbox ?
const naming

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:601: const int midy = rect.y() + rect.height() / 2;
const naming

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:627: const int midy = rect.y() + rect.height() / 2;
const naming

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:660: State state,
Nit: I think we often put multiple params on the same line

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:721: SkIntToScalar(rect.bottom())
indent 2 spaces more

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:769: SkShader* shader =
SkShader::CreateBitmapShader(bitmap,
Is there a smart pointer we can use to auto-unref SkShaders?

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.h
File gfx/native_theme_linux.h (right):

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.h#newcod...
gfx/native_theme_linux.h:189: void DrawTiledImage(SkCanvas* canvas,
Can this be protected?

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.h#newcod...
gfx/native_theme_linux.h:195: SkColor SaturateAndBrighten(SkScalar* hsv,
Can this be protected too?

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.h#newcod...
gfx/native_theme_linux.h:225: static const int kCheckboxAndRadioHeight;
Can we just move these into the .cc file?

http://codereview.chromium.org/6254004/diff/1/webkit/tools/test_shell/test_sh...
File webkit/tools/test_shell/test_shell_win.cc (right):

http://codereview.chromium.org/6254004/diff/1/webkit/tools/test_shell/test_sh...
webkit/tools/test_shell/test_shell_win.cc:141: // This is called indirectly by
the network layer to access resources.
We actually call TestShell::ResourceProvider, don't we?

http://codereview.chromium.org/6254004/diff/1/webkit/tools/test_shell/test_sh...
webkit/tools/test_shell/test_shell_win.cc:810: return
ResourceProvider(resource_id);
Can we just call TestShell::ResourceProvider here and remove the local
ResourceProvider function?

[xiyuan, 2011-01-18 21:30:48]

Sorry it took me a bit long. I was having some fun with the webkit dir reorg. :p

http://codereview.chromium.org/6254004/diff/1/chrome/browser/chromeos/native_...
File chrome/browser/chromeos/native_theme_chromeos.h (right):

http://codereview.chromium.org/6254004/diff/1/chrome/browser/chromeos/native_...
chrome/browser/chromeos/native_theme_chromeos.h:33: const ButtonExtraParams&
button);
On 2011/01/14 22:06:04, tony wrote:
> Can you add OVERRIDE to the end of these methods?  See
base/compiler_specific.h.
> 
> Also, I think the Chromeos bits of this change should be a separate follow up
> change to make this diff smaller.  Dave can help review those parts.

Done. And moved the chromeos specific changes into a new CL:
http://codereview.chromium.org/6247012/ as suggested.

http://codereview.chromium.org/6254004/diff/1/gfx/color_utils.cc
File gfx/color_utils.cc (right):

http://codereview.chromium.org/6254004/diff/1/gfx/color_utils.cc#newcode148
gfx/color_utils.cc:148: SkColor BrightenColor(const HSL& hsl, SkAlpha alpha,
double lightness_amount) {
On 2011/01/14 22:06:04, tony wrote:
> Can we just use HSLShift for this?  It's a little different, so maybe it'll
> force us to rebaseline a lot of tests.  If that's a lot of tests, I would just
> move this method into native_theme_linux.*.  This method doesn't seem like
it'll
> be used outside that file.

Done. Moved it into native_theme_linux.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc
File gfx/native_theme_linux.cc (right):

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:50: data.size(), &bitmap);
On 2011/01/14 22:06:04, tony wrote:
> Should we DCHECK the return value of Decode?  Maybe also DCHECK(data.length()
>
> 0) << " Unable to load " << key; or something.

Done.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:84: return gfx::Size(11, 21);
On 2011/01/14 22:06:04, tony wrote:
> Can we convert 11 and 21 into constants?

Done.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:390: image = state == kDisabled ?
image_disabled_indeterminate :
On 2011/01/14 22:06:04, tony wrote:
> Nit: I normally line up the : below the ?

Done.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:432: const int right = rect.right();
On 2011/01/14 22:06:04, tony wrote:
> Local consts should still be of the form kRight.
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names

Done.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:451: const int border_alpha = state == kHover ? 0x80 :
0x55;
On 2011/01/14 22:06:04, tony wrote:
> const naming

Done.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:459: SkPoint p[2];
On 2011/01/14 22:06:04, tony wrote:
> Please use a variable name other than p.  Maybe gradient_bounds?

Done.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:461: const int dark_end = !light_end;
On 2011/01/14 22:06:04, tony wrote:
> const naming

Done.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:511: const SkColor kLightColor = text.is_listbox ?
On 2011/01/14 22:06:04, tony wrote:
> const naming

Done. border_width -> kBorderWidth

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:601: const int midy = rect.y() + rect.height() / 2;
On 2011/01/14 22:06:04, tony wrote:
> const naming

Done.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:627: const int midy = rect.y() + rect.height() / 2;
On 2011/01/14 22:06:04, tony wrote:
> const naming

Done.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:660: State state,
On 2011/01/14 22:06:04, tony wrote:
> Nit: I think we often put multiple params on the same line

The third param is long and could in. That's why I could not align all params
with the first param and give each a separate line.
Isn't this way more clear? :)

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:721: SkIntToScalar(rect.bottom())
On 2011/01/14 22:06:04, tony wrote:
> indent 2 spaces more

Done.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:769: SkShader* shader =
SkShader::CreateBitmapShader(bitmap,
On 2011/01/14 22:06:04, tony wrote:
> Is there a smart pointer we can use to auto-unref SkShaders?

I poked around in skia code and here are some findings:

We could not use SkRefPtr because it always increase ref count when attached to
an object and skia object is created with ref count 1. So assigning to a
SkRefPtr would leave ref count 1 when SkRefPtr is out of scope. I am not sure if
we should add a method that makes it able to attach to an object without add ref
count as this might be a bug load in the future.

There is a SkAutoUnref that we might be able to use but then we need a cast in
setShader call. It would be something like this:
SkAutoUnref shader(SkShader::CreateBitmapShader(...));
...
paint.setShader(static_cast<SkShader*>(shader.get()));
...

Skia code seems use this pattern when referencing newly create object.
paint.setShader(shader)->unref();

Which way is better? I personally like the skia pattern a bit.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.h
File gfx/native_theme_linux.h (right):

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.h#newcod...
gfx/native_theme_linux.h:189: void DrawTiledImage(SkCanvas* canvas,
On 2011/01/14 22:06:04, tony wrote:
> Can this be protected?

Done.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.h#newcod...
gfx/native_theme_linux.h:195: SkColor SaturateAndBrighten(SkScalar* hsv,
On 2011/01/14 22:06:04, tony wrote:
> Can this be protected too?

Done.

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.h#newcod...
gfx/native_theme_linux.h:225: static const int kCheckboxAndRadioHeight;
On 2011/01/14 22:06:04, tony wrote:
> Can we just move these into the .cc file?

Done.

http://codereview.chromium.org/6254004/diff/1/webkit/tools/test_shell/test_sh...
File webkit/tools/test_shell/test_shell_win.cc (right):

http://codereview.chromium.org/6254004/diff/1/webkit/tools/test_shell/test_sh...
webkit/tools/test_shell/test_shell_win.cc:141: // This is called indirectly by
the network layer to access resources.
On 2011/01/14 22:06:04, tony wrote:
> We actually call TestShell::ResourceProvider, don't we?

Yep, you are right. Removed as suggested.

http://codereview.chromium.org/6254004/diff/1/webkit/tools/test_shell/test_sh...
webkit/tools/test_shell/test_shell_win.cc:810: return
ResourceProvider(resource_id);
On 2011/01/14 22:06:04, tony wrote:
> Can we just call TestShell::ResourceProvider here and remove the local
> ResourceProvider function?

Done.

[tony, 2011-01-18 22:39:19]

LGTM.  BTW, about how many tests need to be rebaselined?

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc
File gfx/native_theme_linux.cc (right):

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:660: State state,
On 2011/01/18 21:30:49, xiyuan wrote:
> On 2011/01/14 22:06:04, tony wrote:
> > Nit: I think we often put multiple params on the same line
> 
> The third param is long and could in. That's why I could not align all params
> with the first param and give each a separate line.
> Isn't this way more clear? :)

*shrug* this is fine.  I don't have a strong feeling one way or another. :)

http://codereview.chromium.org/6254004/diff/1/gfx/native_theme_linux.cc#newco...
gfx/native_theme_linux.cc:769: SkShader* shader =
SkShader::CreateBitmapShader(bitmap,
On 2011/01/18 21:30:49, xiyuan wrote:
> Which way is better? I personally like the skia pattern a bit.

Ok, this seems fine.  I wasn't sure if there was a more common/better pattern,
but since this is what Skia code does, seems fine for us to continue to do this.

http://codereview.chromium.org/6254004/diff/8001/gfx/native_theme_linux.cc#ne...
gfx/native_theme_linux.cc:653: DrawHorizLine(canvas, midx - 2, midx + 2, midy,
paint);
Nit: Can you put () around the ==?  Also, it should be kHovered.

http://codereview.chromium.org/6254004/diff/8001/webkit/glue/webthemeengine_i...
File webkit/glue/webthemeengine_impl_linux.cc (right):

http://codereview.chromium.org/6254004/diff/8001/webkit/glue/webthemeengine_i...
webkit/glue/webthemeengine_impl_linux.cc:95: } else if (part ==
WebKit::WebThemeEngine::PartCheckbox) {
Should this be a switch instead of if/else if?

[DaveMoore, 2011-01-19 17:04:13]

http://codereview.chromium.org/6254004/diff/8001/gfx/native_theme_linux.cc
File gfx/native_theme_linux.cc (right):

http://codereview.chromium.org/6254004/diff/8001/gfx/native_theme_linux.cc#ne...
gfx/native_theme_linux.cc:442: image = button.checked ? image_disabled_on :
image_disabled_off;
Nit: No braces around the single line if / else

http://codereview.chromium.org/6254004/diff/8001/gfx/native_theme_linux.h
File gfx/native_theme_linux.h (right):

http://codereview.chromium.org/6254004/diff/8001/gfx/native_theme_linux.h#new...
gfx/native_theme_linux.h:62: bool is_default;  // Whether the button is default
button.
Nit: Line up the comments

http://codereview.chromium.org/6254004/diff/8001/gfx/native_theme_linux.h#new...
gfx/native_theme_linux.h:127: virtual void PaintArrowButton(
Now that there are other types drawn in this file the methods for scrollbar
drawing should be more specific

GetPartSize()
PaintScrollbarTrack()
PaintScrollbarThumb()


PaintArrowButton() is used for both other controls so it's ok

http://codereview.chromium.org/6254004/diff/8001/gfx/native_theme_linux.h#new...
gfx/native_theme_linux.h:134: Part part,
Nit: The indenting of all the declarations is wrong (including the existing
ones). The subsequent args should line up with the first one.

[xiyuan, 2011-01-19 19:06:42]

CL updated to address comments. Please take another look.

tony: it breaks about 30-40 layout tests: image tests that have checkbox, radio,
menu list button and progress bar.

Running trybots again with webkit changes. I missed webkit.gyp change yesterday
because of the dir reorg. :( We could see how many layout tests break on
linux_layout bot.

http://codereview.chromium.org/6254004/diff/8001/gfx/native_theme_linux.cc
File gfx/native_theme_linux.cc (right):

http://codereview.chromium.org/6254004/diff/8001/gfx/native_theme_linux.cc#ne...
gfx/native_theme_linux.cc:442: image = button.checked ? image_disabled_on :
image_disabled_off;
On 2011/01/19 17:04:13, davemoore wrote:
> Nit: No braces around the single line if / else

Done.

http://codereview.chromium.org/6254004/diff/8001/gfx/native_theme_linux.cc#ne...
gfx/native_theme_linux.cc:653: const bool hovered = state == kHover ||
slider.in_drag;
On 2011/01/18 22:39:19, tony wrote:
> Nit: Can you put () around the ==?  Also, it should be kHovered.

Done.

http://codereview.chromium.org/6254004/diff/8001/gfx/native_theme_linux.h
File gfx/native_theme_linux.h (right):

http://codereview.chromium.org/6254004/diff/8001/gfx/native_theme_linux.h#new...
gfx/native_theme_linux.h:62: bool is_default;  // Whether the button is default
button.
On 2011/01/19 17:04:13, davemoore wrote:
> Nit: Line up the comments

Done.

http://codereview.chromium.org/6254004/diff/8001/gfx/native_theme_linux.h#new...
gfx/native_theme_linux.h:127: virtual void PaintArrowButton(
On 2011/01/19 17:04:13, davemoore wrote:
> Now that there are other types drawn in this file the methods for scrollbar
> drawing should be more specific
> 
> GetPartSize()
> PaintScrollbarTrack()
> PaintScrollbarThumb()
> 
> 
> PaintArrowButton() is used for both other controls so it's ok

Done.

http://codereview.chromium.org/6254004/diff/8001/gfx/native_theme_linux.h#new...
gfx/native_theme_linux.h:134: Part part,
On 2011/01/19 17:04:13, davemoore wrote:
> Nit: The indenting of all the declarations is wrong (including the existing
> ones). The subsequent args should line up with the first one.

Done. I am using the 4-space indent since there are long args that could not fit
in 80 chars if align with first arg.

http://codereview.chromium.org/6254004/diff/8001/webkit/glue/webthemeengine_i...
File webkit/glue/webthemeengine_impl_linux.cc (right):

http://codereview.chromium.org/6254004/diff/8001/webkit/glue/webthemeengine_i...
webkit/glue/webthemeengine_impl_linux.cc:95: } else if (part ==
WebKit::WebThemeEngine::PartCheckbox) {
On 2011/01/18 22:39:19, tony wrote:
> Should this be a switch instead of if/else if?

Done.

[tony, 2011-01-19 19:13:13]

On 2011/01/19 19:06:42, xiyuan wrote:
> CL updated to address comments. Please take another look.
> 
> tony: it breaks about 30-40 layout tests: image tests that have checkbox,
radio,
> menu list button and progress bar.

Rebaselining 30-40 tests sounds fine to me.  The change should be visually minor
so I don't think it's a big deal.

[DaveMoore, 2011-01-19 22:25:03]

LGTM

[xiyuan, 2011-01-19 23:46:20]

CL updated to fix fast/replaced/width100percent-checkbox.html which is a real
layout test break. We should stretch the checkbox/radio image. I moved
DrawBitmapInt which can do stretching from NativeThemeChromeos into
NativeThemeLinux.