Change OmniboxPopupTruncatingLabel to be based on UILabel.

Change OmniboxPopupTruncatingLabel to be based on UILabel.

When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not
support attributed strings. My assumption is that this is why it's based
on CATextLayer. As of iOS 6, however, UILabel has the attributedString
property which allows us to set the font characteristics we need.

Consequently, OmniboxPopupMaterialViewController no longer needs to
create and cache CTFontRefs.

Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS
10.2, and iPad Air/iOS 10.2.

BUG=

Review-Url: https://codereview.chromium.org/2695413003
Cr-Commit-Position: refs/heads/master@{#452638}
Committed: https://chromium.googlesource.com/chromium/src/+/af40f7a6563089a73b4a18e1d9abac7cbff3b88f

[Justin Donnelly, 2017-02-16 18:05:15]

Description was changed from

==========
Change OmniboxPopupTruncatingLabel to be based on UILabel.

BUG=
==========

to

==========
Change OmniboxPopupTruncatingLabel to be based on UILabel.

When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not support
attributed strings. My assumption is that this is why it's based on CATextLayer.
As of iOS 6, however, UILabel has the attributedString property which allows us
to set the font characteristics we need.

This change makes OmniboxPopupTruncatingLabel a much closer copy of
GTMFadeTruncatingLabel. The only difference now is a couple conditional cases to
support UILabels with attributedString set.

BUG=
==========

[Justin Donnelly, 2017-02-16 18:17:41]

Description was changed from

==========
Change OmniboxPopupTruncatingLabel to be based on UILabel.

When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not support
attributed strings. My assumption is that this is why it's based on CATextLayer.
As of iOS 6, however, UILabel has the attributedString property which allows us
to set the font characteristics we need.

This change makes OmniboxPopupTruncatingLabel a much closer copy of
GTMFadeTruncatingLabel. The only difference now is a couple conditional cases to
support UILabels with attributedString set.

BUG=
==========

to

==========
Change OmniboxPopupTruncatingLabel to be based on UILabel.

When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not support
attributed strings. My assumption is that this is why it's based on CATextLayer.
As of iOS 6, however, UILabel has the attributedString property which allows us
to set the font characteristics we need.

Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS 10.2, and
iPad Air/iOS 10.2.

BUG=
==========

[Justin Donnelly, 2017-02-16 18:18:21]

Description was changed from

==========
Change OmniboxPopupTruncatingLabel to be based on UILabel.

When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not support
attributed strings. My assumption is that this is why it's based on CATextLayer.
As of iOS 6, however, UILabel has the attributedString property which allows us
to set the font characteristics we need.

Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS 10.2, and
iPad Air/iOS 10.2.

BUG=
==========

to

==========
Change OmniboxPopupTruncatingLabel to be based on UILabel.

When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not support
attributed strings. My assumption is that this is why it's based on CATextLayer.
As of
iOS 6, however, UILabel has the attributedString property which allows us to set
the
font characteristics we need.

Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS 10.2, and
iPad
Air/iOS 10.2.

BUG=
==========

[Justin Donnelly, 2017-02-16 18:20:18]

Description was changed from

==========
Change OmniboxPopupTruncatingLabel to be based on UILabel.

When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not support
attributed strings. My assumption is that this is why it's based on CATextLayer.
As of
iOS 6, however, UILabel has the attributedString property which allows us to set
the
font characteristics we need.

Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS 10.2, and
iPad
Air/iOS 10.2.

BUG=
==========

to

==========
Change OmniboxPopupTruncatingLabel to be based on UILabel.

When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not
support attributed strings. My assumption is that this is why it's based
on CATextLayer. As of iOS 6, however, UILabel has the attributedString
property which allows us to set the font characteristics we need.

Consequently, OmniboxPopupMaterialViewController no longer needs to
create and cache CTFontRefs.

Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS
10.2, and iPad Air/iOS 10.2.

BUG=
==========

[Justin Donnelly, 2017-02-16 18:28:18]

jdonnelly@chromium.org changed reviewers:
+ justincohen@google.com, rohitrao@chromium.org

[Justin Donnelly, 2017-02-16 18:28:19]

This change makes OmniboxPopupTruncatingLabel a much closer copy of
GTMFadeTruncatingLabel. The only difference now is a couple conditional cases to
support UILabels with attributedString values. It's so close, in fact that I'm
currently trying to set up a Mac google3 environment to add this change to
GTMFadeTruncatingLabel.

In the meantime, can you take a look and let me know if this approach makes
sense and if you'd be open to landing this until I get the change to
GTMFadeTruncatingLabel done.

Aside from general simplification, the motivation for this is to switch answers
in suggest (weather, facts, stock quotes) to use plain UILabels*. If
OmniboxPopupTruncatingLabel is a descendant of UILabel then I can change the
type of the labels in OmniboxPopupMaterialRow to UILabel and just instantiate
the correct type based on whether it's an answer or a regular suggestion.

* The motivation for *this* is that answers generally don't need truncation
except in the case of dictionary definitions, which are usually multi-line and
per direction from UX, we'd prefer truncation with ellipses in this case.

[Justin Donnelly, 2017-02-16 19:09:15]

The CQ bit was checked by jdonnelly@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 19:10:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 19:24:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 19:24:34]

Dry run: This issue passed the CQ dry run.

[Justin Donnelly, 2017-02-16 21:22:29]

Ah, hang on. GTM is managed on github. I'm a bit less confident about making a
change there and getting it pulled over in a timely manner so please do let me
know what you think of making this change here.

Regardless, in the meantime I've filed
https://github.com/google/google-toolbox-for-mac/issues/133 and will follow-up
on the GTM mailing list.

[justincohen, 2017-02-20 19:56:28]

justincohen@chromium.org changed reviewers:
+ justincohen@chromium.org, pschaffner@google.com

[justincohen, 2017-02-20 19:56:28]

I didn't see a resolution on the email 'Ellipses in iOS' thread with
pschaffner@.  Does this depend on that?

[justincohen, 2017-02-20 20:07:25]

https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib...
File ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm (right):

https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:60: UIImage* image
= [self getLinearGradient:requestedRect];
Does it matter this isn't cached like it used to be?

https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:76:
[attributedString drawInRect:requestedRect];
Is lineBreakMode and textAlignment different in self.attributedText?   Why is
this copy necessary?

https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:97: [self.text
drawInRect:requestedRect withAttributes:attributes];
Why not just create an attributed string from text and hit the same path as
above?

[Justin Donnelly, 2017-02-21 17:25:14]

On 2017/02/20 19:56:28, justincohen wrote:
> I didn't see a resolution on the email 'Ellipses in iOS' thread with
> pschaffner@.  Does this depend on that?

There was a lot of subsequent discussion of what the long-term approach should
be but his initial response is still clear, I think:

"For eliding characters from a multiline label, like what you’ve described, you
should use an ellipsis. You are right, it does create inconsistency with our
single-line pattern, but that is OK (for now)."

[Justin Donnelly, 2017-02-21 18:11:26]

The CQ bit was checked by jdonnelly@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 18:12:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Justin Donnelly, 2017-02-21 18:16:50]

https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib...
File ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm (right):

https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:60: UIImage* image
= [self getLinearGradient:requestedRect];
On 2017/02/20 20:07:24, justincohen wrote:
> Does it matter this isn't cached like it used to be?

I don't think so. GTMFadeTruncatingLabel doesn't cache it nor does the omnibox
text field when it handles the clipping itself:

https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/omnibox/omnibox_te...

https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:76:
[attributedString drawInRect:requestedRect];
On 2017/02/20 20:07:24, justincohen wrote:
> Is lineBreakMode and textAlignment different in self.attributedText?

No, they behave the same. Maybe it would be better to just hardcode the line
break mode here to NSLineBreakByClipping (since the other ones probably don't
make sense with fading) and to have the client specify the alignment as part of
their attributed string. I was trying to stay consistent with the .text case and
how GTMFadeTruncatingLabel currently works, but I could go either way. Let me
know what you think.

> Why is this copy necessary?

The copy is necessary because self.attributedText is immutable.

https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:97: [self.text
drawInRect:requestedRect withAttributes:attributes];
On 2017/02/20 20:07:24, justincohen wrote:
> Why not just create an attributed string from text and hit the same path as
> above?

It seemed simpler and safer to me to just copy the code from
GTMFadeTruncatingLabel and match the existing behavior for self.text.

But on second thought, this case is dead code. The only use of this class is in
OmniboxPopupMaterialRow and OmniboxPopupMaterialViewController which use
attributedText. I've removed it. Now this class can just be the attributedText
version of GTMFadeTruncatingLabel until such time as I can get
GTMFadeTruncatingLabel to handle both cases.

[commit-bot: I haz the power, 2017-02-21 19:20:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 19:20:58]

Dry run: This issue passed the CQ dry run.

[justincohen, 2017-02-22 03:52:30]

https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib...
File ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm (right):

https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:60: UIImage* image
= [self getLinearGradient:requestedRect];
On 2017/02/21 18:16:50, Justin Donnelly wrote:
> On 2017/02/20 20:07:24, justincohen wrote:
> > Does it matter this isn't cached like it used to be?
> 
> I don't think so. GTMFadeTruncatingLabel doesn't cache it nor does the omnibox
> text field when it handles the clipping itself:
> 
>
https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/omnibox/omnibox_te...

I had to go back to 2012 ( https://b.corp.google.com/issues/5954680 ) to find
where we added this caching.  Do you think (four years later!) maybe
getLinearGradient is faster, or drawTextInRect is called less?

https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:76:
[attributedString drawInRect:requestedRect];
On 2017/02/21 18:16:50, Justin Donnelly wrote:
> On 2017/02/20 20:07:24, justincohen wrote:
> > Is lineBreakMode and textAlignment different in self.attributedText?
> 
> No, they behave the same. Maybe it would be better to just hardcode the line
> break mode here to NSLineBreakByClipping (since the other ones probably don't
> make sense with fading) and to have the client specify the alignment as part
of
> their attributed string. I was trying to stay consistent with the .text case
and
> how GTMFadeTruncatingLabel currently works, but I could go either way. Let me
> know what you think.
> 
> > Why is this copy necessary?
> 
> The copy is necessary because self.attributedText is immutable.

What's the default linebreakmode?  If the client specifies everything in their
attributed string is any of this necessary?

I don't feel strongly either way -- this is fine too.

[Justin Donnelly, 2017-02-22 17:13:25]

The CQ bit was checked by jdonnelly@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 17:14:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Justin Donnelly, 2017-02-22 17:17:42]

https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib...
File ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm (right):

https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:60: UIImage* image
= [self getLinearGradient:requestedRect];
On 2017/02/22 03:52:29, justincohen wrote:
> On 2017/02/21 18:16:50, Justin Donnelly wrote:
> > On 2017/02/20 20:07:24, justincohen wrote:
> > > Does it matter this isn't cached like it used to be?
> > 
> > I don't think so. GTMFadeTruncatingLabel doesn't cache it nor does the
omnibox
> > text field when it handles the clipping itself:
> > 
> >
>
https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/omnibox/omnibox_te...
> 
> I had to go back to 2012 ( https://b.corp.google.com/issues/5954680 ) to find
> where we added this caching.  Do you think (four years later!) maybe
> getLinearGradient is faster, or drawTextInRect is called less?

I don't honestly know. Without the caching, there's no jank in the simulator but
I don't have an old device to test it on to see if there's a performance
regression. I've restored the caching in the latest patch set, lmkwyt.

(The latest patch set also restores the handling of text alignment which I
mistakenly removed. And I actually tested this in an RTL language this time to
verify it's working correctly.)

https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:76:
[attributedString drawInRect:requestedRect];
On 2017/02/22 03:52:29, justincohen wrote:
> On 2017/02/21 18:16:50, Justin Donnelly wrote:
> > On 2017/02/20 20:07:24, justincohen wrote:
> > > Is lineBreakMode and textAlignment different in self.attributedText?
> > 
> > No, they behave the same. Maybe it would be better to just hardcode the line
> > break mode here to NSLineBreakByClipping (since the other ones probably
don't
> > make sense with fading) and to have the client specify the alignment as part
> of
> > their attributed string. I was trying to stay consistent with the .text case
> and
> > how GTMFadeTruncatingLabel currently works, but I could go either way. Let
me
> > know what you think.
> > 
> > > Why is this copy necessary?
> > 
> > The copy is necessary because self.attributedText is immutable.
> 
> What's the default linebreakmode?  If the client specifies everything in their
> attributed string is any of this necessary?
> 
> I don't feel strongly either way -- this is fine too.

The default is NSLineBreakByTruncatingTail (trailing ellipsis). Yes, we could
have the client specify these properties instead. But if you don't feel
strongly, I'd prefer to keep it this way to make it easier to switch to
GTMFadeTruncatingLabel in the future.

[commit-bot: I haz the power, 2017-02-22 17:27:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 17:27:57]

Dry run: This issue passed the CQ dry run.

[justincohen, 2017-02-22 19:10:22]

lgtm!

[Justin Donnelly1, 2017-02-22 19:48:09]

The CQ bit was checked by jdonnelly@google.com

[commit-bot: I haz the power, 2017-02-22 19:49:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 19:59:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 19:59:27]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Justin Donnelly1, 2017-02-22 20:54:56]

rohitrao: would you mind giving this an OWNERS review when you get a chance?

[rohitrao (ping after 24h), 2017-02-23 21:19:35]

lgtm

[Justin Donnelly, 2017-02-23 21:24:44]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2017-02-23 21:25:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 21:38:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487885084458600,
"parent_rev": "d31fa8e715fc2baa87357e62a106f5fce6f2d4cc", "commit_rev":
"af40f7a6563089a73b4a18e1d9abac7cbff3b88f"}

[commit-bot: I haz the power, 2017-02-23 21:39:07]

Description was changed from

==========
Change OmniboxPopupTruncatingLabel to be based on UILabel.

When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not
support attributed strings. My assumption is that this is why it's based
on CATextLayer. As of iOS 6, however, UILabel has the attributedString
property which allows us to set the font characteristics we need.

Consequently, OmniboxPopupMaterialViewController no longer needs to
create and cache CTFontRefs.

Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS
10.2, and iPad Air/iOS 10.2.

BUG=
==========

to

==========
Change OmniboxPopupTruncatingLabel to be based on UILabel.

When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not
support attributed strings. My assumption is that this is why it's based
on CATextLayer. As of iOS 6, however, UILabel has the attributedString
property which allows us to set the font characteristics we need.

Consequently, OmniboxPopupMaterialViewController no longer needs to
create and cache CTFontRefs.

Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS
10.2, and iPad Air/iOS 10.2.

BUG=

Review-Url: https://codereview.chromium.org/2695413003
Cr-Commit-Position: refs/heads/master@{#452638}
Committed:
https://chromium.googlesource.com/chromium/src/+/af40f7a6563089a73b4a18e1d9ab...
==========

[commit-bot: I haz the power, 2017-02-23 21:39:08]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/af40f7a6563089a73b4a18e1d9ab...