[Mac] Hover/Pressed background for the Omnibox decorations

[Mac] Hover/Pressed background for the Omnibox decorations

- Added tracking areas for mouse interactive decorations
- Adjusted "popup blocker" bubble so that it has the right spacing for the background

BUG=588377, 662186
Committed: https://crrev.com/2edc6f9145d91684f2151462fa20873c29b87b62
Cr-Commit-Position: refs/heads/master@{#431908}

[spqchan, 2016-11-04 23:05:32]

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

[commit-bot: I haz the power, 2016-11-04 23:06:34]

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

[spqchan, 2016-11-04 23:08:10]

Description was changed from

==========
[Mac] Hover decorations

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

to

==========
[Mac] Hover/Pressed background for the Omnibox decorations

- Added tracking areas for mouse interactive decorations
- Adjusted "popup blocker" bubble so that it has the right spacing for the
background

BUG=588377, 662186
==========

[spqchan, 2016-11-04 23:08:10]

spqchan@chromium.org changed reviewers:
+ shrike@chromium.org

[commit-bot: I haz the power, 2016-11-04 23:29:52]

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

[commit-bot: I haz the power, 2016-11-04 23:29:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[spqchan, 2016-11-04 23:40:47]

Patchset #5 (id:80001) has been deleted

[spqchan, 2016-11-04 23:40:52]

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

[commit-bot: I haz the power, 2016-11-04 23:41:38]

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, 2016-11-05 00:03:32]

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

[commit-bot: I haz the power, 2016-11-05 00:03:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[spqchan, 2016-11-05 01:14:46]

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

[commit-bot: I haz the power, 2016-11-05 01:15:09]

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, 2016-11-05 02:04:49]

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

[commit-bot: I haz the power, 2016-11-05 02:04:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[spqchan, 2016-11-08 02:39:31]

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

[commit-bot: I haz the power, 2016-11-08 02:39: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, 2016-11-08 02:58:32]

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

[commit-bot: I haz the power, 2016-11-08 02:58:34]

Dry run: This issue passed the CQ dry run.

[spqchan, 2016-11-08 03:00:30]

PTAL

[spqchan, 2016-11-10 17:15:18]

On 2016/11/08 03:00:30, spqchan wrote:
> PTAL

ping, thanks!

[shrike, 2016-11-10 17:32:39]

On 2016/11/10 17:15:18, spqchan wrote:
> On 2016/11/08 03:00:30, spqchan wrote:
> > PTAL
> 
> ping, thanks!

Thank you for the ping, and sorry for the delay - didn't get to it Tuesday and
lost a day yesterday. I have to sync to ToT (doing that now) and then will check
it out.

[spqchan, 2016-11-10 18:47:20]

On 2016/11/10 17:32:39, shrike wrote:
> On 2016/11/10 17:15:18, spqchan wrote:
> > On 2016/11/08 03:00:30, spqchan wrote:
> > > PTAL
> > 
> > ping, thanks!
> 
> Thank you for the ping, and sorry for the delay - didn't get to it Tuesday and
> lost a day yesterday. I have to sync to ToT (doing that now) and then will
check
> it out.

No worries, take your time. Thanks for the update!

[shrike, 2016-11-10 19:59:21]

On 2016/11/10 18:47:20, spqchan wrote:
> No worries, take your time. Thanks for the update!

Finally got synced up and rebuilt. It's looking good - thank you for the hard
work on this!

For the hover background can you do a fade in rather than a hard change? I think
the MD spec in general calls for fade ins.

With the popup blocked, when the text slides out it looks like the right edge of
the text is clipped just to the right of the vertical line, which looks odd.
Ideally the text would be clipped just to the left of that line so that it looks
like it's sliding into the line. Perhaps that has to do with the text sliding to
the right edge of the hover background, which ends 1pt to the right of the
vertical line?

[spqchan, 2016-11-10 20:03:02]

On 2016/11/10 19:59:21, shrike wrote:
> On 2016/11/10 18:47:20, spqchan wrote:
> > No worries, take your time. Thanks for the update!
> 
> Finally got synced up and rebuilt. It's looking good - thank you for the hard
> work on this!
> 
> For the hover background can you do a fade in rather than a hard change? I
think
> the MD spec in general calls for fade ins.
> 
> With the popup blocked, when the text slides out it looks like the right edge
of
> the text is clipped just to the right of the vertical line, which looks odd.
> Ideally the text would be clipped just to the left of that line so that it
looks
> like it's sliding into the line. Perhaps that has to do with the text sliding
to
> the right edge of the hover background, which ends 1pt to the right of the
> vertical line?

Fade in will require much more work, and it's something I'm working on for the
rest of the toolbar (although it's sitting in the back burner right now)
Hmm, I'll look into the popup decoration and will see what I can do.

[spqchan, 2016-11-11 01:14:16]

On 2016/11/10 20:03:02, spqchan wrote:
> On 2016/11/10 19:59:21, shrike wrote:
> > On 2016/11/10 18:47:20, spqchan wrote:
> > > No worries, take your time. Thanks for the update!
> > 
> > Finally got synced up and rebuilt. It's looking good - thank you for the
hard
> > work on this!
> > 
> > For the hover background can you do a fade in rather than a hard change? I
> think
> > the MD spec in general calls for fade ins.
> > 
> > With the popup blocked, when the text slides out it looks like the right
edge
> of
> > the text is clipped just to the right of the vertical line, which looks odd.
> > Ideally the text would be clipped just to the left of that line so that it
> looks
> > like it's sliding into the line. Perhaps that has to do with the text
sliding
> to
> > the right edge of the hover background, which ends 1pt to the right of the
> > vertical line?
> 
> Fade in will require much more work, and it's something I'm working on for the
> rest of the toolbar (although it's sitting in the back burner right now)
> Hmm, I'll look into the popup decoration and will see what I can do.

[spqchan, 2016-11-11 01:15:10]

On 2016/11/11 01:14:16, spqchan wrote:
> On 2016/11/10 20:03:02, spqchan wrote:
> > On 2016/11/10 19:59:21, shrike wrote:
> > > On 2016/11/10 18:47:20, spqchan wrote:
> > > > No worries, take your time. Thanks for the update!
> > > 
> > > Finally got synced up and rebuilt. It's looking good - thank you for the
> hard
> > > work on this!
> > > 
> > > For the hover background can you do a fade in rather than a hard change? I
> > think
> > > the MD spec in general calls for fade ins.
> > > 
> > > With the popup blocked, when the text slides out it looks like the right
> edge
> > of
> > > the text is clipped just to the right of the vertical line, which looks
odd.
> > > Ideally the text would be clipped just to the left of that line so that it
> > looks
> > > like it's sliding into the line. Perhaps that has to do with the text
> sliding
> > to
> > > the right edge of the hover background, which ends 1pt to the right of the
> > > vertical line?
> > 
> > Fade in will require much more work, and it's something I'm working on for
the
> > rest of the toolbar (although it's sitting in the back burner right now)
> > Hmm, I'll look into the popup decoration and will see what I can do.

Alrighty, I added some padding so that the text is now properly clipped. WDYT?

[spqchan, 2016-11-11 01:15:16]

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

[commit-bot: I haz the power, 2016-11-11 01:15:47]

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, 2016-11-11 01:43:47]

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

[commit-bot: I haz the power, 2016-11-11 01:43:48]

Dry run: This issue passed the CQ dry run.

[shrike, 2016-11-11 17:43:09]

On 2016/11/11 01:15:10, spqchan wrote:
> Alrighty, I added some padding so that the text is now properly clipped. WDYT?

Thank you - looks good!

[spqchan, 2016-11-11 17:44:06]

spqchan@chromium.org changed reviewers:
+ rsesek@chromium.org

[spqchan, 2016-11-11 17:44:07]

Thanks!

+rsesek

[Robert Sesek, 2016-11-11 19:35:07]

https://codereview.chromium.org/2478673002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.h
(right):

https://codereview.chromium.org/2478673002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.h:13: @class
CrTrackingArea;
Unused in this file?

https://codereview.chromium.org/2478673002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.h:137: -
(std::vector<LocationBarDecoration*>)mouseTrackingDecorations;
Return as const& ?

https://codereview.chromium.org/2478673002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/2478673002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:187: for
(size_t i = 0; i < mouseTrackingDecorations_.size(); i++)
for (auto& decoration : mouseTrackingDecorations_) ?

https://codereview.chromium.org/2478673002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm
(right):

https://codereview.chromium.org/2478673002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:807:
static_cast<AutocompleteTextFieldCell*>([field_ cell]);
Is this cast needed? Line 837 doesn't do it.

[spqchan, 2016-11-11 23:25:46]

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

[commit-bot: I haz the power, 2016-11-11 23:26:33]

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, 2016-11-12 00:35:58]

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

[commit-bot: I haz the power, 2016-11-12 00:35:59]

Dry run: This issue passed the CQ dry run.

[spqchan, 2016-11-12 00:36:20]

PTAL

https://codereview.chromium.org/2478673002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.h
(right):

https://codereview.chromium.org/2478673002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.h:13: @class
CrTrackingArea;
On 2016/11/11 19:35:07, Robert Sesek wrote:
> Unused in this file?

Done.

https://codereview.chromium.org/2478673002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.h:137: -
(std::vector<LocationBarDecoration*>)mouseTrackingDecorations;
On 2016/11/11 19:35:07, Robert Sesek wrote:
> Return as const& ?

Done.

https://codereview.chromium.org/2478673002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/2478673002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:187: for
(size_t i = 0; i < mouseTrackingDecorations_.size(); i++)
On 2016/11/11 19:35:07, Robert Sesek wrote:
> for (auto& decoration : mouseTrackingDecorations_) ?

Done. TIL :D

https://codereview.chromium.org/2478673002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm
(right):

https://codereview.chromium.org/2478673002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:807:
static_cast<AutocompleteTextFieldCell*>([field_ cell]);
On 2016/11/11 19:35:07, Robert Sesek wrote:
> Is this cast needed? Line 837 doesn't do it.

Done.

[Robert Sesek, 2016-11-14 18:56:17]

LGTM

[spqchan, 2016-11-14 18:56:55]

On 2016/11/14 18:56:17, Robert Sesek wrote:
> LGTM

thanks!

[spqchan, 2016-11-14 18:57:01]

The CQ bit was checked by spqchan@chromium.org

[commit-bot: I haz the power, 2016-11-14 18:58:08]

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, 2016-11-14 19:48:21]

Description was changed from

==========
[Mac] Hover/Pressed background for the Omnibox decorations

- Added tracking areas for mouse interactive decorations
- Adjusted "popup blocker" bubble so that it has the right spacing for the
background

BUG=588377, 662186
==========

to

==========
[Mac] Hover/Pressed background for the Omnibox decorations

- Added tracking areas for mouse interactive decorations
- Adjusted "popup blocker" bubble so that it has the right spacing for the
background

BUG=588377, 662186
==========

[commit-bot: I haz the power, 2016-11-14 19:48:22]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-11-14 20:10:19]

Description was changed from

==========
[Mac] Hover/Pressed background for the Omnibox decorations

- Added tracking areas for mouse interactive decorations
- Adjusted "popup blocker" bubble so that it has the right spacing for the
background

BUG=588377, 662186
==========

to

==========
[Mac] Hover/Pressed background for the Omnibox decorations

- Added tracking areas for mouse interactive decorations
- Adjusted "popup blocker" bubble so that it has the right spacing for the
background

BUG=588377, 662186

Committed: https://crrev.com/2edc6f9145d91684f2151462fa20873c29b87b62
Cr-Commit-Position: refs/heads/master@{#431908}
==========

[commit-bot: I haz the power, 2016-11-14 20:10:20]

Patchset 11 (id:??) landed as
https://crrev.com/2edc6f9145d91684f2151462fa20873c29b87b62
Cr-Commit-Position: refs/heads/master@{#431908}