OmniboxPopupViewMac refactoring Part 3 (truncation)

OmniboxPopupViewMac refactoring Part 3 (truncation)

This CL changes how omnibox popup results are truncated. Previously content
text would be truncated at 70% which might leave empty space if the description
was short. I've made the following changes to the truncation code:
  - move the truncation logic to the button cell
  - have the content text fill up unused space when truncating
  - use a fade effect to truncate instead of ellipses

BUG=9977, 252988
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217109

[Scott Hess - ex-Googler, 2013-06-25 23:46:09]

I support refactoring, but please break it into "Refactor to move stuff around"
CL, and "I'm going to change things" CL.  Presumably in that order.  That way I
can quickly dispense with the one, and we can spend argument time on only
relevant changes rather than potentially arguing about both in a free-for-all.

Try not to specialize the popup matrix.  I don't really care for the C++
delegate on an objc class, but I can see why it's there.  But that class should
delegate policy to the overall controller.  So OnMatrixRowSelected() makes
sense, but OnMatrixOpenURLForRow() seems way too specific.  I'm also not
entirely certain if using the original event for disposition is right, given
that the modifiers may have changed since then.

[Scott Hess - ex-Googler, 2013-06-25 23:47:38]

On 2013/06/25 23:46:09, shess wrote:
> I support refactoring, but please break it into "Refactor to move stuff
around"
> CL, and "I'm going to change things" CL.  Presumably in that order.  That way
I
> can quickly dispense with the one, and we can spend argument time on only
> relevant changes rather than potentially arguing about both in a free-for-all.
> 
> Try not to specialize the popup matrix.  I don't really care for the C++
> delegate on an objc class, but I can see why it's there.  But that class
should
> delegate policy to the overall controller.  So OnMatrixRowSelected() makes
> sense, but OnMatrixOpenURLForRow() seems way too specific.  I'm also not
> entirely certain if using the original event for disposition is right, given
> that the modifiers may have changed since then.

BTW, I'm perfectly fine with not having the move-things-around CL be complete
with new tests and the like.  Not asking you to waste time, mostly I just want
to not spend time scanning code manually to see what changed.

[sail, 2013-06-26 03:23:02]

Ok, moved the simple refactoring into two separate CLs (crrev.com/17786002 and
crrev.com/17787002). This CL only contains the truncation changes.

[Scott Hess - ex-Googler, 2013-06-28 01:02:48]

Sorry, totally missed this earlier.  I need to go to supper, so you get the
comments as-is.

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right):

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:33: const CGFloat
kOverflowBeforeClip = 2;
2.0

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:38: CGFloat gradientWidth
= MIN(size.height * 2, NSWidth(titleRect) / 4);
std::min().

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:46: NSMinY(titleRect) +
roundf((NSHeight(titleRect) - size.height) / 2.0) - 1;
Sigh.  No std::round until c++11.

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:57: if (!clipping)
Couldn't this result in incorrect drawing if the text reaches to w/in
gradientWidth of NSMaxX(titleRect), but is not clipped?  Perhaps an else
-clipRect:titleRect above in that case?

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:61: // suboptimal, but
since it fades out, that's ok.
This seems complicated.  Something like this using NSCompositeSourceOver seems
easier:
   http://lists.apple.com/archives/cocoa-dev/2009/Mar/msg00797.html
it would involve a different approach - draw the text, then blend over from
transparent to opaque.

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:82: [mask release];
Why not scoped_ptr<>?  Does the mask require specific lifetime?

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:158: descriptionWidth =
(1.0 - kMaxContentsFraction) * availableWidth;
std::min?

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:164: }
I think this entire if() can be replaced with something like:
  tempDescWidth = std::min(descriptionWidth, (1.0 - kMax) * availableWidth);
  contentWidth = std::min(contentWidth, availableWidth - tempDescWidth);
  descriptionWidth = std::min(descriptionWidth, availableWidth - contentWidth);
I'm not sure it's less confusing, though a better name for tempDescWidth might
help.  I'm also not sure it's more confusing :-).

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:171: NSRect
descriptionRect = cellFrame;
if ([descriptionText_ length]){...} around this?

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm (right):

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:135:
withString:rawEnDash];
I almost want to have the spc en-dash spc sequence pushed down into the cell
drawing code, rather than doing this.

[sail, 2013-06-28 18:41:15]

Code is much simpler now. Please take a look.

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right):

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:33: const CGFloat
kOverflowBeforeClip = 2;
On 2013/06/28 01:02:48, shess wrote:
> 2.0

Done.

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:38: CGFloat gradientWidth
= MIN(size.height * 2, NSWidth(titleRect) / 4);
On 2013/06/28 01:02:48, shess wrote:
> std::min().

Done.

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:57: if (!clipping)
On 2013/06/28 01:02:48, shess wrote:
> Couldn't this result in incorrect drawing if the text reaches to w/in
> gradientWidth of NSMaxX(titleRect), but is not clipped?  Perhaps an else
> -clipRect:titleRect above in that case?

Done.
This part is not relevant anymore now that I removed the transparency layer.

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:61: // suboptimal, but
since it fades out, that's ok.
On 2013/06/28 01:02:48, shess wrote:
> This seems complicated.  Something like this using NSCompositeSourceOver seems
> easier:
>    http://lists.apple.com/archives/cocoa-dev/2009/Mar/msg00797.html
> it would involve a different approach - draw the text, then blend over from
> transparent to opaque.

Done.
Yep, I just draw the full string and then draw the gradient from 0 alpha to 1.0
alpha.

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:82: [mask release];
On 2013/06/28 01:02:48, shess wrote:
> Why not scoped_ptr<>?  Does the mask require specific lifetime?

Done.

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:158: descriptionWidth =
(1.0 - kMaxContentsFraction) * availableWidth;
On 2013/06/28 01:02:48, shess wrote:
> std::min?

Done.

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:164: }
On 2013/06/28 01:02:48, shess wrote:
> I think this entire if() can be replaced with something like:
>   tempDescWidth = std::min(descriptionWidth, (1.0 - kMax) * availableWidth);
>   contentWidth = std::min(contentWidth, availableWidth - tempDescWidth);
>   descriptionWidth = std::min(descriptionWidth, availableWidth -
contentWidth);
> I'm not sure it's less confusing, though a better name for tempDescWidth might
> help.  I'm also not sure it's more confusing :-).

Done.

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:171: NSRect
descriptionRect = cellFrame;
On 2013/06/28 01:02:48, shess wrote:
> if ([descriptionText_ length]){...} around this?

Done.

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm (right):

https://codereview.chromium.org/17774002/diff/17001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:135:
withString:rawEnDash];
On 2013/06/28 01:02:48, shess wrote:
> I almost want to have the spc en-dash spc sequence pushed down into the cell
> drawing code, rather than doing this.

Done.

[Scott Hess - ex-Googler, 2013-06-28 20:11:34]

https://codereview.chromium.org/17774002/diff/23001/chrome/browser/ui/cocoa/o...
File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right):

https://codereview.chromium.org/17774002/diff/23001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:78: -
(NSAttributedString*)contentText {
Nobody even ever calls the getters.  I'd be fine with removing them entirely,
but if you're going to keep them add a unit test to check that they round-trip
appropriately.

https://codereview.chromium.org/17774002/diff/23001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:129: CGFloat
descriptionWidth = 0;
I think you should feel free to use abbreviations if descWidth and availWidth
(or totalWidth) would allow things for format more nicely later on.

https://codereview.chromium.org/17774002/diff/23001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:138: }
That's not really what I meant, sorry.  I meant drawing the en-dash separately
from the description, so that the description is just left alone rather than
having arbitrary junk stuffed into it.  Mostly it was about spinning off
unnecessary autorelease values in drawing code.  [That would also imply using
string-drawing with a constant string and the description's leading attributes
rather than creating an autorelease attributed string off the constant string.]

Also, pull back a descriptive variable name for @" \u2013 ".  I don't have a
unicode chart in my head.

Also - feel free to push back on me about pushing the en-dash into the front of
the description text.  You could factor it in/out automatically in the
setter/getter rather than in the drawing code.

https://codereview.chromium.org/17774002/diff/23001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:141: descriptionWidth,
(1.0f - kMaxContentsFraction) * availableWidth);
Now that there is only one ref to this, and it's 1.0 - kMax, maybe it should be
kMinDescriptionFraction.

https://codereview.chromium.org/17774002/diff/23001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:149:
contentRect.size.width = contentWidth;
contentRect and descRect could be generated by NSDivideRect() with cellFrame and
contentWidth.

[Scott Hess - ex-Googler, 2013-06-28 22:53:05]

sailesh, I apologize, I'm going OOT tomorrow and now I'm worried we won't be
done on this.  So feel free to get some other more compliant reviewer involved
if it doesn't tie up this evening.

[sail, 2013-06-28 22:58:16]

On 2013/06/28 22:53:05, shess wrote:
> sailesh, I apologize, I'm going OOT tomorrow and now I'm worried we won't be
> done on this.  So feel free to get some other more compliant reviewer involved
> if it doesn't tie up this evening.

Hey Scott, there's no rush. I'm stuck in core animation land for the next few
days anyways.

[sail, 2013-06-30 18:49:11]

https://codereview.chromium.org/17774002/diff/23001/chrome/browser/ui/cocoa/o...
File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right):

https://codereview.chromium.org/17774002/diff/23001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:78: -
(NSAttributedString*)contentText {
On 2013/06/28 20:11:34, shess wrote:
> Nobody even ever calls the getters.  I'd be fine with removing them entirely,
> but if you're going to keep them add a unit test to check that they round-trip
> appropriately.

Done.

https://codereview.chromium.org/17774002/diff/23001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:129: CGFloat
descriptionWidth = 0;
On 2013/06/28 20:11:34, shess wrote:
> I think you should feel free to use abbreviations if descWidth and availWidth
> (or totalWidth) would allow things for format more nicely later on.

Done.

https://codereview.chromium.org/17774002/diff/23001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:138: }
On 2013/06/28 20:11:34, shess wrote:
> That's not really what I meant, sorry.  I meant drawing the en-dash separately
> from the description, so that the description is just left alone rather than
> having arbitrary junk stuffed into it.  Mostly it was about spinning off
> unnecessary autorelease values in drawing code.  [That would also imply using
> string-drawing with a constant string and the description's leading attributes
> rather than creating an autorelease attributed string off the constant
string.]
> 
> Also, pull back a descriptive variable name for @" \u2013 ".  I don't have a
> unicode chart in my head.
> 
> Also - feel free to push back on me about pushing the en-dash into the front
of
> the description text.  You could factor it in/out automatically in the
> setter/getter rather than in the drawing code.

Done.

https://codereview.chromium.org/17774002/diff/23001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:141: descriptionWidth,
(1.0f - kMaxContentsFraction) * availableWidth);
On 2013/06/28 20:11:34, shess wrote:
> Now that there is only one ref to this, and it's 1.0 - kMax, maybe it should
be
> kMinDescriptionFraction.

Done.

I changed the setter to prepend raw_en_dash to the description.

Drawing the dash separately was a lot more work because I needed to extract the
attribute information from the description anyways.

https://codereview.chromium.org/17774002/diff/23001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:149:
contentRect.size.width = contentWidth;
On 2013/06/28 20:11:34, shess wrote:
> contentRect and descRect could be generated by NSDivideRect() with cellFrame
and
> contentWidth.

Done.

[Scott Hess - ex-Googler, 2013-07-08 20:53:11]

LGTM.  Just a couple nits.

https://codereview.chromium.org/17774002/diff/30001/chrome/browser/ui/cocoa/o...
File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right):

https://codereview.chromium.org/17774002/diff/30001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:44: if (!clipping)
You don't use |clipping| again, but the initialization of |clipping| isn't super
clean to parse.  What about:
  CGFloat clipped_width = NSWidth(titleRect) + kOverflowBeforeClip;
  if (std::floor(textSize.width) <= clipped_width)
    return;

Though ... why the floor()?  Clipped is clipped, even if it's the anti-aliased
bit being clipped.

https://codereview.chromium.org/17774002/diff/30001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:86: NSString* raw_en_dash
= @" \u2013 ";
Should this be rawEnDash now that this isn't embedded in a C++ implementation
file?

[sail, 2013-08-12 17:39:30]

https://codereview.chromium.org/17774002/diff/30001/chrome/browser/ui/cocoa/o...
File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right):

https://codereview.chromium.org/17774002/diff/30001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:44: if (!clipping)
On 2013/07/08 20:53:11, shess wrote:
> You don't use |clipping| again, but the initialization of |clipping| isn't
super
> clean to parse.  What about:
>   CGFloat clipped_width = NSWidth(titleRect) + kOverflowBeforeClip;
>   if (std::floor(textSize.width) <= clipped_width)
>     return;
> 
> Though ... why the floor()?  Clipped is clipped, even if it's the anti-aliased
> bit being clipped.

Done.
Removed the floor.

https://codereview.chromium.org/17774002/diff/30001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:86: NSString* raw_en_dash
= @" \u2013 ";
On 2013/07/08 20:53:11, shess wrote:
> Should this be rawEnDash now that this isn't embedded in a C++ implementation
> file?

Done.

https://codereview.chromium.org/17774002/diff/35001/chrome/browser/ui/cocoa/o...
File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right):

https://codereview.chromium.org/17774002/diff/35001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:46: // The gradient width
is the same as the line height.
Changed this from 2*line_height to 1*line_height. The 2x was too much for the
popup.

[commit-bot: I haz the power, 2013-08-12 17:39:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/17774002/35001

[commit-bot: I haz the power, 2013-08-12 23:05:32]

Change committed as 217109