Turn on canvas_skia for OSX

[OSX] Turn on canvas_skia for OSX

BUG=151935
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221930

[groby-ooo-7-16, 2013-09-05 01:42:17]

Alexei - any reasons we can't just turn on canvas_skia for OSX? As far as I can
tell, there are no consumers of the few routines implemented in canvas_mac.mm
except AutofillDialogControllerImpl via DrawStringWithShadows - and that code
actually needs the skia functionality.

[Alexei Svitkine (slow), 2013-09-05 04:31:28]

If I remember correctly, there's a few things RenderTextMac does differently
than RenderTextWin and RenderTextLinux that I wanted to fix before enabling
this. I'll look up what those were tomorrow, but for now I've sent some try jobs
on this to see if the tests catch it.

[groby-ooo-7-16, 2013-09-05 11:20:50]

Given that the current canvas_mac ignores the font and fails to center the text
and doesn't support many other features at all, I doubt it could be worse :)

Also, as far as I can tell, requestAutocomplete is the only OSX consumer of
canvas text rendering - and canvas_skia's result is *vastly* preferable over
canvas_mac for that one.

[Alexei Svitkine (slow), 2013-09-05 13:31:04]

On 2013/09/05 11:20:50, groby wrote:
> Given that the current canvas_mac ignores the font and fails to center the
text
> and doesn't support many other features at all, I doubt it could be worse :)
> 
> Also, as far as I can tell, requestAutocomplete is the only OSX consumer of
> canvas text rendering - and canvas_skia's result is *vastly* preferable over
> canvas_mac for that one.

There are more things that end up using the canvas code than
requestAutocomplete.

Anything that uses Font::GetStringWidth(), for example, goes through
Canvas::SizeStringInt(). Which would be things like ui::ElideString() and many
others.

[groby-ooo-7-16, 2013-09-05 13:51:31]

On 2013/09/05 13:31:04, Alexei Svitkine wrote:
> On 2013/09/05 11:20:50, groby wrote:
> > Given that the current canvas_mac ignores the font and fails to center the
> text
> > and doesn't support many other features at all, I doubt it could be worse :)
> > 
> > Also, as far as I can tell, requestAutocomplete is the only OSX consumer of
> > canvas text rendering - and canvas_skia's result is *vastly* preferable over
> > canvas_mac for that one.
> 
> There are more things that end up using the canvas code than
> requestAutocomplete.
> 
> Anything that uses Font::GetStringWidth(), for example, goes through
> Canvas::SizeStringInt(). Which would be things like ui::ElideString() and many
> others.
I _think_ except for ElideString SizeStringInt is called only from Views code.
Will verify once I've got more than a phone connection. 

But yeah, that's why I have the fallback CL with just the DrawShadowText lifted
from canvas_skia to canvas_mac. That's the part I absolutely need. If you think
the wholesale switch is too much risk, let's do that instead.

[Alexei Svitkine (slow), 2013-09-05 14:32:09]

LGTM

There is non-Views code that does end up calling Canvas::GetStringWidth(),
though you're right that its use is dominated by views.

Anyway, I am fully supportive of this change (it's been on my TODO for a long
time) and it looks like one of the issues that blocked me from doing this before
is now fixed (size of empty string returns a good height now).

One thing I suggest verifying before landing this is that
Canvas::GetStringWidth("foo") returns the same value pre and post this CL.

[groby-ooo-7-16, 2013-09-05 15:57:47]

On 2013/09/05 14:32:09, Alexei Svitkine wrote:
> LGTM
> 
> There is non-Views code that does end up calling Canvas::GetStringWidth(),
> though you're right that its use is dominated by views.
> 
> Anyway, I am fully supportive of this change (it's been on my TODO for a long
> time) and it looks like one of the issues that blocked me from doing this
before
> is now fixed (size of empty string returns a good height now).
> 
> One thing I suggest verifying before landing this is that
> Canvas::GetStringWidth("foo") returns the same value pre and post this CL.

Will try - think there's value in an actual unit test?

[Alexei Svitkine (slow), 2013-09-05 16:34:53]

On 2013/09/05 15:57:47, groby wrote:
> On 2013/09/05 14:32:09, Alexei Svitkine wrote:
> > LGTM
> > 
> > There is non-Views code that does end up calling Canvas::GetStringWidth(),
> > though you're right that its use is dominated by views.
> > 
> > Anyway, I am fully supportive of this change (it's been on my TODO for a
long
> > time) and it looks like one of the issues that blocked me from doing this
> before
> > is now fixed (size of empty string returns a good height now).
> > 
> > One thing I suggest verifying before landing this is that
> > Canvas::GetStringWidth("foo") returns the same value pre and post this CL.
> 
> Will try - think there's value in an actual unit test?

I'm not sure if it makes to have a unit test with a hardcoded value. Perhaps the
unit test could inline the previous canvas_skia.mm string width implementation,
though...

[groby-ooo-7-16, 2013-09-06 16:36:07]

Added. (And passes fine).

[Alexei Svitkine (slow), 2013-09-06 17:01:05]

Sweet! Some comments below.

https://codereview.chromium.org/23456020/diff/12001/ui/gfx/canvas_mac_unittes...
File ui/gfx/canvas_mac_unittest.mm (right):

https://codereview.chromium.org/23456020/diff/12001/ui/gfx/canvas_mac_unittes...
ui/gfx/canvas_mac_unittest.mm:18: // Mac-specific code for string size
computations. Verbatim from canvas_mac.mm.
Since canvas_mac.mm is going away, change the comment to mention that this
corresponds to then old implementation that used to be in canvas_mac.mm. (So
that the reference to canvas_mac.mm is less confusing.)

https://codereview.chromium.org/23456020/diff/12001/ui/gfx/canvas_mac_unittes...
ui/gfx/canvas_mac_unittest.mm:40: namespace gfx {
Make this surround the anon namespace (currently you have a compile error since
the gfx:: prefix is omitted in the function above.

https://codereview.chromium.org/23456020/diff/12001/ui/gfx/canvas_mac_unittes...
ui/gfx/canvas_mac_unittest.mm:44: void CompareSizes(const char* text) {
Add a comment.

https://codereview.chromium.org/23456020/diff/12001/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

https://codereview.chromium.org/23456020/diff/12001/ui/ui_unittests.gypi#newc...
ui/ui_unittests.gypi:173: 'gfx/canvas_mac_unittest.mm',
Nit: I'd actually name this canvas_unittest_mac.mm instead, since canvas_mac
isn't a thing anymore.

[groby-ooo-7-16, 2013-09-06 17:51:25]

PTAL

https://codereview.chromium.org/23456020/diff/12001/ui/gfx/canvas_mac_unittes...
File ui/gfx/canvas_mac_unittest.mm (right):

https://codereview.chromium.org/23456020/diff/12001/ui/gfx/canvas_mac_unittes...
ui/gfx/canvas_mac_unittest.mm:18: // Mac-specific code for string size
computations. Verbatim from canvas_mac.mm.
On 2013/09/06 17:01:06, Alexei Svitkine wrote:
> Since canvas_mac.mm is going away, change the comment to mention that this
> corresponds to then old implementation that used to be in canvas_mac.mm. (So
> that the reference to canvas_mac.mm is less confusing.)

Done.

https://codereview.chromium.org/23456020/diff/12001/ui/gfx/canvas_mac_unittes...
ui/gfx/canvas_mac_unittest.mm:40: namespace gfx {
Sigh. Bitten by debug code, again. Done.

On 2013/09/06 17:01:06, Alexei Svitkine wrote:
> Make this surround the anon namespace (currently you have a compile error
since
> the gfx:: prefix is omitted in the function above.

https://codereview.chromium.org/23456020/diff/12001/ui/gfx/canvas_mac_unittes...
ui/gfx/canvas_mac_unittest.mm:44: void CompareSizes(const char* text) {
On 2013/09/06 17:01:06, Alexei Svitkine wrote:
> Add a comment.

Done.

https://codereview.chromium.org/23456020/diff/12001/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

https://codereview.chromium.org/23456020/diff/12001/ui/ui_unittests.gypi#newc...
ui/ui_unittests.gypi:173: 'gfx/canvas_mac_unittest.mm',
On 2013/09/06 17:01:06, Alexei Svitkine wrote:
> Nit: I'd actually name this canvas_unittest_mac.mm instead, since canvas_mac
> isn't a thing anymore.

Done.

[Alexei Svitkine (slow), 2013-09-06 17:59:33]

LGTM with a nit

Thanks!

https://codereview.chromium.org/23456020/diff/23001/ui/gfx/canvas_unittest_ma...
File ui/gfx/canvas_unittest_mac.mm (right):

https://codereview.chromium.org/23456020/diff/23001/ui/gfx/canvas_unittest_ma...
ui/gfx/canvas_unittest_mac.mm:80: }  // gfx
Nit: // namespace gfx

[commit-bot: I haz the power, 2013-09-06 18:11:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/23456020/29001

[commit-bot: I haz the power, 2013-09-07 04:39:11]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-07 06:26:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/23456020/29001

[commit-bot: I haz the power, 2013-09-07 07:18:18]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-07 14:07:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/23456020/29001

[commit-bot: I haz the power, 2013-09-07 23:27:01]

Change committed as 221930

[Alexei Svitkine (slow), 2013-09-08 06:51:35]

https://chromiumcodereview.appspot.com/23456020/diff/29001/ui/gfx/canvas_unit...
File ui/gfx/canvas_unittest_mac.mm (right):

https://chromiumcodereview.appspot.com/23456020/diff/29001/ui/gfx/canvas_unit...
ui/gfx/canvas_unittest_mac.mm:12: #include "ui/base/range/range.h"
Looks like this got moved to ui/gfx/range/range.h while your CL was in the cq.