alternate ntp: implement Show/HideBars API to reduce jank when showing/hiding bars

alternate ntp: implement Show/HideBars SearchBox API to reduce jank when showing/hiding bars

this is the chrome side combining melevin@'s renderer parts with my browser parts; gws side is implemented by melevin@.

- when suggestions show/hide, bookmark and info bars hide/show respectively; this causes contents to shift up/down respectively, resulting in jank.
- to reduce jank, these APIs synchronize the showing/hiding of suggestions in gws with the showing/hiding of bookmark and info bars in browser.
- gws sends HideBars() whenever suggestions show, and ShowBars() whenever they close.
- browser only obliges when bookmark and/or info bars are enabled and only on NTP and SERP pages, but always fires onbarshidden event for every HideBars() call, so that gws can proceed to show suggestions.

BUG=176146, 178550, 175776
TEST=verify as per bug rpts


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

[kuan, 2013-03-07 21:09:40]

if desired, pls split the review as follows:
- dhollowa: chrome/browser/ui/search
- pkasting: chrome/browser/ui/browser*.*, chrome/browser/infobars
- sreeram: chrome/renderer
- palmer: chrome/common/render_messages.h

thx.

[dhollowa, 2013-03-08 00:03:31]

https://codereview.chromium.org/12631008/diff/1/chrome/browser/infobars/infob...
File chrome/browser/infobars/infobar_container.h (right):

https://codereview.chromium.org/12631008/diff/1/chrome/browser/infobars/infob...
chrome/browser/infobars/infobar_container.h:113: // InstantOverlayModelObserver:
This should be moved down under the SearchModelObserver: overrides.

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/browser.cc#...
chrome/browser/ui/browser.cc:1810: // If new mode is |SEARCH_SUGGESTIONS| or
|SEARCH_RESULTS|, don't update
A note here mentioning the similar code in infobars would be helpful.  I.e. that
they should stay in sync.  A corresponding note in the infobar code would be
good too.

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/browser.cc#...
chrome/browser/ui/browser.cc:2143: #if !defined(OS_MACOSX)
Please add a note as to why this doesn't apply to Mac.

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/search/sear...
File chrome/browser/ui/search/search_model.h (right):

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search_model.h:29: void SetTopBarsVisible(bool
visible);
Please group this with show_top_bars().  Concepts group tighter than
getter/setter groups.

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/search/sear...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search_tab_helper.cc:86:
IPC_MESSAGE_HANDLER(ChromeViewHostMsg_SearchBoxShowBars,
All ChromeViewHostMsg_SearchBox* messages come through InstantPage.  I'd prefer
to not change that.  Plumb this through via InstantPage -> InstantController ->
SearchTabHelper instead.

Sreeram, I expect you'll agree?

https://codereview.chromium.org/12631008/diff/1/chrome/renderer/searchbox/sea...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/12631008/diff/1/chrome/renderer/searchbox/sea...
chrome/renderer/searchbox/searchbox.cc:158:
IPC_MESSAGE_HANDLER(ChromeViewMsg_SearchBoxBarsHidden, OnBarsHidden)
The lack of symmetry here seems a bit strange...  I.e. no OnBarsShown.  But if
it is not needed then fine.

[Peter Kasting, 2013-03-08 00:51:36]

https://codereview.chromium.org/12631008/diff/1/chrome/browser/infobars/infob...
File chrome/browser/infobars/infobar_container.cc (right):

https://codereview.chromium.org/12631008/diff/1/chrome/browser/infobars/infob...
chrome/browser/infobars/infobar_container.cc:178: const chrome::search::Mode&
new_mode) {
I'm not terribly thrilled by having three different functions which all do
similar-but-slightly-different-looking things to show and hide infobars in
response to various different instant signals.  It was already suboptimal with
two handlers, now it's worse.

Can we refactor this so that the infobar controller has a single "hide/show"
access point, with no knowledge of instant, and the instant system (e.g. the
instant controller) is responsible for calling this at the right times?

Even if we have to put some knowledge of instant into this class, we can at
least clean things up better.  I notice all three functions either hide or show
infobars, and they all use the same code to do each, always guarded by
conditional checks of |infobars_shown_| (possibly plus other things).  This
suggests that we ought to be able to at least  streamline these into calls to
some common core.  (But I'd still prefer for that core to be the only thing in
this file.)

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/browser.cc#...
chrome/browser/ui/browser.cc:1810: // If new mode is |SEARCH_SUGGESTIONS| or
|SEARCH_RESULTS|, don't update
On 2013/03/08 00:03:32, dhollowa wrote:
> A note here mentioning the similar code in infobars would be helpful.  I.e.
that
> they should stay in sync.  A corresponding note in the infobar code would be
> good too.

Better yet, move all this code out to a common location that explicitly tells
this code and the infobar controller what to do, and make this code (and that
code) dumb receivers of these explicit instructions instead of listeners for the
more low-level callbacks.

I already want that for the infobar code anyway (see comments there), the fact
that we have similar code here just suggests even more strongly that
centralizing these decisions in the instant controller or some similar place is
a good move.

[dhollowa, 2013-03-08 01:14:19]

https://codereview.chromium.org/12631008/diff/1/chrome/browser/infobars/infob...
File chrome/browser/infobars/infobar_container.cc (right):

https://codereview.chromium.org/12631008/diff/1/chrome/browser/infobars/infob...
chrome/browser/infobars/infobar_container.cc:178: const chrome::search::Mode&
new_mode) {
On 2013/03/08 00:51:36, Peter Kasting wrote:
> I'm not terribly thrilled by having three different functions which all do
> similar-but-slightly-different-looking things to show and hide infobars in
> response to various different instant signals.  It was already suboptimal with
> two handlers, now it's worse.
> 
> Can we refactor this so that the infobar controller has a single "hide/show"
> access point, with no knowledge of instant, and the instant system (e.g. the
> instant controller) is responsible for calling this at the right times?
> 
> Even if we have to put some knowledge of instant into this class, we can at
> least clean things up better.  I notice all three functions either hide or
show
> infobars, and they all use the same code to do each, always guarded by
> conditional checks of |infobars_shown_| (possibly plus other things).  This
> suggests that we ought to be able to at least  streamline these into calls to
> some common core.  (But I'd still prefer for that core to be the only thing in
> this file.)

+1

Kuan, I think we may be able to collapse these three.  Something like:
  1. Only have the single |ModeChanged| method
  2. Fold |visible| the |Mode| data structure, calling it |top_bar_visible|
maybe.

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/browser.cc#...
chrome/browser/ui/browser.cc:1810: // If new mode is |SEARCH_SUGGESTIONS| or
|SEARCH_RESULTS|, don't update
On 2013/03/08 00:51:36, Peter Kasting wrote:
> On 2013/03/08 00:03:32, dhollowa wrote:
> > A note here mentioning the similar code in infobars would be helpful.  I.e.
> that
> > they should stay in sync.  A corresponding note in the infobar code would be
> > good too.
> 
> Better yet, move all this code out to a common location that explicitly tells
> this code and the infobar controller what to do, and make this code (and that
> code) dumb receivers of these explicit instructions instead of listeners for
the
> more low-level callbacks.
> 
> I already want that for the infobar code anyway (see comments there), the fact
> that we have similar code here just suggests even more strongly that
> centralizing these decisions in the instant controller or some similar place
is
> a good move.

+1

Maybe a utility of the form: c::s::Visibility VisibilityChange(old_mode,
new_mod) {...}

Then here in the handlers:
  switch (VisibiltyChange(old_mode, new_mod))
    NO_CHANGE: return;
    SHOW: ...
    HIDE:
  }


  if (Vis

[palmer, 2013-03-11 18:03:08]

IPC security LGTM.

[kuan, 2013-03-12 18:00:37]

i've redesigned and refactored per david's and peter's comments.  ptal.  thx.

https://codereview.chromium.org/12631008/diff/1/chrome/browser/infobars/infob...
File chrome/browser/infobars/infobar_container.cc (right):

https://codereview.chromium.org/12631008/diff/1/chrome/browser/infobars/infob...
chrome/browser/infobars/infobar_container.cc:178: const chrome::search::Mode&
new_mode) {
On 2013/03/08 01:14:19, dhollowa wrote:
> On 2013/03/08 00:51:36, Peter Kasting wrote:
> > I'm not terribly thrilled by having three different functions which all do
> > similar-but-slightly-different-looking things to show and hide infobars in
> > response to various different instant signals.  It was already suboptimal
with
> > two handlers, now it's worse.
> > 
> > Can we refactor this so that the infobar controller has a single "hide/show"
> > access point, with no knowledge of instant, and the instant system (e.g. the
> > instant controller) is responsible for calling this at the right times?
> > 
> > Even if we have to put some knowledge of instant into this class, we can at
> > least clean things up better.  I notice all three functions either hide or
> show
> > infobars, and they all use the same code to do each, always guarded by
> > conditional checks of |infobars_shown_| (possibly plus other things).  This
> > suggests that we ought to be able to at least  streamline these into calls
to
> > some common core.  (But I'd still prefer for that core to be the only thing
in
> > this file.)
> 
> +1
> 
> Kuan, I think we may be able to collapse these three.  Something like:
>   1. Only have the single |ModeChanged| method
>   2. Fold |visible| the |Mode| data structure, calling it |top_bar_visible|
> maybe.

i've consolidated all 3 notifications into 1: ModelChanged.  however, as per our
offline discussion, i've kept |top_bars_visible_| as part of |SearchModel|, not
|Mode|, 'cos visibility needs to be handled different from |Mode::mode| and
|Mode::origin|.

https://codereview.chromium.org/12631008/diff/1/chrome/browser/infobars/infob...
chrome/browser/infobars/infobar_container.cc:178: const chrome::search::Mode&
new_mode) {
On 2013/03/08 00:51:36, Peter Kasting wrote:
> I'm not terribly thrilled by having three different functions which all do
> similar-but-slightly-different-looking things to show and hide infobars in
> response to various different instant signals.  It was already suboptimal with
> two handlers, now it's worse.
> 
> Can we refactor this so that the infobar controller has a single "hide/show"
> access point, with no knowledge of instant, and the instant system (e.g. the
> instant controller) is responsible for calling this at the right times?
> 
> Even if we have to put some knowledge of instant into this class, we can at
> least clean things up better.  I notice all three functions either hide or
show
> infobars, and they all use the same code to do each, always guarded by
> conditional checks of |infobars_shown_| (possibly plus other things).  This
> suggests that we ought to be able to at least  streamline these into calls to
> some common core.  (But I'd still prefer for that core to be the only thing in
> this file.)

i've consolidated all 3 different functions into just 1.

i decided not to have BrowserInstantController call InfoBarContainer to
show/hide bars, because closing the path from BrowserInstantcontroller to
InfoBarContainer requires either:
1) adding access to InfoBarContainer in base BrowserWindow and exposing a
show/hide method in InfoBarContainer, so that BrowserInstantController can call
them 
2) adding an InstantControllerObserver class that BrowsrInstantController fires
notifications to, with Browser and InfoBarContainer implementing the observer to
handle the notifications.

i feel that since we already have SearchModelObserver, having Browser and
InfobarContainer implement this observer is cleaner than creating a whole new
InstantControllerObserver that they still need to implement.  i think this is
also better than exposing InfoBarContainer in base BrowserWindow class.

https://codereview.chromium.org/12631008/diff/1/chrome/browser/infobars/infob...
File chrome/browser/infobars/infobar_container.h (right):

https://codereview.chromium.org/12631008/diff/1/chrome/browser/infobars/infob...
chrome/browser/infobars/infobar_container.h:113: // InstantOverlayModelObserver:
On 2013/03/08 00:03:32, dhollowa wrote:
> This should be moved down under the SearchModelObserver: overrides.

this method has been removed.

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/browser.cc#...
chrome/browser/ui/browser.cc:1810: // If new mode is |SEARCH_SUGGESTIONS| or
|SEARCH_RESULTS|, don't update
On 2013/03/08 01:14:19, dhollowa wrote:
> On 2013/03/08 00:51:36, Peter Kasting wrote:
> > On 2013/03/08 00:03:32, dhollowa wrote:
> > > A note here mentioning the similar code in infobars would be helpful. 
I.e.
> > that
> > > they should stay in sync.  A corresponding note in the infobar code would
be
> > > good too.
> > 
> > Better yet, move all this code out to a common location that explicitly
tells
> > this code and the infobar controller what to do, and make this code (and
that
> > code) dumb receivers of these explicit instructions instead of listeners for
> the
> > more low-level callbacks.
> > 
> > I already want that for the infobar code anyway (see comments there), the
fact
> > that we have similar code here just suggests even more strongly that
> > centralizing these decisions in the instant controller or some similar place
> is
> > a good move.
> 
> +1
> 
> Maybe a utility of the form: c::s::Visibility VisibilityChange(old_mode,
> new_mod) {...}
> 
> Then here in the handlers:
>   switch (VisibiltyChange(old_mode, new_mod))
>     NO_CHANGE: return;
>     SHOW: ...
>     HIDE:
>   }
> 
> 
>   if (Vis

i've provided a chrome::search::ShouldHandleTopBarsVisibilityChange utility
function that both Browser and InfoBarContainer classes call to determine the
logic.

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/browser.cc#...
chrome/browser/ui/browser.cc:2143: #if !defined(OS_MACOSX)
On 2013/03/08 00:03:32, dhollowa wrote:
> Please add a note as to why this doesn't apply to Mac.

Done.

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/search/sear...
File chrome/browser/ui/search/search_model.h (right):

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search_model.h:29: void SetTopBarsVisible(bool
visible);
On 2013/03/08 00:03:32, dhollowa wrote:
> Please group this with show_top_bars().  Concepts group tighter than
> getter/setter groups.

Done.

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/search/sear...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/12631008/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search_tab_helper.cc:86:
IPC_MESSAGE_HANDLER(ChromeViewHostMsg_SearchBoxShowBars,
On 2013/03/08 00:03:32, dhollowa wrote:
> All ChromeViewHostMsg_SearchBox* messages come through InstantPage.  I'd
prefer
> to not change that.  Plumb this through via InstantPage -> InstantController
->
> SearchTabHelper instead.
> 
> Sreeram, I expect you'll agree?

it was actually sreeram who suggested this needs to be handled in
SearchTabHelper, 'cos InstantPage is per-browser and only receives messages for
the current active tab, where SearchTabHelper is per-tab and will continue to
receive messages even after user switches tabs.  this allows us to keep and
reflect the most-updated state of the tab when user switches back to this tab.

https://codereview.chromium.org/12631008/diff/1/chrome/renderer/searchbox/sea...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/12631008/diff/1/chrome/renderer/searchbox/sea...
chrome/renderer/searchbox/searchbox.cc:158:
IPC_MESSAGE_HANDLER(ChromeViewMsg_SearchBoxBarsHidden, OnBarsHidden)
On 2013/03/08 00:03:32, dhollowa wrote:
> The lack of symmetry here seems a bit strange...  I.e. no OnBarsShown.  But if
> it is not needed then fine.

it doesn't seem to be needed, at least not yet :)

[dhollowa, 2013-03-13 17:21:55]

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/instant/se...
File chrome/browser/instant/search.h (right):

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/instant/se...
chrome/browser/instant/search.h:161: bool
ShouldHandleTopBarsVisibilityChange(const Mode& old_mode,
This should live in search_model.h/cc.  And how about renaming to
|ShouldChangeTopBarsVisibility|?

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_delegate.cc (right):

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_delegate.cc:26:
browser_model_->SetTopBarsVisible(top_bars_visible);
This is a potential problem (having two model-changes) because it will trigger
two observer notifications.  Better to add a new SearchModel::SetState(const
SearchModel::State& state) method that transfers all state in one go.

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_delegate.cc:35:
browser_model_->SetTopBarsVisible(tab_model_->top_bars_visible());
Ditto.

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_model.cc (right):

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.cc:45: top_bars_visible_ = visible;
Do an early return if there's no change to the visibility.

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.cc:46: 
Save off old_state_  here so the receiver can determine what changed.  i.e.:

  FOE(..., ModelChanged(old_state, state_));

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_model.h (right):

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.h:42: // The display mode of UI elements
such as the toolbar, the tab strip, etc.
For cleanliness, let's stick this model data into a struct, like:

 public:
  struct State {
    Mode mode;
    bool top_bars_visible;
  }
 private:
  State state_;

then in SMO...

 virtual void ModelChanged(
    const SearchModel::State& old_state,
    const SearchModel::State& new_state) = 0;

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_model_observer.h (right):

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model_observer.h:11: struct Mode;
Can get rid of this, and you'll need to include search_model.h because of the
inner struct |SearchModel::State|.

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model_observer.h:16: // Informs the observer
that the model has changed.
"... the model's state has changed."

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model_observer.h:17: virtual void
ModelChanged(const Mode& old_mode,
As mentioned:
virtual void ModelChanged(const SearchModel::State& old_state, const
SearchModel::State& new_state) = 0;

[kuan, 2013-03-13 22:21:06]

i've addressed david's comments in patch sets 5 & 6.  ptal.  thx.

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/instant/se...
File chrome/browser/instant/search.h (right):

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/instant/se...
chrome/browser/instant/search.h:161: bool
ShouldHandleTopBarsVisibilityChange(const Mode& old_mode,
On 2013/03/13 17:21:55, dhollowa wrote:
> This should live in search_model.h/cc.  And how about renaming to
> |ShouldChangeTopBarsVisibility|?

Done.

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_delegate.cc (right):

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_delegate.cc:26:
browser_model_->SetTopBarsVisible(top_bars_visible);
On 2013/03/13 17:21:55, dhollowa wrote:
> This is a potential problem (having two model-changes) because it will trigger
> two observer notifications.  Better to add a new SearchModel::SetState(const
> SearchModel::State& state) method that transfers all state in one go.

Done.

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_delegate.cc:35:
browser_model_->SetTopBarsVisible(tab_model_->top_bars_visible());
On 2013/03/13 17:21:55, dhollowa wrote:
> Ditto.

Done.

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_model.cc (right):

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.cc:45: top_bars_visible_ = visible;
On 2013/03/13 17:21:55, dhollowa wrote:
> Do an early return if there's no change to the visibility.

Done.

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.cc:46: 
On 2013/03/13 17:21:55, dhollowa wrote:
> Save off old_state_  here so the receiver can determine what changed.  i.e.:
> 
>   FOE(..., ModelChanged(old_state, state_));

Done.

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_model.h (right):

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.h:42: // The display mode of UI elements
such as the toolbar, the tab strip, etc.
On 2013/03/13 17:21:55, dhollowa wrote:
> For cleanliness, let's stick this model data into a struct, like:
> 
>  public:
>   struct State {
>     Mode mode;
>     bool top_bars_visible;
>   }
>  private:
>   State state_;
> 
> then in SMO...
> 
>  virtual void ModelChanged(
>     const SearchModel::State& old_state,
>     const SearchModel::State& new_state) = 0;

Done.

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_model_observer.h (right):

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model_observer.h:11: struct Mode;
On 2013/03/13 17:21:55, dhollowa wrote:
> Can get rid of this, and you'll need to include search_model.h because of the
> inner struct |SearchModel::State|.

Done.

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model_observer.h:16: // Informs the observer
that the model has changed.
On 2013/03/13 17:21:55, dhollowa wrote:
> "... the model's state has changed."

Done.

https://codereview.chromium.org/12631008/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model_observer.h:17: virtual void
ModelChanged(const Mode& old_mode,
On 2013/03/13 17:21:55, dhollowa wrote:
> As mentioned:
> virtual void ModelChanged(const SearchModel::State& old_state, const
> SearchModel::State& new_state) = 0;

Done.

[dhollowa, 2013-03-14 00:30:58]

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_model.cc (right):

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.cc:79: bool
SearchModel::ShouldChangeTopBarsVisibility(const State& old_state,
Public static methods go to top of class.

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_model.h (right):

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.h:30: // The visibility of top bars
(bookmark and info bars).
nit: Match comment style: "The visibility of top bars such as bookmark and info
bars."

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.h:49: // Set visibility of top bars
(bookmark and info bars).  Notifications are
You can remove the "(bookmark and info bars)" bit here.  It's redundant.

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.h:53: // Get the visibility of top bars
(bookmark and info bars).
You can remove the "(bookmark and info bars)" bit here.  It's redundant.

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.h:61: // |old_mode| and |new_mode|.
old_state, new_state

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.h:62: static bool
ShouldChangeTopBarsVisibility(const State& old_state,
Public static methods go to top of class.

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:65: UpdateModelMode();
UpdateMode would be fine.

[kuan, 2013-03-14 00:44:18]

i've addressed david's comments in patch set 7.  ptal.  thx.

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_model.cc (right):

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.cc:79: bool
SearchModel::ShouldChangeTopBarsVisibility(const State& old_state,
On 2013/03/14 00:30:58, dhollowa wrote:
> Public static methods go to top of class.

Done.

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_model.h (right):

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.h:30: // The visibility of top bars
(bookmark and info bars).
On 2013/03/14 00:30:58, dhollowa wrote:
> nit: Match comment style: "The visibility of top bars such as bookmark and
info
> bars."

Done.

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.h:49: // Set visibility of top bars
(bookmark and info bars).  Notifications are
On 2013/03/14 00:30:58, dhollowa wrote:
> You can remove the "(bookmark and info bars)" bit here.  It's redundant.

Done.

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.h:53: // Get the visibility of top bars
(bookmark and info bars).
On 2013/03/14 00:30:58, dhollowa wrote:
> You can remove the "(bookmark and info bars)" bit here.  It's redundant.

Done.

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.h:61: // |old_mode| and |new_mode|.
On 2013/03/14 00:30:58, dhollowa wrote:
> old_state, new_state

Done.

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.h:62: static bool
ShouldChangeTopBarsVisibility(const State& old_state,
On 2013/03/14 00:30:58, dhollowa wrote:
> Public static methods go to top of class.

Done.

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/12631008/diff/27006/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:65: UpdateModelMode();
On 2013/03/14 00:30:58, dhollowa wrote:
> UpdateMode would be fine.

Done.

[Peter Kasting, 2013-03-15 05:32:01]

I think we can simplify the infobar container's operation even more by just
making it not listen to notifications at all when infobars are hidden. 
Instructions below.

https://codereview.chromium.org/12631008/diff/2002/chrome/browser/infobars/in...
File chrome/browser/infobars/infobar_container.cc (right):

https://codereview.chromium.org/12631008/diff/2002/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.cc:78: // Hide infobars if search
model requires so.
Don't do this.

Instead, make this function begin as follows:

  infobar_service_ = infobar_service;

  if (!search_model_->top_bars_visible())
    return;

  ...everything from the existing function except the opening RemoveAll() call
and the already-done setting of infobar_service_...

Then add "registrar_.RemoveAll();" to the top of HideAllInfoBars(), and change
the beginning of Observe() to:

  // When infobars are hidden, we shouldn't be listening for notifications.
  DCHECK(infobars_shown_);

https://codereview.chromium.org/12631008/diff/2002/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.cc:186: new_state)) {
Nit: No {}

https://codereview.chromium.org/12631008/diff/2002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/12631008/diff/2002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:2126: (state != BookmarkBar::HIDDEN &&
!search_model_->top_bars_visible()))) {
Nit: You can simplify this by removing "state != BookmarkBar::HIDDEN && ", since
in that case, we'll just re-set the state from HIDDEN to HIDDEN and not change
anything.

[dhollowa, 2013-03-15 14:41:58]

lgtm

[kuan, 2013-03-15 15:53:48]

i've addressed peter's comments in patch set 13.  ptal.  thx.

https://codereview.chromium.org/12631008/diff/2002/chrome/browser/infobars/in...
File chrome/browser/infobars/infobar_container.cc (right):

https://codereview.chromium.org/12631008/diff/2002/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.cc:78: // Hide infobars if search
model requires so.
On 2013/03/15 05:32:01, Peter Kasting wrote:
> Don't do this.
> 
> Instead, make this function begin as follows:
> 
>   infobar_service_ = infobar_service;
> 
>   if (!search_model_->top_bars_visible())
>     return;
> 
>   ...everything from the existing function except the opening RemoveAll() call
> and the already-done setting of infobar_service_...
> 
> Then add "registrar_.RemoveAll();" to the top of HideAllInfoBars(), and change
> the beginning of Observe() to:
> 
>   // When infobars are hidden, we shouldn't be listening for notifications.
>   DCHECK(infobars_shown_);

Done.

https://codereview.chromium.org/12631008/diff/2002/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.cc:186: new_state)) {
On 2013/03/15 05:32:01, Peter Kasting wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/12631008/diff/2002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/12631008/diff/2002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:2126: (state != BookmarkBar::HIDDEN &&
!search_model_->top_bars_visible()))) {
On 2013/03/15 05:32:01, Peter Kasting wrote:
> Nit: You can simplify this by removing "state != BookmarkBar::HIDDEN && ",
since
> in that case, we'll just re-set the state from HIDDEN to HIDDEN and not change
> anything.

Done.

[kuan, 2013-03-15 16:32:16]

peter, i modified ChangeInfoBarService in patch set 14, see my reply below. 
ptal.  thx.

https://codereview.chromium.org/12631008/diff/2002/chrome/browser/infobars/in...
File chrome/browser/infobars/infobar_container.cc (right):

https://codereview.chromium.org/12631008/diff/2002/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.cc:78: // Hide infobars if search
model requires so.
On 2013/03/15 15:53:49, kuan wrote:
> On 2013/03/15 05:32:01, Peter Kasting wrote:
> > Don't do this.
> > 
> > Instead, make this function begin as follows:
> > 
> >   infobar_service_ = infobar_service;
> > 
> >   if (!search_model_->top_bars_visible())
> >     return;
> > 
> >   ...everything from the existing function except the opening RemoveAll()
call
> > and the already-done setting of infobar_service_...
> > 
> > Then add "registrar_.RemoveAll();" to the top of HideAllInfoBars(), and
change
> > the beginning of Observe() to:
> > 
> >   // When infobars are hidden, we shouldn't be listening for notifications.
> >   DCHECK(infobars_shown_);
> 
> Done.

i moved HideAllInfoBars() to start of function.  correct me if i'm wrong, but i
think we need to do this whenever tab is detached or changed, which is when this
function is called  from outside like BrowserView?

[Peter Kasting, 2013-03-16 00:40:59]

https://codereview.chromium.org/12631008/diff/107001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_container.cc (right):

https://codereview.chromium.org/12631008/diff/107001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_container.cc:48: HideAllInfoBars();
We don't need to do this here.  If the search model says the top bars aren't
visible, then we must have gotten a call to ModelChanged() in the past at the
time we wanted to make the bars invisible, which would have hidden all the bars.
 Since then, we've never re-added any bars because this function will early-exit
and Observe() won't be reached (since we're not listening to any notifications).
 So there will never be any bars visible in that case, hence the early exit
before calling HideAllBars() is OK.

[kuan, 2013-03-18 15:37:29]

https://codereview.chromium.org/12631008/diff/107001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_container.cc (right):

https://codereview.chromium.org/12631008/diff/107001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_container.cc:48: HideAllInfoBars();
On 2013/03/16 00:40:59, Peter Kasting wrote:
> We don't need to do this here.  If the search model says the top bars aren't
> visible, then we must have gotten a call to ModelChanged() in the past at the
> time we wanted to make the bars invisible, which would have hidden all the
bars.
>  Since then, we've never re-added any bars because this function will
early-exit
> and Observe() won't be reached (since we're not listening to any
notifications).
>  So there will never be any bars visible in that case, hence the early exit
> before calling HideAllBars() is OK.

switching between tabs doesn't work correctly if i don't call HideAllInfoBars().
this is the scenario i tried:
1) forced a crash infobar
2) open 1st ntp, crash infobar shows: expected
3) open 2nd ntp, crash infobar still shows: this is incorrect - crash infobar
shld not show up for subsequent tabs
4) switch back to 1st tab: ChangeInfoBarService crashes due to duplicate
registration when adding notifications to registrar. 

the scenario doesn't trigger or involve ModelChanged from 1993. 
ChangeInfoBarService is called at steps 2 to 4 from
BrowserView::ActiveTabChanged, with search_model_->top_bars_visible() being
true.

[Peter Kasting, 2013-03-18 19:10:30]

On 2013/03/18 15:37:29, kuan wrote:
> switching between tabs doesn't work correctly if i don't call
HideAllInfoBars().

You are supposed to call HideAllInfoBars().  You're just supposed to call it
where it was previously being called, not at the top of the function.  Are you
doing this?  If you removed the call entirely, you'd get behavior like what
you're describing, but that isn't the desired change.

[kuan, 2013-03-19 00:17:10]

On 2013/03/18 19:10:30, Peter Kasting wrote:
> On 2013/03/18 15:37:29, kuan wrote:
> > switching between tabs doesn't work correctly if i don't call
> HideAllInfoBars().
> 
> You are supposed to call HideAllInfoBars().  You're just supposed to call it
> where it was previously being called, not at the top of the function.  Are you
> doing this?  If you removed the call entirely, you'd get behavior like what
> you're describing, but that isn't the desired change.

i'm sorry, that was my bad, i completely removed HideAllInfoBars() :(

however, i've encountered another scenario that requires HideAllInfoBars() to be
called at the start of ChangeInfoBarService.

1) force crash infobar, open 1st ntp, crash infobar shows
2) open 2nd ntp, go to google.de to force a translate infobar
3) switch to 1st tab, type query (crash infobar disappears), hit enter on a
query suggestion
4) while the suggestions are animating close (i'm using a demo gws server that
slows the suggestions animation), switch to 2nd tab, which shows translate
infobar, expected
5) switch back to 1st tab: suggestions are still animating close, but tab shows
translate infobar: incorrect - 1st tab shld hv crash infobar, and infobars
shldn't even show
6) when suggestions finish fading out in 1st tab, the crash infobar replaces the
translate infobar.

at step 4, ChangeInfoBarService tries to show translate infobar for 2nd tab,
which is successful.
at step 5, ChangeInfoBarService tries to show infobars for 1st tab, but infobars
have been set invisible in step 3.  if we don't HideAllInfoBars() before the
early exit, InfoBarContainer still has the translate infobar of 2nd tab, and
ends up showing it.
at step 6, ModelChanged fires to show infobars, so ChangeInfoBarService then
correctly switches to show the crash infobar.

[Peter Kasting, 2013-03-19 02:34:53]

OK, that's a good reason to do it like you have it.  LGTM.

I would add the following comment above the HideInfoBars() call:

We have to call HideAllInfoBars() here and not after the early exit below, to
handle the case where we're switching from a tab with visible infobars to one
where the SearchModel directs us to hide infobars.

[kuan, 2013-03-19 02:52:24]

peter, i've added your comment to the code.  thx!

one more lgtm to go... sreeram.. ptal.  thx!

[sreeram, 2013-03-19 03:06:50]

lgtm

[commit-bot: I haz the power, 2013-03-19 09:04:52]

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

[commit-bot: I haz the power, 2013-03-19 09:05:04]

Failed to apply patch for chrome/browser/ui/search/search_tab_helper.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/search/search_tab_helper.cc
  Hunk #1 FAILED at 5.
  1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/ui/search/search_tab_helper.cc.rej

Patch:       chrome/browser/ui/search/search_tab_helper.cc
Index: chrome/browser/ui/search/search_tab_helper.cc
diff --git a/chrome/browser/ui/search/search_tab_helper.cc
b/chrome/browser/ui/search/search_tab_helper.cc
index
c65d13d7562942f9cfd329061ad72a47aee6bbe0..097e38396606a18b748b26001a44670c40087e97
100644
--- a/chrome/browser/ui/search/search_tab_helper.cc
+++ b/chrome/browser/ui/search/search_tab_helper.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/search/search_tab_helper.h"
 
 #include "chrome/browser/instant/search.h"
+#include "chrome/common/render_messages.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
@@ -35,7 +36,8 @@ namespace chrome {
 namespace search {
 
 SearchTabHelper::SearchTabHelper(content::WebContents* web_contents)
-    : is_search_enabled_(chrome::search::IsInstantExtendedAPIEnabled()),
+    : WebContentsObserver(web_contents),
+      is_search_enabled_(chrome::search::IsInstantExtendedAPIEnabled()),
       user_input_in_progress_(false),
       web_contents_(web_contents) {
   if (!is_search_enabled_)
@@ -60,14 +62,14 @@ void SearchTabHelper::OmniboxEditModelChanged(bool
user_input_in_progress,
   if (!user_input_in_progress && !cancelling)
     return;
 
-  UpdateModel();
+  UpdateMode();
 }
 
 void SearchTabHelper::NavigationEntryUpdated() {
   if (!is_search_enabled_)
     return;
 
-  UpdateModel();
+  UpdateMode();
 }
 
 void SearchTabHelper::Observe(
@@ -75,10 +77,22 @@ void SearchTabHelper::Observe(
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK_EQ(content::NOTIFICATION_NAV_ENTRY_COMMITTED, type);
-  UpdateModel();
+  UpdateMode();
 }
 
-void SearchTabHelper::UpdateModel() {
+bool SearchTabHelper::OnMessageReceived(const IPC::Message& message) {
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(SearchTabHelper, message)
+    IPC_MESSAGE_HANDLER(ChromeViewHostMsg_SearchBoxShowBars,
+                        OnSearchBoxShowBars)
+    IPC_MESSAGE_HANDLER(ChromeViewHostMsg_SearchBoxHideBars,
+                        OnSearchBoxHideBars)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
+}
+
+void SearchTabHelper::UpdateMode() {
   Mode::Type type = Mode::MODE_DEFAULT;
   Mode::Origin origin = Mode::ORIGIN_DEFAULT;
   if (IsNTP(web_contents_)) {
@@ -93,5 +107,17 @@ void SearchTabHelper::UpdateModel() {
   model_.SetMode(Mode(type, origin));
 }
 
+void SearchTabHelper::OnSearchBoxShowBars(int page_id) {
+  if (web_contents()->IsActiveEntry(page_id))
+    model_.SetTopBarsVisible(true);
+}
+
+void SearchTabHelper::OnSearchBoxHideBars(int page_id) {
+  if (web_contents()->IsActiveEntry(page_id)) {
+    model_.SetTopBarsVisible(false);
+    Send(new ChromeViewMsg_SearchBoxBarsHidden(routing_id()));
+  }
+}
+
 }  // namespace search
 }  // namespace chrome

[commit-bot: I haz the power, 2013-03-19 09:28:39]

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

[commit-bot: I haz the power, 2013-03-19 11:23:10]

Change committed as 188994