[Material][Mac] Implement Omnibox Verbose State Chips

[Material][Mac] Implement Omnibox Verbose State Chips

Replaced EVBubbleDecoration with VerboseStateDecoration.
VerboseStateDecoration displays both the security state and the
EV Cert by animating in and out.

Animations only occur when the security level has changed or if the
omnibox becomes narrow or wide.

Verbose states animates in when the security level changes to
HTTPS or an invalid HTTPS. It animates outs if the security level
changes to something else.

If there's not enough room on the omnibox, the security state should
not appear, and will animate out if it's already displayed. It will
animate back in if the user resize the window so that there's enough
room.

BUG=622529
Committed: https://crrev.com/0afe4deba4f93d82430e90d789ab39e3c788c891
Cr-Commit-Position: refs/heads/master@{#416119}

[spqchan, 2016-07-02 01:14:21]

Description was changed from

==========
[Material][Mac] Implement Omnibox Verbose State Chips
==========

to

==========
[Material][Mac] Implement Omnibox Verbose State Chips

- Modified EVBubbleDecoration into SecureVerboseBubbleDecoration
 and refactored some of the code in location_bar_view_mac.
- Added animations to SecureVerboseBubbleDecoration

BUG=622529
==========

[spqchan, 2016-07-02 01:14:22]

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

[spqchan, 2016-07-02 01:14:41]

Description was changed from

==========
[Material][Mac] Implement Omnibox Verbose State Chips

- Modified EVBubbleDecoration into SecureVerboseBubbleDecoration
 and refactored some of the code in location_bar_view_mac.
- Added animations to SecureVerboseBubbleDecoration

BUG=622529
==========

to

==========
[Material][Mac] Implement Omnibox Verbose State Chips

- Renamed EVBubbleDecoration into SecureVerboseBubbleDecoration
 and refactored some of the code in location_bar_view_mac.
- Added animations to SecureVerboseBubbleDecoration

BUG=622529
==========

[spqchan, 2016-07-06 17:15:25]

PTAL

[shrike, 2016-07-08 22:03:07]

Hello spqchan@,

This code is not compiling for me? I get an error about ev_bubble_decoration_
being missing in location_bar_view_mac.mm.

[upolat, 2016-07-08 22:17:59]

https://codereview.chromium.org/2119033002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/2119033002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:992: // The first
run bubble's left edge should line up with the left eƒdge of the
typo: edge?

[spqchan, 2016-07-11 17:57:09]

Whoops sorry, I forgot to rebase it so there was a conflict with the new
changes. I rebased and fixed it so it should work now

https://codereview.chromium.org/2119033002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/2119033002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:992: // The first
run bubble's left edge should line up with the left eƒdge of the
On 2016/07/08 22:17:59, upolat wrote:
> typo: edge?

Done.

[shrike, 2016-07-13 00:26:36]

On 2016/07/11 17:57:09, spqchan wrote:
> Whoops sorry, I forgot to rebase it so there was a conflict with the new
> changes. I rebased and fixed it so it should work now

This is looking really good!

I see that the animate-in duration you're using is 500ms while the spec calls
for 330ms - did they change the animation speed somewhere?

These changes are just for the animate in state, correct? There's more coming
for animate out (which hides the verbose state and scheme), and also to hide the
verbose state while navigating within a site?

[spqchan, 2016-07-15 22:47:32]

On 2016/07/13 00:26:36, shrike wrote:
> On 2016/07/11 17:57:09, spqchan wrote:
> > Whoops sorry, I forgot to rebase it so there was a conflict with the new
> > changes. I rebased and fixed it so it should work now
> 
> This is looking really good!
> 
> I see that the animate-in duration you're using is 500ms while the spec calls
> for 330ms - did they change the animation speed somewhere?
> 
> These changes are just for the animate in state, correct? There's more coming
> for animate out (which hides the verbose state and scheme), and also to hide
the
> verbose state while navigating within a site?

Thanks for the catch, I was using 500 ms since it was easier to test with. Yes,
these changes are for “animate in state” only. 

Also, I noticed that there’s an issue with the animation being fired incorrectly
when you switch tabs. Firing the animation correctly might be tricky, so I think
I’ll just split it into just a CL with just the static verbose state, and then
follow up with a CL with the complete animation. Would it be alright if I submit
this CL with animation code in it? The animation is disabled atm

[spqchan, 2016-07-15 22:48:21]

Description was changed from

==========
[Material][Mac] Implement Omnibox Verbose State Chips

- Renamed EVBubbleDecoration into SecureVerboseBubbleDecoration
 and refactored some of the code in location_bar_view_mac.
- Added animations to SecureVerboseBubbleDecoration

BUG=622529
==========

to

==========
[Material][Mac] Implement Omnibox Verbose State Chips

- Implemented SecureVerboseBubbleDecoration and refactored some of the code in
location_bar_view_mac
- Added "Animate In" animations to SecureVerboseBubbleDecoration. The animations
are currently unused.

BUG=622529
==========

[spqchan, 2016-07-15 22:48:33]

Description was changed from

==========
[Material][Mac] Implement Omnibox Verbose State Chips

- Implemented SecureVerboseBubbleDecoration and refactored some of the code in
location_bar_view_mac
- Added "Animate In" animations to SecureVerboseBubbleDecoration. The animations
are currently unused.

BUG=622529
==========

to

==========
[Material][Mac] Implement Omnibox Verbose State Chips

- Implemented SecureVerboseBubbleDecoration and refactored some of
 the code in location_bar_view_mac
- Added "Animate In" animations to SecureVerboseBubbleDecoration.
The animations are currently unused.

BUG=622529
==========

[spqchan, 2016-07-20 00:33:33]

PTAL

I removed the animation code.

If you want to check out the animation, it's currently in progress here:
https://codereview.chromium.org/2152823004/

The animation is pretty much done, I just need to figure out how to trigger it
properly.

[spqchan, 2016-07-20 00:33:57]

Description was changed from

==========
[Material][Mac] Implement Omnibox Verbose State Chips

- Implemented SecureVerboseBubbleDecoration and refactored some of
 the code in location_bar_view_mac
- Added "Animate In" animations to SecureVerboseBubbleDecoration.
The animations are currently unused.

BUG=622529
==========

to

==========
[Material][Mac] Implement Omnibox Verbose State Chips

- Implemented SecureVerboseBubbleDecoration without animations and 
 refactored some of the code in location_bar_view_mac

BUG=622529
==========

[spqchan, 2016-07-22 00:14:39]

On 2016/07/20 00:33:33, spqchan wrote:
> PTAL
> 
> I removed the animation code.
> 
> If you want to check out the animation, it's currently in progress here:
> https://codereview.chromium.org/2152823004/
> 
> The animation is pretty much done, I just need to figure out how to trigger it
> properly.

Apologies, please hold this CL. I noticed a bug

[spqchan, 2016-08-12 00:43:46]

Description was changed from

==========
[Material][Mac] Implement Omnibox Verbose State Chips

- Implemented SecureVerboseBubbleDecoration without animations and 
 refactored some of the code in location_bar_view_mac

BUG=622529
==========

to

==========
[Material][Mac] Implement Omnibox Verbose State Chips

Replaced EVBubbleDecoration with VerboseStateDecoration.
VerboseStateDecoration displays both the security state and the
EV Cert by animating in and out.

If there's not enough room on the omnibox, the security state should
not appear.

BUG=622529
==========

[spqchan, 2016-08-12 00:46:34]

Okay, finally finished redoing this based on the new specs!

Since you're going to go to vacation, you probably won't have time to review
this. However, if you have time, can you take a look to see if the chip looks
alright? I'll send this review to someone else.

I should start writing tests for this too.

[shrike, 2016-08-12 00:48:35]

On 2016/08/12 00:46:34, spqchan wrote:
> Okay, finally finished redoing this based on the new specs!
> 
> Since you're going to go to vacation, you probably won't have time to review
> this. However, if you have time, can you take a look to see if the chip looks
> alright? I'll send this review to someone else.
> 
> I should start writing tests for this too.

I will try to look at this tomorrow (Friday).

[spqchan, 2016-08-12 00:55:26]

Description was changed from

==========
[Material][Mac] Implement Omnibox Verbose State Chips

Replaced EVBubbleDecoration with VerboseStateDecoration.
VerboseStateDecoration displays both the security state and the
EV Cert by animating in and out.

If there's not enough room on the omnibox, the security state should
not appear.

BUG=622529
==========

to

==========
[Material][Mac] Implement Omnibox Verbose State Chips

Replaced EVBubbleDecoration with VerboseStateDecoration.
VerboseStateDecoration displays both the security state and the
EV Cert by animating in and out.

Verbose states animates in when the security level changes to
HTTPS or an invalid HTTPS. It animates outs if the security level
changes to something else.

If there's not enough room on the omnibox, the security state should
not appear, and will animate out if it's already displayed.

BUG=622529
==========

[spqchan, 2016-08-12 21:12:54]

Description was changed from

==========
[Material][Mac] Implement Omnibox Verbose State Chips

Replaced EVBubbleDecoration with VerboseStateDecoration.
VerboseStateDecoration displays both the security state and the
EV Cert by animating in and out.

Verbose states animates in when the security level changes to
HTTPS or an invalid HTTPS. It animates outs if the security level
changes to something else.

If there's not enough room on the omnibox, the security state should
not appear, and will animate out if it's already displayed.

BUG=622529
==========

to

==========
[Material][Mac] Implement Omnibox Verbose State Chips

Replaced EVBubbleDecoration with VerboseStateDecoration.
VerboseStateDecoration displays both the security state and the
EV Cert by animating in and out.

Animations only occur when the security level has changed or if the
omnibox becomes narrow or wide.

Verbose states animates in when the security level changes to
HTTPS or an invalid HTTPS. It animates outs if the security level
changes to something else.

If there's not enough room on the omnibox, the security state should
not appear, and will animate out if it's already displayed. It will
animate back in if the user resize the window so that there's enough
room.

BUG=622529
==========

[shrike, 2016-08-12 23:58:24]

On 2016/08/12 00:46:34, spqchan wrote:
> Okay, finally finished redoing this based on the new specs!
> 
> Since you're going to go to vacation, you probably won't have time to review
> this. However, if you have time, can you take a look to see if the chip looks
> alright? I'll send this review to someone else.
> 
> I should start writing tests for this too.

Hello spqchan@,

This looks good. A couple questions:

1. When I create a new tab and go to apple.com, the security state animates into
view very slowly. If I go to someplace like badssl.com, and then click a bad
option, the Not Secure state animates in quickly. Is this still some testing
code in place? Or is the page load making the animation janky (because if I
close the apple.com tab and then choose File -> Reopen Closed Tab, the Secure
animates in quickly)?

2. If I navigate within the apple.com site the Secure state does not animate
into place (expected). However if I create a new tab and search for "ama", my
Google results page animates in Secure, but then if I click the first link which
takes me to Amazon, Amazon is also a secure site and the Secure does not animate
into place. It seems like even though both sites are secure that we would want
to animate Secure so that you know the new site you're going to is secure. (I'm
not sure what the spec dictates at this point - just pointing this out in case
it's an uncovered edge case.)

[spqchan, 2016-08-13 00:06:25]

On 2016/08/12 23:58:24, shrike wrote:
> On 2016/08/12 00:46:34, spqchan wrote:
> > Okay, finally finished redoing this based on the new specs!
> > 
> > Since you're going to go to vacation, you probably won't have time to review
> > this. However, if you have time, can you take a look to see if the chip
looks
> > alright? I'll send this review to someone else.
> > 
> > I should start writing tests for this too.
> 
> Hello spqchan@,
> 
> This looks good. A couple questions:
> 
> 1. When I create a new tab and go to http://apple.com, the security state
animates into
> view very slowly. If I go to someplace like http://badssl.com, and then click
a bad
> option, the Not Secure state animates in quickly. Is this still some testing
> code in place? Or is the page load making the animation janky (because if I
> close the http://apple.com tab and then choose File -> Reopen Closed Tab, the
Secure
> animates in quickly)?
> 
> 2. If I navigate within the http://apple.com site the Secure state does not
animate
> into place (expected). However if I create a new tab and search for "ama", my
> Google results page animates in Secure, but then if I click the first link
which
> takes me to Amazon, Amazon is also a secure site and the Secure does not
animate
> into place. It seems like even though both sites are secure that we would want
> to animate Secure so that you know the new site you're going to is secure.
(I'm
> not sure what the spec dictates at this point - just pointing this out in case
> it's an uncovered edge case.)

Thanks for having a look!

1) Hmm that's a good point. There's no testing code in place, it's most likely
the page load... 
I'm using a NSTimer for the animation, perhaps there's something in the
NSRunLoop that's slowing it down =/?

2) That's expected behavior, animation should only happens when the security
level has changed

[shrike, 2016-08-13 00:22:58]

On 2016/08/13 00:06:25, spqchan wrote:
> 1) Hmm that's a good point. There's no testing code in place, it's most likely
> the page load... 
> I'm using a NSTimer for the animation, perhaps there's something in the
> NSRunLoop that's slowing it down =/?

Yeah, that's probably it. I'm guessing that the timer is generating a set number
of frames, so if the frame rate falls the animation runs longer? In that case
you could try changing it so that the timer runs for a specific amount of time
and on each firing you look to see which frame you should be displaying at that
time. That will get the animation to always have the same run time, but it still
can be janky if enough frames are skipped between successive timer firings.

CoreAnimation seems like a solution that avoids the jank, but also I have to
believe we've solved this problem elsewhere in the Cocoa UI.

[spqchan, 2016-08-13 00:28:10]

On 2016/08/13 00:22:58, shrike wrote:
> On 2016/08/13 00:06:25, spqchan wrote:
> > 1) Hmm that's a good point. There's no testing code in place, it's most
likely
> > the page load... 
> > I'm using a NSTimer for the animation, perhaps there's something in the
> > NSRunLoop that's slowing it down =/?
> 
> Yeah, that's probably it. I'm guessing that the timer is generating a set
number
> of frames, so if the frame rate falls the animation runs longer? In that case
> you could try changing it so that the timer runs for a specific amount of time
> and on each firing you look to see which frame you should be displaying at
that
> time. That will get the animation to always have the same run time, but it
still
> can be janky if enough frames are skipped between successive timer firings.
> 
> CoreAnimation seems like a solution that avoids the jank, but also I have to
> believe we've solved this problem elsewhere in the Cocoa UI.

The timer is set to fire at each  1/60 interval. I'm not sure how I can look for
each frame.
Do you mean to get a starting time reference and each time the timer fires, I'll
calculate the frame based on the current time from the starting time reference?
That could work but I have a feeling that it'll still be janky. I'm thinking of
giving CADisplayLink a try, WDYT? 

CoreAnimation is also a possibility, but I'm not sure if we'll be able to
relayout the omnibox for each frame

[shrike, 2016-08-13 00:34:25]

On 2016/08/13 00:28:10, spqchan wrote:
> The timer is set to fire at each  1/60 interval. I'm not sure how I can look
for
> each frame.
> Do you mean to get a starting time reference and each time the timer fires,
I'll
> calculate the frame based on the current time from the starting time
reference?

Exactly.

> That could work but I have a feeling that it'll still be janky.

Exactly :-/

> I'm thinking of
> giving CADisplayLink a try, WDYT? 

I've never used/heard of that API but it seems like it might be the answer, and
that it might fit best with your current timer-based implementation.

> CoreAnimation is also a possibility, but I'm not sure if we'll be able to
> relayout the omnibox for each frame

No matter which way you go (CA or displaylink) you have to relayout the Omnibox
for the frame you're about to draw.

[spqchan, 2016-08-15 20:00:33]

On 2016/08/13 00:34:25, shrike wrote:
> On 2016/08/13 00:28:10, spqchan wrote:
> > The timer is set to fire at each  1/60 interval. I'm not sure how I can look
> for
> > each frame.
> > Do you mean to get a starting time reference and each time the timer fires,
> I'll
> > calculate the frame based on the current time from the starting time
> reference?
> 
> Exactly.
> 
> > That could work but I have a feeling that it'll still be janky.
> 
> Exactly :-/
> 
> > I'm thinking of
> > giving CADisplayLink a try, WDYT? 
> 
> I've never used/heard of that API but it seems like it might be the answer,
and
> that it might fit best with your current timer-based implementation.
> 
> > CoreAnimation is also a possibility, but I'm not sure if we'll be able to
> > relayout the omnibox for each frame
> 
> No matter which way you go (CA or displaylink) you have to relayout the
Omnibox
> for the frame you're about to draw.

I switched from frame-based to time-based animation and it looks like it works
well.

[spqchan, 2016-08-15 20:02:30]

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

[spqchan, 2016-08-15 20:02:31]

Hey Robert,

Do you have the time to review this? Unfortunately, Jayson is out this week.

The specs are detailed in this doc if you want to have an idea what this should
look like:
https://docs.google.com/document/d/1WpdSVOC4r97kghn2p66Cgn11X3UoJRQV2HbZRoJRg...


Thanks!

Sarah

[Robert Sesek, 2016-08-16 22:25:33]

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm
(right):

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:119:
&location_icon_decoration, NULL);
nullptr

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:617: // If the
security level has changed, check if the verbose needs to be
"check if the verbose" ... verbose what?

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:620:
security_state::SecurityStateModel::SecurityLevel current_security =
naming: new_security_level ? looking at |security_level_| seems like the
"current" state to me.

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:750: NSColor*
sRGBColor =
naming: use under_scores, and below

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.h
(right):

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.h:22: class
VerboseStateBubbleDecoration : public BubbleDecoration {
"VerboseState" is kind of a weird name IMO. It's really conveying the security
state, right? Is there another security state decoration that would confound
using the name SecurityStateBubbleDecoration?

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.h:24:
explicit VerboseStateBubbleDecoration(LocationIconDecoration* location_icon,
No explicit since it takes more than one param.

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm
(right):

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:43: //
http://crbug.com/49822
This bug was archived. Does it still apply?

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:98: -
(id)initWithOwner:(VerboseStateBubbleDecoration*)owner
instancetype preferred in new code

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:131:
[[NSRunLoop mainRunLoop] addTimer:timer_ forMode:NSRunLoopCommonModes];
Why do you need CommonModes here?

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:275:
CGFloat textOffset = NSMinX(decoration_frame);
naming: under_scores, and other locals in this method

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:316:
base::scoped_nsobject<NSAttributedString> str([[NSAttributedString alloc]
naming: |text|, given that you have text_rect and text_height, etc?

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration_unittest.mm
(right):

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration_unittest.mm:17:
VerboseStateBubbleDecorationTest() : icon_(NULL), decoration_(&icon_, NULL) {}
NULL -> nullptr

https://codereview.chromium.org/2119033002/diff/520001/ui/gfx/animation/tween.h
File ui/gfx/animation/tween.h (right):

https://codereview.chromium.org/2119033002/diff/520001/ui/gfx/animation/tween...
ui/gfx/animation/tween.h:34: FAST_OUT_SLOW_IN_EXPO,  // Variant of
FAST_OUT_SLOW_IN.
What's _EXPO mean? How does it vary?

[spqchan, 2016-08-17 00:41:05]

On 2016/08/16 22:25:33, Robert Sesek wrote:
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
> File
> chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm
> (right):
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
>
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:119:
> &location_icon_decoration, NULL);
> nullptr
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:617: // If the
> security level has changed, check if the verbose needs to be
> "check if the verbose" ... verbose what?
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:620:
> security_state::SecurityStateModel::SecurityLevel current_security =
> naming: new_security_level ? looking at |security_level_| seems like the
> "current" state to me.
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:750: NSColor*
> sRGBColor =
> naming: use under_scores, and below
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.h
> (right):
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.h:22:
class
> VerboseStateBubbleDecoration : public BubbleDecoration {
> "VerboseState" is kind of a weird name IMO. It's really conveying the security
> state, right? Is there another security state decoration that would confound
> using the name SecurityStateBubbleDecoration?
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.h:24:
> explicit VerboseStateBubbleDecoration(LocationIconDecoration* location_icon,
> No explicit since it takes more than one param.
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm
> (right):
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:43: //
> http://crbug.com/49822
> This bug was archived. Does it still apply?
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:98: -
> (id)initWithOwner:(VerboseStateBubbleDecoration*)owner
> instancetype preferred in new code
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:131:
> [[NSRunLoop mainRunLoop] addTimer:timer_ forMode:NSRunLoopCommonModes];
> Why do you need CommonModes here?
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:275:
> CGFloat textOffset = NSMinX(decoration_frame);
> naming: under_scores, and other locals in this method
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:316:
> base::scoped_nsobject<NSAttributedString> str([[NSAttributedString alloc]
> naming: |text|, given that you have text_rect and text_height, etc?
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
> File
>
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration_unittest.mm
> (right):
> 
>
https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
>
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration_unittest.mm:17:
> VerboseStateBubbleDecorationTest() : icon_(NULL), decoration_(&icon_, NULL) {}
> NULL -> nullptr
> 
>
https://codereview.chromium.org/2119033002/diff/520001/ui/gfx/animation/tween.h
> File ui/gfx/animation/tween.h (right):
> 
>
https://codereview.chromium.org/2119033002/diff/520001/ui/gfx/animation/tween...
> ui/gfx/animation/tween.h:34: FAST_OUT_SLOW_IN_EXPO,  // Variant of
> FAST_OUT_SLOW_IN.
> What's _EXPO mean? How does it vary?

PTAL

[spqchan, 2016-08-17 00:41:18]

PTAL

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm
(right):

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:119:
&location_icon_decoration, NULL);
On 2016/08/16 22:25:32, Robert Sesek wrote:
> nullptr

Done.

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:617: // If the
security level has changed, check if the verbose needs to be
On 2016/08/16 22:25:32, Robert Sesek wrote:
> "check if the verbose" ... verbose what?

Done.

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:620:
security_state::SecurityStateModel::SecurityLevel current_security =
On 2016/08/16 22:25:32, Robert Sesek wrote:
> naming: new_security_level ? looking at |security_level_| seems like the
> "current" state to me.

Done.

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:750: NSColor*
sRGBColor =
On 2016/08/16 22:25:32, Robert Sesek wrote:
> naming: use under_scores, and below

Done.

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.h
(right):

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.h:22: class
VerboseStateBubbleDecoration : public BubbleDecoration {
On 2016/08/16 22:25:32, Robert Sesek wrote:
> "VerboseState" is kind of a weird name IMO. It's really conveying the security
> state, right? Is there another security state decoration that would confound
> using the name SecurityStateBubbleDecoration?

That's true, I was using what they used on specs. I changed it to
SecurityStateBubbleDecoration

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.h:24:
explicit VerboseStateBubbleDecoration(LocationIconDecoration* location_icon,
On 2016/08/16 22:25:32, Robert Sesek wrote:
> No explicit since it takes more than one param.

Done.

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm
(right):

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:43: //
http://crbug.com/49822
On 2016/08/16 22:25:32, Robert Sesek wrote:
> This bug was archived. Does it still apply?

It still does, the star icon should be omitted but it's still there.
I created crbug.com/638427 and took ownership for the TODO

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:98: -
(id)initWithOwner:(VerboseStateBubbleDecoration*)owner
On 2016/08/16 22:25:32, Robert Sesek wrote:
> instancetype preferred in new code

Done.

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:131:
[[NSRunLoop mainRunLoop] addTimer:timer_ forMode:NSRunLoopCommonModes];
On 2016/08/16 22:25:32, Robert Sesek wrote:
> Why do you need CommonModes here?

We want the timer to run when the user is performing stuff like a mouseDrag
(which resizes the omnibox)

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:275:
CGFloat textOffset = NSMinX(decoration_frame);
On 2016/08/16 22:25:32, Robert Sesek wrote:
> naming: under_scores, and other locals in this method

Done.

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:316:
base::scoped_nsobject<NSAttributedString> str([[NSAttributedString alloc]
On 2016/08/16 22:25:32, Robert Sesek wrote:
> naming: |text|, given that you have text_rect and text_height, etc?

Done.

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration_unittest.mm
(right):

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration_unittest.mm:17:
VerboseStateBubbleDecorationTest() : icon_(NULL), decoration_(&icon_, NULL) {}
On 2016/08/16 22:25:33, Robert Sesek wrote:
> NULL -> nullptr

Done.

https://codereview.chromium.org/2119033002/diff/520001/ui/gfx/animation/tween.h
File ui/gfx/animation/tween.h (right):

https://codereview.chromium.org/2119033002/diff/520001/ui/gfx/animation/tween...
ui/gfx/animation/tween.h:34: FAST_OUT_SLOW_IN_EXPO,  // Variant of
FAST_OUT_SLOW_IN.
On 2016/08/16 22:25:33, Robert Sesek wrote:
> What's _EXPO mean? How does it vary?

Done.

[Robert Sesek, 2016-08-18 14:43:24]

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.h
(right):

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.h:24:
explicit VerboseStateBubbleDecoration(LocationIconDecoration* location_icon,
On 2016/08/17 00:41:18, spqchan wrote:
> On 2016/08/16 22:25:32, Robert Sesek wrote:
> > No explicit since it takes more than one param.
> 
> Done.

Not done in latest patchset.

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm
(right):

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:131:
[[NSRunLoop mainRunLoop] addTimer:timer_ forMode:NSRunLoopCommonModes];
On 2016/08/17 00:41:18, spqchan wrote:
> On 2016/08/16 22:25:32, Robert Sesek wrote:
> > Why do you need CommonModes here?
> 
> We want the timer to run when the user is performing stuff like a mouseDrag
> (which resizes the omnibox)

Add a comment then?

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:751: NSColor*
sRGB_color =
naming: all lowercase

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.h
(right):

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.h:35: //
Called by VerboseAnimation when the animation has progressed.
Update comment.

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.mm
(right):

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.mm:5:
#import
"chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.h"
#include <cmath>

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.mm:210:
full_label_.reset([label retain]);
copy NSStings

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.mm:325:
text_rect.origin.y = roundf(NSMidY(text_rect) - textHeight / 2.0) - 1;
Prefer std::round() [c++11]

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.mm:355:
[line setLineWidth:1];
Not cr_lineWidth ?

[spqchan, 2016-08-18 18:37:18]

PTAL

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.h
(right):

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.h:24:
explicit VerboseStateBubbleDecoration(LocationIconDecoration* location_icon,
On 2016/08/18 14:43:23, Robert Sesek wrote:
> On 2016/08/17 00:41:18, spqchan wrote:
> > On 2016/08/16 22:25:32, Robert Sesek wrote:
> > > No explicit since it takes more than one param.
> > 
> > Done.
> 
> Not done in latest patchset.

Done.

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm
(right):

https://codereview.chromium.org/2119033002/diff/520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/verbose_state_bubble_decoration.mm:131:
[[NSRunLoop mainRunLoop] addTimer:timer_ forMode:NSRunLoopCommonModes];
On 2016/08/18 14:43:23, Robert Sesek wrote:
> On 2016/08/17 00:41:18, spqchan wrote:
> > On 2016/08/16 22:25:32, Robert Sesek wrote:
> > > Why do you need CommonModes here?
> > 
> > We want the timer to run when the user is performing stuff like a mouseDrag
> > (which resizes the omnibox)
> 
> Add a comment then?

Done.

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:751: NSColor*
sRGB_color =
On 2016/08/18 14:43:23, Robert Sesek wrote:
> naming: all lowercase

Done.

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.h
(right):

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.h:35: //
Called by VerboseAnimation when the animation has progressed.
On 2016/08/18 14:43:23, Robert Sesek wrote:
> Update comment.

Done.

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.mm
(right):

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.mm:5:
#import
"chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.h"
On 2016/08/18 14:43:23, Robert Sesek wrote:
> #include <cmath>

Done.

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.mm:210:
full_label_.reset([label retain]);
On 2016/08/18 14:43:24, Robert Sesek wrote:
> copy NSStings

Done.

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.mm:325:
text_rect.origin.y = roundf(NSMidY(text_rect) - textHeight / 2.0) - 1;
On 2016/08/18 14:43:24, Robert Sesek wrote:
> Prefer std::round() [c++11]

Done.

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.mm:355:
[line setLineWidth:1];
On 2016/08/18 14:43:23, Robert Sesek wrote:
> Not cr_lineWidth ?

Done.

[Robert Sesek, 2016-08-18 19:33:59]

LGTM

https://codereview.chromium.org/2119033002/diff/620001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.mm
(right):

https://codereview.chromium.org/2119033002/diff/620001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.mm:215:
full_label_.reset([[NSString alloc] initWithString:label]);
[label copy] is what I had in mind, but this is fine.

[spqchan, 2016-08-18 21:08:28]

spqchan@chromium.org changed reviewers:
+ pkasting@google.com

[spqchan, 2016-08-18 21:08:30]

Thanks!

+pkasting for component/toolbar/ OWNER

https://codereview.chromium.org/2119033002/diff/620001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.mm
(right):

https://codereview.chromium.org/2119033002/diff/620001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.mm:215:
full_label_.reset([[NSString alloc] initWithString:label]);
On 2016/08/18 19:33:59, Robert Sesek wrote:
> [label copy] is what I had in mind, but this is fine.

Done.

[Peter Kasting, 2016-08-18 21:14:59]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2016-08-18 21:15:00]

https://codereview.chromium.org/2119033002/diff/660001/components/toolbar/too...
File components/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/2119033002/diff/660001/components/toolbar/too...
components/toolbar/toolbar_model_impl.cc:140: case SecurityStateModel::NONE:
Nit: I'm inclined to just change all these to a "default:".

https://codereview.chromium.org/2119033002/diff/660001/ui/gfx/animation/tween.h
File ui/gfx/animation/tween.h (right):

https://codereview.chromium.org/2119033002/diff/660001/ui/gfx/animation/tween...
ui/gfx/animation/tween.h:34: FAST_OUT_SLOW_IN_EXPO,  // Exponential variant of
FAST_OUT_SLOW_IN.
I'm not keen on the functional creep of how we keep getting more animations over
time.  We should have a small number of animation curves and use them everywhere
-- we already have about 3x as many as we should have.

Can we push the designers to either:
* Use one of the existing curves
* Change one of the existing curves to do what they want, then use it

In either case, we shouldn't wind up with a unique thing for this UI surface.

[spqchan, 2016-08-18 21:18:25]

spqchan@chromium.org changed reviewers:
- pkasting@google.com

[spqchan, 2016-08-18 21:56:32]

PTAL

https://codereview.chromium.org/2119033002/diff/660001/components/toolbar/too...
File components/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/2119033002/diff/660001/components/toolbar/too...
components/toolbar/toolbar_model_impl.cc:140: case SecurityStateModel::NONE:
On 2016/08/18 21:15:00, Peter Kasting wrote:
> Nit: I'm inclined to just change all these to a "default:".

Done.

https://codereview.chromium.org/2119033002/diff/660001/ui/gfx/animation/tween.h
File ui/gfx/animation/tween.h (right):

https://codereview.chromium.org/2119033002/diff/660001/ui/gfx/animation/tween...
ui/gfx/animation/tween.h:34: FAST_OUT_SLOW_IN_EXPO,  // Exponential variant of
FAST_OUT_SLOW_IN.
On 2016/08/18 21:15:00, Peter Kasting wrote:
> I'm not keen on the functional creep of how we keep getting more animations
over
> time.  We should have a small number of animation curves and use them
everywhere
> -- we already have about 3x as many as we should have.
> 
> Can we push the designers to either:
> * Use one of the existing curves
> * Change one of the existing curves to do what they want, then use it
> 
> In either case, we shouldn't wind up with a unique thing for this UI surface.

I can email emilyschechter to see if UI is willing to use an existing curve. I'm
not sure how we can change
one of the existing curves. I did noticed that FAST_IN_OUT is unused though and
removed it. Is that what you mean?

One more alternative, I can just store the curve in
security_state_bubble_decoration and use it from there but it makes more sense
to have something that can be shared between platforms/

[Peter Kasting, 2016-08-18 22:05:10]

https://codereview.chromium.org/2119033002/diff/660001/ui/gfx/animation/tween.h
File ui/gfx/animation/tween.h (right):

https://codereview.chromium.org/2119033002/diff/660001/ui/gfx/animation/tween...
ui/gfx/animation/tween.h:34: FAST_OUT_SLOW_IN_EXPO,  // Exponential variant of
FAST_OUT_SLOW_IN.
On 2016/08/18 21:56:32, spqchan wrote:
> On 2016/08/18 21:15:00, Peter Kasting wrote:
> > I'm not keen on the functional creep of how we keep getting more animations
> over
> > time.  We should have a small number of animation curves and use them
> everywhere
> > -- we already have about 3x as many as we should have.
> > 
> > Can we push the designers to either:
> > * Use one of the existing curves
> > * Change one of the existing curves to do what they want, then use it
> > 
> > In either case, we shouldn't wind up with a unique thing for this UI
surface.
> 
> I can email emilyschechter to see if UI is willing to use an existing curve.
I'm
> not sure how we can change
> one of the existing curves.

For example, we decide to change the constants on the existing FAST_OUT_SLOW_IN
curve to match the new ones you want, so everyone using that curve get the new
behavior.

> I did noticed that FAST_IN_OUT is unused though and
> removed it. Is that what you mean?

That's certainly a good thing to do, but what I'm getting at really is that the
number of values in the enum is merely symptomatic of the root problem, which is
that we have lots of different animation behaviors.  We should be trying to get
to a world with a small number, each of which is used broadly.  Removing an
unused enum value while adding a new one with one use doesn't make that
underlying problem better.

[spqchan, 2016-08-18 22:18:33]

https://codereview.chromium.org/2119033002/diff/660001/ui/gfx/animation/tween.h
File ui/gfx/animation/tween.h (right):

https://codereview.chromium.org/2119033002/diff/660001/ui/gfx/animation/tween...
ui/gfx/animation/tween.h:34: FAST_OUT_SLOW_IN_EXPO,  // Exponential variant of
FAST_OUT_SLOW_IN.
On 2016/08/18 22:05:09, Peter Kasting wrote:
> On 2016/08/18 21:56:32, spqchan wrote:
> > On 2016/08/18 21:15:00, Peter Kasting wrote:
> > > I'm not keen on the functional creep of how we keep getting more
animations
> > over
> > > time.  We should have a small number of animation curves and use them
> > everywhere
> > > -- we already have about 3x as many as we should have.
> > > 
> > > Can we push the designers to either:
> > > * Use one of the existing curves
> > > * Change one of the existing curves to do what they want, then use it
> > > 
> > > In either case, we shouldn't wind up with a unique thing for this UI
> surface.
> > 
> > I can email emilyschechter to see if UI is willing to use an existing curve.
> I'm
> > not sure how we can change
> > one of the existing curves.
> 
> For example, we decide to change the constants on the existing
FAST_OUT_SLOW_IN
> curve to match the new ones you want, so everyone using that curve get the new
> behavior.
> 
> > I did noticed that FAST_IN_OUT is unused though and
> > removed it. Is that what you mean?
> 
> That's certainly a good thing to do, but what I'm getting at really is that
the
> number of values in the enum is merely symptomatic of the root problem, which
is
> that we have lots of different animation behaviors.  We should be trying to
get
> to a world with a small number, each of which is used broadly.  Removing an
> unused enum value while adding a new one with one use doesn't make that
> underlying problem better.

I see what you mean. Sure thing, currently we're using FAST_OUT_SLOW_IN to
animate in, and the variant to animate out. I'll ask to see if we can just stick
with FAST_OUT_SLOW_IN for animating out

[Peter Kasting, 2016-08-18 22:19:28]

LGTM once that gets resolved

[spqchan, 2016-08-22 16:53:39]

On 2016/08/18 22:19:28, Peter Kasting wrote:
> LGTM once that gets resolved

Update from UI: We don't need the new curve anymore because we are getting rid
of "animating out". I'll go remove that part of the code

[Peter Kasting, 2016-08-22 22:39:42]

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:620: // animate it
out if it's already fully displayed.
This isn't the right way to do things (sorry, krb just pointed me at this, I
didn't look before).

I believe you're trying to handle toggling between tabs that have different
security states showing.  If so, then instead of comparing new security level to
current (which is not only complicated, but incorrect if we switch from an
animating tab to a different tab that has the same level -- the animation should
snap open, but it will continue running instead), you should check whether this
update is the result of a tab switch or an up[date within the current tab.

At least on views, this distinction is obvious:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/location_bar/loc...
Here, if Update() is called with a null pointer, this is an update to the
current tab.  If not, it is a tab change.

Fixing this should allow you to greatly simplify the APIs involved here, because
instead of animatein/out and hasanimatedin/out, you can just do a single "set
state to(state, bool animate)" method.

[Kevin Bailey, 2016-08-23 14:10:38]

krb@chromium.org changed reviewers:
+ krb@chromium.org

[Kevin Bailey, 2016-08-23 14:10:41]

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:620: // animate it
out if it's already fully displayed.
On 2016/08/22 22:39:42, Peter Kasting wrote:
> This isn't the right way to do things (sorry, krb just pointed me at this, I
> didn't look before).
> 
> I believe you're trying to handle toggling between tabs that have different
> security states showing.  If so, then instead of comparing new security level
to
> current (which is not only complicated, but incorrect if we switch from an
> animating tab to a different tab that has the same level -- the animation
should
> snap open, but it will continue running instead), you should check whether
this
> update is the result of a tab switch or an up[date within the current tab.
> 
> At least on views, this distinction is obvious:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/location_bar/loc...
> Here, if Update() is called with a null pointer, this is an update to the
> current tab.  If not, it is a tab change.
> 
> Fixing this should allow you to greatly simplify the APIs involved here,
because
> instead of animatein/out and hasanimatedin/out, you can just do a single "set
> state to(state, bool animate)" method.

I'm not sure it simplifies things. First, when the user switches tabs, what it
really should do is restore the state of that other tab. If that tab was
animating, it should pick up where it left off. Since the odds of a user
catching a previous animation are nil, we can just instantly show or hide but
it's technically cheating.

Second, 'has_tab_changed' is just another condition added to the mix:

if (has_tab_changed) {
  if (ShouldShowSecurityState())
    Show();
  else
    Hide();
} else {
  // level change detection logic
}

Sure, you can push it all to a subroutine but I wouldn't call that
simplification.

[Peter Kasting, 2016-08-23 18:57:55]

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:620: // animate it
out if it's already fully displayed.
On 2016/08/23 14:10:41, Kevin Bailey wrote:
> On 2016/08/22 22:39:42, Peter Kasting wrote:
> > This isn't the right way to do things (sorry, krb just pointed me at this, I
> > didn't look before).
> > 
> > I believe you're trying to handle toggling between tabs that have different
> > security states showing.  If so, then instead of comparing new security
level
> to
> > current (which is not only complicated, but incorrect if we switch from an
> > animating tab to a different tab that has the same level -- the animation
> should
> > snap open, but it will continue running instead), you should check whether
> this
> > update is the result of a tab switch or an up[date within the current tab.
> > 
> > At least on views, this distinction is obvious:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/location_bar/loc...
> > Here, if Update() is called with a null pointer, this is an update to the
> > current tab.  If not, it is a tab change.
> > 
> > Fixing this should allow you to greatly simplify the APIs involved here,
> because
> > instead of animatein/out and hasanimatedin/out, you can just do a single
"set
> > state to(state, bool animate)" method.
> 
> I'm not sure it simplifies things. First, when the user switches tabs, what it
> really should do is restore the state of that other tab. If that tab was
> animating, it should pick up where it left off. Since the odds of a user
> catching a previous animation are nil, we can just instantly show or hide but
> it's technically cheating.

It's not cheating, for two reasons.

First, the animation is not really an inherent property of the tab, it's just a
UI affordance to help indicate that something has changed about the state within
this tab.  When the user manually switches tabs, there's no reason to show that
affordance.

Second, if we ignore that and treat the animation as a property of the tab, the
total animation time is 150 ms.  If we were already in the middle of the
animation when the user changed tabs away, they would have to complete the tab
switch away and then back in less than whatever is left of the 150 ms, and we'd
need to pick up where that left us.  That's extremely unlikely -- it probably
takes a good 100 ms just to punch the next tab + previous tab shortcuts -- and
the remaining animation time would be so short that it would be a few pixels of
movement.  That's more likely to look glitchy than intentional.

So I think we get a better result, overall, from not trying to resume the
animation in this case.

> Second, 'has_tab_changed' is just another condition added to the mix:
> 
> if (has_tab_changed) {
>   if (ShouldShowSecurityState())
>     Show();
>   else
>     Hide();
> } else {
>   // level change detection logic
> }
> 
> Sure, you can push it all to a subroutine but I wouldn't call that
> simplification.

It reduces the class' API surface by collapsing four methods to one.  Even if we
only hide the complexity in the implementation instead of exposing it in the
interface, that's still a win.  We isolate the logic that animates from the
logic that decides what state we should be in, effectively separating semantics
from presentation, and the API surface is simpler.

Also, we entirely omit the idea of looking at the previous state and deciding if
we've changed.  The implementation, on the callee side, basically becomes:

  if (!animate)
    Reset(show ? 1 : 0);
  else if (show)
    Show();
  else
    Hide();

[spqchan, 2016-08-24 01:27:41]

On 2016/08/23 18:57:55, Peter Kasting wrote:
>
https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):
> 
>
https://codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:620: // animate
it
> out if it's already fully displayed.
> On 2016/08/23 14:10:41, Kevin Bailey wrote:
> > On 2016/08/22 22:39:42, Peter Kasting wrote:
> > > This isn't the right way to do things (sorry, krb just pointed me at this,
I
> > > didn't look before).
> > > 
> > > I believe you're trying to handle toggling between tabs that have
different
> > > security states showing.  If so, then instead of comparing new security
> level
> > to
> > > current (which is not only complicated, but incorrect if we switch from an
> > > animating tab to a different tab that has the same level -- the animation
> > should
> > > snap open, but it will continue running instead), you should check whether
> > this
> > > update is the result of a tab switch or an up[date within the current tab.
> > > 
> > > At least on views, this distinction is obvious:
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/location_bar/loc...
> > > Here, if Update() is called with a null pointer, this is an update to the
> > > current tab.  If not, it is a tab change.
> > > 
> > > Fixing this should allow you to greatly simplify the APIs involved here,
> > because
> > > instead of animatein/out and hasanimatedin/out, you can just do a single
> "set
> > > state to(state, bool animate)" method.
> > 
> > I'm not sure it simplifies things. First, when the user switches tabs, what
it
> > really should do is restore the state of that other tab. If that tab was
> > animating, it should pick up where it left off. Since the odds of a user
> > catching a previous animation are nil, we can just instantly show or hide
but
> > it's technically cheating.
> 
> It's not cheating, for two reasons.
> 
> First, the animation is not really an inherent property of the tab, it's just
a
> UI affordance to help indicate that something has changed about the state
within
> this tab.  When the user manually switches tabs, there's no reason to show
that
> affordance.
> 
> Second, if we ignore that and treat the animation as a property of the tab,
the
> total animation time is 150 ms.  If we were already in the middle of the
> animation when the user changed tabs away, they would have to complete the tab
> switch away and then back in less than whatever is left of the 150 ms, and
we'd
> need to pick up where that left us.  That's extremely unlikely -- it probably
> takes a good 100 ms just to punch the next tab + previous tab shortcuts -- and
> the remaining animation time would be so short that it would be a few pixels
of
> movement.  That's more likely to look glitchy than intentional.
> 
> So I think we get a better result, overall, from not trying to resume the
> animation in this case.
> 
> > Second, 'has_tab_changed' is just another condition added to the mix:
> > 
> > if (has_tab_changed) {
> >   if (ShouldShowSecurityState())
> >     Show();
> >   else
> >     Hide();
> > } else {
> >   // level change detection logic
> > }
> > 
> > Sure, you can push it all to a subroutine but I wouldn't call that
> > simplification.
> 
> It reduces the class' API surface by collapsing four methods to one.  Even if
we
> only hide the complexity in the implementation instead of exposing it in the
> interface, that's still a win.  We isolate the logic that animates from the
> logic that decides what state we should be in, effectively separating
semantics
> from presentation, and the API surface is simpler.
> 
> Also, we entirely omit the idea of looking at the previous state and deciding
if
> we've changed.  The implementation, on the callee side, basically becomes:
> 
>   if (!animate)
>     Reset(show ? 1 : 0);
>   else if (show)
>     Show();
>   else
>     Hide();

Weird...I can no longer find the string of comments. Anyway, I added the logic
for the changed tab so that security state reanimates when you change it.
Also update: animating out is still happening, but only if the browser size has
switched from "wide" to "narrow", in which the security verbose needs to be
hidden
UI is currently looking at the existing functions in tween.h to see if there's
one they can reuse.

[Peter Kasting, 2016-08-24 03:22:15]

On 2016/08/24 01:27:41, spqchan wrote:
> Anyway, I added the logic
> for the changed tab so that security state reanimates when you change it.

That seems like the wrong behavior.  We don't want the tab to flash stuff at you
every time you switch back to it, especially when nothing in the tab has
changed.

And in general the animation code here seems to be getting more and more
complicated, stateful, and full of logic.  The corresponding code I'm suggesting
in the views side is basically one line on the caller side and six lines in the
callee side.  Can we start with something very simple here and only add
complication later, if it turns out to be really necessary?

[spqchan, 2016-08-24 03:56:20]

On 2016/08/24 03:22:15, Peter Kasting wrote:
> On 2016/08/24 01:27:41, spqchan wrote:
> > Anyway, I added the logic
> > for the changed tab so that security state reanimates when you change it.
> 
> That seems like the wrong behavior.  We don't want the tab to flash stuff at
you
> every time you switch back to it, especially when nothing in the tab has
> changed.
> 
> And in general the animation code here seems to be getting more and more
> complicated, stateful, and full of logic.  The corresponding code I'm
suggesting
> in the views side is basically one line on the caller side and six lines in
the
> callee side.  Can we start with something very simple here and only add
> complication later, if it turns out to be really necessary?

Alright, I'm confused right now. What is the correct behaviour for switching
between tabs then?

Earlier, I would only animate the security state for two reason:
 - Connection state has change (no distinction between tab switch and
navigation)
 - Browser has resized (if it becomes too narrow, animate out. If it then
becomes wide enough, animate it back in)

From the earlier comments, I need to add something in for tab switching?

[Peter Kasting, 2016-08-24 04:02:02]

On 2016/08/24 03:56:20, spqchan wrote:
> On 2016/08/24 03:22:15, Peter Kasting wrote:
> > On 2016/08/24 01:27:41, spqchan wrote:
> > > Anyway, I added the logic
> > > for the changed tab so that security state reanimates when you change it.
> > 
> > That seems like the wrong behavior.  We don't want the tab to flash stuff at
> you
> > every time you switch back to it, especially when nothing in the tab has
> > changed.
> > 
> > And in general the animation code here seems to be getting more and more
> > complicated, stateful, and full of logic.  The corresponding code I'm
> suggesting
> > in the views side is basically one line on the caller side and six lines in
> the
> > callee side.  Can we start with something very simple here and only add
> > complication later, if it turns out to be really necessary?
> 
> Alright, I'm confused right now. What is the correct behaviour for switching
> between tabs then?
> 
> Earlier, I would only animate the security state for two reason:
>  - Connection state has change (no distinction between tab switch and
> navigation)
>  - Browser has resized (if it becomes too narrow, animate out. If it then
> becomes wide enough, animate it back in)
> 
> From the earlier comments, I need to add something in for tab switching?

My impression was:

* When the state changes from "not showing" to "showing" for the current tab,
animate that.
* The reverse direction -- going from showing to not showing -- is apparently
not animated, per your comment about "getting rid of animating out".  So that
just snaps.
* When changing tabs, never animate.  Always just show the new tab's current
state immediately.  (This makes sense as the tabs are totally independent and we
wouldn't want to animate the controls from one tab's state to the other's.)
* When resizing the browser, again, don't animate, either larger or smaller. 
I'm going off views' existing EV chip behavior here, but basically, not
animating makes the UI the most responsive in this case, and there's no need to
call attention to the text change.

So the summary is "never animate for anything except going from insecure ->
secure within the same tab".  Which is also way simpler to implement.

[Kevin Bailey, 2016-08-24 04:19:19]

On 2016/08/24 04:02:02, Peter Kasting wrote:
> 
> My impression was:
> 
> * When the state changes from "not showing" to "showing" for the current tab,
> animate that.
> * The reverse direction -- going from showing to not showing -- is apparently
> not animated, per your comment about "getting rid of animating out".  So that
> just snaps.
> * When changing tabs, never animate.  Always just show the new tab's current
> state immediately.  (This makes sense as the tabs are totally independent and
we
> wouldn't want to animate the controls from one tab's state to the other's.)
> * When resizing the browser, again, don't animate, either larger or smaller. 
> I'm going off views' existing EV chip behavior here, but basically, not
> animating makes the UI the most responsive in this case, and there's no need
to
> call attention to the text change.
> 
> So the summary is "never animate for anything except going from insecure ->
> secure within the same tab".  Which is also way simpler to implement.

I would say that going from secure -> insecure is far more important to animate.
;)

[spqchan, 2016-08-24 04:21:26]

On 2016/08/24 04:02:02, Peter Kasting wrote:
> On 2016/08/24 03:56:20, spqchan wrote:
> > On 2016/08/24 03:22:15, Peter Kasting wrote:
> > > On 2016/08/24 01:27:41, spqchan wrote:
> > > > Anyway, I added the logic
> > > > for the changed tab so that security state reanimates when you change
it.
> > > 
> > > That seems like the wrong behavior.  We don't want the tab to flash stuff
at
> > you
> > > every time you switch back to it, especially when nothing in the tab has
> > > changed.
> > > 
> > > And in general the animation code here seems to be getting more and more
> > > complicated, stateful, and full of logic.  The corresponding code I'm
> > suggesting
> > > in the views side is basically one line on the caller side and six lines
in
> > the
> > > callee side.  Can we start with something very simple here and only add
> > > complication later, if it turns out to be really necessary?
> > 
> > Alright, I'm confused right now. What is the correct behaviour for switching
> > between tabs then?
> > 
> > Earlier, I would only animate the security state for two reason:
> >  - Connection state has change (no distinction between tab switch and
> > navigation)
> >  - Browser has resized (if it becomes too narrow, animate out. If it then
> > becomes wide enough, animate it back in)
> > 
> > From the earlier comments, I need to add something in for tab switching?
> 
> My impression was:
> 
> * When the state changes from "not showing" to "showing" for the current tab,
> animate that.
> * The reverse direction -- going from showing to not showing -- is apparently
> not animated, per your comment about "getting rid of animating out".  So that
> just snaps.
> * When changing tabs, never animate.  Always just show the new tab's current
> state immediately.  (This makes sense as the tabs are totally independent and
we
> wouldn't want to animate the controls from one tab's state to the other's.)
> * When resizing the browser, again, don't animate, either larger or smaller. 
> I'm going off views' existing EV chip behavior here, but basically, not
> animating makes the UI the most responsive in this case, and there's no need
to
> call attention to the text change.
> 
> So the summary is "never animate for anything except going from insecure ->
> secure within the same tab".  Which is also way simpler to implement.

Okay gotcha.
My impression for the first two is the exact same.
For the tabs, I'll have to ask UI about that. I'll shoot them an email
For resizing, the security state is different from the EV cert, it needs to
animate in/out if the browser becomes too narrow/wide
   - If it's too narrow, the security state needs to be hidden

[Peter Kasting, 2016-08-24 05:21:48]

On 2016/08/24 04:21:26, spqchan wrote:
> For resizing, the security state is different from the EV cert, it needs to
> animate in/out if the browser becomes too narrow/wide
>    - If it's too narrow, the security state needs to be hidden

That's exactly what we did with the EV cert name on views -- we
showed/truncated/hid it depending on omnibox length.  That's why I cited that
control.  (I don't know what Mac did.  It wouldn't surprise me if none of that
ever got implemented.)

But we intentionally chose not to animate this transition.  The cert name isn't
actually changing, so there's no state change to indicate.  More importantly,
when resizing the user is generally resizing continuously, so it's important for
the UI elements to be instantly responsive instead of animate over a time delay.

Also, I don't think it makes a whole lot of sense to not animate an actual state
change from shown -> not shown, but to animate a non-state change from shown ->
hidden-due-to-size.

[spqchan, 2016-08-29 17:17:02]

On 2016/08/24 05:21:48, Peter Kasting wrote:
> On 2016/08/24 04:21:26, spqchan wrote:
> > For resizing, the security state is different from the EV cert, it needs to
> > animate in/out if the browser becomes too narrow/wide
> >    - If it's too narrow, the security state needs to be hidden
> 
> That's exactly what we did with the EV cert name on views -- we
> showed/truncated/hid it depending on omnibox length.  That's why I cited that
> control.  (I don't know what Mac did.  It wouldn't surprise me if none of that
> ever got implemented.)
> 
> But we intentionally chose not to animate this transition.  The cert name
isn't
> actually changing, so there's no state change to indicate.  More importantly,
> when resizing the user is generally resizing continuously, so it's important
for
> the UI elements to be instantly responsive instead of animate over a time
delay.
> 
> Also, I don't think it makes a whole lot of sense to not animate an actual
state
> change from shown -> not shown, but to animate a non-state change from shown
->
> hidden-due-to-size.

From what I checked, the EV cert gets truncated not hidden. The secure verbose,
however should hide so animating it out would actually make it less janky.
Anyway, quick update on the tween, animating out is going to use the existing
FAST OUT SLOW IN function.

However, I'm not sure if this CL is okay to land at the moment, since krb
recently brought up some issues regarding the security model.
krb, any updates on it?

[Kevin Bailey, 2016-08-29 17:33:07]

On 2016/08/29 17:17:02, spqchan wrote:
> 
> From what I checked, the EV cert gets truncated not hidden. The secure
verbose,
> however should hide so animating it out would actually make it less janky.
> Anyway, quick update on the tween, animating out is going to use the existing
> FAST OUT SLOW IN function.
> 
> However, I'm not sure if this CL is okay to land at the moment, since krb
> recently brought up some issues regarding the security model.
> krb, any updates on it?

Were you referring to my comment, "I would say that going from secure..." ?
There I was simply clarifying that the previous comment, 'So the summary is
"never animate for anything except going from insecure -> secure within the same
tab".' should probably be, "...except going between secure and insecure..."

[spqchan, 2016-08-29 18:55:19]

On 2016/08/29 17:33:07, Kevin Bailey wrote:
> On 2016/08/29 17:17:02, spqchan wrote:
> > 
> > From what I checked, the EV cert gets truncated not hidden. The secure
> verbose,
> > however should hide so animating it out would actually make it less janky.
> > Anyway, quick update on the tween, animating out is going to use the
existing
> > FAST OUT SLOW IN function.
> > 
> > However, I'm not sure if this CL is okay to land at the moment, since krb
> > recently brought up some issues regarding the security model.
> > krb, any updates on it?
> 
> Were you referring to my comment, "I would say that going from secure..." ?
> There I was simply clarifying that the previous comment, 'So the summary is
> "never animate for anything except going from insecure -> secure within the
same
> tab".' should probably be, "...except going between secure and insecure..."

Sorry, I should've been more clear. I was talking about the issue you raised on
an email thread regarding the security model being in sync with the omnibox,
since it currently cannot tell the difference between the page loading or if
it's HTTP.

[Kevin Bailey, 2016-08-29 19:48:10]

On 2016/08/29 18:55:19, spqchan wrote:
> 
> Sorry, I should've been more clear. I was talking about the issue you raised
on
> an email thread regarding the security model being in sync with the omnibox,
> since it currently cannot tell the difference between the page loading or if
> it's HTTP.

The issue there is that an HTTP site won't show a chip. I'm not aware of any
concrete progress on the issue, except acknowledgment of some bugs filed. My
impression is that everyone is generally ok with this intermediate
functionality, which we will improve as the security model improves, but I don't
want to speak for them.

[Peter Kasting, 2016-08-30 01:30:25]

On 2016/08/29 17:17:02, spqchan wrote:
> On 2016/08/24 05:21:48, Peter Kasting wrote:
> > On 2016/08/24 04:21:26, spqchan wrote:
> > > For resizing, the security state is different from the EV cert, it needs
to
> > > animate in/out if the browser becomes too narrow/wide
> > >    - If it's too narrow, the security state needs to be hidden
> > 
> > That's exactly what we did with the EV cert name on views -- we
> > showed/truncated/hid it depending on omnibox length.  That's why I cited
that
> > control.  (I don't know what Mac did.  It wouldn't surprise me if none of
that
> > ever got implemented.)
> > 
> > But we intentionally chose not to animate this transition.  The cert name
> isn't
> > actually changing, so there's no state change to indicate.  More
importantly,
> > when resizing the user is generally resizing continuously, so it's important
> for
> > the UI elements to be instantly responsive instead of animate over a time
> delay.
> > 
> > Also, I don't think it makes a whole lot of sense to not animate an actual
> state
> > change from shown -> not shown, but to animate a non-state change from shown
> ->
> > hidden-due-to-size.
> 
> From what I checked, the EV cert gets truncated not hidden. The secure
verbose,
> however should hide so animating it out would actually make it less janky.
> Anyway, quick update on the tween, animating out is going to use the existing
> FAST OUT SLOW IN function.

Is that just for hiding this when things get too narrow, and we're still not
animating a state change from verbose -> not verbose?  Or are we animating that
too?  krb's change at https://codereview.chromium.org/2144903004/ does the
former based on the previous comments on this CL, I want to make sure we're all
in sync with each other and with the UI designers.

> However, I'm not sure if this CL is okay to land at the moment

The change I linked above reads a lot more simply than this one because (a) it
doesn't implement animating in and out based on omnibox width, (b) it uses
gfx::SlideAnimation instead of implementing its own custom animation code, and
(c) because SlideAnimation automatically no-ops when you ask it to animate to a
state it's already at, the location bar code does not need any logic to check
the current animation state or the previous security level.

(a) is more an issue of how we decide to break implementing the relevant
functionality here across CLs, though if this were views code I would be worried
by how your implementation runs animations from Layout() -- Layout() should be
idempotent, and we'd instead want to trigger "omnibox too narrow, hide this"
from someplace like OnBoundsChanged() instead.  I don't know enough about how
the Mac code works to know if that concern applies here, but I would think so.

(b) and (c), though, seem like possible ways to simplify this CL even if you
don't break (a) off.  Mac code should be able to rely on the stuff in
ui/gfx/animation/ (looks like it's used in
ui/cocoa/tabs/alert_indicator_button_cocoa.mm at least), and Kevin's CL is a
potential reference for how to make use of that here.  Can you simplify any in
these regards?

[spqchan, 2016-08-31 03:31:26]

On 2016/08/30 01:30:25, Peter Kasting wrote:
> On 2016/08/29 17:17:02, spqchan wrote:
> > On 2016/08/24 05:21:48, Peter Kasting wrote:
> > > On 2016/08/24 04:21:26, spqchan wrote:
> > > > For resizing, the security state is different from the EV cert, it needs
> to
> > > > animate in/out if the browser becomes too narrow/wide
> > > >    - If it's too narrow, the security state needs to be hidden
> > > 
> > > That's exactly what we did with the EV cert name on views -- we
> > > showed/truncated/hid it depending on omnibox length.  That's why I cited
> that
> > > control.  (I don't know what Mac did.  It wouldn't surprise me if none of
> that
> > > ever got implemented.)
> > > 
> > > But we intentionally chose not to animate this transition.  The cert name
> > isn't
> > > actually changing, so there's no state change to indicate.  More
> importantly,
> > > when resizing the user is generally resizing continuously, so it's
important
> > for
> > > the UI elements to be instantly responsive instead of animate over a time
> > delay.
> > > 
> > > Also, I don't think it makes a whole lot of sense to not animate an actual
> > state
> > > change from shown -> not shown, but to animate a non-state change from
shown
> > ->
> > > hidden-due-to-size.
> > 
> > From what I checked, the EV cert gets truncated not hidden. The secure
> verbose,
> > however should hide so animating it out would actually make it less janky.
> > Anyway, quick update on the tween, animating out is going to use the
existing
> > FAST OUT SLOW IN function.
> 
> Is that just for hiding this when things get too narrow, and we're still not
> animating a state change from verbose -> not verbose?  Or are we animating
that
> too?  krb's change at https://codereview.chromium.org/2144903004/ does the
> former based on the previous comments on this CL, I want to make sure we're
all
> in sync with each other and with the UI designers.
> 
> > However, I'm not sure if this CL is okay to land at the moment
> 
> The change I linked above reads a lot more simply than this one because (a) it
> doesn't implement animating in and out based on omnibox width, (b) it uses
> gfx::SlideAnimation instead of implementing its own custom animation code, and
> (c) because SlideAnimation automatically no-ops when you ask it to animate to
a
> state it's already at, the location bar code does not need any logic to check
> the current animation state or the previous security level.
> 
> (a) is more an issue of how we decide to break implementing the relevant
> functionality here across CLs, though if this were views code I would be
worried
> by how your implementation runs animations from Layout() -- Layout() should be
> idempotent, and we'd instead want to trigger "omnibox too narrow, hide this"
> from someplace like OnBoundsChanged() instead.  I don't know enough about how
> the Mac code works to know if that concern applies here, but I would think so.
> 
> (b) and (c), though, seem like possible ways to simplify this CL even if you
> don't break (a) off.  Mac code should be able to rely on the stuff in
> ui/gfx/animation/ (looks like it's used in
> ui/cocoa/tabs/alert_indicator_button_cocoa.mm at least), and Kevin's CL is a
> potential reference for how to make use of that here.  Can you simplify any in
> these regards?

Sorry for the slow response, I’ve been swamped.

Anyway, thanks for the update krb! If people are okay with the intermediate
phase for now, then I think this will be fine. I’ll check to make sure before I
land :)

SlideAnimation would make things cleaner though. However, AFAIK it’s
frame-based, not timer based which *might* be an issue. It’s something I’ll have
to try out. I can simplify things, but it won’t be easy to do it without
refactoring the location_bar_view_mac code. I personally didn’t like to put it
in Layout(), but it’s currently one of the better places to put it. Anyway, if
it’s fine, I can submit this for now, and simplify things in following CL.

[Peter Kasting, 2016-08-31 03:47:08]

On 2016/08/31 03:31:26, spqchan wrote:
> SlideAnimation would make things cleaner though. However, AFAIK it’s
> frame-based, not timer based which *might* be an issue.

Not quite sure what this means, so I can't be helpful.
 
> I can simplify things, but it won’t be easy to do it without
> refactoring the location_bar_view_mac code. I personally didn’t like to put it
> in Layout(), but it’s currently one of the better places to put it.

That was part of why I thought maybe keeping that functionality out of this CL
so it operated more in parallel to krb's might make sense -- then if you need to
refactor LocationBarMac to make adding more animation possible to do in the
right way, you can do it before adding that functionality in its own patch. 
Would perhaps be clearer and easier than trying to land it one way now and then
refactor.

> Anyway, if
> it’s fine, I can submit this for now, and simplify things in following CL.

Up to you, I'm not an OWNER for the Mac code so I wouldn't have the authority to
tell you how best to implement this even if I knew the code well enough to do
so.

I only encourage you, if you leave simplification until after, do make sure to
get it done -- we have a lot of UI code that was to be "cleaned up later" and
later never happened :(

[spqchan, 2016-08-31 17:01:16]

+jochen for components/omnibox_strings.grdp OWNER

[spqchan, 2016-08-31 17:01:26]

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

[spqchan, 2016-08-31 17:01:28]

+jochen for components/omnibox_strings.grdp OWNER

[spqchan, 2016-09-01 01:26:26]

spqchan@chromium.org changed reviewers:
- jochen@chromium.org

[spqchan, 2016-09-01 01:26:28]

- Rebased on krb's changes
- Removing jochen as reviewer since the string is no longer changed
- Switched to SlideAnimation
- Added a TODO with a crbug attached for location_bar_view_mac

[spqchan, 2016-09-01 01:26:38]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-09-01 01:26:38]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2119033002/#ps800001
(title: "nit")

[commit-bot: I haz the power, 2016-09-01 01:27:10]

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-09-01 01:29:09]

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

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

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  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-09-01 18:22:50]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-09-01 18:22:50]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2119033002/#ps820001
(title: "rebased")

[commit-bot: I haz the power, 2016-09-01 18:23:19]

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-09-01 20:06:42]

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

[commit-bot: I haz the power, 2016-09-01 20:06:44]

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-09-01 22:20:11]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-09-01 22:20:12]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2119033002/#ps860001
(title: "nit")

[commit-bot: I haz the power, 2016-09-01 22:20:46]

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-09-01 23:30:42]

Description was changed from

==========
[Material][Mac] Implement Omnibox Verbose State Chips

Replaced EVBubbleDecoration with VerboseStateDecoration.
VerboseStateDecoration displays both the security state and the
EV Cert by animating in and out.

Animations only occur when the security level has changed or if the
omnibox becomes narrow or wide.

Verbose states animates in when the security level changes to
HTTPS or an invalid HTTPS. It animates outs if the security level
changes to something else.

If there's not enough room on the omnibox, the security state should
not appear, and will animate out if it's already displayed. It will
animate back in if the user resize the window so that there's enough
room.

BUG=622529
==========

to

==========
[Material][Mac] Implement Omnibox Verbose State Chips

Replaced EVBubbleDecoration with VerboseStateDecoration.
VerboseStateDecoration displays both the security state and the
EV Cert by animating in and out.

Animations only occur when the security level has changed or if the
omnibox becomes narrow or wide.

Verbose states animates in when the security level changes to
HTTPS or an invalid HTTPS. It animates outs if the security level
changes to something else.

If there's not enough room on the omnibox, the security state should
not appear, and will animate out if it's already displayed. It will
animate back in if the user resize the window so that there's enough
room.

BUG=622529
==========

[commit-bot: I haz the power, 2016-09-01 23:30:44]

Committed patchset #44 (id:860001)

[commit-bot: I haz the power, 2016-09-01 23:33:59]

Description was changed from

==========
[Material][Mac] Implement Omnibox Verbose State Chips

Replaced EVBubbleDecoration with VerboseStateDecoration.
VerboseStateDecoration displays both the security state and the
EV Cert by animating in and out.

Animations only occur when the security level has changed or if the
omnibox becomes narrow or wide.

Verbose states animates in when the security level changes to
HTTPS or an invalid HTTPS. It animates outs if the security level
changes to something else.

If there's not enough room on the omnibox, the security state should
not appear, and will animate out if it's already displayed. It will
animate back in if the user resize the window so that there's enough
room.

BUG=622529
==========

to

==========
[Material][Mac] Implement Omnibox Verbose State Chips

Replaced EVBubbleDecoration with VerboseStateDecoration.
VerboseStateDecoration displays both the security state and the
EV Cert by animating in and out.

Animations only occur when the security level has changed or if the
omnibox becomes narrow or wide.

Verbose states animates in when the security level changes to
HTTPS or an invalid HTTPS. It animates outs if the security level
changes to something else.

If there's not enough room on the omnibox, the security state should
not appear, and will animate out if it's already displayed. It will
animate back in if the user resize the window so that there's enough
room.

BUG=622529

Committed: https://crrev.com/0afe4deba4f93d82430e90d789ab39e3c788c891
Cr-Commit-Position: refs/heads/master@{#416119}
==========

[commit-bot: I haz the power, 2016-09-01 23:34:00]

Patchset 44 (id:??) landed as
https://crrev.com/0afe4deba4f93d82430e90d789ab39e3c788c891
Cr-Commit-Position: refs/heads/master@{#416119}