alternate ntp (cros/partial-win): add tab-related stuff and toolbar/tab background change

alternate ntp (cros/partial-win): add tab-related stuff and toolbar/tab background change

- add tracking of mode change to tab
- add fading in of gradient background for toolbar and tab 100ms after mode
  changes from NTP to SEARCH
  * cros implementation is complete, but win toolbar background is not done yet
- update toolbar image for NTP page
- modify to draw bookmark bar background like toolbar's

BUG=134533
TEST=none yet.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144604

[kuan, 2012-06-26 14:26:51]

i'll run tryjobs after the image cl is landed, but for now, ptal, thx!

[sky, 2012-06-26 17:11:27]

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
File chrome/browser/ui/search/toolbar_search_animator.cc (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:33: Reset(false /* don't
notify observers */, NULL);
Why do you need to reset in the destructor?

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:90: DCHECK(animation ==
background_animation_.get());
DCHECK_EQ

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:97: DCHECK(animation ==
background_animation_.get());
DCHECK_EG

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
File chrome/browser/ui/search/toolbar_search_animator.h (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:27: // Animate fade in of new
background.
Remove this sentence. Also, how about a description something like:
ToolbarSearchAnimator is used to track the background state of the toolbar and
related classes. To use ToolbarSearchAnimator add a
ToolbarSearchAnimatorObserver. The ToolbarSearchAnimatorObserver is then
notified appropriately.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:39:
BACKGROUND_STATE_SHOW_NON_APPLICABLE = 0,
Should this be BACKGROUND_STATE_SHOW_DEFAULT? Also, why the show in all of
these, eg BACKGROUND_STATE_DEFAULT, BACKGROUND_STATE_NTP are just as descriptive
and shorter:)

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:53: //
|new_background_opacity| contains a valid opacity value only if new
Should the opacity always be 1 when not default?

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:55: //
BACKGROUND_STATE_SHOW_NEW or BACKGROUND_STATE_SHOW_BOTH.
This comment isn't right, it can also by NON_APPLICABLE.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:62: int*
new_background_opacity) const;
I think it makes more sense to have opacity from 0-1 rather than 0-255.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:64: // Called from
SearchDelegate::StopObservingTab when a tab is deactivated or
nit: use () after method in comments.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
File chrome/browser/ui/search/toolbar_search_animator_observer.h (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator_observer.h:18: virtual void
BackgroundChanging() = 0;
These names should better match the interface:
OnToolbarBackgroundAnimatorProgressed/Completed/Canceled. Also, as far as I can
see no one needs to know the difference between Progressed/Completed, so nuke
Completed. If someone really needs to tell when done they can query for state.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc:278:
chrome::search::Mode mode =
I tend to think this code makes more sense in PaintToolbarBackground directly.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc:281: // If mode
is SEARCH, we might be waiting to fade in or fading in new
Update comment.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc:284: // Get
current state of background animation to paint for SEARCH mode.
Fold this into a better comment on 281.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:448: int
active_index = GetActiveIndex();
Might this happen when there is no active tab?

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_renderer_data.h (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_renderer_data.h:54: bool search_enabled;
search_enabled shouldn't be per tab. Put a method to determine if search is
enabled on TabController.

[dhollowa, 2012-06-26 18:01:24]

A couple high-level observations:
 1. In NTP mode the tab-strip color does not match the contents area color.
 2. In DEFAULT mode the tab-strip color does not match the bookmark bar color.

It should be pretty quick to add these color changes to the contents area and
the bookmarks - and worth it, so we can verify the visuals.

[dhollowa, 2012-06-26 18:16:27]

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
File chrome/browser/ui/search/toolbar_search_animator.cc (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:18: const int kMinOpacity =
0;
As per sky's comment, these should be floats from 0.0f - 1.0f.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
File chrome/browser/ui/search/toolbar_search_animator.h (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:19: }  // namespace ui
nit: Typically the trailing "// namespace ui" is omitted for forward
declarations.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:100: // Caches the search
model from browser.
// Weak.  Owned by Browser.  Non-null.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:109: // The timer to delay
start of animation after mode changes from NTP to
...from |MODE_NTP| to |MODE_SEARCH|.

[kuan, 2012-06-26 23:25:49]

i've addressed all but one comments in patch set 3 (which also has rebasing).

david, i addressed your 1st high-level comment i.e.  make NTP contents and
toolbar color the same.
however, i don't know enough about bookmark bar background to quickly figure out
your 2nd high-level comment.  can i do that after i land this cl?

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
File chrome/browser/ui/search/toolbar_search_animator.cc (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:18: const int kMinOpacity =
0;
On 2012/06/26 18:16:27, dhollowa wrote:
> As per sky's comment, these should be floats from 0.0f - 1.0f.

Done.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:33: Reset(false /* don't
notify observers */, NULL);
On 2012/06/26 17:11:27, sky wrote:
> Why do you need to reset in the destructor?

i did it to stop timer and animation, but i guess they wld stop when this class
is destructed?

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:90: DCHECK(animation ==
background_animation_.get());
On 2012/06/26 17:11:27, sky wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:97: DCHECK(animation ==
background_animation_.get());
On 2012/06/26 17:11:27, sky wrote:
> DCHECK_EG

Done.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
File chrome/browser/ui/search/toolbar_search_animator.h (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:19: }  // namespace ui
On 2012/06/26 18:16:27, dhollowa wrote:
> nit: Typically the trailing "// namespace ui" is omitted for forward
> declarations.

Done.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:27: // Animate fade in of new
background.
On 2012/06/26 17:11:27, sky wrote:
> Remove this sentence. Also, how about a description something like:
> ToolbarSearchAnimator is used to track the background state of the toolbar and
> related classes. To use ToolbarSearchAnimator add a
> ToolbarSearchAnimatorObserver. The ToolbarSearchAnimatorObserver is then
> notified appropriately.

Done.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:39:
BACKGROUND_STATE_SHOW_NON_APPLICABLE = 0,
On 2012/06/26 17:11:27, sky wrote:
> Should this be BACKGROUND_STATE_SHOW_DEFAULT? Also, why the show in all of
> these, eg BACKGROUND_STATE_DEFAULT, BACKGROUND_STATE_NTP are just as
descriptive
> and shorter:)

Done.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:53: //
|new_background_opacity| contains a valid opacity value only if new
On 2012/06/26 17:11:27, sky wrote:
> Should the opacity always be 1 when not default?

the opacity is for the new background, so it shouldn't be 1 when background
state is not DEFAULT until the last animation iteration - this value wld be 0-1
as the animation progresses.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:55: //
BACKGROUND_STATE_SHOW_NEW or BACKGROUND_STATE_SHOW_BOTH.
On 2012/06/26 17:11:27, sky wrote:
> This comment isn't right, it can also by NON_APPLICABLE.

Done.  i assume u mean the names of the states are wrong, which i corrected.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:62: int*
new_background_opacity) const;
On 2012/06/26 17:11:27, sky wrote:
> I think it makes more sense to have opacity from 0-1 rather than 0-255.

Done.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:64: // Called from
SearchDelegate::StopObservingTab when a tab is deactivated or
On 2012/06/26 17:11:27, sky wrote:
> nit: use () after method in comments.

Done.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:100: // Caches the search
model from browser.
On 2012/06/26 18:16:27, dhollowa wrote:
> // Weak.  Owned by Browser.  Non-null.

Done.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.h:109: // The timer to delay
start of animation after mode changes from NTP to
On 2012/06/26 18:16:27, dhollowa wrote:
> ...from |MODE_NTP| to |MODE_SEARCH|.

Done.  also changed for all comments in this file.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
File chrome/browser/ui/search/toolbar_search_animator_observer.h (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator_observer.h:18: virtual void
BackgroundChanging() = 0;
On 2012/06/26 17:11:27, sky wrote:
> These names should better match the interface:
> OnToolbarBackgroundAnimatorProgressed/Completed/Canceled. Also, as far as I
can
> see no one needs to know the difference between Progressed/Completed, so nuke
> Completed. If someone really needs to tell when done they can query for state.

Done.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc:278:
chrome::search::Mode mode =
On 2012/06/26 17:11:27, sky wrote:
> I tend to think this code makes more sense in PaintToolbarBackground directly.

if i move this code to PaintToolbarBackground, how do i call it twice to paint
NTP background and then SEARCH background?

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc:281: // If mode
is SEARCH, we might be waiting to fade in or fading in new
On 2012/06/26 17:11:27, sky wrote:
> Update comment.

Done.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc:284: // Get
current state of background animation to paint for SEARCH mode.
On 2012/06/26 17:11:27, sky wrote:
> Fold this into a better comment on 281.

Done.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:448: int
active_index = GetActiveIndex();
On 2012/06/26 17:11:27, sky wrote:
> Might this happen when there is no active tab?

Done.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_renderer_data.h (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_renderer_data.h:54: bool search_enabled;
On 2012/06/26 17:11:27, sky wrote:
> search_enabled shouldn't be per tab. Put a method to determine if search is
> enabled on TabController.

Done.  i assume u meant add the method all the way up including
TabStripController.

[sky, 2012-06-27 00:06:17]

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
File chrome/browser/ui/search/toolbar_search_animator.cc (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:33: Reset(false /* don't
notify observers */, NULL);
On 2012/06/26 23:25:49, kuan wrote:
> On 2012/06/26 17:11:27, sky wrote:
> > Why do you need to reset in the destructor?
> 
> i did it to stop timer and animation, but i guess they wld stop when this
class
> is destructed?

That's right.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc:278:
chrome::search::Mode mode =
On 2012/06/26 23:25:49, kuan wrote:
> On 2012/06/26 17:11:27, sky wrote:
> > I tend to think this code makes more sense in PaintToolbarBackground
directly.
> 
> if i move this code to PaintToolbarBackground, how do i call it twice to paint
> NTP background and then SEARCH background?

Good point. Leave it then.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_renderer_data.h (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_renderer_data.h:54: bool search_enabled;
On 2012/06/26 23:25:49, kuan wrote:
> On 2012/06/26 17:11:27, sky wrote:
> > search_enabled shouldn't be per tab. Put a method to determine if search is
> > enabled on TabController.
> 
> Done.  i assume u meant add the method all the way up including
> TabStripController.

I was thinking you would pass the state into the TabStrip so that it doesn't
need to be on TabStripController.

[kuan, 2012-06-27 00:50:03]

patch set 4 has:
- changes to address scott's comments
- fixes to paint bookmark bar like toolbar; in the process, moved
GetToolbarBackground* from BrowserNonClientFrame to BrowserView (which is
different from 1993)
- fixed build break in unittests.

ptal.  thanks.

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_renderer_data.h (right):

http://codereview.chromium.org/10662032/diff/3002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_renderer_data.h:54: bool search_enabled;
On 2012/06/27 00:06:17, sky wrote:
> On 2012/06/26 23:25:49, kuan wrote:
> > On 2012/06/26 17:11:27, sky wrote:
> > > search_enabled shouldn't be per tab. Put a method to determine if search
is
> > > enabled on TabController.
> > 
> > Done.  i assume u meant add the method all the way up including
> > TabStripController.
> 
> I was thinking you would pass the state into the TabStrip so that it doesn't
> need to be on TabStripController.

Done.

[dhollowa, 2012-06-27 01:15:47]

LGTM.  For search/ stuff.

[sky, 2012-06-27 04:18:36]

Make sure you look at the interactive_ui_test failures on windows. They look
real.

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/search/t...
File chrome/browser/ui/search/toolbar_search_animator_observer.h (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_observer.h:26: TabContents*
tab_contents) = 0;
The use of tab_contents here is confusing. Can you pass in an enum indicating
why the animation was canceled?

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:449:
DCHECK(active_index != -1);
Why both the DCHECK and the return? And why no DCHECK on 479? Shouldn't this
only be sent when the active index is valid? I think you should ensure that
happens by updating the toolbarsearchanimator before the tabstrip.

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:499: if (!tp) {
Just a DCHECK, no early return.

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_renderer_data.h (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_renderer_data.h:59: double
new_background_opacity;
new is confusing here. How about search_background_opacity. It's not entirely
correct, but new is ambiguous.

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_strip.h (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_strip.h:479: bool
instant_extended_api_enabled_;
const

[Ben Goodger (Google), 2012-06-27 04:21:21]

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/browser....
chrome/browser/ui/browser.h:276: return search_delegate_.get();
-2 spaces indent

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:499: if (!tp) {
On 2012/06/27 04:18:36, sky wrote:
> Just a DCHECK, no early return.

what's the purpose of dchecking when you immediately deref anyway?

[kuan, 2012-06-27 10:29:52]

i've addressed all but one comments in patch set 5 - i'm still trying to figure
out why interactive_ui_tests TagDragControllerTest on win fails.

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/browser....
chrome/browser/ui/browser.h:276: return search_delegate_.get();
On 2012/06/27 04:21:21, Ben Goodger (Google) wrote:
> -2 spaces indent

Done.

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/search/t...
File chrome/browser/ui/search/toolbar_search_animator_observer.h (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_observer.h:26: TabContents*
tab_contents) = 0;
On 2012/06/27 04:18:36, sky wrote:
> The use of tab_contents here is confusing. Can you pass in an enum indicating
> why the animation was canceled?

u mean ADD an enum like
ToolbarSearchAnimator::TAB_DEACTIVATED|TAB_DETACHED|TAB_CLOSING?
how should the observer use this enum?

BrowserTabStripObserver needs |tab_contents| to figure out the index of the
affected tab in the tab model so that the correct TabRendererData is updated, is
there another way?

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:449:
DCHECK(active_index != -1);
On 2012/06/27 04:18:36, sky wrote:
> Why both the DCHECK and the return? And why no DCHECK on 479? Shouldn't this
> only be sent when the active index is valid? I think you should ensure that
> happens by updating the toolbarsearchanimator before the tabstrip.

when reviewing the cl for 1993 repo, u asked me to make sure active index is not
-1, so i added DCHECK and returned if so.
this callback is not for ToolbarSearchAnimator, it's for SearchModelObserver.

@479, i didn't DCHECK if |tab_contents| doesn't exist in tab model, 'cos then
there's no TabRendererData to update.  this update is solely for the purpose of
painting the tab at 100% opacity if it's reactivated.  if the tab doesn't exist,
then there's no way to reactivate it. 

i apologize but i'm kinda confused what to modify here.

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_renderer_data.h (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_renderer_data.h:59: double
new_background_opacity;
On 2012/06/27 04:18:36, sky wrote:
> new is confusing here. How about search_background_opacity. It's not entirely
> correct, but new is ambiguous.

Done, also changed ToolbarSearchAnimator::GetCurrentBackgroundState and
everywhere else.

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_strip.h (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_strip.h:479: bool
instant_extended_api_enabled_;
On 2012/06/27 04:18:36, sky wrote:
> const

Done.

[kuan, 2012-06-27 13:11:33]

in patch set 6, i fixed the crash in interactive_ui_tests TabDragControllerTest
on win.  ptal.  thanks.

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:449:
DCHECK(active_index != -1);
On 2012/06/27 10:29:52, kuan wrote:
> On 2012/06/27 04:18:36, sky wrote:
> > Why both the DCHECK and the return? And why no DCHECK on 479? Shouldn't this
> > only be sent when the active index is valid? I think you should ensure that
> > happens by updating the toolbarsearchanimator before the tabstrip.
> 
> when reviewing the cl for 1993 repo, u asked me to make sure active index is
not
> -1, so i added DCHECK and returned if so.
> this callback is not for ToolbarSearchAnimator, it's for SearchModelObserver.
> 
> @479, i didn't DCHECK if |tab_contents| doesn't exist in tab model, 'cos then
> there's no TabRendererData to update.  this update is solely for the purpose
of
> painting the tab at 100% opacity if it's reactivated.  if the tab doesn't
exist,
> then there's no way to reactivate it. 
> 
> i apologize but i'm kinda confused what to modify here.

actually, it was when reviewing THIS cl that u asked me to make sure that active
index != 1, not the cl for 1993 repo.

[sky, 2012-06-27 14:11:44]

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/search/t...
File chrome/browser/ui/search/toolbar_search_animator_observer.h (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_observer.h:26: TabContents*
tab_contents) = 0;
On 2012/06/27 10:29:52, kuan wrote:
> On 2012/06/27 04:18:36, sky wrote:
> > The use of tab_contents here is confusing. Can you pass in an enum
indicating
> > why the animation was canceled?
> 
> u mean ADD an enum like
> ToolbarSearchAnimator::TAB_DEACTIVATED|TAB_DETACHED|TAB_CLOSING?
> how should the observer use this enum?
> 
> BrowserTabStripObserver needs |tab_contents| to figure out the index of the
> affected tab in the tab model so that the correct TabRendererData is updated,
is
> there another way?

AH, I see. In that case I don't understand why you ever need to pass in NULL?

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:449:
DCHECK(active_index != -1);
On 2012/06/27 13:11:33, kuan wrote:
> On 2012/06/27 10:29:52, kuan wrote:
> > On 2012/06/27 04:18:36, sky wrote:
> > > Why both the DCHECK and the return? And why no DCHECK on 479? Shouldn't
this
> > > only be sent when the active index is valid? I think you should ensure
that
> > > happens by updating the toolbarsearchanimator before the tabstrip.
> > 
> > when reviewing the cl for 1993 repo, u asked me to make sure active index is
> not
> > -1, so i added DCHECK and returned if so.
> > this callback is not for ToolbarSearchAnimator, it's for
SearchModelObserver.
> > 
> > @479, i didn't DCHECK if |tab_contents| doesn't exist in tab model, 'cos
then
> > there's no TabRendererData to update.  this update is solely for the purpose
> of
> > painting the tab at 100% opacity if it's reactivated.  if the tab doesn't
> exist,
> > then there's no way to reactivate it. 
> > 
> > i apologize but i'm kinda confused what to modify here.
> 
> actually, it was when reviewing THIS cl that u asked me to make sure that
active
> index != 1, not the cl for 1993 repo.

Ya, sorry. The confusion is on my part too. The question is, does the searchmode
only change when we have active tabs? If so, DCHECK, if it can change when there
are no active tabs than conditional return. Ask Dave for which it is.

[dhollowa, 2012-06-27 15:05:20]

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:449:
DCHECK(active_index != -1);
On 2012/06/27 14:11:44, sky wrote:
> On 2012/06/27 13:11:33, kuan wrote:
> > On 2012/06/27 10:29:52, kuan wrote:
> > > On 2012/06/27 04:18:36, sky wrote:
> > > > Why both the DCHECK and the return? And why no DCHECK on 479? Shouldn't
> this
> > > > only be sent when the active index is valid? I think you should ensure
> that
> > > > happens by updating the toolbarsearchanimator before the tabstrip.
> > > 
> > > when reviewing the cl for 1993 repo, u asked me to make sure active index
is
> > not
> > > -1, so i added DCHECK and returned if so.
> > > this callback is not for ToolbarSearchAnimator, it's for
> SearchModelObserver.
> > > 
> > > @479, i didn't DCHECK if |tab_contents| doesn't exist in tab model, 'cos
> then
> > > there's no TabRendererData to update.  this update is solely for the
purpose
> > of
> > > painting the tab at 100% opacity if it's reactivated.  if the tab doesn't
> > exist,
> > > then there's no way to reactivate it. 
> > > 
> > > i apologize but i'm kinda confused what to modify here.
> > 
> > actually, it was when reviewing THIS cl that u asked me to make sure that
> active
> > index != 1, not the cl for 1993 repo.
> 
> Ya, sorry. The confusion is on my part too. The question is, does the
searchmode
> only change when we have active tabs? If so, DCHECK, if it can change when
there
> are no active tabs than conditional return. Ask Dave for which it is.

Yes, when there are no tabs then no SearchModel changes can happen.  Model
changes only happen when the active tab changes, or there are changes to the
active tab.

[sky, 2012-06-27 16:34:32]

On Wed, Jun 27, 2012 at 8:05 AM,  <dhollowa@chromium.org> wrote:
>
>
http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
> File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc
> (right):
>
>
http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
> chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:449:
> DCHECK(active_index != -1);
> On 2012/06/27 14:11:44, sky wrote:
>>
>> On 2012/06/27 13:11:33, kuan wrote:
>> > On 2012/06/27 10:29:52, kuan wrote:
>> > > On 2012/06/27 04:18:36, sky wrote:
>> > > > Why both the DCHECK and the return? And why no DCHECK on 479?
>
> Shouldn't
>>
>> this
>> > > > only be sent when the active index is valid? I think you should
>
> ensure
>>
>> that
>> > > > happens by updating the toolbarsearchanimator before the
>
> tabstrip.
>>
>> > >
>> > > when reviewing the cl for 1993 repo, u asked me to make sure
>
> active index is
>>
>> > not
>> > > -1, so i added DCHECK and returned if so.
>> > > this callback is not for ToolbarSearchAnimator, it's for
>> SearchModelObserver.
>> > >
>> > > @479, i didn't DCHECK if |tab_contents| doesn't exist in tab
>
> model, 'cos
>>
>> then
>> > > there's no TabRendererData to update.  this update is solely for
>
> the purpose
>>
>> > of
>> > > painting the tab at 100% opacity if it's reactivated.  if the tab
>
> doesn't
>>
>> > exist,
>> > > then there's no way to reactivate it.
>> > >
>> > > i apologize but i'm kinda confused what to modify here.
>> >
>> > actually, it was when reviewing THIS cl that u asked me to make sure
>
> that
>>
>> active
>> > index != 1, not the cl for 1993 repo.
>
>
>> Ya, sorry. The confusion is on my part too. The question is, does the
>
> searchmode
>>
>> only change when we have active tabs? If so, DCHECK, if it can change
>
> when there
>>
>> are no active tabs than conditional return. Ask Dave for which it is.
>
>
> Yes, when there are no tabs then no SearchModel changes can happen.
> Model changes only happen when the active tab changes, or there are
> changes to the active tab.

Does that include the active tab changing to NULL when the last tab is removed?

  -Scott

[kuan, 2012-06-27 16:38:14]

scott, i've fixed a bug brought up by your comment in patch set 7.

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/search/t...
File chrome/browser/ui/search/toolbar_search_animator_observer.h (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_observer.h:26: TabContents*
tab_contents) = 0;
On 2012/06/27 14:11:44, sky wrote:
> On 2012/06/27 10:29:52, kuan wrote:
> > On 2012/06/27 04:18:36, sky wrote:
> > > The use of tab_contents here is confusing. Can you pass in an enum
> indicating
> > > why the animation was canceled?
> > 
> > u mean ADD an enum like
> > ToolbarSearchAnimator::TAB_DEACTIVATED|TAB_DETACHED|TAB_CLOSING?
> > how should the observer use this enum?
> > 
> > BrowserTabStripObserver needs |tab_contents| to figure out the index of the
> > affected tab in the tab model so that the correct TabRendererData is
updated,
> is
> > there another way?
> 
> AH, I see. In that case I don't understand why you ever need to pass in NULL?

aha.. i'm embarrassed to say that's a bug on my part :(
|tab_contents| is NULL when animation is canceled because of mode change (as
commented above).  i was firing cancelation notification, BUT the observer
BrowserTabStripController wasn't doing anything 'cos |tab_contents| is NULL. :(
i have fixed this in BrowserTabStripController to get active index if
|tab_contents| is NULL, and verified it manually and visually.
While BrowserTabStripController does get ModeChanged like ToolbarSearchAnimator,
the order of which gets notification first is not gauranteed; if
BrowserTabStripController gets ModeChanged before ToolbarSearchAnimator does,
the state in ToolbarSearchAnimator is not updated.  so i need to fire a
cancelation notification to make sure observers update their states accordingly.

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:449:
DCHECK(active_index != -1);
On 2012/06/27 15:05:20, dhollowa wrote:
> On 2012/06/27 14:11:44, sky wrote:
> > On 2012/06/27 13:11:33, kuan wrote:
> > > On 2012/06/27 10:29:52, kuan wrote:
> > > > On 2012/06/27 04:18:36, sky wrote:
> > > > > Why both the DCHECK and the return? And why no DCHECK on 479?
Shouldn't
> > this
> > > > > only be sent when the active index is valid? I think you should ensure
> > that
> > > > > happens by updating the toolbarsearchanimator before the tabstrip.
> > > > 
> > > > when reviewing the cl for 1993 repo, u asked me to make sure active
index
> is
> > > not
> > > > -1, so i added DCHECK and returned if so.
> > > > this callback is not for ToolbarSearchAnimator, it's for
> > SearchModelObserver.
> > > > 
> > > > @479, i didn't DCHECK if |tab_contents| doesn't exist in tab model, 'cos
> > then
> > > > there's no TabRendererData to update.  this update is solely for the
> purpose
> > > of
> > > > painting the tab at 100% opacity if it's reactivated.  if the tab
doesn't
> > > exist,
> > > > then there's no way to reactivate it. 
> > > > 
> > > > i apologize but i'm kinda confused what to modify here.
> > > 
> > > actually, it was when reviewing THIS cl that u asked me to make sure that
> > active
> > > index != 1, not the cl for 1993 repo.
> > 
> > Ya, sorry. The confusion is on my part too. The question is, does the
> searchmode
> > only change when we have active tabs? If so, DCHECK, if it can change when
> there
> > are no active tabs than conditional return. Ask Dave for which it is.
> 
> Yes, when there are no tabs then no SearchModel changes can happen.  Model
> changes only happen when the active tab changes, or there are changes to the
> active tab.

so i kept code as is: both DCHECK and conditional return.

[sky, 2012-06-27 17:00:48]

On Wed, Jun 27, 2012 at 9:38 AM,  <kuan@chromium.org> wrote:
> scott, i've fixed a bug brought up by your comment in patch set 7.
>
>
>
>
http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/search/t...
> File chrome/browser/ui/search/toolbar_search_animator_observer.h
> (right):
>
>
http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/search/t...
> chrome/browser/ui/search/toolbar_search_animator_observer.h:26:
> TabContents* tab_contents) = 0;
> On 2012/06/27 14:11:44, sky wrote:
>>
>> On 2012/06/27 10:29:52, kuan wrote:
>> > On 2012/06/27 04:18:36, sky wrote:
>> > > The use of tab_contents here is confusing. Can you pass in an enum
>> indicating
>> > > why the animation was canceled?
>> >
>> > u mean ADD an enum like
>> > ToolbarSearchAnimator::TAB_DEACTIVATED|TAB_DETACHED|TAB_CLOSING?
>> > how should the observer use this enum?
>> >
>> > BrowserTabStripObserver needs |tab_contents| to figure out the index
>
> of the
>>
>> > affected tab in the tab model so that the correct TabRendererData is
>
> updated,
>>
>> is
>> > there another way?
>
>
>> AH, I see. In that case I don't understand why you ever need to pass
>
> in NULL?
>
> aha.. i'm embarrassed to say that's a bug on my part :(
> |tab_contents| is NULL when animation is canceled because of mode change
> (as commented above).  i was firing cancelation notification, BUT the
> observer BrowserTabStripController wasn't doing anything 'cos
> |tab_contents| is NULL. :(
> i have fixed this in BrowserTabStripController to get active index if
> |tab_contents| is NULL, and verified it manually and visually.
> While BrowserTabStripController does get ModeChanged like
> ToolbarSearchAnimator, the order of which gets notification first is not
> gauranteed; if BrowserTabStripController gets ModeChanged before
> ToolbarSearchAnimator does, the state in ToolbarSearchAnimator is not
> updated.  so i need to fire a cancelation notification to make sure
> observers update their states accordingly.
>
>
>
http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
> File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc
> (right):
>
>
http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
> chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:449:
> DCHECK(active_index != -1);
> On 2012/06/27 15:05:20, dhollowa wrote:
>>
>> On 2012/06/27 14:11:44, sky wrote:
>> > On 2012/06/27 13:11:33, kuan wrote:
>> > > On 2012/06/27 10:29:52, kuan wrote:
>> > > > On 2012/06/27 04:18:36, sky wrote:
>> > > > > Why both the DCHECK and the return? And why no DCHECK on 479?
>
> Shouldn't
>>
>> > this
>> > > > > only be sent when the active index is valid? I think you
>
> should ensure
>>
>> > that
>> > > > > happens by updating the toolbarsearchanimator before the
>
> tabstrip.
>>
>> > > >
>> > > > when reviewing the cl for 1993 repo, u asked me to make sure
>
> active index
>>
>> is
>> > > not
>> > > > -1, so i added DCHECK and returned if so.
>> > > > this callback is not for ToolbarSearchAnimator, it's for
>> > SearchModelObserver.
>> > > >
>> > > > @479, i didn't DCHECK if |tab_contents| doesn't exist in tab
>
> model, 'cos
>>
>> > then
>> > > > there's no TabRendererData to update.  this update is solely for
>
> the
>>
>> purpose
>> > > of
>> > > > painting the tab at 100% opacity if it's reactivated.  if the
>
> tab doesn't
>>
>> > > exist,
>> > > > then there's no way to reactivate it.
>> > > >
>> > > > i apologize but i'm kinda confused what to modify here.
>> > >
>> > > actually, it was when reviewing THIS cl that u asked me to make
>
> sure that
>>
>> > active
>> > > index != 1, not the cl for 1993 repo.
>> >
>> > Ya, sorry. The confusion is on my part too. The question is, does
>
> the
>>
>> searchmode
>> > only change when we have active tabs? If so, DCHECK, if it can
>
> change when
>>
>> there
>> > are no active tabs than conditional return. Ask Dave for which it
>
> is.
>
>> Yes, when there are no tabs then no SearchModel changes can happen.
>
> Model
>>
>> changes only happen when the active tab changes, or there are changes
>
> to the
>>
>> active tab.
>
>
> so i kept code as is: both DCHECK and conditional return.
>
> http://codereview.chromium.org/10662032/

It sounds like you only need the DCHECK, but wait for final comment from Dave.

  -Scott

[dhollowa, 2012-06-27 17:54:58]

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/10662032/diff/25006/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:449:
DCHECK(active_index != -1);
On 2012/06/27 16:38:14, kuan wrote:
> On 2012/06/27 15:05:20, dhollowa wrote:
> > On 2012/06/27 14:11:44, sky wrote:
> > > On 2012/06/27 13:11:33, kuan wrote:
> > > > On 2012/06/27 10:29:52, kuan wrote:
> > > > > On 2012/06/27 04:18:36, sky wrote:
> > > > > > Why both the DCHECK and the return? And why no DCHECK on 479?
> Shouldn't
> > > this
> > > > > > only be sent when the active index is valid? I think you should
ensure
> > > that
> > > > > > happens by updating the toolbarsearchanimator before the tabstrip.
> > > > > 
> > > > > when reviewing the cl for 1993 repo, u asked me to make sure active
> index
> > is
> > > > not
> > > > > -1, so i added DCHECK and returned if so.
> > > > > this callback is not for ToolbarSearchAnimator, it's for
> > > SearchModelObserver.
> > > > > 
> > > > > @479, i didn't DCHECK if |tab_contents| doesn't exist in tab model,
'cos
> > > then
> > > > > there's no TabRendererData to update.  this update is solely for the
> > purpose
> > > > of
> > > > > painting the tab at 100% opacity if it's reactivated.  if the tab
> doesn't
> > > > exist,
> > > > > then there's no way to reactivate it. 
> > > > > 
> > > > > i apologize but i'm kinda confused what to modify here.
> > > > 
> > > > actually, it was when reviewing THIS cl that u asked me to make sure
that
> > > active
> > > > index != 1, not the cl for 1993 repo.
> > > 
> > > Ya, sorry. The confusion is on my part too. The question is, does the
> > searchmode
> > > only change when we have active tabs? If so, DCHECK, if it can change when
> > there
> > > are no active tabs than conditional return. Ask Dave for which it is.
> > 
> > Yes, when there are no tabs then no SearchModel changes can happen.  Model
> > changes only happen when the active tab changes, or there are changes to the
> > active tab.
> 
> so i kept code as is: both DCHECK and conditional return.

You can remove the conditional return.  |ModeChanged| only fires with non-null
tab, aka active_index != -1.

[kuan, 2012-06-27 18:02:26]

i've removed the conditional return in patch set 8.  ptal.  thanks.

[sky, 2012-06-27 21:23:47]

LGTM

http://codereview.chromium.org/10662032/diff/31008/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/10662032/diff/31008/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:449:
DCHECK(active_index != -1);
DCHECK_NE

http://codereview.chromium.org/10662032/diff/31008/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:461:
DCHECK(active_index != -1);
DCHECK_NE

http://codereview.chromium.org/10662032/diff/31008/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:462: if
(active_index == -1)
As discussed, remove this.

[kuan, 2012-06-27 21:31:39]

i've addressed all comments in patch set 9.

http://codereview.chromium.org/10662032/diff/31008/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/10662032/diff/31008/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:449:
DCHECK(active_index != -1);
On 2012/06/27 21:23:47, sky wrote:
> DCHECK_NE

Done.

http://codereview.chromium.org/10662032/diff/31008/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:461:
DCHECK(active_index != -1);
On 2012/06/27 21:23:47, sky wrote:
> DCHECK_NE

Done.

http://codereview.chromium.org/10662032/diff/31008/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:462: if
(active_index == -1)
On 2012/06/27 21:23:47, sky wrote:
> As discussed, remove this.

Done.

[kuan, 2012-06-27 21:50:57]

sending this to CQ, patch set 9 only has DCHECK changes, patch set 8 passes all
tryjobs - win, linux, mac and chromeos.

[commit-bot: I haz the power, 2012-06-27 21:51:46]

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

[commit-bot: I haz the power, 2012-06-27 23:17:23]

Change committed as 144604