Issue 2144903004: New location security strings and animation.

Kevin Bailey

Description was changed from ========== New location security strings and animation. Changes to show security ...

4 years, 5 months ago (2016-07-13 19:49:50 UTC) #1

Kevin Bailey

This is a work in progress for the EV chip changes. It's definitely not ready, ...

4 years, 5 months ago (2016-07-13 19:52:41 UTC) #2

Justin Donnelly

Broadly speaking, I don't see any issue with the approach here, but I'm not that ...

4 years, 5 months ago (2016-07-15 14:33:55 UTC) #3

Broadly speaking, I don't see any issue with the approach here, but I'm not
that familiar with the existing code so I look forward to hearing
pkasting's feedback.

My only questions are regarding the behavior of sites with an EV cert. In
your description, you say that the security level will be shown instead of
the EV cert organization name. But my assumption was that we would still
show the organization name in the EV cert case.

On the other hand, the code seems to allow for still handling the EV cert
case, since you're still checking ShouldShowEVBubble() to determine the
label to show. Which is what I expected, but since
ShouldShowSecureVerbose() adds additional conditions (such
as IsModeMaterial()) won't this cause a regression where the EV cert bubble
will no longer be shown in pre-MD UI?

On Wed, Jul 13, 2016 at 12:52 PM, <krb@chromium.org> wrote:

> Reviewers:
> CL: https://codereview.chromium.org/2144903004/
>
> Message:
> This is a work in progress for the EV chip changes.
> It's definitely not ready, but since I'll be away from
> it for a bit, it seemed a good idea to get some early
> feedback.
>
> One thing that I've noticed is that, after this change,
> location_icon_view will look a lot like content_setting_view.
> I will be keeping my eye out for ways to pull common
> bits down into icon_label_bubble_view.
>
> Description:
> New location security strings and animation.
>
> Changes to show security level instead of EV cert and animate it away.
> Dangerous
> site levels are not animated.
>
> BUG=
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+158, -26 lines):
> M chrome/browser/ui/views/location_bar/icon_label_bubble_view.h
> M chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
> M chrome/browser/ui/views/location_bar/location_bar_view.h
> M chrome/browser/ui/views/location_bar/location_bar_view.cc
> M chrome/browser/ui/views/location_bar/location_icon_view.h
> M chrome/browser/ui/views/location_bar/location_icon_view.cc
> M components/omnibox_strings.grdp
> M components/toolbar/toolbar_model.h
> M components/toolbar/toolbar_model_impl.h
> M components/toolbar/toolbar_model_impl.cc
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Peter Kasting

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

4 years, 5 months ago (2016-07-19 21:17:18 UTC) #4

Peter Kasting

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc#newcode128 chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:128: std::cout << "Icon::GetPreferredSize\n"; You may want to use the ...

4 years, 5 months ago (2016-07-19 21:17:21 UTC) #5

Kevin Bailey

Before going much further, I thought I should verify this first. inline On Fri, Jul ...

4 years, 4 months ago (2016-07-28 18:54:03 UTC) #6

Before going much further, I thought I should verify this first. inline

On Fri, Jul 15, 2016 at 7:33 AM, Justin Donnelly <jdonnelly@chromium.org>
wrote:

> Broadly speaking, I don't see any issue with the approach here, but I'm
> not that familiar with the existing code so I look forward to hearing
> pkasting's feedback.
>
> My only questions are regarding the behavior of sites with an EV cert. In
> your description, you say that the security level will be shown instead of
> the EV cert organization name. But my assumption was that we would still
> show the organization name in the EV cert case.
>

The PRD mentions a few phases. It appears to me that phase 1B is already
implemented, so I assumed I was implementing phase 2A, which clearly shows
"Secure" instead of a site name. Correct?

On the other hand, the code seems to allow for still handling the EV cert
> case, since you're still checking ShouldShowEVBubble() to determine the
> label to show. Which is what I expected, but since
> ShouldShowSecureVerbose() adds additional conditions (such
> as IsModeMaterial()) won't this cause a regression where the EV cert bubble
> will no longer be shown in pre-MD UI?
>

The material check is a copy-paste error. I'll re-work that. Does that
clear things up?



> On Wed, Jul 13, 2016 at 12:52 PM, <krb@chromium.org> wrote:
>
>> Reviewers:
>> CL: https://codereview.chromium.org/2144903004/
>>
>> Message:
>> This is a work in progress for the EV chip changes.
>> It's definitely not ready, but since I'll be away from
>> it for a bit, it seemed a good idea to get some early
>> feedback.
>>
>> One thing that I've noticed is that, after this change,
>> location_icon_view will look a lot like content_setting_view.
>> I will be keeping my eye out for ways to pull common
>> bits down into icon_label_bubble_view.
>>
>> Description:
>> New location security strings and animation.
>>
>> Changes to show security level instead of EV cert and animate it away.
>> Dangerous
>> site levels are not animated.
>>
>> BUG=
>>
>> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>>
>> Affected files (+158, -26 lines):
>> M chrome/browser/ui/views/location_bar/icon_label_bubble_view.h
>> M chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
>> M chrome/browser/ui/views/location_bar/location_bar_view.h
>> M chrome/browser/ui/views/location_bar/location_bar_view.cc
>> M chrome/browser/ui/views/location_bar/location_icon_view.h
>> M chrome/browser/ui/views/location_bar/location_icon_view.cc
>> M components/omnibox_strings.grdp
>> M components/toolbar/toolbar_model.h
>> M components/toolbar/toolbar_model_impl.h
>> M components/toolbar/toolbar_model_impl.cc
>>
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Peter Kasting

On Thu, Jul 28, 2016 at 11:53 AM, Kevin Bailey <krb@chromium.org> wrote: > On Fri, ...

4 years, 4 months ago (2016-07-28 18:56:54 UTC) #7

Justin Donnelly

Peter, here's the PRD <https://docs.google.com/document/d/1aZkYU0x4GxQi9mEpR3mSGZpLXRrnXh_WMRozO79runo/edit#heading=h.7fg39m89taxo> for reference. Kevin, if we don't already have a bug ...

4 years, 4 months ago (2016-07-28 19:58:43 UTC) #8

Kevin Bailey

Description was changed from ========== New location security strings and animation. Changes to show security ...

4 years, 4 months ago (2016-07-28 20:06:21 UTC) #9

Kevin Bailey

On Thu, Jul 28, 2016 at 12:58 PM, Justin Donnelly <jdonnelly@chromium.org> wrote: > Peter, here's ...

4 years, 4 months ago (2016-07-28 20:07:18 UTC) #10

Kevin Bailey

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc#newcode128 chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:128: std::cout << "Icon::GetPreferredSize\n"; On 2016/07/19 21:17:19, Peter Kasting wrote: ...

4 years, 4 months ago (2016-08-12 18:49:15 UTC) #11

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:128: std::cout <<
"Icon::GetPreferredSize\n";
On 2016/07/19 21:17:19, Peter Kasting wrote:
> You may want to use the VLOG macro for this kind of debug logging instead.

ack

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:258: // separator
and padding.
On 2016/07/19 21:17:20, Peter Kasting wrote:
> I'm not really sure why.  You'll want to explain this more.
> 
> Also make sure you're not breaking the content setting animations by making
> functional changes in this base class.

I integrated the comment better. Strictly speaking, the change is not necessary,
although I think it reads and represents what's going on better. But now that
we're not shrinking, I'm not sure it belongs in this CL. Let me know if you'd
prefer leaving it out.

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:262:
size.set_width(std::max(current_width, size.width()));
On 2016/07/19 21:17:20, Peter Kasting wrote:
> Unless GetInternalSpacing() or WidthMultiplier() can be negative, I don't
think
> you need the max() call here.

Done.

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:340: if
(!IsShrinking() || WidthMultiplier() > 0) {
On 2016/07/19 21:17:19, Peter Kasting wrote:
> This doesn't break the keyword search presentation (by removing the separator
in
> the steady state), does it?

We don't shrink any more, so reverted.

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right):

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:105: virtual int
GetLabelPreferredWidth() const;
On 2016/07/19 21:17:20, Peter Kasting wrote:
> Nit: Place up with other virtual functions for which this is the base class

Removed. It was something I added, that turned out not to be useful.

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:395: // Tell the
location icon to animate here?
On 2016/07/19 21:17:20, Peter Kasting wrote:
> Why?  This function is all about mucking with a specific page action icon. 
Not
> sure why that would affect the location icon state.

ack

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:541: :
GetToolbarModel()->GetSecureVerboseText();
On 2016/07/19 21:17:20, Peter Kasting wrote:
> Nit: Could factor GetToolbarModel() out.  And since you do this same
computation
> later, maybe the whole thing should get a helper.

Done.

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:627:
location_icon_view_->StartAnimation();
On 2016/07/19 21:17:20, Peter Kasting wrote:
> Yeah, definitely don't do this from layout, that can be called anywhere.

It turns out to be quite useful to do this here. Is there a performance concern?

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:796: // Called twice
per page visit. 'contents' never set.
On 2016/07/19 21:17:20, Peter Kasting wrote:
> That's because |contents| is set when switching tabs, rather than when the
> current tab updates, IIRC.

I only see it set on the very first page load. Strange.

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:797: // Still looking
for what's called once.
On 2016/07/19 21:17:20, Peter Kasting wrote:
> I'm not sure there is anything.  Update() is probably the only function
> currently.  You may need to plumb additional data or callbacks.  Or maybe you
> just need to look for whether we're changing the current URL during this
> Update(), or something.

ack

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1066: return
ui::MaterialDesignController::IsModeMaterial() &&
On 2016/07/19 21:17:20, Peter Kasting wrote:
> Why condition this on material mode?

Done.

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1070:
security_state::SecurityStateModel::NONE;
On 2016/07/19 21:17:20, Peter Kasting wrote:
> I thought we wanted to be verbose about HTTP too?  Or is that a later stage?

Done.

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.h:323: bool
ShouldShowSecureVerbose() const;
On 2016/07/19 21:17:21, Peter Kasting wrote:
> This isn't a very good function name, because "secure verbose" is more like
two
> adjectives than it is like a noun.  You'll want a clearer name, and some
> comments on what it means.

Done.

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_icon_view.cc:181: // They don't
tell you but, to get it slide "down", you must do both:
On 2016/07/19 21:17:21, Peter Kasting wrote:
> Who doesn't tell you?  If you're trying to animate from 1 to 0, then it seems
> pretty clear you have to ask to start at 1 and then animate to 0, since when
> you're constructed you default to state 0 and when an animation stops it
leaves
> the state wherever it is.
> 
> If you want to simply run the animation and have that mean "make the width
> shrink", you can always modify the code that gets the current value to return
> something that will shrink as the animation goes from 0 to 1.

slide_animation.h says:

  // Begin a hiding animation or reverse a showing animation in progress.
  virtual void Hide();

I think the "Begin" part is a little misleading. If you don't call 'Reset(1)',
then 'Hide()' simply reverses the direction, so maybe that's what it should have
been called.

Backing up a bit, I would have expected the methods to be named more like,
'Grow()' and 'Shrink()'. 'Show()' and 'Hide()' are too similar (for my taste) to
the well-known methods to display a window after creation.

I know I'm being fairly nitpicky here, but when method names tend to evoke the
wrong understanding, it doesn't hurt when there's a comment to clear things up.

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_icon_view.h (right):

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_icon_view.h:24: private
gfx::AnimationDelegate {
On 2016/07/19 21:17:21, Peter Kasting wrote:
> Private inheritance is banned.  Use composition or public inheritance.

Done.

https://codereview.chromium.org/2144903004/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_icon_view.h:83: bool
animation_already_started_;
On 2016/07/19 21:17:21, Peter Kasting wrote:
> Nit: Would probably remove "already_".  But note that the animation itself can
> tell you if it's running, what direction, etc.

Done.

https://codereview.chromium.org/2144903004/diff/1/components/omnibox_strings....
File components/omnibox_strings.grdp (right):

https://codereview.chromium.org/2144903004/diff/1/components/omnibox_strings....
components/omnibox_strings.grdp:16: <message name="IDS_SECURE_VERBOSE_HTTPS"
desc="Text for the HTTPS Omnibox Secure Verbose State.">
On 2016/07/19 21:17:21, Peter Kasting wrote:
> Nit: These sorts of descriptions aren't very helpful to translators since they
> don't know what a "secure verbose state" is supposed to be.  You'll want to
say
> something like "Text that appears briefly in the address bar when the user
> navigates to a site using valid HTTPS." or something similar.

Done.

https://codereview.chromium.org/2144903004/diff/1/components/toolbar/toolbar_...
File components/toolbar/toolbar_model.h (right):

https://codereview.chromium.org/2144903004/diff/1/components/toolbar/toolbar_...
components/toolbar/toolbar_model.h:89: virtual
scoped_refptr<net::X509Certificate> GetCertificate() const = 0;
On 2016/07/19 21:17:21, Peter Kasting wrote:
> Nit: Seems like this should maybe be moved up one or two items to be with the
> other GetXXX() functions.

Done. Fixed in toolbar_model_impl.h too.

https://codereview.chromium.org/2144903004/diff/1/components/toolbar/toolbar_...
File components/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/2144903004/diff/1/components/toolbar/toolbar_...
components/toolbar/toolbar_model_impl.cc:158: return base::string16();
On 2016/07/19 21:17:21, Peter Kasting wrote:
> Nit: NOTREACHED()?

Done.

Peter Kasting

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc#newcode264 chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:264: size.set_width(current_width); It seems like there are two effects this ...

4 years, 4 months ago (2016-08-15 23:29:50 UTC) #12

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:264:
size.set_width(current_width);
It seems like there are two effects this change has:

(1) The code is longer and more complicated.
(2) We will shrink from (max) to (image width) in the same time we grew from (0)
to (max), instead of shrinking at the same pixel rate (and thus taking a shorter
time to shrink than grow).

Change (2) doesn't seem desirable.  It seems like shrinking at a constant
physical speed instead of over an equal time period is better.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:144: load_state_(new
LoadState()),
If we always have a non-null |load_state_|, why use a scoped_ptr to begin with? 
Just use a member-by-value.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:337:
GetToolbarModel()->GetSecurityLevel(false))
This is necessary only if we use the "EV bubble" for more cases than just EV. 
In that case, it (and all related constants/enums/classes) should be renamed.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:352: if
((security_level == security_state::SecurityStateModel::NONE) ||
Simpler than all the changes you've made here is to just add to the top of the
function:

  if (security_level == security_state::SecurityStateModel::NONE)
    security_level = security_state::SecurityStateModel::SECURITY_ERROR;

(Though this implies that maybe the toolbar model itself should change instead.)

But I didn't think we were yet showing warnings about HTTP pages, so I'm
confused as to why we want this.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:628:
location_icon_view_->StartAnimation();
Don't start and stop animations from Layout().  Layout() can be called any
number of times at any point in time (whether or not it is today).  Trigger
animation changes directly from some sort of listener or handler that is called
when the corresponding state changes.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1054: return
ShouldShowEVBubble() ? GetToolbarModel()->GetEVCertName()
Nit: This is now the only caller of this function, and the function is simple;
I'd inline it here.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1070: if
(RE2::FullMatch(url, "http(s?)://([^/]*)((/?).*)", &prefix, &site))
Please don't process things with a regex.

What are you trying to do here?  It looks like you want to return the hostname
of the site.  If so, just use the GURL::host() for the actual site we're
currently on.  This should, I think, be omnibox_view_->model()->PermanentURL().

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1100:
load_state_->last_returned_ = !load_state_->last_returned_;
I don't understand this logic.  It means each time we do a navigation we'll
change whether we want to show or hide the animation.

In general, the core problem here is that this function doesn't just calculate
if the UI should be showing, it actually toggles things.  This means calling the
function too many or too few times changes how the UI appears.  This isn't the
right way to handle state changes.  Instead a ShouldShowXXX() function should
return the same thing if you call it repeatedly.

This is tied to my comments about not kicking off animations from Layout().  You
want to isolate the event that should trigger an animation show or hide, then
add listener code that kicks off the animation from that point.  ShouldShowXXX()
should simply return whether we're before or after that point.

It seems like the core problem here is that you can't figure out how to tell
when the right navigation state changes happen and you're hacking around it. 
Don't do that.  Get creis or other nav controller people to handhold you through
doing the right thing.

You may wish to look back in version history for how we implemented the Origin
Chip and refer to some of that code.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1102: } else {
Nit: No else after return.

Anyway here you could have just done:

      ...
      load_state_->last_returned_ = !load_state_->last_returned_;
    } else {
      load_state_->last_returned_ = true;
    }
    return load_state_->last_returned_;

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1109:
web_contents->GetController().GetLastCommittedEntryIndex()) {
Nit:

  } else if (load_state_->last_index_ !=
                 web_contents->GetController().GetLastCommittedEntryIndex()) {

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:324: std::string
TrimSite(const std::string& url) const;
Nit: Neither of the new functions here is 100% clear from the name, so you
should probably add descriptive comments for each.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:326: // Returns true if
the security chip should be displayed and is ready.
Nit: What does "and is ready" mean from the perspective of the reader here?  I
think you mean it to distinguish the case of "we should in theory be displaying
the chip but in practice can't do so yet", but the caller just cares about
"should I show the chip _right now_", and that's the sense I'd read "should be
displayed" in.  So I'd just remove these three words.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:479: };
Nit: We normally put struct defs at the top of the section, right above
typedefs.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:172:
animation_.SetSlideDuration(label()->GetPreferredSize().width() * 10);
This doesn't seem right.  We should use the same time period to animate any of
our messages, rather than basing it on the message length.  This way we can pick
a specific short animation duration and ensure all animations comply with it,
regardless of translation etc.  This will also allow setting this duration once
at construction.

Plus this value is way too high.  Most animations in the UI, especially ones
shown frequently as these will be, should take 100 ms or less.  We're not Apple,
we shouldn't "tune for the demo, annoy users long-term" :)

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:173:
animation_.Reset(0);
Shouldn't reset to 0 here; we should animate from wherever we're at, and the
caller should ensure we get animated (or snapped) to 0 when necessary.

This is related to my comments about not running animations from LBV::Layout().

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:179: if
(animation_.is_animating() || animation_.GetCurrentValue() == 1)
Why is this conditional needed?

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:184: // Since things
are moving (sideways), we need a re-layout and paint.
Nit: This comment is obvious, so I'd remove it.

https://codereview.chromium.org/2144903004/diff/100001/components/omnibox_str...
File components/omnibox_strings.grdp (right):

https://codereview.chromium.org/2144903004/diff/100001/components/omnibox_str...
components/omnibox_strings.grdp:22: <message name="IDS_SECURE_VERBOSE_DANGEROUS"
desc="Text shown in the address bar for sites with known malware, social
engineering attacks or UwS .">
Nit: Extra space before period

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

https://codereview.chromium.org/2144903004/diff/100001/components/toolbar/too...
components/toolbar/toolbar_model_impl.cc:158: NOTREACHED();
You may get a compiler warning about not all control paths returning a value
here.

Kevin Bailey

Just replying to some comments. I'll have to go back and ask the authors about ...

4 years, 4 months ago (2016-08-16 16:39:37 UTC) #13

Just replying to some comments. I'll have to go back and ask the authors about
accomplishing an event driven approach.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:264:
size.set_width(current_width);
On 2016/08/15 23:29:49, Peter Kasting wrote:
> It seems like there are two effects this change has:
> 
> (1) The code is longer and more complicated.
> (2) We will shrink from (max) to (image width) in the same time we grew from
(0)
> to (max), instead of shrinking at the same pixel rate (and thus taking a
shorter
> time to shrink than grow).
> 
> Change (2) doesn't seem desirable.  It seems like shrinking at a constant
> physical speed instead of over an equal time period is better.

Done, but see comment in location_icon_view.cc.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:144: load_state_(new
LoadState()),
On 2016/08/15 23:29:49, Peter Kasting wrote:
> If we always have a non-null |load_state_|, why use a scoped_ptr to begin
with? 
> Just use a member-by-value.

The scoped_ptr is to free it, and having it a new'd structure at all is to get
around the const methods. If our policy is to use mutable here, I'm happy to
change it.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:337:
GetToolbarModel()->GetSecurityLevel(false))
On 2016/08/15 23:29:49, Peter Kasting wrote:
> This is necessary only if we use the "EV bubble" for more cases than just EV. 
> In that case, it (and all related constants/enums/classes) should be renamed.

Sorry, I'm not understanding the comment. The caller is simply asking for what
color the EV bubble should be. Now we must return red in some cases.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:352: if
((security_level == security_state::SecurityStateModel::NONE) ||
On 2016/08/15 23:29:49, Peter Kasting wrote:
> Simpler than all the changes you've made here is to just add to the top of the
> function:
> 
>   if (security_level == security_state::SecurityStateModel::NONE)
>     security_level = security_state::SecurityStateModel::SECURITY_ERROR;

Done.

> (Though this implies that maybe the toolbar model itself should change
instead.)

I'm not entirely sure what you're implying but I chatted with groby@ about
something like this and we agreed that this week's UI decisions shouldn't affect
the security model, which should instead be more accurate than it is now.

> But I didn't think we were yet showing warnings about HTTP pages, so I'm
> confused as to why we want this.

In a previous comment, you wrote, "I thought we wanted to be verbose about HTTP
too?  Or is that a later stage?" and I think jdonnely mentioned it too. The doc
says, "Eventually 'Insecure'." I'll double-check.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:628:
location_icon_view_->StartAnimation();
On 2016/08/15 23:29:49, Peter Kasting wrote:
> Don't start and stop animations from Layout().  Layout() can be called any
> number of times at any point in time (whether or not it is today).  Trigger
> animation changes directly from some sort of listener or handler that is
called
> when the corresponding state changes.

I had hoped to use this strategy, but, for example, the NavigationEntry
notification comes way too early, and requires us to monitor the WebContents
anyways. I think this will require some of the other pieces to get re-plumbed.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1054: return
ShouldShowEVBubble() ? GetToolbarModel()->GetEVCertName()
On 2016/08/15 23:29:49, Peter Kasting wrote:
> Nit: This is now the only caller of this function, and the function is simple;
> I'd inline it here.

The Mac code should use it. That's where I borrowed it from.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1070: if
(RE2::FullMatch(url, "http(s?)://([^/]*)((/?).*)", &prefix, &site))
On 2016/08/15 23:29:49, Peter Kasting wrote:
> Please don't process things with a regex.
> 
> What are you trying to do here?  It looks like you want to return the hostname
> of the site.  If so, just use the GURL::host() for the actual site we're
> currently on.  This should, I think, be
omnibox_view_->model()->PermanentURL().

Done.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1100:
load_state_->last_returned_ = !load_state_->last_returned_;
On 2016/08/15 23:29:49, Peter Kasting wrote:
> I don't understand this logic.  It means each time we do a navigation we'll
> change whether we want to show or hide the animation.
> 
> In general, the core problem here is that this function doesn't just calculate
> if the UI should be showing, it actually toggles things.  This means calling
the
> function too many or too few times changes how the UI appears.  This isn't the
> right way to handle state changes.  Instead a ShouldShowXXX() function should
> return the same thing if you call it repeatedly.

btw, it does return the same thing (if other state doesn't change.) This
particular path will update 'last_index_' and not fall through here next time.

> This is tied to my comments about not kicking off animations from Layout(). 
You
> want to isolate the event that should trigger an animation show or hide, then
> add listener code that kicks off the animation from that point. 
ShouldShowXXX()
> should simply return whether we're before or after that point.
> 
> It seems like the core problem here is that you can't figure out how to tell
> when the right navigation state changes happen and you're hacking around it. 
> Don't do that.  Get creis or other nav controller people to handhold you
through
> doing the right thing.
> 
> You may wish to look back in version history for how we implemented the Origin
> Chip and refer to some of that code.

Will do.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1102: } else {
On 2016/08/15 23:29:49, Peter Kasting wrote:
> Nit: No else after return.
> 
> Anyway here you could have just done:
> 
>       ...
>       load_state_->last_returned_ = !load_state_->last_returned_;
>     } else {
>       load_state_->last_returned_ = true;
>     }
>     return load_state_->last_returned_;

Done, and more.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1109:
web_contents->GetController().GetLastCommittedEntryIndex()) {
On 2016/08/15 23:29:49, Peter Kasting wrote:
> Nit:
> 
>   } else if (load_state_->last_index_ !=
>                  web_contents->GetController().GetLastCommittedEntryIndex()) {

Done.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:324: std::string
TrimSite(const std::string& url) const;
On 2016/08/15 23:29:49, Peter Kasting wrote:
> Nit: Neither of the new functions here is 100% clear from the name, so you
> should probably add descriptive comments for each.

Done.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:326: // Returns true if
the security chip should be displayed and is ready.
On 2016/08/15 23:29:49, Peter Kasting wrote:
> Nit: What does "and is ready" mean from the perspective of the reader here?  I
> think you mean it to distinguish the case of "we should in theory be
displaying
> the chip but in practice can't do so yet", but the caller just cares about
> "should I show the chip _right now_", and that's the sense I'd read "should be
> displayed" in.  So I'd just remove these three words.

Done.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:479: };
On 2016/08/15 23:29:49, Peter Kasting wrote:
> Nit: We normally put struct defs at the top of the section, right above
> typedefs.

Done.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:172:
animation_.SetSlideDuration(label()->GetPreferredSize().width() * 10);
On 2016/08/15 23:29:50, Peter Kasting wrote:
> This doesn't seem right.  We should use the same time period to animate any of
> our messages, rather than basing it on the message length.  This way we can
pick
> a specific short animation duration and ensure all animations comply with it,
> regardless of translation etc.  This will also allow setting this duration
once
> at construction.

Ok, was just copying other code (I think omnibox_result_view.) To make sure I'm
on the correct page, this seems to be the opposite of what you requested in
icon_label_bubble_view.cc.

> Plus this value is way too high.  Most animations in the UI, especially ones
> shown frequently as these will be, should take 100 ms or less.  We're not
Apple,
> we shouldn't "tune for the demo, annoy users long-term" :)

Ah, sorry, forgot about it. I had it high so I could 1) make sure it was
animating at all and 2) see clearly what the animation was doing.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:173:
animation_.Reset(0);
On 2016/08/15 23:29:50, Peter Kasting wrote:
> Shouldn't reset to 0 here; we should animate from wherever we're at, and the
> caller should ensure we get animated (or snapped) to 0 when necessary.
> 
> This is related to my comments about not running animations from
LBV::Layout().

I'll have to work on this with the rest of the approach.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:179: if
(animation_.is_animating() || animation_.GetCurrentValue() == 1)
On 2016/08/15 23:29:50, Peter Kasting wrote:
> Why is this conditional needed?

This replaces 'animation_already_started_'. It is needed because this approach
repeatedly calls 'StopAnimation()'.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:184: // Since things
are moving (sideways), we need a re-layout and paint.
On 2016/08/15 23:29:50, Peter Kasting wrote:
> Nit: This comment is obvious, so I'd remove it.

Done.

https://codereview.chromium.org/2144903004/diff/100001/components/omnibox_str...
File components/omnibox_strings.grdp (right):

https://codereview.chromium.org/2144903004/diff/100001/components/omnibox_str...
components/omnibox_strings.grdp:22: <message name="IDS_SECURE_VERBOSE_DANGEROUS"
desc="Text shown in the address bar for sites with known malware, social
engineering attacks or UwS .">
On 2016/08/15 23:29:50, Peter Kasting wrote:
> Nit: Extra space before period

Done.

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

https://codereview.chromium.org/2144903004/diff/100001/components/toolbar/too...
components/toolbar/toolbar_model_impl.cc:158: NOTREACHED();
On 2016/08/15 23:29:50, Peter Kasting wrote:
> You may get a compiler warning about not all control paths returning a value
> here.

I don't appear to, but I'm happy to remove any doubt.

Peter Kasting

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/views/location_bar/location_bar_view.cc File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode144 chrome/browser/ui/views/location_bar/location_bar_view.cc:144: load_state_(new LoadState()), On 2016/08/16 16:39:36, Kevin Bailey wrote: > ...

4 years, 4 months ago (2016-08-16 18:15:06 UTC) #14

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:144: load_state_(new
LoadState()),
On 2016/08/16 16:39:36, Kevin Bailey wrote:
> On 2016/08/15 23:29:49, Peter Kasting wrote:
> > If we always have a non-null |load_state_|, why use a scoped_ptr to begin
> with? 
> > Just use a member-by-value.
> 
> The scoped_ptr is to free it, and having it a new'd structure at all is to get
> around the const methods. If our policy is to use mutable here, I'm happy to
> change it.

If an object represents an internal cache rather than state that is directly or
indirectly visible to external observers, use mutable.  If the object doesn't
fit those constraints, the method should not be const.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:337:
GetToolbarModel()->GetSecurityLevel(false))
On 2016/08/16 16:39:36, Kevin Bailey wrote:
> On 2016/08/15 23:29:49, Peter Kasting wrote:
> > This is necessary only if we use the "EV bubble" for more cases than just
EV. 
> > In that case, it (and all related constants/enums/classes) should be
renamed.
> 
> Sorry, I'm not understanding the comment. The caller is simply asking for what
> color the EV bubble should be. Now we must return red in some cases.

You're changing the meaning of "EV bubble" to "verbose security bubble".  It's
not for "display EV cert name", it's for "display some verbose security
information".

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:172:
animation_.SetSlideDuration(label()->GetPreferredSize().width() * 10);
On 2016/08/16 16:39:36, Kevin Bailey wrote:
> On 2016/08/15 23:29:50, Peter Kasting wrote:
> > This doesn't seem right.  We should use the same time period to animate any
of
> > our messages, rather than basing it on the message length.  This way we can
> pick
> > a specific short animation duration and ensure all animations comply with
it,
> > regardless of translation etc.  This will also allow setting this duration
> once
> > at construction.
> 
> Ok, was just copying other code (I think omnibox_result_view.) To make sure
I'm
> on the correct page, this seems to be the opposite of what you requested in
> icon_label_bubble_view.cc.

Yes, I thought about noting why I'm apparently requesting the opposite.  Setting
the same timing here (and the same as the content setting code; see below) will
make sure that the different states the user gets into all have animations that
take the same time, even if the string lengths are different.  Because these
will happen at disparate points in time, the user will notice dissimilarities in
total animation time more than dissimilarities in speed.

The comment in the other spot will ensure that, especially for very short
strings, the open and close animations don't appear to run at different speeds. 
Since these animations happen within a few seconds of each other, a noticeably
slower travel rate when closing would feel "different" more than a different
total time would.

I'm having a tough time articulating this precisely, but basically both issues
are about making the perception of the different types of animations we do more
consistent.  Why this is well-served by shooting for equal times in one place
and equal speeds in another is basically about one feeling like "different
animations" and the other "two halves of the same animation".

> > Plus this value is way too high.  Most animations in the UI, especially ones
> > shown frequently as these will be, should take 100 ms or less.  We're not
> Apple,
> > we shouldn't "tune for the demo, annoy users long-term" :)
> 
> Ah, sorry, forgot about it. I had it high so I could 1) make sure it was
> animating at all and 2) see clearly what the animation was doing.

I suggest using the same animation timing constants (ideally, from a shared
location; maybe just hoist to IconLabelBubbleView) as the
ContentSettingImageView: 150 ms each to open/close, and 3200 ms open in between.
 Consistency is good.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:179: if
(animation_.is_animating() || animation_.GetCurrentValue() == 1)
On 2016/08/16 16:39:36, Kevin Bailey wrote:
> On 2016/08/15 23:29:50, Peter Kasting wrote:
> > Why is this conditional needed?
> 
> This replaces 'animation_already_started_'. It is needed because this approach
> repeatedly calls 'StopAnimation()'.

But if it's removed, the only effect will be that we call Reset(0) again after
calling Reset(0) once, which should be a no-op.

Kevin Bailey

I decided to align with spqchan's equivalent CL and bail on HTTP support. It makes ...

4 years, 4 months ago (2016-08-17 16:33:30 UTC) #15

I decided to align with spqchan's equivalent CL and bail on HTTP support. It
makes a lot of the problems go away. We can revisit the support when the
security model is better.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:337:
GetToolbarModel()->GetSecurityLevel(false))
On 2016/08/16 18:15:05, Peter Kasting wrote:
> On 2016/08/16 16:39:36, Kevin Bailey wrote:
> > On 2016/08/15 23:29:49, Peter Kasting wrote:
> > > This is necessary only if we use the "EV bubble" for more cases than just
> EV. 
> > > In that case, it (and all related constants/enums/classes) should be
> renamed.
> > 
> > Sorry, I'm not understanding the comment. The caller is simply asking for
what
> > color the EV bubble should be. Now we must return red in some cases.
> 
> You're changing the meaning of "EV bubble" to "verbose security bubble".  It's
> not for "display EV cert name", it's for "display some verbose security
> information".

Ok, all I noticed was this enum.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:173:
animation_.Reset(0);
On 2016/08/16 16:39:37, Kevin Bailey wrote:
> On 2016/08/15 23:29:50, Peter Kasting wrote:
> > Shouldn't reset to 0 here; we should animate from wherever we're at, and the
> > caller should ensure we get animated (or snapped) to 0 when necessary.
> > 
> > This is related to my comments about not running animations from
> LBV::Layout().
> 
> I'll have to work on this with the rest of the approach.

Done, and I don't think we need the checks any more either.

https://codereview.chromium.org/2144903004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:179: if
(animation_.is_animating() || animation_.GetCurrentValue() == 1)
On 2016/08/16 18:15:06, Peter Kasting wrote:
> On 2016/08/16 16:39:36, Kevin Bailey wrote:
> > On 2016/08/15 23:29:50, Peter Kasting wrote:
> > > Why is this conditional needed?
> > 
> > This replaces 'animation_already_started_'. It is needed because this
approach
> > repeatedly calls 'StopAnimation()'.
> 
> But if it's removed, the only effect will be that we call Reset(0) again after
> calling Reset(0) once, which should be a no-op.

Done.

Peter Kasting

+CC maxwalker This seems mostly there, but I'm a bit uneasy that Max changed the ...

4 years, 4 months ago (2016-08-18 06:24:38 UTC) #16

+CC maxwalker

This seems mostly there, but I'm a bit uneasy that Max changed the spec for this
in the time since I last saw it, so that instead of animating away (like the
content settings animation) or appearing only the first 1 or n times on visiting
a site, this is permanently visible every time.  This seems a bit excessive to
me; specifically:

* I'm worried about long translations (Malayalam) or narrow windows reducing the
visibility of the origin (and rest of the URL)
* I'm worried about whether this being present so frequently makes users any
less likely to pay attention to it (instead of it it appears occasionally)
* I'm worried about visual noise of having More Stuff visible in the omnibox
commonly

I'd like to understand what led to that change.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:35: static
constexpr int kOpenTimeMS = 150;
Nit: Should be protected, and can be defined in the .cc file and merely declared
in the .h file.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:25: #include
"chrome/browser/ssl/chrome_security_state_model_client.h"
Nit: I think none of these new #includes are necessary anymore.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:540: base::string16
label = GetSecurityText();
Nit: |security_text| might be a better name.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:809: if
(ShouldShowSecurityChip() != showing_security_chip_) {
If it is safe to rely on the animation calls being no-ops when the state isn't
changing (which is I think currently the case), we can simplify this. 
|showing_security_chip_| can be entirely removed.  We can rename
StartAnimation() and StopAnimation() to make it clearer they aren't always
called when the animation is stopped and started, respectively.  The result is
that the code here just becomes:

  if (ShouldShowSecurityChip())
    location_icon_view_->ShowSecurityChip();
  else
    location_icon_view_->HideSecurityChip();

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1075: if (level ==
security_state::SecurityStateModel::SecurityLevel::EV_SECURE ||
Nit: Could just

  return level == ... ||
         level == ... ||
         level == ...;

I suggest a "using security_state::SecurityStateModel::SecurityLevel;" inside
this function to reduce the verbosity of this statement.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1078:
security_state::SecurityStateModel::SecurityLevel::SECURITY_ERROR) {
What about SECURITY_WARNING and SECURITY_POLICY_WARNING?

Maybe this should return true iff ToolbarModelImpl::GetSecureVerboseText()
returns a nonempty string (see also comments in .grd file).

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1081: return false;
Checking: Does the toolbar model return NONE any time we're editing, and thus
automatically hide this in that case?

If so, I don't think we need the ShouldShowKeywordBubble() check at the top
since that can only show when editing.

If not, then I think the chip can be showing while editing, which isn't right.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:321: // Returns text
describing the URL's security level, for the icon view.
Nit: icon view -> security chip?

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:328: // page
transition.
Nit: No longer accurate

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.h (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.h:58: double
WidthMultiplier() const override;
Nit: These two overrides should be private, and they should be in their own
section, with a "// gfx::AnimationDelegate:" comment above it, above the
non-overrides.

https://codereview.chromium.org/2144903004/diff/160001/components/omnibox_str...
File components/omnibox_strings.grdp (right):

https://codereview.chromium.org/2144903004/diff/160001/components/omnibox_str...
components/omnibox_strings.grdp:21: </message>
We don't currently show this string in the UI, so I would not include it in this
patch and not have the toolbar model have a case for HTTP.

This prevents the toolbar model's code from being out of sync with what the UI
actually shows, which is confusing for the reader.  Better to just update both
places later when we add this.

Kevin Bailey

Just addressing your comments. It looks like some things changed yet again, so there will ...

4 years, 4 months ago (2016-08-18 16:09:36 UTC) #17

Just addressing your comments. It looks like some things changed yet again, so
there will be another round.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:35: static
constexpr int kOpenTimeMS = 150;
On 2016/08/18 06:24:37, Peter Kasting wrote:
> Nit: Should be protected, and can be defined in the .cc file and merely
declared
> in the .h file.

Made protected, but what's the advantage of moving to .cc ?

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:25: #include
"chrome/browser/ssl/chrome_security_state_model_client.h"
On 2016/08/18 06:24:37, Peter Kasting wrote:
> Nit: I think none of these new #includes are necessary anymore.

Done.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:540: base::string16
label = GetSecurityText();
On 2016/08/18 06:24:37, Peter Kasting wrote:
> Nit: |security_text| might be a better name.

If you prefer. I think once we cross line 540, it's more interesting to the
reader that it will be a label.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:809: if
(ShouldShowSecurityChip() != showing_security_chip_) {
On 2016/08/18 06:24:37, Peter Kasting wrote:
> If it is safe to rely on the animation calls being no-ops when the state isn't
> changing (which is I think currently the case), we can simplify this. 
> |showing_security_chip_| can be entirely removed.  We can rename
> StartAnimation() and StopAnimation() to make it clearer they aren't always
> called when the animation is stopped and started, respectively.  The result is
> that the code here just becomes:
> 
>   if (ShouldShowSecurityChip())
>     location_icon_view_->ShowSecurityChip();
>   else
>     location_icon_view_->HideSecurityChip();

Done, but it looks like this whole thing needs to change again so, done for now.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1075: if (level ==
security_state::SecurityStateModel::SecurityLevel::EV_SECURE ||
On 2016/08/18 06:24:37, Peter Kasting wrote:
> Nit: Could just
> 
>   return level == ... ||
>          level == ... ||
>          level == ...;
> 
> I suggest a "using security_state::SecurityStateModel::SecurityLevel;" inside
> this function to reduce the verbosity of this statement.

I can go as far as SecurityStateModel, so I don't know that it helps much.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1078:
security_state::SecurityStateModel::SecurityLevel::SECURITY_ERROR) {
On 2016/08/18 06:24:37, Peter Kasting wrote:
> What about SECURITY_WARNING and SECURITY_POLICY_WARNING?
> 
> Maybe this should return true iff ToolbarModelImpl::GetSecureVerboseText()
> returns a nonempty string (see also comments in .grd file).

I asked spqchan why it wasn't handled in her CL and she said that, according to
Emily, we're not doing it yet. If the model simply isn't providing those enums
yet, then we could reduce this to 'level != NONE'.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1081: return false;
On 2016/08/18 06:24:37, Peter Kasting wrote:
> Checking: Does the toolbar model return NONE any time we're editing, and thus
> automatically hide this in that case?
> 
> If so, I don't think we need the ShouldShowKeywordBubble() check at the top
> since that can only show when editing.
> 
> If not, then I think the chip can be showing while editing, which isn't right.

If the user starts editing after things have settled, the level instantly drops
(according to my logs.) If I changed the above to:

if (ShouldShowKeywordBubble() || omnibox_view_->IsEditingOrEmpty()) return
false;

then, prior to it settling, it would be bouncing all over the place, which is
one reason I added the earlier state machine. So I think for this particular
case, we should leave it as is.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:321: // Returns text
describing the URL's security level, for the icon view.
On 2016/08/18 06:24:37, Peter Kasting wrote:
> Nit: icon view -> security chip?

I'm trying to tell the reader where it goes. If I say security chip, I'm not
even sure that they can search for that.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:328: // page
transition.
On 2016/08/18 06:24:37, Peter Kasting wrote:
> Nit: No longer accurate

Done.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.h (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.h:58: double
WidthMultiplier() const override;
On 2016/08/18 06:24:37, Peter Kasting wrote:
> Nit: These two overrides should be private, and they should be in their own
> section, with a "// gfx::AnimationDelegate:" comment above it, above the
> non-overrides.

Let me know if I misunderstood.

https://codereview.chromium.org/2144903004/diff/160001/components/omnibox_str...
File components/omnibox_strings.grdp (right):

https://codereview.chromium.org/2144903004/diff/160001/components/omnibox_str...
components/omnibox_strings.grdp:21: </message>
On 2016/08/18 06:24:37, Peter Kasting wrote:
> We don't currently show this string in the UI, so I would not include it in
this
> patch and not have the toolbar model have a case for HTTP.
> 
> This prevents the toolbar model's code from being out of sync with what the UI
> actually shows, which is confusing for the reader.  Better to just update both
> places later when we add this.

Re-aligned with Mac CL.

groby-ooo-7-16

On 2016/08/18 16:09:36, Kevin Bailey wrote: > Just addressing your comments. It looks like some ...

4 years, 4 months ago (2016-08-18 22:24:19 UTC) #18

On 2016/08/18 16:09:36, Kevin Bailey wrote:
> Just addressing your comments. It looks like some things changed yet again, so
> there will be another round.

Naive question: Would it make sense to land this as a baseline, and then address
the changes as a second CL? It's not like we're shipping this as-is, and it
might make the review process a bit easier.

("No" is a perfectly fine answer - I was merely wondering if we're accidentally
creating a perma-CL)

>
https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
> File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right):
> 
>
https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:35: static
> constexpr int kOpenTimeMS = 150;
> On 2016/08/18 06:24:37, Peter Kasting wrote:
> > Nit: Should be protected, and can be defined in the .cc file and merely
> declared
> > in the .h file.
> 
> Made protected, but what's the advantage of moving to .cc ?
> 
>
https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
> File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):
> 
>
https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:25: #include
> "chrome/browser/ssl/chrome_security_state_model_client.h"
> On 2016/08/18 06:24:37, Peter Kasting wrote:
> > Nit: I think none of these new #includes are necessary anymore.
> 
> Done.
> 
>
https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:540: base::string16
> label = GetSecurityText();
> On 2016/08/18 06:24:37, Peter Kasting wrote:
> > Nit: |security_text| might be a better name.
> 
> If you prefer. I think once we cross line 540, it's more interesting to the
> reader that it will be a label.
> 
>
https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:809: if
> (ShouldShowSecurityChip() != showing_security_chip_) {
> On 2016/08/18 06:24:37, Peter Kasting wrote:
> > If it is safe to rely on the animation calls being no-ops when the state
isn't
> > changing (which is I think currently the case), we can simplify this. 
> > |showing_security_chip_| can be entirely removed.  We can rename
> > StartAnimation() and StopAnimation() to make it clearer they aren't always
> > called when the animation is stopped and started, respectively.  The result
is
> > that the code here just becomes:
> > 
> >   if (ShouldShowSecurityChip())
> >     location_icon_view_->ShowSecurityChip();
> >   else
> >     location_icon_view_->HideSecurityChip();
> 
> Done, but it looks like this whole thing needs to change again so, done for
now.
> 
>
https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:1075: if (level ==
> security_state::SecurityStateModel::SecurityLevel::EV_SECURE ||
> On 2016/08/18 06:24:37, Peter Kasting wrote:
> > Nit: Could just
> > 
> >   return level == ... ||
> >          level == ... ||
> >          level == ...;
> > 
> > I suggest a "using security_state::SecurityStateModel::SecurityLevel;"
inside
> > this function to reduce the verbosity of this statement.
> 
> I can go as far as SecurityStateModel, so I don't know that it helps much.
> 
>
https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:1078:
> security_state::SecurityStateModel::SecurityLevel::SECURITY_ERROR) {
> On 2016/08/18 06:24:37, Peter Kasting wrote:
> > What about SECURITY_WARNING and SECURITY_POLICY_WARNING?
> > 
> > Maybe this should return true iff ToolbarModelImpl::GetSecureVerboseText()
> > returns a nonempty string (see also comments in .grd file).
> 
> I asked spqchan why it wasn't handled in her CL and she said that, according
to
> Emily, we're not doing it yet. If the model simply isn't providing those enums
> yet, then we could reduce this to 'level != NONE'.
> 
>
https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:1081: return false;
> On 2016/08/18 06:24:37, Peter Kasting wrote:
> > Checking: Does the toolbar model return NONE any time we're editing, and
thus
> > automatically hide this in that case?
> > 
> > If so, I don't think we need the ShouldShowKeywordBubble() check at the top
> > since that can only show when editing.
> > 
> > If not, then I think the chip can be showing while editing, which isn't
right.
> 
> If the user starts editing after things have settled, the level instantly
drops
> (according to my logs.) If I changed the above to:
> 
> if (ShouldShowKeywordBubble() || omnibox_view_->IsEditingOrEmpty()) return
> false;
> 
> then, prior to it settling, it would be bouncing all over the place, which is
> one reason I added the earlier state machine. So I think for this particular
> case, we should leave it as is.
> 
>
https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
> File chrome/browser/ui/views/location_bar/location_bar_view.h (right):
> 
>
https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/location_bar_view.h:321: // Returns text
> describing the URL's security level, for the icon view.
> On 2016/08/18 06:24:37, Peter Kasting wrote:
> > Nit: icon view -> security chip?
> 
> I'm trying to tell the reader where it goes. If I say security chip, I'm not
> even sure that they can search for that.
> 
>
https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/location_bar_view.h:328: // page
> transition.
> On 2016/08/18 06:24:37, Peter Kasting wrote:
> > Nit: No longer accurate
> 
> Done.
> 
>
https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
> File chrome/browser/ui/views/location_bar/location_icon_view.h (right):
> 
>
https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/location_icon_view.h:58: double
> WidthMultiplier() const override;
> On 2016/08/18 06:24:37, Peter Kasting wrote:
> > Nit: These two overrides should be private, and they should be in their own
> > section, with a "// gfx::AnimationDelegate:" comment above it, above the
> > non-overrides.
> 
> Let me know if I misunderstood.
> 
>
https://codereview.chromium.org/2144903004/diff/160001/components/omnibox_str...
> File components/omnibox_strings.grdp (right):
> 
>
https://codereview.chromium.org/2144903004/diff/160001/components/omnibox_str...
> components/omnibox_strings.grdp:21: </message>
> On 2016/08/18 06:24:37, Peter Kasting wrote:
> > We don't currently show this string in the UI, so I would not include it in
> this
> > patch and not have the toolbar model have a case for HTTP.
> > 
> > This prevents the toolbar model's code from being out of sync with what the
UI
> > actually shows, which is confusing for the reader.  Better to just update
both
> > places later when we add this.
> 
> Re-aligned with Mac CL.

Justin Donnelly

On 2016/08/18 06:24:38, Peter Kasting wrote: > +CC maxwalker > > This seems mostly there, ...

4 years, 4 months ago (2016-08-19 14:34:29 UTC) #19

Kevin Bailey

Here is the patch to support not animating if the security level doesn't change.

4 years, 4 months ago (2016-08-19 18:07:45 UTC) #20

Peter Kasting

On 2016/08/19 14:34:29, Justin Donnelly wrote: > On 2016/08/18 06:24:38, Peter Kasting wrote: > > ...

4 years, 4 months ago (2016-08-19 22:28:06 UTC) #21

emilyschechter

On 2016/08/19 22:28:06, Peter Kasting wrote: > On 2016/08/19 14:34:29, Justin Donnelly wrote: > > ...

4 years, 4 months ago (2016-08-19 22:50:05 UTC) #22

On 2016/08/19 22:28:06, Peter Kasting wrote:
> On 2016/08/19 14:34:29, Justin Donnelly wrote:
> > On 2016/08/18 06:24:38, Peter Kasting wrote:
> > > +CC maxwalker
> > > 
> > > This seems mostly there, but I'm a bit uneasy that Max changed the spec
for
> > this
> > > in the time since I last saw it, so that instead of animating away (like
the
> > > content settings animation) or appearing only the first 1 or n times on
> > visiting
> > > a site, this is permanently visible every time.  This seems a bit
excessive
> to
> > > me; specifically:
> > > 
> > > * I'm worried about long translations (Malayalam) or narrow windows
reducing
> > the
> > > visibility of the origin (and rest of the URL)
> > > * I'm worried about whether this being present so frequently makes users
any
> > > less likely to pay attention to it (instead of it it appears occasionally)
> > > * I'm worried about visual noise of having More Stuff visible in the
omnibox
> > > commonly
> > > 
> > > I'd like to understand what led to that change.
> > 
> > This was direction from UI review, per emilyschechter@.
> 
> Added Emily to CC.  Emily, is there a doc or something somewhere that gives to
> rationale for this, especially wrt to the concerns above?

Yep! Totally get your concerns.

Re: verbose state triggering frequency: That was our original proposal for the
reasons you mentioned, but UI review decided against it. Their primary concern
was that users would not have a good mental model for when the verbose state
would appear, which would make the feature feel flaky. Please see
https://groups.google.com/a/google.com/forum/#!topic/chrome-ui-review/VfH_4PO...

Re: long translations, narrow device screens, origin visibility: We mocked long
translations on narrow windows, it's indeed an issue. The guidance there was to
not show verbose states at all for Touch Tablet Narrow and Touch Phone Narrow.
We also have a separate discussion to make sure that we're truncating
urls/verbose states correctly for windows that are too small (non-touch desktop
and Touch Phone).

Happy to provide any other clarifications.

Peter Kasting

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/views/location_bar/icon_label_bubble_view.h File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/views/location_bar/icon_label_bubble_view.h#newcode35 chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:35: static constexpr int kOpenTimeMS = 150; On 2016/08/18 16:09:36, ...

4 years, 4 months ago (2016-08-20 01:16:43 UTC) #23

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:35: static
constexpr int kOpenTimeMS = 150;
On 2016/08/18 16:09:36, Kevin Bailey wrote:
> On 2016/08/18 06:24:37, Peter Kasting wrote:
> > Nit: Should be protected, and can be defined in the .cc file and merely
> declared
> > in the .h file.
> 
> Made protected, but what's the advantage of moving to .cc ?

Not much, don't worry about it.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1075: if (level ==
security_state::SecurityStateModel::SecurityLevel::EV_SECURE ||
On 2016/08/18 16:09:36, Kevin Bailey wrote:
> On 2016/08/18 06:24:37, Peter Kasting wrote:
> > Nit: Could just
> > 
> >   return level == ... ||
> >          level == ... ||
> >          level == ...;
> > 
> > I suggest a "using security_state::SecurityStateModel::SecurityLevel;"
inside
> > this function to reduce the verbosity of this statement.
> 
> I can go as far as SecurityStateModel, so I don't know that it helps much.

Helps enough, probably.  A type alias might get you the rest of the way:

  using SecurityLevel = security_state::SecurityStateModel::SecurityLevel;

But as long as the conditional terms each fit on one line, you're probably good.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1081: return false;
On 2016/08/18 16:09:36, Kevin Bailey wrote:
> On 2016/08/18 06:24:37, Peter Kasting wrote:
> > Checking: Does the toolbar model return NONE any time we're editing, and
thus
> > automatically hide this in that case?
> > 
> > If so, I don't think we need the ShouldShowKeywordBubble() check at the top
> > since that can only show when editing.
> > 
> > If not, then I think the chip can be showing while editing, which isn't
right.
> 
> If the user starts editing after things have settled, the level instantly
drops
> (according to my logs.) If I changed the above to:
> 
> if (ShouldShowKeywordBubble() || omnibox_view_->IsEditingOrEmpty()) return
> false;
> 
> then, prior to it settling, it would be bouncing all over the place, which is
> one reason I added the earlier state machine. So I think for this particular
> case, we should leave it as is.

I don't understand this comment.

What does "prior to it settling, it would be bouncing around all over the place"
mean?  Can you describe the precise series of events you're thinking of?

I still don't understand how the ShouldShowKeywordBubble() check is relevant. 
The only way that something would "bounce around" without that conditional but
not with it is if we would otherwise be showing both the security chip and the
keyword bubble at the same time.  But if that's the case it seems like we
already have problems (as I was trying to say).

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:321: // Returns text
describing the URL's security level, for the icon view.
On 2016/08/18 16:09:36, Kevin Bailey wrote:
> On 2016/08/18 06:24:37, Peter Kasting wrote:
> > Nit: icon view -> security chip?
> 
> I'm trying to tell the reader where it goes. If I say security chip, I'm not
> even sure that they can search for that.

Well, we use the phrase "security chip" to describe the feature elsewhere in
this file, so we should be naming it the same.  If you don't think "security
chip" itself is obvious, and that needs to be described somewhere, that's fine,
but this doesn't seem like the place (and you're not actually explaining that
with the current comment either, because you don't mention "security chip").

Really, all the terminology needs to change.  In MD, we no longer have "bubbles"
to speak of for EV/security or keyword search.  We just have labels with a
divider next to them.  So maybe everything everywhere that talks about EV
bubble, security chip, keyword search bubble, etc. should all just say "security
label" and "keyword search label" or something.

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:817: }
I don't understand why this whole complicated block is needed.  What are you
trying to solve?  Does this have something to do with handling toggling between
tabs that are supposed to have different security states showing?  If so that
shouldn't be based on looking at the previous security state, it should be based
on whether this Update() is begin called for a tab switch or an update to the
current tab's state, which you can tell by whether |contents| is non-null.

Something like:

// Here, replacing this whole added chunk
  location_icon_view_->SetSecurityChipState(ShouldShowSecurityChip(),
!contents);

// In LocationIconView, replacing all the new methods there
  void SetSecurityChipState(bool showing, bool animate);

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:112:
SECURITY_CHIP_TEXT_AND_BORDER,
Nit: MD no longer puts a border around this, so might as well remove
_AND_BORDER.

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:172: void
LocationIconView::AnimateSecurityChip() {
Nit: .cc definition order must match .h declaration order.

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:185: return
animation_.is_animating() && animation_.GetCurrentValue() < 1;
Why is the second clause necessary?

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.h (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.h:72: //
gfx::AnimationDelegate:
Nit: The public section puts overrides above non-virtual functions, so do that
in the private section too.

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.h:74: 
Nit: No blank line

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.h:86: 
Nit: No blank line

https://codereview.chromium.org/2144903004/diff/200001/components/toolbar/too...
File components/toolbar/toolbar_model.h (right):

https://codereview.chromium.org/2144903004/diff/200001/components/toolbar/too...
components/toolbar/toolbar_model.h:77: // Returns text for the omnibox secure
verbose chip.
Seems like the changes in these files are copied from spqchan's.  If that CL is
expected to land first, just make this change dependent on that one.

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

https://codereview.chromium.org/2144903004/diff/200001/components/toolbar/too...
components/toolbar/toolbar_model_impl.cc:155: NOTREACHED();
This block is unreachable and can trigger warnings on Windows.  Simplest fix is
to entirely remove this.

Kevin Bailey

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/views/location_bar/location_bar_view.cc File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode1075 chrome/browser/ui/views/location_bar/location_bar_view.cc:1075: if (level == security_state::SecurityStateModel::SecurityLevel::EV_SECURE || On 2016/08/20 01:16:43, Peter ...

4 years, 4 months ago (2016-08-22 15:58:57 UTC) #24

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1075: if (level ==
security_state::SecurityStateModel::SecurityLevel::EV_SECURE ||
On 2016/08/20 01:16:43, Peter Kasting wrote:
> On 2016/08/18 16:09:36, Kevin Bailey wrote:
> > On 2016/08/18 06:24:37, Peter Kasting wrote:
> > > Nit: Could just
> > > 
> > >   return level == ... ||
> > >          level == ... ||
> > >          level == ...;
> > > 
> > > I suggest a "using security_state::SecurityStateModel::SecurityLevel;"
> inside
> > > this function to reduce the verbosity of this statement.
> > 
> > I can go as far as SecurityStateModel, so I don't know that it helps much.
> 
> Helps enough, probably.  A type alias might get you the rest of the way:
> 
>   using SecurityLevel = security_state::SecurityStateModel::SecurityLevel;
> 
> But as long as the conditional terms each fit on one line, you're probably
good.

Done.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1081: return false;
On 2016/08/20 01:16:43, Peter Kasting wrote:
> On 2016/08/18 16:09:36, Kevin Bailey wrote:
> > On 2016/08/18 06:24:37, Peter Kasting wrote:
> > > Checking: Does the toolbar model return NONE any time we're editing, and
> thus
> > > automatically hide this in that case?
> > > 
> > > If so, I don't think we need the ShouldShowKeywordBubble() check at the
top
> > > since that can only show when editing.
> > > 
> > > If not, then I think the chip can be showing while editing, which isn't
> right.
> > 
> > If the user starts editing after things have settled, the level instantly
> drops
> > (according to my logs.) If I changed the above to:
> > 
> > if (ShouldShowKeywordBubble() || omnibox_view_->IsEditingOrEmpty()) return
> > false;
> > 
> > then, prior to it settling, it would be bouncing all over the place, which
is
> > one reason I added the earlier state machine. So I think for this particular
> > case, we should leave it as is.
> 
> I don't understand this comment.
> 
> What does "prior to it settling, it would be bouncing around all over the
place"
> mean?  Can you describe the precise series of events you're thinking of?
> 
> I still don't understand how the ShouldShowKeywordBubble() check is relevant. 
> The only way that something would "bounce around" without that conditional but
> not with it is if we would otherwise be showing both the security chip and the
> keyword bubble at the same time.  But if that's the case it seems like we
> already have problems (as I was trying to say).

'omnibox_view_->IsEditingOrEmpty()' almost appears as an RNG in my log.
Therefore, to your original question, "Does the toolbar return NONE ...", I
don't believe that there's any correlation. It is neither, "If so..." nor "If
not...".

I am (now) not seeing 'ShouldShowKeywordBubble()' ever set, perhaps because
we're calling it in fewer places, so I'm taking it out.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:321: // Returns text
describing the URL's security level, for the icon view.
On 2016/08/20 01:16:43, Peter Kasting wrote:
> On 2016/08/18 16:09:36, Kevin Bailey wrote:
> > On 2016/08/18 06:24:37, Peter Kasting wrote:
> > > Nit: icon view -> security chip?
> > 
> > I'm trying to tell the reader where it goes. If I say security chip, I'm not
> > even sure that they can search for that.
> 
> Well, we use the phrase "security chip" to describe the feature elsewhere in
> this file, so we should be naming it the same.  If you don't think "security
> chip" itself is obvious, and that needs to be described somewhere, that's
fine,
> but this doesn't seem like the place (and you're not actually explaining that
> with the current comment either, because you don't mention "security chip").
> 
> Really, all the terminology needs to change.  In MD, we no longer have
"bubbles"
> to speak of for EV/security or keyword search.  We just have labels with a
> divider next to them.  So maybe everything everywhere that talks about EV
> bubble, security chip, keyword search bubble, etc. should all just say
"security
> label" and "keyword search label" or something.

I think my version would be a bit verbose compared to the other method
descriptions.

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:817: }
On 2016/08/20 01:16:43, Peter Kasting wrote:
> I don't understand why this whole complicated block is needed.  What are you
> trying to solve?  Does this have something to do with handling toggling
between
> tabs that are supposed to have different security states showing?

Correct. I assumed that you saw this in:

codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm

lines 621 on.

> If so that
> shouldn't be based on looking at the previous security state, it should be
based
> on whether this Update() is begin called for a tab switch or an update to the
> current tab's state, which you can tell by whether |contents| is non-null.
> 
> Something like:
> 
> // Here, replacing this whole added chunk
>   location_icon_view_->SetSecurityChipState(ShouldShowSecurityChip(),
> !contents);
> 
> // In LocationIconView, replacing all the new methods there
>   void SetSecurityChipState(bool showing, bool animate);

As I mentioned in a previous comment, I've never seen 'web_contents' not be
null, except at start-up.

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:112:
SECURITY_CHIP_TEXT_AND_BORDER,
On 2016/08/20 01:16:43, Peter Kasting wrote:
> Nit: MD no longer puts a border around this, so might as well remove
> _AND_BORDER.

Done, but to avoid having to follow a fickle UI, perhaps we should drop _TEXT
too.

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:172: void
LocationIconView::AnimateSecurityChip() {
On 2016/08/20 01:16:43, Peter Kasting wrote:
> Nit: .cc definition order must match .h declaration order.

I'm confused. It appears to match to me.

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:185: return
animation_.is_animating() && animation_.GetCurrentValue() < 1;
On 2016/08/20 01:16:43, Peter Kasting wrote:
> Why is the second clause necessary?

It isn't; I thought 'is_animating' was staying true longer than necessary, but
verified not.

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.h (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.h:72: //
gfx::AnimationDelegate:
On 2016/08/20 01:16:43, Peter Kasting wrote:
> Nit: The public section puts overrides above non-virtual functions, so do that
> in the private section too.

I moved them up and added an annotation.

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.h:74: 
On 2016/08/20 01:16:43, Peter Kasting wrote:
> Nit: No blank line

Might be ok after latest change.

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.h:86: 
On 2016/08/20 01:16:43, Peter Kasting wrote:
> Nit: No blank line

Done.

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

https://codereview.chromium.org/2144903004/diff/200001/components/toolbar/too...
components/toolbar/toolbar_model_impl.cc:155: NOTREACHED();
On 2016/08/20 01:16:43, Peter Kasting wrote:
> This block is unreachable and can trigger warnings on Windows.  Simplest fix
is
> to entirely remove this.

Done.

Peter Kasting

Didn't re-review, just replying. The primary thing that needs attention is the block in LocationBarView::Update(). ...

4 years, 4 months ago (2016-08-22 22:58:09 UTC) #25

Didn't re-review, just replying.

The primary thing that needs attention is the block in
LocationBarView::Update().

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1081: return false;
On 2016/08/22 15:58:56, Kevin Bailey wrote:
> On 2016/08/20 01:16:43, Peter Kasting wrote:
> > On 2016/08/18 16:09:36, Kevin Bailey wrote:
> > > On 2016/08/18 06:24:37, Peter Kasting wrote:
> > > > Checking: Does the toolbar model return NONE any time we're editing, and
> > thus
> > > > automatically hide this in that case?
> > > > 
> > > > If so, I don't think we need the ShouldShowKeywordBubble() check at the
> top
> > > > since that can only show when editing.
> > > > 
> > > > If not, then I think the chip can be showing while editing, which isn't
> > right.
> > > 
> > > If the user starts editing after things have settled, the level instantly
> > drops
> > > (according to my logs.) If I changed the above to:
> > > 
> > > if (ShouldShowKeywordBubble() || omnibox_view_->IsEditingOrEmpty()) return
> > > false;
> > > 
> > > then, prior to it settling, it would be bouncing all over the place, which
> is
> > > one reason I added the earlier state machine. So I think for this
particular
> > > case, we should leave it as is.
> > 
> > I don't understand this comment.
> > 
> > What does "prior to it settling, it would be bouncing around all over the
> place"
> > mean?  Can you describe the precise series of events you're thinking of?
> > 
> > I still don't understand how the ShouldShowKeywordBubble() check is
relevant. 
> > The only way that something would "bounce around" without that conditional
but
> > not with it is if we would otherwise be showing both the security chip and
the
> > keyword bubble at the same time.  But if that's the case it seems like we
> > already have problems (as I was trying to say).
> 
> 'omnibox_view_->IsEditingOrEmpty()' almost appears as an RNG in my log.
> Therefore, to your original question, "Does the toolbar return NONE ...", I
> don't believe that there's any correlation. It is neither, "If so..." nor "If
> not...".

ToolbarModel::GetSecurityLevel()'s argument is called |ignore_editing|.  If that
is false -- as it is here -- you can see from
https://cs.chromium.org/chromium/src/components/toolbar/toolbar_model_impl.cc...
that, when the omnibox is editing, we return NONE.

The reason you're getting confused is because you're looking at
IsEditingOrEmpty(), which also returns true when the omnibox is
empty-but-not-editing -- that is, on the New Tab Page.  The result of this is
not RNG; "editing" or "not editing and on NTP" are the only cases where it
should be true.

I think just removing the ShouldShowKeywordBubble() call was the right change
here.

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:321: // Returns text
describing the URL's security level, for the icon view.
On 2016/08/22 15:58:56, Kevin Bailey wrote:
> On 2016/08/20 01:16:43, Peter Kasting wrote:
> > On 2016/08/18 16:09:36, Kevin Bailey wrote:
> > > On 2016/08/18 06:24:37, Peter Kasting wrote:
> > > > Nit: icon view -> security chip?
> > > 
> > > I'm trying to tell the reader where it goes. If I say security chip, I'm
not
> > > even sure that they can search for that.
> > 
> > Well, we use the phrase "security chip" to describe the feature elsewhere in
> > this file, so we should be naming it the same.  If you don't think "security
> > chip" itself is obvious, and that needs to be described somewhere, that's
> fine,
> > but this doesn't seem like the place (and you're not actually explaining
that
> > with the current comment either, because you don't mention "security chip").
> > 
> > Really, all the terminology needs to change.  In MD, we no longer have
> "bubbles"
> > to speak of for EV/security or keyword search.  We just have labels with a
> > divider next to them.  So maybe everything everywhere that talks about EV
> > bubble, security chip, keyword search bubble, etc. should all just say
> "security
> > label" and "keyword search label" or something.
> 
> I think my version would be a bit verbose compared to the other method
> descriptions.

Never let that discourage you from leaving good comments, if they would actually
be useful!

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:817: }
On 2016/08/22 15:58:56, Kevin Bailey wrote:
> On 2016/08/20 01:16:43, Peter Kasting wrote:
> > I don't understand why this whole complicated block is needed.  What are you
> > trying to solve?  Does this have something to do with handling toggling
> between
> > tabs that are supposed to have different security states showing?
> 
> Correct. I assumed that you saw this in:
> 
>
codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
> 
> lines 621 on.

I left a similar comment there.  THanks for pointing me at it.

> > If so that
> > shouldn't be based on looking at the previous security state, it should be
> based
> > on whether this Update() is begin called for a tab switch or an update to
the
> > current tab's state, which you can tell by whether |contents| is non-null.
> > 
> > Something like:
> > 
> > // Here, replacing this whole added chunk
> >   location_icon_view_->SetSecurityChipState(ShouldShowSecurityChip(),
> > !contents);
> > 
> > // In LocationIconView, replacing all the new methods there
> >   void SetSecurityChipState(bool showing, bool animate);
> 
> As I mentioned in a previous comment, I've never seen 'web_contents' not be
> null, except at start-up.

If |contents| was always non-null here, we'd always call
omnibox_view_->OnTabChanged(contents); above, and that in turn would cause
noticeable glitches in the omnibox, e.g. reverting your edits when the
underlying page updated.

If you look at
https://cs.chromium.org/chromium/src/chrome/browser/ui/browser.cc?rcl=1471879...
, which is the thing that will indirectly call this, we only set the argument to
non-null when |should_restore_state| is true.  Checking the callers of that
function, they all pass false except ActiveTabChanged().

So I think you must have been looking at the wrong thing, or the wrong time, or
something.  Relying on whether this argument is null or not should be reliable.

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:112:
SECURITY_CHIP_TEXT_AND_BORDER,
On 2016/08/22 15:58:56, Kevin Bailey wrote:
> On 2016/08/20 01:16:43, Peter Kasting wrote:
> > Nit: MD no longer puts a border around this, so might as well remove
> > _AND_BORDER.
> 
> Done, but to avoid having to follow a fickle UI, perhaps we should drop _TEXT
> too.

I think leaving TEXT is good.  If that wasn't there it would sound like the
color for the whole UI, which I'd probably think of as the background color.

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:172: void
LocationIconView::AnimateSecurityChip() {
On 2016/08/22 15:58:56, Kevin Bailey wrote:
> On 2016/08/20 01:16:43, Peter Kasting wrote:
> > Nit: .cc definition order must match .h declaration order.
> 
> I'm confused. It appears to match to me.

The issue is primarily with the existing methods in this file.  SetBackground()
is currently defined last in this .cc, but in the .h there are private methods
after it.  As a result, when you add your methods here after SetBackground(),
that's correct relative to that function, but not relative to e.g.
GetMinimumSizeForPreferredSize().

What I would do here is send a quick separate CL that just takes the existing
.cc functions and reorders them to match the .h.  Then this CL can add your new
methods in the right spot.

Kevin Bailey

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/views/location_bar/location_bar_view.cc File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode1081 chrome/browser/ui/views/location_bar/location_bar_view.cc:1081: return false; On 2016/08/22 22:58:09, Peter Kasting wrote: > ...

4 years, 4 months ago (2016-08-23 15:17:13 UTC) #26

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1081: return false;
On 2016/08/22 22:58:09, Peter Kasting wrote:
> 
> ToolbarModel::GetSecurityLevel()'s argument is called |ignore_editing|.  If
that
> is false -- as it is here -- you can see from
>
https://cs.chromium.org/chromium/src/components/toolbar/toolbar_model_impl.cc...
> that, when the omnibox is editing, we return NONE.
> 
> The reason you're getting confused is because you're looking at
> IsEditingOrEmpty(), which also returns true when the omnibox is
> empty-but-not-editing -- that is, on the New Tab Page.  The result of this is
> not RNG; "editing" or "not editing and on NTP" are the only cases where it
> should be true.
> 
> I think just removing the ShouldShowKeywordBubble() call was the right change
> here.

You should see my log. The omnibox_view_ goes from empty to editing(partial
string) to editing(full string) back to empty and finally to not-editing(full
string). So, for example, if says that it's empty, it does not mean that we're
on a new tab.

Saying "RNG" is hyperbole but it's still unpredictable.

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:817: }
On 2016/08/22 22:58:09, Peter Kasting wrote:
> 
> If |contents| was always non-null here, we'd always call
> omnibox_view_->OnTabChanged(contents); above, and that in turn would cause
> noticeable glitches in the omnibox, e.g. reverting your edits when the
> underlying page updated.
> 
> If you look at
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/browser.cc?rcl=1471879...
> , which is the thing that will indirectly call this, we only set the argument
to
> non-null when |should_restore_state| is true.  Checking the callers of that
> function, they all pass false except ActiveTabChanged().
> 
> So I think you must have been looking at the wrong thing, or the wrong time,
or
> something.  Relying on whether this argument is null or not should be
reliable.

When the user presses back arrow or forwards, contents is null, and we have to
compare security levels to know what to do. The security model can transition
straight from secure to secure in this case. (I suspect we're on the same page;
just pointing out all these cases we need to handle.)

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:172: void
LocationIconView::AnimateSecurityChip() {
On 2016/08/22 22:58:09, Peter Kasting wrote:
> On 2016/08/22 15:58:56, Kevin Bailey wrote:
> > On 2016/08/20 01:16:43, Peter Kasting wrote:
> > > Nit: .cc definition order must match .h declaration order.
> > 
> > I'm confused. It appears to match to me.
> 
> The issue is primarily with the existing methods in this file. 
SetBackground()
> is currently defined last in this .cc, but in the .h there are private methods
> after it.  As a result, when you add your methods here after SetBackground(),
> that's correct relative to that function, but not relative to e.g.
> GetMinimumSizeForPreferredSize().
> 
> What I would do here is send a quick separate CL that just takes the existing
> .cc functions and reorders them to match the .h.  Then this CL can add your
new
> methods in the right spot.

The issue is entirely with the existing methods. I'll send a CL.

Peter Kasting

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/views/location_bar/location_bar_view.cc File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode1081 chrome/browser/ui/views/location_bar/location_bar_view.cc:1081: return false; On 2016/08/23 15:17:13, Kevin Bailey wrote: > ...

4 years, 4 months ago (2016-08-23 18:01:57 UTC) #27

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1081: return false;
On 2016/08/23 15:17:13, Kevin Bailey wrote:
> On 2016/08/22 22:58:09, Peter Kasting wrote:
> > 
> > ToolbarModel::GetSecurityLevel()'s argument is called |ignore_editing|.  If
> that
> > is false -- as it is here -- you can see from
> >
>
https://cs.chromium.org/chromium/src/components/toolbar/toolbar_model_impl.cc...
> > that, when the omnibox is editing, we return NONE.
> > 
> > The reason you're getting confused is because you're looking at
> > IsEditingOrEmpty(), which also returns true when the omnibox is
> > empty-but-not-editing -- that is, on the New Tab Page.  The result of this
is
> > not RNG; "editing" or "not editing and on NTP" are the only cases where it
> > should be true.
> > 
> > I think just removing the ShouldShowKeywordBubble() call was the right
change
> > here.
> 
> You should see my log. The omnibox_view_ goes from empty to editing(partial
> string) to editing(full string) back to empty and finally to not-editing(full
> string). So, for example, if says that it's empty, it does not mean that we're
> on a new tab.

Since I don't know what you're looking at to determine empty versus editing, how
you're getting these strings, or what states you're monitoring, I can't tell you
what you should expect to see.  But I can tell you that the code is not
unpredictable, because the behavior visible to the user is not unpredictable.  I
would have to walk through things with you step by step to be sure we were on
the same page.

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:817: }
On 2016/08/23 15:17:13, Kevin Bailey wrote:
> On 2016/08/22 22:58:09, Peter Kasting wrote:
> > 
> > If |contents| was always non-null here, we'd always call
> > omnibox_view_->OnTabChanged(contents); above, and that in turn would cause
> > noticeable glitches in the omnibox, e.g. reverting your edits when the
> > underlying page updated.
> > 
> > If you look at
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/browser.cc?rcl=1471879...
> > , which is the thing that will indirectly call this, we only set the
argument
> to
> > non-null when |should_restore_state| is true.  Checking the callers of that
> > function, they all pass false except ActiveTabChanged().
> > 
> > So I think you must have been looking at the wrong thing, or the wrong time,
> or
> > something.  Relying on whether this argument is null or not should be
> reliable.
> 
> When the user presses back arrow or forwards, contents is null,

Yes, because that's an update to the state of the existing tab.

> and we have to
> compare security levels to know what to do.

We don't need to look at the old level, and neither does the location icon view.
 All we need do is decide what the new state should be.  The animation handles
the fact that, if we're already in that state, we don't need to animate.

Kevin Bailey

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode817 chrome/browser/ui/views/location_bar/location_bar_view.cc:817: } On 2016/08/23 18:01:57, Peter Kasting wrote: > > ...

4 years, 4 months ago (2016-08-23 20:01:39 UTC) #28

Peter Kasting

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode817 chrome/browser/ui/views/location_bar/location_bar_view.cc:817: } On 2016/08/23 20:01:39, Kevin Bailey wrote: > On ...

4 years, 4 months ago (2016-08-23 20:11:00 UTC) #29

Kevin Bailey

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode817 chrome/browser/ui/views/location_bar/location_bar_view.cc:817: } On 2016/08/23 20:11:00, Peter Kasting wrote: > > ...

4 years, 4 months ago (2016-08-23 20:39:25 UTC) #30

Peter Kasting

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode817 chrome/browser/ui/views/location_bar/location_bar_view.cc:817: } On 2016/08/23 20:39:25, Kevin Bailey wrote: > On ...

4 years, 4 months ago (2016-08-23 20:42:20 UTC) #31

groby-ooo-7-16

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

4 years, 4 months ago (2016-08-23 22:11:14 UTC) #32

groby-ooo-7-16

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode817 chrome/browser/ui/views/location_bar/location_bar_view.cc:817: } On 2016/08/23 20:42:20, Peter Kasting wrote: > On ...

4 years, 4 months ago (2016-08-23 22:11:15 UTC) #33

Peter Kasting

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode817 chrome/browser/ui/views/location_bar/location_bar_view.cc:817: } On 2016/08/23 22:11:15, groby wrote: > On 2016/08/23 ...

4 years, 4 months ago (2016-08-23 22:17:18 UTC) #34

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:817: }
On 2016/08/23 22:11:15, groby wrote:
> On 2016/08/23 20:42:20, Peter Kasting wrote:
> > On 2016/08/23 20:39:25, Kevin Bailey wrote:
> > > On 2016/08/23 20:11:00, Peter Kasting wrote:
> > > > 
> > > > You always tell the icon to animate (or snap, if this is a tab switch)
to
> > the
> > > > state you want.  The animation object itself will convert this to a
no-op
> if
> > > you
> > > > e.g. call Show() when we're already animating open.
> > > 
> > > The UI designers want it such that, if you go from one level to the same
> > level,
> > > do not animate. I believe it is to avoid UI churn. This is why I believe
we
> > need
> > > to look at the previous level.
> > 
> > I'm confused.  That's what you should get with my proposal, no?  That's why
I
> > mentioned the no-oping in the animation layer.
> 
> I think the confusion here stems from a previous comment: "We don't need to
look
> at the old level, and neither does the location icon view."
> 
> But as I understand it, we do, to avoid unnecessary animation. E.g. if you
> navigate back/forth in the same tab, between https sites, we do not want to
> animate the icon. So somewhere we need to keep track of the current state of
the
> icon. I.e.:
> 
> LocationIconView::SetState(State security_state, bool animate) {
>  if (security_state == current_state_)
>     return;
>   
>   if (!animate)
>     SetState(state);
>   else
>     AnimateTo(state);
>   
>   ...
> }
> 
> I think that's what everybody in this debate is actually saying, and we're
> trying to read between each others lines. Peter: Is that what you had in mind?

No.  We don't need to track the previous level.  If you try to animate to a
state you're already at, the animation will automatically do nothing.  In other
words, if you took the sample code block you have above, and remove the initial
conditional check that early returns, the behavior will be the exact same as
with the check.

I think people are assuming that if you call SlideAnimation::Show(), it causes
the animation to run from 0 to 1.  But it doesn't.  It runs from whatever the
current value is to 1.  When the current value is 1, that means nothing happens.
 When the animation is already in the process of showing, it doesn't change
anything about that.  Only if the animation is stopped at a value other than the
target, or in the midst of animating the wrong direction, will the call have any
effect.

So all these comments have been me trying to say "rely on this".

groby-ooo-7-16

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode817 chrome/browser/ui/views/location_bar/location_bar_view.cc:817: } On 2016/08/23 22:17:18, Peter Kasting wrote: > On ...

4 years, 4 months ago (2016-08-23 23:12:40 UTC) #35

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:817: }
On 2016/08/23 22:17:18, Peter Kasting wrote:
> On 2016/08/23 22:11:15, groby wrote:
> > On 2016/08/23 20:42:20, Peter Kasting wrote:
> > > On 2016/08/23 20:39:25, Kevin Bailey wrote:
> > > > On 2016/08/23 20:11:00, Peter Kasting wrote:
> > > > > 
> > > > > You always tell the icon to animate (or snap, if this is a tab switch)
> to
> > > the
> > > > > state you want.  The animation object itself will convert this to a
> no-op
> > if
> > > > you
> > > > > e.g. call Show() when we're already animating open.
> > > > 
> > > > The UI designers want it such that, if you go from one level to the same
> > > level,
> > > > do not animate. I believe it is to avoid UI churn. This is why I believe
> we
> > > need
> > > > to look at the previous level.
> > > 
> > > I'm confused.  That's what you should get with my proposal, no?  That's
why
> I
> > > mentioned the no-oping in the animation layer.
> > 
> > I think the confusion here stems from a previous comment: "We don't need to
> look
> > at the old level, and neither does the location icon view."
> > 
> > But as I understand it, we do, to avoid unnecessary animation. E.g. if you
> > navigate back/forth in the same tab, between https sites, we do not want to
> > animate the icon. So somewhere we need to keep track of the current state of
> the
> > icon. I.e.:
> > 
> > LocationIconView::SetState(State security_state, bool animate) {
> >  if (security_state == current_state_)
> >     return;
> >   
> >   if (!animate)
> >     SetState(state);
> >   else
> >     AnimateTo(state);
> >   
> >   ...
> > }
> > 
> > I think that's what everybody in this debate is actually saying, and we're
> > trying to read between each others lines. Peter: Is that what you had in
mind?
> 
> No.  We don't need to track the previous level.  If you try to animate to a
> state you're already at, the animation will automatically do nothing.  In
other
> words, if you took the sample code block you have above, and remove the
initial
> conditional check that early returns, the behavior will be the exact same as
> with the check.
> 
> I think people are assuming that if you call SlideAnimation::Show(), it causes
> the animation to run from 0 to 1.  But it doesn't.  It runs from whatever the
> current value is to 1.  When the current value is 1, that means nothing
happens.
>  When the animation is already in the process of showing, it doesn't change
> anything about that.  Only if the animation is stopped at a value other than
the
> target, or in the midst of animating the wrong direction, will the call have
any
> effect.
> 
> So all these comments have been me trying to say "rely on this".

Thank you for clarifying that! That does cover the current behavior, I think.

Alas, UX currently calls for re-animating when states change.  From the spec:
"Navigating from one verbose state connection to another (e.g. secure to
malware) there would be an animation to transition between icons and text."

I'd prefer we figure that one out in a follow-up CL, though.

Peter Kasting

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode817 chrome/browser/ui/views/location_bar/location_bar_view.cc:817: } On 2016/08/23 23:12:40, groby wrote: > On 2016/08/23 ...

4 years, 4 months ago (2016-08-23 23:19:15 UTC) #36

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:817: }
On 2016/08/23 23:12:40, groby wrote:
> On 2016/08/23 22:17:18, Peter Kasting wrote:
> > On 2016/08/23 22:11:15, groby wrote:
> > > On 2016/08/23 20:42:20, Peter Kasting wrote:
> > > > On 2016/08/23 20:39:25, Kevin Bailey wrote:
> > > > > On 2016/08/23 20:11:00, Peter Kasting wrote:
> > > > > > 
> > > > > > You always tell the icon to animate (or snap, if this is a tab
switch)
> > to
> > > > the
> > > > > > state you want.  The animation object itself will convert this to a
> > no-op
> > > if
> > > > > you
> > > > > > e.g. call Show() when we're already animating open.
> > > > > 
> > > > > The UI designers want it such that, if you go from one level to the
same
> > > > level,
> > > > > do not animate. I believe it is to avoid UI churn. This is why I
believe
> > we
> > > > need
> > > > > to look at the previous level.
> > > > 
> > > > I'm confused.  That's what you should get with my proposal, no?  That's
> why
> > I
> > > > mentioned the no-oping in the animation layer.
> > > 
> > > I think the confusion here stems from a previous comment: "We don't need
to
> > look
> > > at the old level, and neither does the location icon view."
> > > 
> > > But as I understand it, we do, to avoid unnecessary animation. E.g. if you
> > > navigate back/forth in the same tab, between https sites, we do not want
to
> > > animate the icon. So somewhere we need to keep track of the current state
of
> > the
> > > icon. I.e.:
> > > 
> > > LocationIconView::SetState(State security_state, bool animate) {
> > >  if (security_state == current_state_)
> > >     return;
> > >   
> > >   if (!animate)
> > >     SetState(state);
> > >   else
> > >     AnimateTo(state);
> > >   
> > >   ...
> > > }
> > > 
> > > I think that's what everybody in this debate is actually saying, and we're
> > > trying to read between each others lines. Peter: Is that what you had in
> mind?
> > 
> > No.  We don't need to track the previous level.  If you try to animate to a
> > state you're already at, the animation will automatically do nothing.  In
> other
> > words, if you took the sample code block you have above, and remove the
> initial
> > conditional check that early returns, the behavior will be the exact same as
> > with the check.
> > 
> > I think people are assuming that if you call SlideAnimation::Show(), it
causes
> > the animation to run from 0 to 1.  But it doesn't.  It runs from whatever
the
> > current value is to 1.  When the current value is 1, that means nothing
> happens.
> >  When the animation is already in the process of showing, it doesn't change
> > anything about that.  Only if the animation is stopped at a value other than
> the
> > target, or in the midst of animating the wrong direction, will the call have
> any
> > effect.
> > 
> > So all these comments have been me trying to say "rely on this".
> 
> Thank you for clarifying that! That does cover the current behavior, I think.
> 
> Alas, UX currently calls for re-animating when states change.  From the spec:
> "Navigating from one verbose state connection to another (e.g. secure to
> malware) there would be an animation to transition between icons and text."
> 
> I'd prefer we figure that one out in a follow-up CL, though.

OK, that contradicts Kevin's earlier comment.

I would say let's get the CL in without that and we can decide whether to do it,
what the animation should look like, and how to implement that all later.

groby-ooo-7-16

On 2016/08/23 23:19:15, Peter Kasting wrote: > https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc > File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): > > https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode817 ...

4 years, 4 months ago (2016-08-23 23:26:09 UTC) #37

On 2016/08/23 23:19:15, Peter Kasting wrote:
>
https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
> File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):
> 
>
https://codereview.chromium.org/2144903004/diff/200001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:817: }
> On 2016/08/23 23:12:40, groby wrote:
> > On 2016/08/23 22:17:18, Peter Kasting wrote:
> > > On 2016/08/23 22:11:15, groby wrote:
> > > > On 2016/08/23 20:42:20, Peter Kasting wrote:
> > > > > On 2016/08/23 20:39:25, Kevin Bailey wrote:
> > > > > > On 2016/08/23 20:11:00, Peter Kasting wrote:
> > > > > > > 
> > > > > > > You always tell the icon to animate (or snap, if this is a tab
> switch)
> > > to
> > > > > the
> > > > > > > state you want.  The animation object itself will convert this to
a
> > > no-op
> > > > if
> > > > > > you
> > > > > > > e.g. call Show() when we're already animating open.
> > > > > > 
> > > > > > The UI designers want it such that, if you go from one level to the
> same
> > > > > level,
> > > > > > do not animate. I believe it is to avoid UI churn. This is why I
> believe
> > > we
> > > > > need
> > > > > > to look at the previous level.
> > > > > 
> > > > > I'm confused.  That's what you should get with my proposal, no? 
That's
> > why
> > > I
> > > > > mentioned the no-oping in the animation layer.
> > > > 
> > > > I think the confusion here stems from a previous comment: "We don't need
> to
> > > look
> > > > at the old level, and neither does the location icon view."
> > > > 
> > > > But as I understand it, we do, to avoid unnecessary animation. E.g. if
you
> > > > navigate back/forth in the same tab, between https sites, we do not want
> to
> > > > animate the icon. So somewhere we need to keep track of the current
state
> of
> > > the
> > > > icon. I.e.:
> > > > 
> > > > LocationIconView::SetState(State security_state, bool animate) {
> > > >  if (security_state == current_state_)
> > > >     return;
> > > >   
> > > >   if (!animate)
> > > >     SetState(state);
> > > >   else
> > > >     AnimateTo(state);
> > > >   
> > > >   ...
> > > > }
> > > > 
> > > > I think that's what everybody in this debate is actually saying, and
we're
> > > > trying to read between each others lines. Peter: Is that what you had in
> > mind?
> > > 
> > > No.  We don't need to track the previous level.  If you try to animate to
a
> > > state you're already at, the animation will automatically do nothing.  In
> > other
> > > words, if you took the sample code block you have above, and remove the
> > initial
> > > conditional check that early returns, the behavior will be the exact same
as
> > > with the check.
> > > 
> > > I think people are assuming that if you call SlideAnimation::Show(), it
> causes
> > > the animation to run from 0 to 1.  But it doesn't.  It runs from whatever
> the
> > > current value is to 1.  When the current value is 1, that means nothing
> > happens.
> > >  When the animation is already in the process of showing, it doesn't
change
> > > anything about that.  Only if the animation is stopped at a value other
than
> > the
> > > target, or in the midst of animating the wrong direction, will the call
have
> > any
> > > effect.
> > > 
> > > So all these comments have been me trying to say "rely on this".
> > 
> > Thank you for clarifying that! That does cover the current behavior, I
think.
> > 
> > Alas, UX currently calls for re-animating when states change.  From the
spec:
> > "Navigating from one verbose state connection to another (e.g. secure to
> > malware) there would be an animation to transition between icons and text."
> > 
> > I'd prefer we figure that one out in a follow-up CL, though.
> 
> OK, that contradicts Kevin's earlier comment.

I might well misunderstand, or UX direction has changed. Don't trust me :)

> I would say let's get the CL in without that and we can decide whether to do
it,
> what the animation should look like, and how to implement that all later.

+1

Kevin Bailey

Here is the version that does not look at security level changes, however, you should ...

4 years, 3 months ago (2016-08-25 18:01:21 UTC) #38

groby-ooo-7-16

On 2016/08/25 18:01:21, Kevin Bailey wrote: > Here is the version that does not look ...

4 years, 3 months ago (2016-08-26 01:16:38 UTC) #39

Peter Kasting

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/views/location_bar/location_bar_view.cc File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode811 chrome/browser/ui/views/location_bar/location_bar_view.cc:811: } AnimateSecurityChip() is unclear, because it doesn't say whether ...

4 years, 3 months ago (2016-08-26 20:59:13 UTC) #40

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:811: }
AnimateSecurityChip() is unclear, because it doesn't say whether we're showing
or hiding the chip.

I still think this API is not only more complicated but less clear than
something like:

Here:
  location_icon_view_->SetSecurityState(ShouldShowSecurityChip(), !contents);

In LocationIconView:
  // Triggers a security chip state change.  |showing| describes the desired new
  // state.  |updates_current_tab| is true if this is a change to the current
  // tab's state (as opposed to a switch to a different tab).
  LocationIconView::SetSecurityState(bool showing, bool updates_current_tab);

(You could also convert the second arg to an enum, which might be more clear, or
change the name or reverse the meaning of it.)

It's not just about having a smaller API surface, although that does matter. 
It's also about making the details of when and how the chip animates an
implementation detail inside LocationIconView, instead of having LocationBarView
control them.  LocationBarView is already extremely complicated, and the more
logic it can push into its children, the better.

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:323: base::string16
GetSecurityText() const;
Nit: Blank line below

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:328: // verified
security level.)
Nit: As a reader I'm not really sure what "verified" means.  Maybe something
along the lines of "Returns true when the current page is explicitly secure or
insecure.  In these cases we should show this state in the security chip."

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:166: void
LocationIconView::AnimationProgressed(const gfx::Animation*) {
Nit: These two functions' definition order is reversed from the declaration
order.

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.h (right):

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.h:60: bool
IsAnimatingSecurityChip();
This method seems to be unused.

Kevin Bailey

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/views/location_bar/location_bar_view.cc File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode811 chrome/browser/ui/views/location_bar/location_bar_view.cc:811: } On 2016/08/26 20:59:12, Peter Kasting wrote: > AnimateSecurityChip() ...

4 years, 3 months ago (2016-08-29 13:38:10 UTC) #41

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:811: }
On 2016/08/26 20:59:12, Peter Kasting wrote:
> AnimateSecurityChip() is unclear, because it doesn't say whether we're showing
> or hiding the chip.

That would be trivially fixed by adopting Sarah's name, AnimateIn.

> I still think this API is not only more complicated but less clear than
> something like:
> 
> Here:
>   location_icon_view_->SetSecurityState(ShouldShowSecurityChip(), !contents);

I'm sorry but you won't convince me that a parameter of '!contents' is more
clear than AnimateIn(). That you need enums to make it readable is proof of
this.

> In LocationIconView:
>   // Triggers a security chip state change.  |showing| describes the desired
new
>   // state.  |updates_current_tab| is true if this is a change to the current
>   // tab's state (as opposed to a switch to a different tab).
>   LocationIconView::SetSecurityState(bool showing, bool updates_current_tab);
> 
> (You could also convert the second arg to an enum, which might be more clear,
or
> change the name or reverse the meaning of it.)
> 
> It's not just about having a smaller API surface, although that does matter. 
> It's also about making the details of when and how the chip animates an
> implementation detail inside LocationIconView, instead of having
LocationBarView
> control them.  LocationBarView is already extremely complicated, and the more
> logic it can push into its children, the better.

You don't move lines from one file to another simply to make them balanced. You
put lines where they belong. If something is too complicated, one can move the
logic to a subroutine, which we also could have done here.

The problem with smearing responsibility across classes is that it requires the
reader to understand the whole collection to understand any of it. It makes it
so that a tiny change here causes a problem over there. It makes the classes
unusable elsewhere. Smearing undermines decoupling.

I hope you like my version. I think it at least draws a line between these 2
classes.

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:323: base::string16
GetSecurityText() const;
On 2016/08/26 20:59:12, Peter Kasting wrote:
> Nit: Blank line below

Done.

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:328: // verified
security level.)
On 2016/08/26 20:59:12, Peter Kasting wrote:
> Nit: As a reader I'm not really sure what "verified" means.  Maybe something
> along the lines of "Returns true when the current page is explicitly secure or
> insecure.  In these cases we should show this state in the security chip."

Done.

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.cc:166: void
LocationIconView::AnimationProgressed(const gfx::Animation*) {
On 2016/08/26 20:59:12, Peter Kasting wrote:
> Nit: These two functions' definition order is reversed from the declaration
> order.

Done.

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_icon_view.h (right):

https://codereview.chromium.org/2144903004/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_icon_view.h:60: bool
IsAnimatingSecurityChip();
On 2016/08/26 20:59:12, Peter Kasting wrote:
> This method seems to be unused.

Done.

Peter Kasting

Sure, this is fine. LGTM. https://codereview.chromium.org/2144903004/diff/260001/chrome/browser/ui/views/location_bar/location_icon_view.cc File chrome/browser/ui/views/location_bar/location_icon_view.cc (right): https://codereview.chromium.org/2144903004/diff/260001/chrome/browser/ui/views/location_bar/location_icon_view.cc#newcode153 chrome/browser/ui/views/location_bar/location_icon_view.cc:153: } else { Nit: ...

4 years, 3 months ago (2016-08-30 01:12:42 UTC) #42

Kevin Bailey

https://codereview.chromium.org/2144903004/diff/260001/chrome/browser/ui/views/location_bar/location_icon_view.cc File chrome/browser/ui/views/location_bar/location_icon_view.cc (right): https://codereview.chromium.org/2144903004/diff/260001/chrome/browser/ui/views/location_bar/location_icon_view.cc#newcode153 chrome/browser/ui/views/location_bar/location_icon_view.cc:153: } else { On 2016/08/30 01:12:42, Peter Kasting wrote: ...

4 years, 3 months ago (2016-08-30 14:07:15 UTC) #43

Kevin Bailey

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

4 years, 3 months ago (2016-08-30 16:38:47 UTC) #44