Add NSString category for providing iOS6-style string drawing APIs.

Add NSString category for providing iOS6-style string drawing APIs.

The iOS6-style APIs have been deprecated, but the replacements are
more verbose and brittle so this category wraps the replacements in
methods similar to the iOS6-style APIs.

This category lives in a new target and directory: src/ui/ios, so
this CL sets up all the gyp/DEPS stuff and only adds a single
method. There will be more methods added in subsequent CLs.

BUG=364419
Committed: https://crrev.com/c018ddc14ace7181edda4d4be225cf6e88e44d2c
Cr-Commit-Position: refs/heads/master@{#293193}

[lliabraa, 2014-08-22 15:52:13]

@lpromero - can you please do a first-pass iOS review? Then I'll send to
stuartmorgan and then src/ui/OWNERS

[lpromero, 2014-08-22 16:08:45]

On 2014/08/22 15:52:13, lliabraa wrote:
> @lpromero - can you please do a first-pass iOS review? Then I'll send to
> stuartmorgan and then src/ui/OWNERS

lgtm

[lliabraa, 2014-08-22 18:37:49]

+stuartmorgan for iOS review.

n.b. I uploaded patchset 2 after lpromero's review because I got a try job
failure.

[lliabraa, 2014-08-25 19:41:53]

https://chromiumcodereview.appspot.com/497503004/diff/20001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://chromiumcodereview.appspot.com/497503004/diff/20001/ui/gfx/gfx.gyp#ne...
ui/gfx/gfx.gyp:314: 'export_dependent_settings': [
maybe this is not correct...I get the following warning when building a static
library that depends on gfx:
-dynamic not specified the following flags are invalid: -ObjC

Should any target that uses this new NSString category depend on ui/ios.gyp:ios
explicitly?

[stuartmorgan, 2014-08-25 20:17:38]

I'm hesitant here; usually we want to polyfill old APIs to look like new ones,
so that code becomes more modern over time, but this is the opposite. This feels
like like we're encouraging things that are probably not practices we want
(silently handling nil, when in most cases trying to measure text with a nil
font should presumably be an error, forcing rounding behaviors)

https://chromiumcodereview.appspot.com/497503004/diff/20001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://chromiumcodereview.appspot.com/497503004/diff/20001/ui/gfx/gfx.gyp#ne...
ui/gfx/gfx.gyp:314: 'export_dependent_settings': [
On 2014/08/25 19:41:52, lliabraa wrote:
> maybe this is not correct...I get the following warning when building a static
> library that depends on gfx:
> -dynamic not specified the following flags are invalid: -ObjC
> 
> Should any target that uses this new NSString category depend on
ui/ios.gyp:ios
> explicitly?

The ui/ gyp files aren't set up the way they used to be, and I'm not used to the
pattern they are using. It looks like yes, in this model.

https://chromiumcodereview.appspot.com/497503004/diff/20001/ui/ios/NSString+C...
File ui/ios/NSString+CrStringDrawing.mm (right):

https://chromiumcodereview.appspot.com/497503004/diff/20001/ui/ios/NSString+C...
ui/ios/NSString+CrStringDrawing.mm:15: ceil(sizeWithAttributes.height));
Do we actually want the ceil? That's just to exactly match the behavior of the
old API it seems like, which is different from just providing a similar
structure of API.

[lliabraa, 2014-08-27 22:03:10]

lliabraa@chromium.org changed reviewers:
+ tfarina@chromium.org

[lliabraa, 2014-08-27 22:03:10]

+tfarina - Can you please take a look a the gyp files in this CL? I see you've
recently been refactoring gyp in ui/ so I'd like to make sure these new targets
look right to you.

[lliabraa, 2014-08-28 12:13:20]

@stuartmorgan - what do you think of having two methods: one is a drop-in for
the iOS 6 behavior, the other is a convenience wrapper to avoid NSDictionary
boilerplate.

https://chromiumcodereview.appspot.com/497503004/diff/20001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://chromiumcodereview.appspot.com/497503004/diff/20001/ui/gfx/gfx.gyp#ne...
ui/gfx/gfx.gyp:314: 'export_dependent_settings': [
On 2014/08/25 20:17:37, stuartmorgan wrote:
> On 2014/08/25 19:41:52, lliabraa wrote:
> > maybe this is not correct...I get the following warning when building a
static
> > library that depends on gfx:
> > -dynamic not specified the following flags are invalid: -ObjC
> > 
> > Should any target that uses this new NSString category depend on
> ui/ios.gyp:ios
> > explicitly?
> 
> The ui/ gyp files aren't set up the way they used to be, and I'm not used to
the
> pattern they are using. It looks like yes, in this model.

changed to link_settings in the ui_ios.gyp file.

https://chromiumcodereview.appspot.com/497503004/diff/20001/ui/ios/NSString+C...
File ui/ios/NSString+CrStringDrawing.mm (right):

https://chromiumcodereview.appspot.com/497503004/diff/20001/ui/ios/NSString+C...
ui/ios/NSString+CrStringDrawing.mm:15: ceil(sizeWithAttributes.height));
On 2014/08/25 20:17:37, stuartmorgan wrote:
> Do we actually want the ceil? That's just to exactly match the behavior of the
> old API it seems like, which is different from just providing a similar
> structure of API.

I've split this into two methods. One is a drop-in replacement for sizeWithFont
(nil-checking, rounding); the other is a convenience wrapper to avoid the
boilerplate of the attributes dictionary (DCHECK on nill font and no rounding).

[tfarina, 2014-08-28 13:11:41]

I will take a look later tonight.

Please, do not land this without sending to Scott or Ben.

Thank you,

[stuartmorgan, 2014-08-28 15:18:14]

LGTM with some nits. You can add the non-behavior-preserving version if you
really want to, but as noted below I'm not sure what it's getting us.

https://chromiumcodereview.appspot.com/497503004/diff/80001/ui/ios/NSString+C...
File ui/ios/NSString+CrStringDrawing.h (right):

https://chromiumcodereview.appspot.com/497503004/diff/80001/ui/ios/NSString+C...
ui/ios/NSString+CrStringDrawing.h:27: // returned are rounded up to integer
values.
Please add a note that this is deprecated and no new uses should be added, and
put a TODO (you can put me if you like) to remove it. My guess is that, e.g.,
GetStringWidthF doesn't actually need rounded values, and that we shouldn't
actually be using this there. Any specific call sites that want nil handling, or
rounding, should be doing it locally.

https://chromiumcodereview.appspot.com/497503004/diff/80001/ui/ios/NSString+C...
File ui/ios/NSString+CrStringDrawing.mm (right):

https://chromiumcodereview.appspot.com/497503004/diff/80001/ui/ios/NSString+C...
ui/ios/NSString+CrStringDrawing.mm:11: - (CGSize)cr_sizeWithFont:(UIFont*)font {
It's not really clear to me why
  [foo sizeWithAttributes:@{ NSFontAttributeName : font }]
is enough worse than this to warrant a utility method. It's not actually more
brittle since the utility doesn't handle nil, and it's barely more verbose. Is
this extremely common or something?

https://chromiumcodereview.appspot.com/497503004/diff/80001/ui/ios/NSString+C...
File ui/ios/NSString+CrStringDrawing_unittest.mm (right):

https://chromiumcodereview.appspot.com/497503004/diff/80001/ui/ios/NSString+C...
ui/ios/NSString+CrStringDrawing_unittest.mm:20: TEST_F(NSStringCrStringDrawing,
testIntegralSizeWithFont) {
Remove "test" from the name.

[lliabraa, 2014-08-28 19:25:06]

Thanks for the review

https://chromiumcodereview.appspot.com/497503004/diff/80001/ui/ios/NSString+C...
File ui/ios/NSString+CrStringDrawing.h (right):

https://chromiumcodereview.appspot.com/497503004/diff/80001/ui/ios/NSString+C...
ui/ios/NSString+CrStringDrawing.h:27: // returned are rounded up to integer
values.
On 2014/08/28 15:18:14, stuartmorgan wrote:
> Please add a note that this is deprecated and no new uses should be added, and
> put a TODO (you can put me if you like) to remove it. My guess is that, e.g.,
> GetStringWidthF doesn't actually need rounded values, and that we shouldn't
> actually be using this there. Any specific call sites that want nil handling,
or
> rounding, should be doing it locally.

Done.

https://chromiumcodereview.appspot.com/497503004/diff/80001/ui/ios/NSString+C...
File ui/ios/NSString+CrStringDrawing.mm (right):

https://chromiumcodereview.appspot.com/497503004/diff/80001/ui/ios/NSString+C...
ui/ios/NSString+CrStringDrawing.mm:11: - (CGSize)cr_sizeWithFont:(UIFont*)font {
On 2014/08/28 15:18:14, stuartmorgan wrote:
> It's not really clear to me why
>   [foo sizeWithAttributes:@{ NSFontAttributeName : font }]
> is enough worse than this to warrant a utility method. It's not actually more
> brittle since the utility doesn't handle nil, and it's barely more verbose. Is
> this extremely common or something?

As discussed offline, this category should have a method to do pixel-aligning
and dictionary construction and a method to serve as a drop-in replacement for
existing call sites.

https://chromiumcodereview.appspot.com/497503004/diff/80001/ui/ios/NSString+C...
File ui/ios/NSString+CrStringDrawing_unittest.mm (right):

https://chromiumcodereview.appspot.com/497503004/diff/80001/ui/ios/NSString+C...
ui/ios/NSString+CrStringDrawing_unittest.mm:20: TEST_F(NSStringCrStringDrawing,
testIntegralSizeWithFont) {
On 2014/08/28 15:18:14, stuartmorgan wrote:
> Remove "test" from the name.

Done.

[tfarina, 2014-08-29 03:27:35]

gyp changes lgtm.

You will have to ask Scott or Ben what they think about the introduction of this
new ios directory under ui/.

https://codereview.chromium.org/497503004/diff/100001/ui/ios/ui_ios_tests.gyp
File ui/ios/ui_ios_tests.gyp (left):

https://codereview.chromium.org/497503004/diff/100001/ui/ios/ui_ios_tests.gyp...
ui/ios/ui_ios_tests.gyp:11: 'target_name': 'boringssl_unittests',
if you tweak the similarity, you can make ui_ios_tests.gyp show as a new file,
rather than a copy of some other random file.

https://codereview.chromium.org/497503004/diff/100001/ui/ios/ui_ios_tests.gyp
File ui/ios/ui_ios_tests.gyp (right):

https://codereview.chromium.org/497503004/diff/100001/ui/ios/ui_ios_tests.gyp...
ui/ios/ui_ios_tests.gyp:12: 'type': '<(gtest_target_type)',
does this needs to be gtest_target_type?

[stuartmorgan, 2014-08-29 14:35:52]

https://codereview.chromium.org/497503004/diff/100001/ui/ios/ui_ios_tests.gyp
File ui/ios/ui_ios_tests.gyp (left):

https://codereview.chromium.org/497503004/diff/100001/ui/ios/ui_ios_tests.gyp...
ui/ios/ui_ios_tests.gyp:11: 'target_name': 'boringssl_unittests',
On 2014/08/29 03:27:35, tfarina wrote:
> if you tweak the similarity, you can make ui_ios_tests.gyp show as a new file,
> rather than a copy of some other random file.

Git is the source of truth now, not svn; does this actually matter any more?

[stuartmorgan, 2014-08-29 14:48:32]

https://codereview.chromium.org/497503004/diff/100001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/497503004/diff/100001/ui/gfx/gfx.gyp#newcode313
ui/gfx/gfx.gyp:313: "<(DEPTH)/ui/ios/ui_ios.gyp:ui_ios",
s/"/'/g

https://codereview.chromium.org/497503004/diff/100001/ui/ios/NSString+CrStrin...
File ui/ios/NSString+CrStringDrawing.mm (right):

https://codereview.chromium.org/497503004/diff/100001/ui/ios/NSString+CrStrin...
ui/ios/NSString+CrStringDrawing.mm:12: CGFloat alignValueToUpperPixel(CGFloat
value) {
Let's upstream the existing utility (with its test) into ui/ios/, rather than
duplicate the code.

I'm okay with a TODO and a bug so this doesn't end up being a yak-shaving
expedition, but I'd like to not have two copies stay around indefinitely.

https://codereview.chromium.org/497503004/diff/100001/ui/ios/NSString+CrStrin...
File ui/ios/NSString+CrStringDrawing_unittest.mm (right):

https://codereview.chromium.org/497503004/diff/100001/ui/ios/NSString+CrStrin...
ui/ios/NSString+CrStringDrawing_unittest.mm:51: 
We should have unit test coverage of the other new method. They can just check
that for various strings, the values returned are at least the values returned
by a direct call to sizeWithAttributes, and are pixel aligned.

[lliabraa, 2014-09-02 13:29:14]

lliabraa@chromium.org changed reviewers:
+ sky@chromium.org

[lliabraa, 2014-09-02 13:29:15]

Thanks for the reviews!

+sky for ui/OWNERS review
+ben for ui/OWNERS FYI

https://codereview.chromium.org/497503004/diff/100001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/497503004/diff/100001/ui/gfx/gfx.gyp#newcode313
ui/gfx/gfx.gyp:313: "<(DEPTH)/ui/ios/ui_ios.gyp:ui_ios",
On 2014/08/29 14:48:31, stuartmorgan wrote:
> s/"/'/g

Done.

https://codereview.chromium.org/497503004/diff/100001/ui/ios/NSString+CrStrin...
File ui/ios/NSString+CrStringDrawing.mm (right):

https://codereview.chromium.org/497503004/diff/100001/ui/ios/NSString+CrStrin...
ui/ios/NSString+CrStringDrawing.mm:12: CGFloat alignValueToUpperPixel(CGFloat
value) {
On 2014/08/29 14:48:31, stuartmorgan wrote:
> Let's upstream the existing utility (with its test) into ui/ios/, rather than
> duplicate the code.
> 
> I'm okay with a TODO and a bug so this doesn't end up being a yak-shaving
> expedition, but I'd like to not have two copies stay around indefinitely.

Done.

https://codereview.chromium.org/497503004/diff/100001/ui/ios/NSString+CrStrin...
File ui/ios/NSString+CrStringDrawing_unittest.mm (right):

https://codereview.chromium.org/497503004/diff/100001/ui/ios/NSString+CrStrin...
ui/ios/NSString+CrStringDrawing_unittest.mm:51: 
On 2014/08/29 14:48:31, stuartmorgan wrote:
> We should have unit test coverage of the other new method. They can just check
> that for various strings, the values returned are at least the values returned
> by a direct call to sizeWithAttributes, and are pixel aligned.

Done.

https://codereview.chromium.org/497503004/diff/100001/ui/ios/ui_ios_tests.gyp
File ui/ios/ui_ios_tests.gyp (left):

https://codereview.chromium.org/497503004/diff/100001/ui/ios/ui_ios_tests.gyp...
ui/ios/ui_ios_tests.gyp:11: 'target_name': 'boringssl_unittests',
On 2014/08/29 03:27:35, tfarina wrote:
> if you tweak the similarity, you can make ui_ios_tests.gyp show as a new file,
> rather than a copy of some other random file.

Acknowledged.

https://codereview.chromium.org/497503004/diff/100001/ui/ios/ui_ios_tests.gyp...
ui/ios/ui_ios_tests.gyp:11: 'target_name': 'boringssl_unittests',
On 2014/08/29 14:35:52, stuartmorgan wrote:
> On 2014/08/29 03:27:35, tfarina wrote:
> > if you tweak the similarity, you can make ui_ios_tests.gyp show as a new
file,
> > rather than a copy of some other random file.
> 
> Git is the source of truth now, not svn; does this actually matter any more?

Acknowledged.

https://codereview.chromium.org/497503004/diff/100001/ui/ios/ui_ios_tests.gyp
File ui/ios/ui_ios_tests.gyp (right):

https://codereview.chromium.org/497503004/diff/100001/ui/ios/ui_ios_tests.gyp...
ui/ios/ui_ios_tests.gyp:12: 'type': '<(gtest_target_type)',
On 2014/08/29 03:27:34, tfarina wrote:
> does this needs to be gtest_target_type?

Seems that most gtest targets use this variable for their type. Why would it be
something else?

[sky, 2014-09-02 18:01:04]

sky@chromium.org changed reviewers:
+ thakis@chromium.org

[sky, 2014-09-02 18:01:05]

sky->thakis as he's a better reviewer for mac related stuff.

https://codereview.chromium.org/497503004/diff/120001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/497503004/diff/120001/build/all.gyp#newcode43
build/all.gyp:43: '../ui/ios/ui_ios_tests.gyp:ui_ios_unittests',
nit: sort.

[sky, 2014-09-02 18:01:32]

sky@chromium.org changed reviewers:
- sky@chromium.org

[Nico, 2014-09-02 18:04:30]

lgtm

https://codereview.chromium.org/497503004/diff/120001/ui/ios/NSString+CrStrin...
File ui/ios/NSString+CrStringDrawing.mm (right):

https://codereview.chromium.org/497503004/diff/120001/ui/ios/NSString+CrStrin...
ui/ios/NSString+CrStringDrawing.mm:14: static CGFloat scale = [[UIScreen
mainScreen] scale];
Is this an expensive method? If not, don't cache it.

[lliabraa, 2014-09-03 18:02:33]

thanks for the reviews

https://codereview.chromium.org/497503004/diff/120001/ui/ios/NSString+CrStrin...
File ui/ios/NSString+CrStringDrawing.mm (right):

https://codereview.chromium.org/497503004/diff/120001/ui/ios/NSString+CrStrin...
ui/ios/NSString+CrStringDrawing.mm:14: static CGFloat scale = [[UIScreen
mainScreen] scale];
On 2014/09/02 18:04:30, Nico (hiding) wrote:
> Is this an expensive method? If not, don't cache it.

Done.

[lliabraa, 2014-09-03 18:03:14]

The CQ bit was checked by lliabraa@chromium.org

[lliabraa, 2014-09-03 18:08:08]

The CQ bit was unchecked by lliabraa@chromium.org

[lliabraa, 2014-09-03 18:08:11]

The CQ bit was checked by lliabraa@chromium.org

[commit-bot: I haz the power, 2014-09-03 18:08:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lliabraa@chromium.org/497503004/140001

[commit-bot: I haz the power, 2014-09-03 19:08:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-09-03 20:37:55]

Committed patchset #8 (id:140001) as f4daf2bd009952a06de8733880719d6e2c1fc2a3

[commit-bot: I haz the power, 2014-09-10 03:27:40]

Patchset 8 (id:??) landed as
https://crrev.com/c018ddc14ace7181edda4d4be225cf6e88e44d2c
Cr-Commit-Position: refs/heads/master@{#293193}