[OriginChip] Show the page info bubble on the location bar icon.

[OriginChip] Show the page info bubble on the location bar icon.

This also requires exposing a method indicating if the page info bubble is being shown, so that loss of focus doesn't cause the origin chip to reappear.

BUG=349067
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258750

[groby-ooo-7-16, 2014-03-12 19:53:46]

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_icon_view.cc:34: if
(location_bar_->GetOmniboxView()->IsEditingOrEmpty() ||
Random drive-by-question: Why not handle this in |page_info_helper_|?

[Peter Kasting, 2014-03-12 21:11:27]

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_icon_view.cc:34: if
(location_bar_->GetOmniboxView()->IsEditingOrEmpty() ||
On 2014/03/12 19:53:46, groby wrote:
> Random drive-by-question: Why not handle this in |page_info_helper_|?

Yeah, it's not clear to me why the code like this got moved from one class to
the other.

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/page_info_helper.cc (right):

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/page_info_helper.cc:36: NOTREACHED();
Nit: While here: Please fix this style violation by changing to just:

  DCHECK(nav_entry);

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:428:
!WebsiteSettingsPopupView::IsPopupShowing() &&
Adding this worries me because I'm concerned the origin chip won't automatically
reappear when the website settings popup disappears.

Why is this necessary anyway?  How can the website settings popup be showing if
the omnibox has focus and the origin chip is hidden?

[Justin Donnelly, 2014-03-14 15:27:51]

Grrr... I swore I mailed out these comments 2 days ago.

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_icon_view.cc:34: if
(location_bar_->GetOmniboxView()->IsEditingOrEmpty() ||
On 2014/03/12 21:11:27, Peter Kasting wrote:
> On 2014/03/12 19:53:46, groby wrote:
> > Random drive-by-question: Why not handle this in |page_info_helper_|?
> 
> Yeah, it's not clear to me why the code like this got moved from one class to
> the other.

PageInfoHelper is used in 3 places:
- The location icon in the location bar.
- The location icon in the origin chip.
- The EV bubble in the location bar.

The latter two don't care whether the omnibox is empty or being edited.  When
they appear they always represent the current content.  Having the check in
PageInfoHelper didn't allow for 1. always succeeding for the new origin chip and
2. conditionally succeeeding for the omnibox location icon, depending on
IsEditingOrEmpty().

So I moved the check here, which is the code path used by (and only by) the
location omnibox location icon.

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/page_info_helper.cc (right):

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/page_info_helper.cc:36: NOTREACHED();
On 2014/03/12 21:11:27, Peter Kasting wrote:
> Nit: While here: Please fix this style violation by changing to just:
> 
>   DCHECK(nav_entry);

Done.

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:428:
!WebsiteSettingsPopupView::IsPopupShowing() &&
On 2014/03/12 21:11:27, Peter Kasting wrote:
> Adding this worries me because I'm concerned the origin chip won't
automatically
> reappear when the website settings popup disappears.

The way this behaves (tested in Linux, CrOS and Windows) is that when the popup
is closed (either by click on the x, hitting esc, or clicking anywhere outside
the popup), focus reverts to the Omnibox.  Then the normal focus loss to reshow
the chip can proceed as before.

But I hear you that it hardly seems guaranteed that focus will always and
forever revert to the omnibox.  Other approaches I considered seemed more
complex and still potentially flakey.  Suggestions?

> Why is this necessary anyway?  How can the website settings popup be showing
if
> the omnibox has focus and the origin chip is hidden?

The point of this CL is to allow clicking on the location icon in the omnibox to
show the website settings after the origin chip is hidden by clicking on it.

[Peter Kasting, 2014-03-18 01:23:12]

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:428:
!WebsiteSettingsPopupView::IsPopupShowing() &&
On 2014/03/14 15:27:51, Justin Donnelly wrote:
> On 2014/03/12 21:11:27, Peter Kasting wrote:
> > Adding this worries me because I'm concerned the origin chip won't
> automatically
> > reappear when the website settings popup disappears.
> 
> The way this behaves (tested in Linux, CrOS and Windows) is that when the
popup
> is closed (either by click on the x, hitting esc, or clicking anywhere outside
> the popup), focus reverts to the Omnibox.  Then the normal focus loss to
reshow
> the chip can proceed as before.

What if we just left this line out?  If you've hidden the chip, but user input
isn't in progress, and you click the location icon, we revert back to showing
the chip (and show the popup window).

I dunno, maybe that feels lame.  If so, I can't think of anything better than
what you're doing here.

https://codereview.chromium.org/197623002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/197623002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_icon_view.cc:45: // showing the
page info instead.
This copy-and-paste makes me think we should have some common OnClickOrTap()
function which takes a const ui::LocatedEvent& and contains the common bits, and
then OnGestureEvent() can just do:

  if (event_type() != ui::ET_GESTURE_TAP)
    return;
  OnClickOrTap(*event);
  event->SetHandled();

https://codereview.chromium.org/197623002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/197623002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:55: bool
is_popup_showing = false;
Using a process-wide variable for this makes me nervous.  This means that if we
have two windows, and one is showing the popup, and the other window's omnibox
calls IsPopupShowing(), it will return true.  That seems wrong, though I'm not
sure in your current patch it will cause problems.  (Maybe.  You could test the
following: click the origin chip in window A, then click the location icon in
window B and see if the origin chip in A re-appears.)

I would prefer this be scoped to the window.

[Justin Donnelly, 2014-03-18 15:54:48]

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/197623002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:428:
!WebsiteSettingsPopupView::IsPopupShowing() &&
On 2014/03/18 01:23:13, Peter Kasting wrote:
> On 2014/03/14 15:27:51, Justin Donnelly wrote:
> > On 2014/03/12 21:11:27, Peter Kasting wrote:
> > > Adding this worries me because I'm concerned the origin chip won't
> > automatically
> > > reappear when the website settings popup disappears.
> > 
> > The way this behaves (tested in Linux, CrOS and Windows) is that when the
> popup
> > is closed (either by click on the x, hitting esc, or clicking anywhere
outside
> > the popup), focus reverts to the Omnibox.  Then the normal focus loss to
> reshow
> > the chip can proceed as before.
> 
> What if we just left this line out?  If you've hidden the chip, but user input
> isn't in progress, and you click the location icon, we revert back to showing
> the chip (and show the popup window).
> 
> I dunno, maybe that feels lame.  If so, I can't think of anything better than
> what you're doing here.

Yeah, I initially tried this.  It's not horrible, but it does feel a little
lame.

https://codereview.chromium.org/197623002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/197623002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_icon_view.cc:45: // showing the
page info instead.
On 2014/03/18 01:23:13, Peter Kasting wrote:
> This copy-and-paste makes me think we should have some common OnClickOrTap()
> function which takes a const ui::LocatedEvent& and contains the common bits,
and
> then OnGestureEvent() can just do:
> 
>   if (event_type() != ui::ET_GESTURE_TAP)
>     return;
>   OnClickOrTap(*event);
>   event->SetHandled();

Done.

https://codereview.chromium.org/197623002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/197623002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:55: bool
is_popup_showing = false;
On 2014/03/18 01:23:13, Peter Kasting wrote:
> Using a process-wide variable for this makes me nervous.  This means that if
we
> have two windows, and one is showing the popup, and the other window's omnibox
> calls IsPopupShowing(), it will return true.  That seems wrong, though I'm not
> sure in your current patch it will cause problems.  (Maybe.  You could test
the
> following: click the origin chip in window A, then click the location icon in
> window B and see if the origin chip in A re-appears.)

This works because the omnibox in window A loses focus and re-shows the chip
before window B opens the info popup.

> I would prefer this be scoped to the window.

SGTM, but it's not clear to me how to go about doing this effectively.  The
settings popup has a reference to an anchor view (the location icon, in this
case) but the there's no reference from the window or anything in it to the
settings popup.  This class could keep a static map between the anchor views and
the popups but that seems like overkill given that it wouldn't currently have
any visible effect.

Thoughts?  Did you have something else in mind?

[Peter Kasting, 2014-03-18 18:36:41]

On 2014/03/18 15:54:48, Justin Donnelly wrote:
> > I would prefer this be scoped to the window.
> 
> SGTM, but it's not clear to me how to go about doing this effectively.  The
> settings popup has a reference to an anchor view (the location icon, in this
> case) but the there's no reference from the window or anything in it to the
> settings popup.  This class could keep a static map between the anchor views
and
> the popups but that seems like overkill given that it wouldn't currently have
> any visible effect.
> 
> Thoughts?  Did you have something else in mind?

BrowserView::ShowWebsiteSettings() currently calls
WebsiteSettingsPopupView::ShowPopup().  If ShowPopup() returned a weak pointer
to the popup which BrowserView stored, then instead of adding
WebsiteSettingsPopupView::IsPopupShowing(), we could plumb the call from
omnibox_view_views.cc up to the BrowserView to check the weak pointer. 
Alternately we could shift the location the weak pointer was stored by making
ShowWebsiteSettings() return the weak pointer to its caller, and so forth
however many levels you'd want to pass the value back.

(We could also do a solution using a sort of observer pattern instead of a weak
pointer, it'd be effectively the same.)

Maybe this is too much complexity though.

[Peter Kasting, 2014-03-18 18:37:36]

LGTM, I trust whatever you decide on the popup issue

https://codereview.chromium.org/197623002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_icon_view.h (right):

https://codereview.chromium.org/197623002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_icon_view.h:30: void
OnClickOrTap(const ui::LocatedEvent& event);
Nit: This can be private, no?

[Justin Donnelly, 2014-03-21 20:07:46]

https://codereview.chromium.org/197623002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_icon_view.h (right):

https://codereview.chromium.org/197623002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_icon_view.h:30: void
OnClickOrTap(const ui::LocatedEvent& event);
On 2014/03/18 18:37:46, Peter Kasting wrote:
> Nit: This can be private, no?

Done.

[Justin Donnelly, 2014-03-21 20:08:32]

On 2014/03/18 18:36:41, Peter Kasting wrote:
> If ShowPopup() returned a weak pointer to the popup [...]

Thanks for the explanation, that makes sense.

> Maybe this is too much complexity though.

I think so. It's a lot of code to solve a problem that doesn't exist yet. But
I've added a note on this point in case it comes up in the future.

[Justin Donnelly, 2014-03-21 20:09:03]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-03-21 20:09:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/197623002/100001

[commit-bot: I haz the power, 2014-03-21 20:11:16]

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

[commit-bot: I haz the power, 2014-03-21 20:11:16]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[Justin Donnelly, 2014-03-21 20:34:58]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-03-21 20:37:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/197623002/100001

[commit-bot: I haz the power, 2014-03-22 03:01:57]

Change committed as 258750