Cleanup gfx::Canvas now that 10562027 has landed

Cleanup gfx::Canvas now that 10562027 has landed

Bug=None
Test=Compiles on Mac and CrOS
R=oshima,sky
TBR=sadrul,sail

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

[pkotwicz, 2012-07-10 19:22:42]

I don't think that http://codereview.chromium.org/10704093/ will land soon (ie
this week)
I think it makes sense to go forward and commit this patch

[oshima, 2012-07-10 20:09:38]

Can you file a bug for places that needs to be converted to
HDC, and add TODO to these places? Then lgtm.

[pkotwicz, 2012-07-12 00:05:00]

Changes as requested.
Implemented HDC where required inhttp://codereview.chromium.org/10690148/

[pkotwicz, 2012-07-15 23:08:57]

Scott for OWNERS

[sky, 2012-07-16 14:26:51]

http://codereview.chromium.org/10701063/diff/48/ash/shell/app_list.cc
File ash/shell/app_list.cc (right):

http://codereview.chromium.org/10701063/diff/48/ash/shell/app_list.cc#newcode207
ash/shell/app_list.cc:207: icon_size, ui::SCALE_FACTOR_100P, false /* is_opaque
*/);
How come we don't want this at the current scale?

http://codereview.chromium.org/10701063/diff/48/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

http://codereview.chromium.org/10701063/diff/48/ui/compositor/layer.cc#newcod...
ui/compositor/layer.cc:434: bool scale_content = scale_content_;
Why do you need scale_content here?

http://codereview.chromium.org/10701063/diff/48/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

http://codereview.chromium.org/10701063/diff/48/ui/gfx/canvas.cc#newcode29
ui/gfx/canvas.cc:29: gfx::Size pixel_size =
size.Scale(ui::GetScaleFactorScale(scale_factor));
Member initialize canvas_ to NULL.

http://codereview.chromium.org/10701063/diff/48/ui/gfx/canvas.cc#newcode41
ui/gfx/canvas.cc:41: scale_factor_ = scale_factor;
Doesn't line 28 set the scale_factor_?

http://codereview.chromium.org/10701063/diff/48/ui/gfx/canvas.cc#newcode63
ui/gfx/canvas.cc:63: Canvas::Canvas(SkCanvas* canvas, ui::ScaleFactor
scale_factor)
Move to match position in header.

http://codereview.chromium.org/10701063/diff/48/ui/gfx/canvas.cc#newcode75
ui/gfx/canvas.cc:75: ui::ScaleFactor scale_factor) {
nit: spacing

[pkotwicz, 2012-07-17 17:19:51]

http://codereview.chromium.org/10701063/diff/48/ash/shell/app_list.cc
File ash/shell/app_list.cc (right):

http://codereview.chromium.org/10701063/diff/48/ash/shell/app_list.cc#newcode207
ash/shell/app_list.cc:207: icon_size, ui::SCALE_FACTOR_100P, false /* is_opaque
*/);
This is part of the ash shell demo code, so I don't think the scale factor
really matters here.

http://codereview.chromium.org/10701063/diff/48/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

http://codereview.chromium.org/10701063/diff/48/ui/compositor/layer.cc#newcod...
ui/compositor/layer.cc:434: bool scale_content = scale_content_;
On 2012/07/16 14:26:51, sky wrote:
> Why do you need scale_content here?

Done.

http://codereview.chromium.org/10701063/diff/48/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

http://codereview.chromium.org/10701063/diff/48/ui/gfx/canvas.cc#newcode29
ui/gfx/canvas.cc:29: gfx::Size pixel_size =
size.Scale(ui::GetScaleFactorScale(scale_factor));
canvas_ is initialized in line 30 as part of the constructor.
Do you want me to initialize it in the initializer list too?

http://codereview.chromium.org/10701063/diff/48/ui/gfx/canvas.cc#newcode41
ui/gfx/canvas.cc:41: scale_factor_ = scale_factor;
On 2012/07/16 14:26:51, sky wrote:
> Doesn't line 28 set the scale_factor_?

Done.

http://codereview.chromium.org/10701063/diff/48/ui/gfx/canvas.cc#newcode63
ui/gfx/canvas.cc:63: Canvas::Canvas(SkCanvas* canvas, ui::ScaleFactor
scale_factor)
On 2012/07/16 14:26:51, sky wrote:
> Move to match position in header.

Done.

http://codereview.chromium.org/10701063/diff/48/ui/gfx/canvas.cc#newcode75
ui/gfx/canvas.cc:75: ui::ScaleFactor scale_factor) {
On 2012/07/16 14:26:51, sky wrote:
> nit: spacing

Done.

[sky, 2012-07-17 21:58:31]

LGTM

http://codereview.chromium.org/10701063/diff/48/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

http://codereview.chromium.org/10701063/diff/48/ui/gfx/canvas.cc#newcode29
ui/gfx/canvas.cc:29: gfx::Size pixel_size =
size.Scale(ui::GetScaleFactorScale(scale_factor));
On 2012/07/17 17:19:52, pkotwicz wrote:
> canvas_ is initialized in line 30 as part of the constructor.
> Do you want me to initialize it in the initializer list too?
> 

Yes.

[commit-bot: I haz the power, 2012-07-20 16:49:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10701063/22001

[commit-bot: I haz the power, 2012-07-20 16:49:44]

Presubmit check for 10701063-22001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/chromeos/status
    chrome/browser/profiles

Presubmit checks took 3.5s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2012-07-20 17:11:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10701063/22001

[commit-bot: I haz the power, 2012-07-20 17:36:42]

Try job failure for 10701063-22001 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-07-20 18:06:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10701063/31003

[commit-bot: I haz the power, 2012-07-20 18:36:01]

Try job failure for 10701063-31003 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-20 20:22:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10701063/21018

[commit-bot: I haz the power, 2012-07-20 20:54:59]

Try job failure for 10701063-21018 on win for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-07-23 16:50:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10701063/24051

[commit-bot: I haz the power, 2012-07-23 17:41:48]

Try job failure for 10701063-24051 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-07-23 18:20:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10701063/23095

[commit-bot: I haz the power, 2012-07-23 20:27:18]

Change committed as 147915

[commit-bot: I haz the power, 2012-07-24 15:10:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10701063/31055

[commit-bot: I haz the power, 2012-07-24 16:51:19]

Change committed as 148123