Initial RenderTextMac implementation using CoreText.

Initial RenderTextMac implementation using CoreText.

Has support for drawing and sizing the text, but not selection and cursor movement.

BUG=125664
TEST=Existing RenderText unit tests and manual testing by enabling RenderText on Mac in print_web_view_helper.cc code.

[Alexei Svitkine (slow), 2012-06-18 15:02:49]

thakis: review
msw: fyi

[Nico, 2012-06-18 15:48:01]

I haven't finished reading render_test_mac.cc. It says "pixels" in several
places where it doesn't really mean pixels (think hidpi), as far as I can tell.

http://codereview.chromium.org/10543057/diff/23004/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

http://codereview.chromium.org/10543057/diff/23004/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:19: // Returns the pixel height of |ct_font|.
View units height, not pixel height, I think.

http://codereview.chromium.org/10543057/diff/23004/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:24: // Creates a CTFont with the given font name and
pixel size. Ownership is
Again, not pixel size I think. (You don't want to have to scale this by 2x on
hidpi screens at the calling site)

[Alexei Svitkine (slow), 2012-06-18 18:50:41]

On Mon, Jun 18, 2012 at 11:48 AM, <thakis@chromium.org> wrote:

> I haven't finished reading render_test_mac.cc. It says "pixels" in several
> places where it doesn't really mean pixels (think hidpi), as far as I can
> tell.
>

Actually, I think pixels is appropriate here - or more specifically units
that the SkCanvas / SkPaint draws in (which may not even be pixels -
depending on the SkDevice backend - but is in fact pixels for the common
raster canvas case I believe). Either way, it's not "view units" - since
it's not related to UI necessarily.

In fact, one of the first uses of this will be print header / footer code,
which doesn't care at all about whether your screen is HiDPI or not - since
the destination is not the screen. (It ends up going to a Skia PDF device
that doesn't actually rasterize text but outputs PDF text spans that are
selectable / copyable, etc. from the PDF.)

I guess the question is what CoreText does - does its output depend on the
screen DPI? Do you know?

-Alexei

[msw, 2012-06-18 21:18:59]

Driveby gyp nits.

http://codereview.chromium.org/10543057/diff/23004/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/10543057/diff/23004/ui/ui.gyp#newcode670
ui/ui.gyp:670: ['toolkit_views==0 and use_canvas_skia==0 and OS!="mac"', {
What build configuration are we trying to exclude these sources from?
It'd be clearer to just specify with OS=="android" or whatever.

http://codereview.chromium.org/10543057/diff/23004/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

http://codereview.chromium.org/10543057/diff/23004/ui/ui_unittests.gypi#newco...
ui/ui_unittests.gypi:197: ['toolkit_views==1 or OS=="mac"', {
similarly, does it make more sense to generally include this now and just
exclude it for android, etc?

[Alexei Svitkine (slow), 2012-06-22 15:55:47]

(Still waiting for a more in-depth review from Nico.)

http://codereview.chromium.org/10543057/diff/23004/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/10543057/diff/23004/ui/ui.gyp#newcode670
ui/ui.gyp:670: ['toolkit_views==0 and use_canvas_skia==0 and OS!="mac"', {
On 2012/06/18 21:18:59, msw wrote:
> What build configuration are we trying to exclude these sources from?
> It'd be clearer to just specify with OS=="android" or whatever.

Done.

http://codereview.chromium.org/10543057/diff/23004/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

http://codereview.chromium.org/10543057/diff/23004/ui/ui_unittests.gypi#newco...
ui/ui_unittests.gypi:197: ['toolkit_views==1 or OS=="mac"', {
On 2012/06/18 21:18:59, msw wrote:
> similarly, does it make more sense to generally include this now and just
> exclude it for android, etc?

Done.

[msw, 2012-06-22 17:42:10]

Remember to add GetBaseline when you land your other CL.

http://codereview.chromium.org/10543057/diff/31001/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/10543057/diff/31001/base/mac/foundation_util.h...
base/mac/foundation_util.h:10: #include
<ApplicationServices/ApplicationServices.h>
Lint: Include "ApplicationServices/ApplicationServices.h" not in alphabetical
order

http://codereview.chromium.org/10543057/diff/31001/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/10543057/diff/31001/ui/ui.gyp#newcode680
ui/ui.gyp:680: 'gfx/render_text_linux.cc',
nit: sorry, but I think android builds automatically exclude *_win and *_linux.
This seems fine if you don't care to contact an android dev (and nico doesn't
know)...

[Nico, 2012-06-22 18:04:10]

Sorry, please ping me faster when I forget about a review.

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:21: return CTFontGetAscent(ct_font) +
CTFontGetDescent(ct_font);
Have you checked that GetAscent() actually returns pixels and now view
coordinates?
https://developer.apple.com/library/mac/#documentation/Carbon/reference/CTFon...
doesn't say either way. (You can put your normal lion laptop into hidpi mode to
check)

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:234: CGColorRef foreground =
gfx::SkColorToCGColorRef(style.foreground);
color space?

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:241: CFNumberRef underline = CFNumberCreate(NULL,
kCFNumberSInt32Type, &value);
Leaks (?)

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:254: CTFontCreateCopyWithSymbolicTraits(font, 0.0,
NULL, traits, traits);
Leaks?

[Alexei Svitkine (slow), 2012-06-22 19:35:29]

(I've also added several TODOs about some areas for improvement in the code.)

http://codereview.chromium.org/10543057/diff/31001/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/10543057/diff/31001/base/mac/foundation_util.h...
base/mac/foundation_util.h:10: #include
<ApplicationServices/ApplicationServices.h>
On 2012/06/22 17:42:10, msw wrote:
> Lint: Include "ApplicationServices/ApplicationServices.h" not in alphabetical
> order

Done.

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:21: return CTFontGetAscent(ct_font) +
CTFontGetDescent(ct_font);
On 2012/06/22 18:04:10, Nico wrote:
> Have you checked that GetAscent() actually returns pixels and now view
> coordinates?
>
https://developer.apple.com/library/mac/#documentation/Carbon/reference/CTFon...
> doesn't say either way. (You can put your normal lion laptop into hidpi mode
to
> check)

I'll have to check.

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:234: CGColorRef foreground =
gfx::SkColorToCGColorRef(style.foreground);
On 2012/06/22 18:04:10, Nico wrote:
> color space?

Can you elaborate the concern? This code only sets the color so that it could
later retrieve it in |ComputeRuns()| - since CoreText isn't actually used to
draw the text.

So assuming that CoreText doesn't do any munging to the color and that 
gfx::CGColorRefToSkColor(gfx::SkColorToCGColorRef()) is an identity function,
this should be fine I think.

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:241: CFNumberRef underline = CFNumberCreate(NULL,
kCFNumberSInt32Type, &value);
On 2012/06/22 18:04:10, Nico wrote:
> Leaks (?)

Fixed.

Why is this case different than kCTForegroundColorAttributeName with a
CGColorRef? For that case, it seems the param doesn't get retained by the call,
whereas for these two, it does.

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:254: CTFontCreateCopyWithSymbolicTraits(font, 0.0,
NULL, traits, traits);
On 2012/06/22 18:04:10, Nico wrote:
> Leaks?

Fixed.

http://codereview.chromium.org/10543057/diff/31001/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/10543057/diff/31001/ui/ui.gyp#newcode680
ui/ui.gyp:680: 'gfx/render_text_linux.cc',
On 2012/06/22 17:42:10, msw wrote:
> nit: sorry, but I think android builds automatically exclude *_win and
*_linux.
> This seems fine if you don't care to contact an android dev (and nico doesn't
> know)...

Done.

[Nico, 2012-06-22 19:38:05]

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:234: CGColorRef foreground =
gfx::SkColorToCGColorRef(style.foreground);
On 2012/06/22 19:35:29, Alexei Svitkine wrote:
> On 2012/06/22 18:04:10, Nico wrote:
> > color space?
> 
> Can you elaborate the concern? This code only sets the color so that it could
> later retrieve it in |ComputeRuns()| - since CoreText isn't actually used to
> draw the text.
> 
> So assuming that CoreText doesn't do any munging to the color and that 
> gfx::CGColorRefToSkColor(gfx::SkColorToCGColorRef()) is an identity function,
> this should be fine I think.

Ok.

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:241: CFNumberRef underline = CFNumberCreate(NULL,
kCFNumberSInt32Type, &value);
On 2012/06/22 19:35:29, Alexei Svitkine wrote:
> On 2012/06/22 18:04:10, Nico wrote:
> > Leaks (?)
> 
> Fixed.
> 
> Why is this case different than kCTForegroundColorAttributeName with a
> CGColorRef? For that case, it seems the param doesn't get retained by the
call,
> whereas for these two, it does.

I didn't notice this for the color because SkCOlorToCGColorRef isn't called
CreateCGColorRefWithSkColor() (I think the latter would be a better name). Are
you sure the color isn't retained above? That'd be weird.

[Alexei Svitkine (slow), 2012-07-16 22:43:07]

I've updated the CL to rebase it on current TOT and also fix an issue with
invalid glyphs being returned by CTRunGetGlyphs(), causing asserts in Skia.

I've also finally had a chance to test this against HiDPI Lion.

Nico, PTAL when you're back.

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:21: return CTFontGetAscent(ct_font) +
CTFontGetDescent(ct_font);
On 2012/06/22 18:04:10, Nico wrote:
> Have you checked that GetAscent() actually returns pixels and now view
> coordinates?
>
https://developer.apple.com/library/mac/#documentation/Carbon/reference/CTFon...
> doesn't say either way. (You can put your normal lion laptop into hidpi mode
to
> check)

I've verified that the metrics values returned are the same regardless of
whether the system is using a HiDPI resolution or not. (For the values returned
here and elsewhere in the file.)

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:241: CFNumberRef underline = CFNumberCreate(NULL,
kCFNumberSInt32Type, &value);
On 2012/06/22 19:38:05, Nico wrote:
> On 2012/06/22 19:35:29, Alexei Svitkine wrote:
> > On 2012/06/22 18:04:10, Nico wrote:
> > > Leaks (?)
> > 
> > Fixed.
> > 
> > Why is this case different than kCTForegroundColorAttributeName with a
> > CGColorRef? For that case, it seems the param doesn't get retained by the
> call,
> > whereas for these two, it does.
> 
> I didn't notice this for the color because SkCOlorToCGColorRef isn't called
> CreateCGColorRefWithSkColor() (I think the latter would be a better name). Are
> you sure the color isn't retained above? That'd be weird.

It indeed seems to be the case. If I CFRelease() |foreground| in ApplyStyles(),
I later get the following error when I try to retrive it in ComputeRuns():

[90095:-1600719552:272684540156830:WARNING:foundation_util.h(322)] Expected
value for key CTForegroundColor to be CGColor but it was Not A Type instead

(i.e. probably that memory location is no longer a CGColor since it was
completely deallocated and possibly allocated for another object...)

[Alexei Svitkine (slow), 2012-07-21 02:43:31]

ping?

[Alexei Svitkine (slow), 2012-07-21 14:10:58]

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:241: CFNumberRef underline = CFNumberCreate(NULL,
kCFNumberSInt32Type, &value);
On 2012/07/16 22:43:08, Alexei Svitkine wrote:
> On 2012/06/22 19:38:05, Nico wrote:
> > On 2012/06/22 19:35:29, Alexei Svitkine wrote:
> > > On 2012/06/22 18:04:10, Nico wrote:
> > > > Leaks (?)
> > > 
> > > Fixed.
> > > 
> > > Why is this case different than kCTForegroundColorAttributeName with a
> > > CGColorRef? For that case, it seems the param doesn't get retained by the
> > call,
> > > whereas for these two, it does.
> > 
> > I didn't notice this for the color because SkCOlorToCGColorRef isn't called
> > CreateCGColorRefWithSkColor() (I think the latter would be a better name).
Are
> > you sure the color isn't retained above? That'd be weird.
> 
> It indeed seems to be the case. If I CFRelease() |foreground| in
ApplyStyles(),
> I later get the following error when I try to retrive it in ComputeRuns():
> 
> [90095:-1600719552:272684540156830:WARNING:foundation_util.h(322)] Expected
> value for key CTForegroundColor to be CGColor but it was Not A Type instead
> 
> (i.e. probably that memory location is no longer a CGColor since it was
> completely deallocated and possibly allocated for another object...)

Thinking about this some more, this really sounds like a bug in CoreText.

I guess the right way to work-around this is to keep track of the CGColor
objects we create and manage their lifetimes so that their lifetimes are greater
than that of |line_|, assuming CoreText holds weak references to them. (If the
CoreText bug gets fixed, then this extra book-keeping will be harmless.)

Do you agree that that's the right solution here?

[Nico, 2012-07-21 16:17:23]

You might want to mention in a comment why you use pixels instead of points.
Other than that (and ios comment below), lgtm.

You can do the CGColorRef change in a follow-up if you want.

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:241: CFNumberRef underline = CFNumberCreate(NULL,
kCFNumberSInt32Type, &value);
On 2012/07/21 14:10:58, Alexei Svitkine wrote:
> On 2012/07/16 22:43:08, Alexei Svitkine wrote:
> > On 2012/06/22 19:38:05, Nico wrote:
> > > On 2012/06/22 19:35:29, Alexei Svitkine wrote:
> > > > On 2012/06/22 18:04:10, Nico wrote:
> > > > > Leaks (?)
> > > > 
> > > > Fixed.
> > > > 
> > > > Why is this case different than kCTForegroundColorAttributeName with a
> > > > CGColorRef? For that case, it seems the param doesn't get retained by
the
> > > call,
> > > > whereas for these two, it does.
> > > 
> > > I didn't notice this for the color because SkCOlorToCGColorRef isn't
called
> > > CreateCGColorRefWithSkColor() (I think the latter would be a better name).
> Are
> > > you sure the color isn't retained above? That'd be weird.
> > 
> > It indeed seems to be the case. If I CFRelease() |foreground| in
> ApplyStyles(),
> > I later get the following error when I try to retrive it in ComputeRuns():
> > 
> > [90095:-1600719552:272684540156830:WARNING:foundation_util.h(322)] Expected
> > value for key CTForegroundColor to be CGColor but it was Not A Type instead
> > 
> > (i.e. probably that memory location is no longer a CGColor since it was
> > completely deallocated and possibly allocated for another object...)
> 
> Thinking about this some more, this really sounds like a bug in CoreText.
> 
> I guess the right way to work-around this is to keep track of the CGColor
> objects we create and manage their lifetimes so that their lifetimes are
greater
> than that of |line_|, assuming CoreText holds weak references to them. (If the
> CoreText bug gets fixed, then this extra book-keeping will be harmless.)
> 
> Do you agree that that's the right solution here?

That sounds right to me. Bonus points for filing a rdar with apple.

http://codereview.chromium.org/10543057/diff/74001/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

http://codereview.chromium.org/10543057/diff/74001/ui/ui_unittests.gypi#newco...
ui/ui_unittests.gypi:200: ['OS=="android"', {
ios?

[Alexei Svitkine (slow), 2012-07-23 14:33:35]

+mark for base/ OWNERS (thakis y u not in base/ OWNERS anymore?)
+sky for ui/ OWNERS

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

http://codereview.chromium.org/10543057/diff/31001/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:241: CFNumberRef underline = CFNumberCreate(NULL,
kCFNumberSInt32Type, &value);
On 2012/07/21 16:17:23, Nico wrote:
> That sounds right to me. Bonus points for filing a rdar with apple.

I did some more testing here and it appears it's actually consistent for all the
values that I'm setting via CFAttributedStringSetAttribute(). None of them get
retained (according to CFGetRetainCount()).

I also checked whether they end up getting released when the CoreText |line_|
gets released, and they don't. So they're proper weak refs and we'll have to
keep track of all of these ourselves and make sure to CFRelease() them when
necessary.

I've changed these back to not use ScopedCFTypeRef for now and added a big TODO
that I plan to address in a followup CL.

http://codereview.chromium.org/10543057/diff/74001/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

http://codereview.chromium.org/10543057/diff/74001/ui/ui_unittests.gypi#newco...
ui/ui_unittests.gypi:200: ['OS=="android"', {
On 2012/07/21 16:17:23, Nico wrote:
> ios?

Done.

[Alexei Svitkine (slow), 2012-07-23 14:33:51]

+mark for base/ OWNERS (thakis y u not in base/ OWNERS anymore?)
+sky for ui/ OWNERS

[Mark Mentovai, 2012-07-23 15:20:39]

LGTM for base OWNERS approval. I only reviewed in base.

[sky, 2012-07-23 16:14:01]

LGTM

[commit-bot: I haz the power, 2012-07-23 16:34:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10543057/90010

[commit-bot: I haz the power, 2012-07-23 16:35:06]

Presubmit check for 10543057-90010 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  ui/gfx/render_text_mac.cc, line 231, 131 chars

Presubmit checks took 2.2s to calculate.

[stuartmorgan, 2012-07-24 10:55:47]

Could I convince you to move your foundation_util changes to another file? They
aren't foundation, and more importantly they are things that aren't available on
iOS, so this broke the iOS FYI bot.

Fixing it would mean putting a bunch of ifdef's which seems much uglier than
putting non-foundation utility methods somewhere other than foundation_util.

[stuartmorgan, 2012-07-24 10:56:24]

(And yes, we are working on getting our bot into CQ and main waterfall so you'd
have known about this in advance; we're close but not quite there yet)

[Alexei Svitkine (slow), 2012-07-24 12:43:21]

On Tue, Jul 24, 2012 at 6:55 AM, <stuartmorgan@chromium.org> wrote:

> Could I convince you to move your foundation_util changes to another file?
> They
> aren't foundation, and more importantly they are things that aren't
> available on
> iOS, so this broke the iOS FYI bot.
>

Which ones aren't available on iOS? I was under the impression that
CoreText was available on iOS. Perhaps we're just missing an include of
CoreText/CoreText.h on iOS?


> Fixing it would mean putting a bunch of ifdef's which seems much uglier
> than
> putting non-foundation utility methods somewhere other than
> foundation_util.
>

Putting it into a separate file would mean duplicating a lot of the macro
infrastructure that's in foundation_util with different names and making
parallel classes to things like ScopedCFTypeRef and such... Per earlier
discussion with Mark it didn't seem like that was the way to go.

-Alexei

[stuartmorgan, 2012-07-24 12:53:00]

2012/7/24 Alexei Svitkine <asvitkine@chromium.org>

>
>
> On Tue, Jul 24, 2012 at 6:55 AM, <stuartmorgan@chromium.org> wrote:
>
>> Could I convince you to move your foundation_util changes to another
>> file? They
>> aren't foundation, and more importantly they are things that aren't
>> available on
>> iOS, so this broke the iOS FYI bot.
>>
>
> Which ones aren't available on iOS? I was under the impression that
> CoreText was available on iOS. Perhaps we're just missing an include of
> CoreText/CoreText.h on iOS?
>
>
>> Fixing it would mean putting a bunch of ifdef's which seems much uglier
>> than
>> putting non-foundation utility methods somewhere other than
>> foundation_util.
>>
>
> Putting it into a separate file would mean duplicating a lot of the macro
> infrastructure that's in foundation_util with different names and making
> parallel classes to things like ScopedCFTypeRef and such... Per earlier
> discussion with Mark it didn't seem like that was the way to go.
>
> -Alexei
>

[stuartmorgan, 2012-07-24 12:56:17]

Sorry, not sure what happened there.

I haven't dug all the way in, but you can't include ApplicationServices on iOS
at all. I'll poke a bit and see if maybe those things are there and just in
another framework.

[stuartmorgan, 2012-07-24 13:02:59]

On 2012/07/24 12:43:21, Alexei Svitkine wrote:
> Perhaps we're just missing an include of CoreText/CoreText.h on iOS?

You're right, sorry for freaking out :) I'll follow up with a fix.