views impl: hide top infobar arrow when omnibox popup shows

views impl: hide top infobar arrow when omnibox popup shows

BUG=174077
TEST=verify per bug rpt

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

[kuan, 2013-07-08 17:09:20]

hi peter, ptal.  thanks.

fyi, my initial fix was based on your recommendation (thanks for the pointer!) :
show/hide the top infobar arrow in LocationBarView::OnChanged() which is the
views implementation of OmniboxEditController::OnChanged().
however, it was triggered a lot, even when popup state doesn't change.  i
thought of adding a boolean data member to track the popup state change to
reduce calls to update infobar arrow.
but with more digging, i thought maybe it's better done through
OmniboxPopupContentsView::UpdatePopupAppearance(), where the popup state change
actually happens.

[Peter Kasting, 2013-07-08 19:08:27]

I don't think LocationBarView should really be involved in this transaction.  It
seems like the popup should either get a direct handle to the BrowserView or
else should fire a notification the BrowserView listens to.  Then I'd also leave
the BrowserView responsible for the actual Get/Set arrow height calls; its
interface to the external world should just be some
OnOmniboxPopupShownOrHidden() method or something.

[kuan, 2013-07-09 16:39:24]

i've implemented the notification approach in patch set 3.  ptal.  thx.

[kuan, 2013-07-09 16:39:34]

i mean patch set 2.

[Peter Kasting, 2013-07-09 17:17:23]

The new header file is not present in this CL.  Otherwise things look OK.

[kuan, 2013-07-09 17:29:13]

On 2013/07/09 17:17:23, Peter Kasting wrote:
> The new header file is not present in this CL.  Otherwise things look OK.

i've uploaded the new file in patch set 3.  ptal.  thx.

[Peter Kasting, 2013-07-09 17:35:20]

LGTM

https://codereview.chromium.org/18859004/diff/9001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_popup_model_observer.h (right):

https://codereview.chromium.org/18859004/diff/9001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_popup_model_observer.h:8: // This class
defines the observer interface for the |OmniboxPopupModel|.
This comment restates the obvious.  Instead, say why people might want this
functionality.

https://codereview.chromium.org/18859004/diff/9001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_popup_model_observer.h:15: virtual
~OmniboxPopupModelObserver() {}
Is this necessary?  I'd just omit this entirely.

[kuan, 2013-07-09 17:48:47]

i've addressed all comments, sending to cq now...

https://codereview.chromium.org/18859004/diff/9001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_popup_model_observer.h (right):

https://codereview.chromium.org/18859004/diff/9001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_popup_model_observer.h:8: // This class
defines the observer interface for the |OmniboxPopupModel|.
On 2013/07/09 17:35:20, Peter Kasting wrote:
> This comment restates the obvious.  Instead, say why people might want this
> functionality.

Done.

https://codereview.chromium.org/18859004/diff/9001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_popup_model_observer.h:15: virtual
~OmniboxPopupModelObserver() {}
On 2013/07/09 17:35:20, Peter Kasting wrote:
> Is this necessary?  I'd just omit this entirely.

Done.

[commit-bot: I haz the power, 2013-07-09 17:49:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/18859004/19001

[commit-bot: I haz the power, 2013-07-10 02:32:17]

Change committed as 210726