alternate ntp: toolbar background and separator animation

alternate ntp: toolbar/tab background animation

- modify SearchModelObserver::ModeChanged callback to pass old and new modes, so that toolbar/tab background animation is only triggered when transitioning from NTP to SEARCH, and not other transition to SEARCH
- in the process, simplify methods to get background opacity; callers need not worry about search modes when calling the API; the returned opacity tells them what to do
- enhance ToolbarSearchAnimator per discussion with dhollowa@
- added unittest to ToolbarSearchAnimator

BUG=137223
TEST=verify per bug rpt.


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

[dhollowa, 2012-07-24 00:24:49]

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

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:63: default:
You can remove |default:|.

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:76: default:
You can remove |default:|.

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:98: // the default
initialized mode of |DEFAULT| to NTP or SEARCH mode, e.g. to
nit: ...|NTP| or |SEARCH|...  and various comments below.

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:101: from_mode_ = mode.mode;
nit: from_mode_ = to_mode_ = mode.mode;

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:114: if (mode.animate &&
from_mode_ == Mode::MODE_NTP && mode.is_search() &&
This would read more clearly if you used s/mode.is_search()/to_mode_ ==
Mode::MODE_NTP/.

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:138: ((from_mode_ !=
Mode::MODE_DEFAULT && mode.is_default()) ||
You're guaranteed that from_mode_ is not equal to to_mode_, right?  So this
clause could simply be |to_mode_ == Mode::MODE_DEFAULT|.

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:142: from_mode_ =
old_to_mode;
This conditional setting of the |from_mode_| variable seems fragile and
complicated.  Would it simplify things if the SearchModelObserver interface gave
it to you instead?  Seems like it might.

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

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator_observer.h:17: // toollbar
gradient background.
nit: toolbar (spelling)

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator_observer.h:30: // toolbar
separator.
"the toolbar separator"

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator_observer.h:33: // Called when
toolbar separator animation is canceled and jumps to the end
"the toolbar separator"

[kuan, 2012-07-30 23:21:01]

i've addressed dhollowa's comments in patch set 5, including our offline
discussions.  ptal.  thx.

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

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:63: default:
On 2012/07/24 00:24:49, dhollowa wrote:
> You can remove |default:|.

doesn't build if removed: "control reaches end of non-void function".

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:76: default:
On 2012/07/24 00:24:49, dhollowa wrote:
> You can remove |default:|.

ditto.

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:98: // the default
initialized mode of |DEFAULT| to NTP or SEARCH mode, e.g. to
On 2012/07/24 00:24:49, dhollowa wrote:
> nit: ...|NTP| or |SEARCH|...  and various comments below.

Done.

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:101: from_mode_ = mode.mode;
On 2012/07/24 00:24:49, dhollowa wrote:
> nit: from_mode_ = to_mode_ = mode.mode;

Done.

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:114: if (mode.animate &&
from_mode_ == Mode::MODE_NTP && mode.is_search() &&
On 2012/07/24 00:24:49, dhollowa wrote:
> This would read more clearly if you used s/mode.is_search()/to_mode_ ==
> Mode::MODE_NTP/.

Done.

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:138: ((from_mode_ !=
Mode::MODE_DEFAULT && mode.is_default()) ||
On 2012/07/24 00:24:49, dhollowa wrote:
> You're guaranteed that from_mode_ is not equal to to_mode_, right?  So this
> clause could simply be |to_mode_ == Mode::MODE_DEFAULT|.

Done.

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator.cc:142: from_mode_ =
old_to_mode;
On 2012/07/24 00:24:49, dhollowa wrote:
> This conditional setting of the |from_mode_| variable seems fragile and
> complicated.  Would it simplify things if the SearchModelObserver interface
gave
> it to you instead?  Seems like it might.

Done.

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

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator_observer.h:17: // toollbar
gradient background.
On 2012/07/24 00:24:49, dhollowa wrote:
> nit: toolbar (spelling)

Done.

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator_observer.h:30: // toolbar
separator.
On 2012/07/24 00:24:49, dhollowa wrote:
> "the toolbar separator"

Done.

http://codereview.chromium.org/10816027/diff/5001/chrome/browser/ui/search/to...
chrome/browser/ui/search/toolbar_search_animator_observer.h:33: // Called when
toolbar separator animation is canceled and jumps to the end
On 2012/07/24 00:24:49, dhollowa wrote:
> "the toolbar separator"

Done.

[kuan, 2012-08-01 21:13:14]

hi david and scott,

while waiting for review, i've discovered (from other chrome code) that i can
differentiate between animation end and animation cancelation by simply setting
and clearing delegate, so i've simplified my animation code to take the same
approach and remove AnimateState - one less state to keep track of.
ptal.  thx!

[dhollowa, 2012-08-01 22:26:22]

Looking good.  As discussed we should explore whether to animate later in the
page-load cycle.

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_model_observer_bridge.h (right):

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_model_observer_bridge.h:31: newMode:new_mode];
nit: this should all fit on one line.

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
File chrome/browser/ui/search/toolbar_search_animator.cc (right):

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator.cc:84: // If mode transitions
from |NTP| to |SEARCH| and we're not animating
"If the mode"...

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

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_observer.h:31: virtual void
OnToolbarSeparatorAnimatorProgressed() {}
Should be pure virtual.  ... = 0;

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_observer.h:35: virtual void
OnToolbarSeparatorAnimatorCanceled() {}
Should be pure virtual.  ... = 0;

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
File chrome/browser/ui/search/toolbar_search_animator_unittest.cc (right):

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_unittest.cc:43: class
ToolbarSearchAnimatorTestObserver : public ToolbarSearchAnimatorObserver {
Please include this in the anonymous namespace.

[dhollowa, 2012-08-01 22:41:59]

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
File chrome/browser/ui/search/toolbar_search_animator_unittest.cc (right):

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_unittest.cc:101: class
ToolbarSearchAnimatorTest : public BrowserWithTestWindowTest {
I noticed that the tests take 4+s to run:
[----------] 11 tests from ToolbarSearchAnimatorTest (4091 ms total)

This is too much.  I would suggest a private method to set the animation times
shorter during tests.

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_unittest.cc:371:
Times(AtLeast(3));
Are these |AtLeast(...)| calls dependent on the animation times used?  If so,
this is probably overly fragile and may be flaky.  If we can guarantee
|AtLeast(1)| with *any* animation duration, then fine, otherwise we should find
a deterministic test that is independent of duration.

[dhollowa, 2012-08-02 17:43:02]

sky: After discussions with Marcin, this CL may change substantially.  So please
hold-off reviewing until further notice.  Thanks.

[kuan, 2012-08-02 21:54:02]

in patch set 11, i've
- addressed dhollowa's comments
- removed all separator fading code (per discussion w/ marcin)
- in SearchTabHelper, delay SearchModel's mode update from
NavigateToPendingEntry to receipt of  NOTIFICATION_NAV_ENTRY_COMMITTED
notification, removing the former in the process.
ptal.  thx.

fyi, the toolbar separator won't fade in or out - it would pop in when the
Suggestions retract upwards; i'll work on that in a future cl.

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_model_observer_bridge.h (right):

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_model_observer_bridge.h:31: newMode:new_mode];
On 2012/08/01 22:26:23, dhollowa wrote:
> nit: this should all fit on one line.

Done.

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
File chrome/browser/ui/search/toolbar_search_animator.cc (right):

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator.cc:84: // If mode transitions
from |NTP| to |SEARCH| and we're not animating
On 2012/08/01 22:26:23, dhollowa wrote:
> "If the mode"...

Done.

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

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_observer.h:31: virtual void
OnToolbarSeparatorAnimatorProgressed() {}
On 2012/08/01 22:26:23, dhollowa wrote:
> Should be pure virtual.  ... = 0;

method is removed.

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_observer.h:35: virtual void
OnToolbarSeparatorAnimatorCanceled() {}
On 2012/08/01 22:26:23, dhollowa wrote:
> Should be pure virtual.  ... = 0;

ditto.

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
File chrome/browser/ui/search/toolbar_search_animator_unittest.cc (right):

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_unittest.cc:43: class
ToolbarSearchAnimatorTestObserver : public ToolbarSearchAnimatorObserver {
On 2012/08/01 22:26:23, dhollowa wrote:
> Please include this in the anonymous namespace.

if i do that, i can't friend this and ToolbarSearchAnimatorTest in
ToolbarSearchAnimator.

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_unittest.cc:101: class
ToolbarSearchAnimatorTest : public BrowserWithTestWindowTest {
On 2012/08/01 22:41:59, dhollowa wrote:
> I noticed that the tests take 4+s to run:
> [----------] 11 tests from ToolbarSearchAnimatorTest (4091 ms total)
> 
> This is too much.  I would suggest a private method to set the animation times
> shorter during tests.

Done.

http://codereview.chromium.org/10816027/diff/24012/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_unittest.cc:371:
Times(AtLeast(3));
On 2012/08/01 22:41:59, dhollowa wrote:
> Are these |AtLeast(...)| calls dependent on the animation times used?  If so,
> this is probably overly fragile and may be flaky.  If we can guarantee
> |AtLeast(1)| with *any* animation duration, then fine, otherwise we should
find
> a deterministic test that is independent of duration.

Done.

[dhollowa, 2012-08-02 22:35:38]

sky: This is reviewable now.  As kuan mentions, the separator animation is now
removed.  The separator logic will follow work on the suggestions-list.  Thanks.

http://codereview.chromium.org/10816027/diff/19006/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_tab_helper.h (right):

http://codereview.chromium.org/10816027/diff/19006/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_tab_helper.h:66: bool has_navigated_;
Can you remove this in favor of |NavigationController::IsInitialNavigation|?

http://codereview.chromium.org/10816027/diff/19006/chrome/browser/ui/search/t...
File chrome/browser/ui/search/toolbar_search_animator.h (right):

http://codereview.chromium.org/10816027/diff/19006/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator.h:26: // ToolbarSearchAnimator
is used to track the gradient background and separator
delete "and separator states".

[kuan, 2012-08-03 04:55:58]

i've addressed dhollowa's comments in patch set 12.  ptal.  thx.

http://codereview.chromium.org/10816027/diff/19006/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_tab_helper.h (right):

http://codereview.chromium.org/10816027/diff/19006/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_tab_helper.h:66: bool has_navigated_;
On 2012/08/02 22:35:38, dhollowa wrote:
> Can you remove this in favor of |NavigationController::IsInitialNavigation|?

IsInitialNavigation is already false by the time we receive NAV_ENTRY_COMMITTED
notification, so i can't use it.
i cld use it previously 'cos it was false in NavigateToPendingEntry.

http://codereview.chromium.org/10816027/diff/19006/chrome/browser/ui/search/t...
File chrome/browser/ui/search/toolbar_search_animator.h (right):

http://codereview.chromium.org/10816027/diff/19006/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator.h:26: // ToolbarSearchAnimator
is used to track the gradient background and separator
On 2012/08/02 22:35:38, dhollowa wrote:
> delete "and separator states".

Done.

[kuan, 2012-08-03 04:59:42]

http://codereview.chromium.org/10816027/diff/19006/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_tab_helper.h (right):

http://codereview.chromium.org/10816027/diff/19006/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_tab_helper.h:66: bool has_navigated_;
On 2012/08/03 04:55:58, kuan wrote:
> On 2012/08/02 22:35:38, dhollowa wrote:
> > Can you remove this in favor of |NavigationController::IsInitialNavigation|?
> 
> IsInitialNavigation is already false by the time we receive
NAV_ENTRY_COMMITTED
> notification, so i can't use it.
> i cld use it previously 'cos it was false in NavigateToPendingEntry.

sorry, typo, i mean: "i cld use it previously 'cos it was _true_ in
NavigateToPendingEntry".

[dhollowa, 2012-08-03 16:30:23]

LGTM.  Thanks.

[sky, 2012-08-03 17:29:40]

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_delegate.h (right):

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_delegate.h:32: virtual void ModeChanged(const
Mode& old_mode, const Mode& new_mode) OVERRIDE;
Would be nice if you did this in a separate change to make for a smaller review.

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_tab_helper.cc (left):

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_tab_helper.cc:91: if (!is_search_enabled_)
Are you sure removing this doesn't lead to flicker?

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_tab_helper.h (right):

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_tab_helper.h:66: bool has_navigated_;
Can you query the navigation stack rather than maintaining a field?

[kuan, 2012-08-03 17:46:49]

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_delegate.h (right):

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_delegate.h:32: virtual void ModeChanged(const
Mode& old_mode, const Mode& new_mode) OVERRIDE;
On 2012/08/03 17:29:40, sky wrote:
> Would be nice if you did this in a separate change to make for a smaller
review.

i'm sorry abt that :(
this cl, and its original intention, went through so many transformations and
metamorphoses, that modifying ModeChanged API wasn't even the original
intention, it just evolved as the way to fix the mode transition bug :(

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_tab_helper.cc (left):

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_tab_helper.cc:91: if (!is_search_enabled_)
On 2012/08/03 17:29:40, sky wrote:
> Are you sure removing this doesn't lead to flicker?

i tested lots of times, with different mode transitions, and didn't see the
flicker.  so i think we're good.
if i miss it and flicker happens, i'll find another solution.  is that ok?

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_tab_helper.h (right):

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_tab_helper.h:66: bool has_navigated_;
On 2012/08/03 17:29:40, sky wrote:
> Can you query the navigation stack rather than maintaining a field?

Initially, when we would update mode in NavigateToPendingEntry,
NavigationController::IsInitialNavigation provided what i need - returns true
for the very first URL in any tab.
now that i've removed NavigateToPendingEntry and only update mode on receipt of
NOTIFICATION_NAV_ENTRY_COMMITTED, IsInitialNavigation unfortunately returns
false by then.
i can't find anything else in NavigationController that would indicate the very
first URL in any tab by the time NAV_ENTRY_COMMITTED is received.

[sky, 2012-08-03 22:18:41]

Thanks for the clarification on the comments I had.

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_tab_helper.h (right):

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_tab_helper.h:51: // |animate| is flag in model's
mode.
|animate| is set in the Mode passed to the model.

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/t...
File chrome/browser/ui/search/toolbar_search_animator_unittest.cc (right):

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_unittest.cc:11: #include
"chrome/browser/ui/search/toolbar_search_animator.h"
Include order should be the same as that of the the .cc file. So, this should be
your first include, newline, then the rest.

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_unittest.cc:27: class
MockObserver : public ToolbarSearchAnimatorObserver {
Ben says no gmock. Write the code by hand so it's more readable.

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_renderer_data.cc (right):

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_renderer_data.cc:17:
gradient_background_opacity(-1.0f) {
should this be 1.0?

[kuan, 2012-08-13 20:19:46]

i've addressed all comments in patch set 13, which unfortunately is also rebased
:(
also, i've reverted to use NavigateToPendingPentry and
NavigationController::IsInitialNavigation (details in comments).

ptal.  thanks.

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_tab_helper.h (right):

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_tab_helper.h:51: // |animate| is flag in model's
mode.
On 2012/08/03 22:18:41, sky wrote:
> |animate| is set in the Mode passed to the model.

Done.

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_tab_helper.h:66: bool has_navigated_;
On 2012/08/03 17:46:49, kuan wrote:
> On 2012/08/03 17:29:40, sky wrote:
> > Can you query the navigation stack rather than maintaining a field?
> 
> Initially, when we would update mode in NavigateToPendingEntry,
> NavigationController::IsInitialNavigation provided what i need - returns true
> for the very first URL in any tab.
> now that i've removed NavigateToPendingEntry and only update mode on receipt
of
> NOTIFICATION_NAV_ENTRY_COMMITTED, IsInitialNavigation unfortunately returns
> false by then.
> i can't find anything else in NavigationController that would indicate the
very
> first URL in any tab by the time NAV_ENTRY_COMMITTED is received.

i've reverted to use NavigateToPendingEntry and
NavigationController::IsInitialNavigation, 'cos dhollowa will be modifying the
code to sometimes update mode for pending entries and sometimes for committed
entries; i've left a TODO for him to handle the cases properly then.

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/t...
File chrome/browser/ui/search/toolbar_search_animator_unittest.cc (right):

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_unittest.cc:11: #include
"chrome/browser/ui/search/toolbar_search_animator.h"
On 2012/08/03 22:18:41, sky wrote:
> Include order should be the same as that of the the .cc file. So, this should
be
> your first include, newline, then the rest.

Done.

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator_unittest.cc:27: class
MockObserver : public ToolbarSearchAnimatorObserver {
On 2012/08/03 22:18:41, sky wrote:
> Ben says no gmock. Write the code by hand so it's more readable.

Done.

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_renderer_data.cc (right):

http://codereview.chromium.org/10816027/diff/13023/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_renderer_data.cc:17:
gradient_background_opacity(-1.0f) {
On 2012/08/03 22:18:41, sky wrote:
> should this be 1.0?

Done.

[sky, 2012-08-13 21:10:08]

LGTM

http://codereview.chromium.org/10816027/diff/22004/chrome/browser/ui/search/t...
File chrome/browser/ui/search/toolbar_search_animator.cc (right):

http://codereview.chromium.org/10816027/diff/22004/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator.cc:124:
background_animation_->set_delegate(NULL);
Seems cleaner and less error prone to nuke the animation, eg
background_animation_.reset() and create it each time you need it.

[kuan, 2012-08-13 22:02:13]

i've addressed scott's comments in patch set 14.  ptal. thx.

http://codereview.chromium.org/10816027/diff/22004/chrome/browser/ui/search/t...
File chrome/browser/ui/search/toolbar_search_animator.cc (right):

http://codereview.chromium.org/10816027/diff/22004/chrome/browser/ui/search/t...
chrome/browser/ui/search/toolbar_search_animator.cc:124:
background_animation_->set_delegate(NULL);
On 2012/08/13 21:10:08, sky wrote:
> Seems cleaner and less error prone to nuke the animation, eg
> background_animation_.reset() and create it each time you need it.

Done.

[commit-bot: I haz the power, 2012-08-14 18:29:29]

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

[commit-bot: I haz the power, 2012-08-14 19:25:39]

Try job failure for 10816027-19033 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-08-14 19:54:04]

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

[commit-bot: I haz the power, 2012-08-14 21:24:55]

Change committed as 151567