Reload Instant NTP and Instant-process tabs on search url change

Reload Instant NTP and Instant-process tabs on search url change

. Add a default impl of instant_service_observer.

. Move pref change listener for default search provider to
instant_service and pass to browser_instant_controller and
instant_ntp_prerenderer via instant_service_observer.

. Add a search-domain-check listener to instant_service in a similar
manner.

. Replaced ReloadStaleNTP, PreloadInstantNTP etc with ReloadInstantNTP

. Introduce InstantUnitTestBase
  - InstantServiceTest and BrowserInstantControllerTest are subclasses.

. InstantService Unit Test
  - Use a mock observer to verify the search provider change and 
    google url update is triggered on observers.
  - Verify the prerendered NTP is reloaded.

. BrowserInstantController Unit Test
  - Moved the tab reload test from interactive ui test to 
     BrowserInstantControllerTest.


R=jered@chromium.org, samarth@chromium.org
TBR=sky@chromium.org
BUG=254206
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225063

[Anuj, 2013-07-26 17:25:05]

PTAL.

[samarth, 2013-07-26 23:33:14]

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
chrome/browser/search/instant_service.cc:115: registrar_.Add(this,
Move this up next to the other notification registrar call.

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
chrome/browser/search/instant_service.cc:418: // no need to temper with instant
resources.
s/temper/tamper/
s/instant/Instant/

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
chrome/browser/search/instant_service.cc:422: // Google URL change is just the
scheme change (http <-> https).
This matters.  Instant Extended should only happen over https.

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
File chrome/browser/search/instant_service_observer.cc (right):

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
chrome/browser/search/instant_service_observer.cc:7: void
InstantServiceObserver::ThemeInfoChanged(const ThemeBackgroundInfo&) {}
nit: I don't know if there's a style guide rule for this, but I prefer putting
the } on a separate line.

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
File chrome/browser/search/instant_service_observer.h (right):

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
chrome/browser/search/instant_service_observer.h:23: virtual void
DefaultSearchProviderChanged();
Please add comment for both the new methods.

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

https://codereview.chromium.org/20388003/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.cc:227: Profile* browser_profile =
profile();
Just inline this call below.

https://codereview.chromium.org/20388003/diff/1/chrome/browser/ui/browser_ins...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/20388003/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.h:89: // Reloads the tabs in
instant process to ensure that their privileged status
I would move this comment down to MaybeReload...

https://codereview.chromium.org/20388003/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.h:94: void
MaybeReloadInstantWebContents();
How about ReloadTabsInInstantProcess();

https://codereview.chromium.org/20388003/diff/1/chrome/browser/ui/search/inst...
File chrome/browser/ui/search/instant_ntp_prerenderer.cc (right):

https://codereview.chromium.org/20388003/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_ntp_prerenderer.cc:67: DCHECK(!ntp());
Aren't you going to trigger this DCHECK when it is called from stale check
timer?

[Anuj, 2013-07-31 07:16:17]

I have added some instant_service unittest. Please take a look while I add more
unit tests and some interactive_ui tests.

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
chrome/browser/search/instant_service.cc:115: registrar_.Add(this,
On 2013/07/26 23:33:15, samarth wrote:
> Move this up next to the other notification registrar call.

Done.

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
chrome/browser/search/instant_service.cc:418: // no need to temper with instant
resources.
On 2013/07/26 23:33:15, samarth wrote:
> s/temper/tamper/
> s/instant/Instant/

Done.

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
chrome/browser/search/instant_service.cc:422: // Google URL change is just the
scheme change (http <-> https).
On 2013/07/26 23:33:15, samarth wrote:
> This matters.  Instant Extended should only happen over https.

As discussed and commented above, this imitates the behavior of
GoogleURLTracker::OnURLFetchComplete. Expanded the comment.

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
File chrome/browser/search/instant_service_observer.cc (right):

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
chrome/browser/search/instant_service_observer.cc:7: void
InstantServiceObserver::ThemeInfoChanged(const ThemeBackgroundInfo&) {}
On 2013/07/26 23:33:15, samarth wrote:
> nit: I don't know if there's a style guide rule for this, but I prefer putting
> the } on a separate line.

I just ran "git cl format". Let me know if you feel strongly about it, otherwise
I will like to rely on automated formatting.

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
File chrome/browser/search/instant_service_observer.h (right):

https://codereview.chromium.org/20388003/diff/1/chrome/browser/search/instant...
chrome/browser/search/instant_service_observer.h:23: virtual void
DefaultSearchProviderChanged();
On 2013/07/26 23:33:15, samarth wrote:
> Please add comment for both the new methods.

Done.

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

https://codereview.chromium.org/20388003/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.cc:227: Profile* browser_profile =
profile();
On 2013/07/26 23:33:15, samarth wrote:
> Just inline this call below.

Done.

https://codereview.chromium.org/20388003/diff/1/chrome/browser/ui/browser_ins...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/20388003/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.h:89: // Reloads the tabs in
instant process to ensure that their privileged status
On 2013/07/26 23:33:15, samarth wrote:
> I would move this comment down to MaybeReload...

Done.

https://codereview.chromium.org/20388003/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.h:94: void
MaybeReloadInstantWebContents();
On 2013/07/26 23:33:15, samarth wrote:
> How about ReloadTabsInInstantProcess();

Done.

https://codereview.chromium.org/20388003/diff/1/chrome/browser/ui/search/inst...
File chrome/browser/ui/search/instant_ntp_prerenderer.cc (right):

https://codereview.chromium.org/20388003/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_ntp_prerenderer.cc:67: DCHECK(!ntp());
On 2013/07/26 23:33:15, samarth wrote:
> Aren't you going to trigger this DCHECK when it is called from stale check
> timer?

Done.

[Anuj, 2013-08-01 23:20:05]

Done with tests. PTAL.

[samarth, 2013-08-02 21:51:15]

Looks good overall. Mostly nits and comments for the test code.

Thanks for cleaning things up!
Samarth

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:120: AddObserver(ntp_prerenderer());
Objects should add themselves as observers rather than the other way around.

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:424: // Google URL change is just the
scheme change (http <-> https). Since this
nit: How about:
Only the scheme changed. Ignore it since we do not prompt the user in this case.

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/ins...
File chrome/browser/search/instant_service_unittest.cc (right):

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:36: namespace chrome {
Why is this in namespace chrome? InstantService is not that namespace.

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:37: using base::HistogramBase;
nit: add an empty line above

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:116: }  // namespace chrome
nit: add an empty line above

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/sea...
File chrome/browser/search/search_test_utils.cc (right):

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/sea...
chrome/browser/search/search_test_utils.cc:13: namespace search_test_utils {
In general, I'm all for sharing code. But given how little is here, I'm not
convinced this is worth the new file + new namespace.

If you convert all of the tests that need this into unit tests, you could make a
base test class that all of them derive from. I would be OK with that.

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

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1549:
IN_PROC_BROWSER_TEST_F(InstantExtendedTest, OnDefaultSearchProviderChanged) {
This can almost certainly become a unit test. There's no
browser_instant_controller_unittest right now. Ideally it would go there, but
you can add it to instant_controller_unittest.

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1672: // Change
the Google Base URL
Can you not do this in a unit test? Everything else in this test works in a unit
test, and I prefer unit tests to interactive ui tests when possible.

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

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.cc:68: void
InstantNTPPrerenderer::ReloadInstantNTP() { ResetNTP(GetInstantURL()); }
nit: please keep on its own line.

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.cc:289: void
InstantNTPPrerenderer::GoogleURLUpdated() { ReloadInstantNTP(); }
also here

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

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.h:140: // Called when the
default search provider changes. Resets InstantNTP.
nit: just delete this line

[Anuj, 2013-08-09 07:22:00]

PTAL.  Updated CL description.

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:120: AddObserver(ntp_prerenderer());
On 2013/08/02 21:51:15, samarth wrote:
> Objects should add themselves as observers rather than the other way around.

Done.

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:424: // Google URL change is just the
scheme change (http <-> https). Since this
On 2013/08/02 21:51:15, samarth wrote:
> nit: How about:
> Only the scheme changed. Ignore it since we do not prompt the user in this
case.

Done.

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/ins...
File chrome/browser/search/instant_service_unittest.cc (right):

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:36: namespace chrome {
On 2013/08/02 21:51:15, samarth wrote:
> Why is this in namespace chrome? InstantService is not that namespace.

Done.

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:37: using base::HistogramBase;
On 2013/08/02 21:51:15, samarth wrote:
> nit: add an empty line above

Done.

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:116: }  // namespace chrome
On 2013/08/02 21:51:15, samarth wrote:
> nit: add an empty line above

Done.

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/sea...
File chrome/browser/search/search_test_utils.cc (right):

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/sea...
chrome/browser/search/search_test_utils.cc:13: namespace search_test_utils {
On 2013/08/02 21:51:15, samarth wrote:
> In general, I'm all for sharing code. But given how little is here, I'm not
> convinced this is worth the new file + new namespace.
> 
> If you convert all of the tests that need this into unit tests, you could make
a
> base test class that all of them derive from. I would be OK with that.

I have introduced instant_unittest_base. The InstantServiceTest and
BrowserInstantControllerTest are the subclasses.
Although BrowserWithTestWindowTest doesn't really feel like unit-test.
InstantNTPPrerendererTest can be moved on to the new InstantUnitTestBase, but
perhaps another CL.

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

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1549:
IN_PROC_BROWSER_TEST_F(InstantExtendedTest, OnDefaultSearchProviderChanged) {
On 2013/08/02 21:51:15, samarth wrote:
> This can almost certainly become a unit test. There's no
> browser_instant_controller_unittest right now. Ideally it would go there, but
> you can add it to instant_controller_unittest.

Done.

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1672: // Change
the Google Base URL
On 2013/08/02 21:51:15, samarth wrote:
> Can you not do this in a unit test? Everything else in this test works in a
unit
> test, and I prefer unit tests to interactive ui tests when possible.

Done.

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

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.cc:68: void
InstantNTPPrerenderer::ReloadInstantNTP() { ResetNTP(GetInstantURL()); }
On 2013/08/02 21:51:15, samarth wrote:
> nit: please keep on its own line.

Done.

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.cc:289: void
InstantNTPPrerenderer::GoogleURLUpdated() { ReloadInstantNTP(); }
On 2013/08/02 21:51:15, samarth wrote:
> also here

Done.

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

https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.h:140: // Called when the
default search provider changes. Resets InstantNTP.
On 2013/08/02 21:51:15, samarth wrote:
> nit: just delete this line

Done.

[Anuj Sharma, 2013-08-12 08:47:19]

Ping.
On Aug 9, 2013 12:22 AM, <skanuj@chromium.org> wrote:

> PTAL.  Updated CL description.
>
>
> https://codereview.chromium.**org/20388003/diff/21001/**
>
chrome/browser/search/instant_**service.cc<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/instant_service.cc>
> File chrome/browser/search/instant_**service.cc (right):
>
> https://codereview.chromium.**org/20388003/diff/21001/**
>
chrome/browser/search/instant_**service.cc#newcode120<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/instant_service.cc#newcode120>
> chrome/browser/search/instant_**service.cc:120:
> AddObserver(ntp_prerenderer())**;
> On 2013/08/02 21:51:15, samarth wrote:
>
>> Objects should add themselves as observers rather than the other way
>>
> around.
>
> Done.
>
> https://codereview.chromium.**org/20388003/diff/21001/**
>
chrome/browser/search/instant_**service.cc#newcode424<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/instant_service.cc#newcode424>
> chrome/browser/search/instant_**service.cc:424: // Google URL change is
> just the scheme change (http <-> https). Since this
> On 2013/08/02 21:51:15, samarth wrote:
>
>> nit: How about:
>> Only the scheme changed. Ignore it since we do not prompt the user in
>>
> this case.
>
> Done.
>
> https://codereview.chromium.**org/20388003/diff/21001/**
>
chrome/browser/search/instant_**service_unittest.cc<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/instant_service_unittest.cc>
> File chrome/browser/search/instant_**service_unittest.cc (right):
>
> https://codereview.chromium.**org/20388003/diff/21001/**
>
chrome/browser/search/instant_**service_unittest.cc#newcode36<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/instant_service_unittest.cc#newcode36>
> chrome/browser/search/instant_**service_unittest.cc:36: namespace chrome {
> On 2013/08/02 21:51:15, samarth wrote:
>
>> Why is this in namespace chrome? InstantService is not that namespace.
>>
>
> Done.
>
> https://codereview.chromium.**org/20388003/diff/21001/**
>
chrome/browser/search/instant_**service_unittest.cc#newcode37<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/instant_service_unittest.cc#newcode37>
> chrome/browser/search/instant_**service_unittest.cc:37: using
> base::HistogramBase;
> On 2013/08/02 21:51:15, samarth wrote:
>
>> nit: add an empty line above
>>
>
> Done.
>
> https://codereview.chromium.**org/20388003/diff/21001/**
>
chrome/browser/search/instant_**service_unittest.cc#newcode116<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/instant_service_unittest.cc#newcode116>
> chrome/browser/search/instant_**service_unittest.cc:116: }  // namespace
> chrome
> On 2013/08/02 21:51:15, samarth wrote:
>
>> nit: add an empty line above
>>
>
> Done.
>
> https://codereview.chromium.**org/20388003/diff/21001/**
>
chrome/browser/search/search_**test_utils.cc<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/search_test_utils.cc>
> File chrome/browser/search/search_**test_utils.cc (right):
>
> https://codereview.chromium.**org/20388003/diff/21001/**
>
chrome/browser/search/search_**test_utils.cc#newcode13<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/search/search_test_utils.cc#newcode13>
> chrome/browser/search/search_**test_utils.cc:13: namespace
> search_test_utils {
> On 2013/08/02 21:51:15, samarth wrote:
>
>> In general, I'm all for sharing code. But given how little is here,
>>
> I'm not
>
>> convinced this is worth the new file + new namespace.
>>
>
>  If you convert all of the tests that need this into unit tests, you
>>
> could make a
>
>> base test class that all of them derive from. I would be OK with that.
>>
>
> I have introduced instant_unittest_base. The InstantServiceTest and
> BrowserInstantControllerTest are the subclasses.
> Although BrowserWithTestWindowTest doesn't really feel like unit-test.
> InstantNTPPrerendererTest can be moved on to the new
> InstantUnitTestBase, but perhaps another CL.
>
> https://codereview.chromium.**org/20388003/diff/21001/**
>
chrome/browser/ui/search/**instant_extended_interactive_**uitest.cc<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/instant_extended_interactive_uitest.cc>
> File chrome/browser/ui/search/**instant_extended_interactive_**uitest.cc
> (right):
>
> https://codereview.chromium.**org/20388003/diff/21001/**
> chrome/browser/ui/search/**instant_extended_interactive_**
>
uitest.cc#newcode1549<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/instant_extended_interactive_uitest.cc#newcode1549>
> chrome/browser/ui/search/**instant_extended_interactive_**uitest.cc:1549:
> IN_PROC_BROWSER_TEST_F(**InstantExtendedTest,
> OnDefaultSearchProviderChanged**) {
> On 2013/08/02 21:51:15, samarth wrote:
>
>> This can almost certainly become a unit test. There's no
>> browser_instant_controller_**unittest right now. Ideally it would go
>>
> there, but
>
>> you can add it to instant_controller_unittest.
>>
>
> Done.
>
> https://codereview.chromium.**org/20388003/diff/21001/**
> chrome/browser/ui/search/**instant_extended_interactive_**
>
uitest.cc#newcode1672<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/instant_extended_interactive_uitest.cc#newcode1672>
> chrome/browser/ui/search/**instant_extended_interactive_**uitest.cc:1672:
> //
> Change the Google Base URL
> On 2013/08/02 21:51:15, samarth wrote:
>
>> Can you not do this in a unit test? Everything else in this test works
>>
> in a unit
>
>> test, and I prefer unit tests to interactive ui tests when possible.
>>
>
> Done.
>
> https://codereview.chromium.**org/20388003/diff/21001/**
>
chrome/browser/ui/search/**instant_ntp_prerenderer.cc<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/instant_ntp_prerenderer.cc>
> File chrome/browser/ui/search/**instant_ntp_prerenderer.cc (right):
>
> https://codereview.chromium.**org/20388003/diff/21001/**
>
chrome/browser/ui/search/**instant_ntp_prerenderer.cc#**newcode68<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/instant_ntp_prerenderer.cc#newcode68>
> chrome/browser/ui/search/**instant_ntp_prerenderer.cc:68: void
> InstantNTPPrerenderer::**ReloadInstantNTP() { ResetNTP(GetInstantURL()); }
> On 2013/08/02 21:51:15, samarth wrote:
>
>> nit: please keep on its own line.
>>
>
> Done.
>
> https://codereview.chromium.**org/20388003/diff/21001/**
>
chrome/browser/ui/search/**instant_ntp_prerenderer.cc#**newcode289<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/instant_ntp_prerenderer.cc#newcode289>
> chrome/browser/ui/search/**instant_ntp_prerenderer.cc:**289: void
> InstantNTPPrerenderer::**GoogleURLUpdated() { ReloadInstantNTP(); }
> On 2013/08/02 21:51:15, samarth wrote:
>
>> also here
>>
>
> Done.
>
> https://codereview.chromium.**org/20388003/diff/21001/**
>
chrome/browser/ui/search/**instant_ntp_prerenderer.h<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/instant_ntp_prerenderer.h>
> File chrome/browser/ui/search/**instant_ntp_prerenderer.h (right):
>
> https://codereview.chromium.**org/20388003/diff/21001/**
>
chrome/browser/ui/search/**instant_ntp_prerenderer.h#**newcode140<https://codereview.chromium.org/20388003/diff/21001/chrome/browser/ui/search/instant_ntp_prerenderer.h#newcode140>
> chrome/browser/ui/search/**instant_ntp_prerenderer.h:140: // Called when
> the default search provider changes. Resets InstantNTP.
> On 2013/08/02 21:51:15, samarth wrote:
>
>> nit: just delete this line
>>
>
> Done.
>
>
https://codereview.chromium.**org/20388003/<https://codereview.chromium.org/2...
>

[samarth, 2013-08-12 19:07:28]

Almost there. Thanks for your patience.

Thanks,
Samarth

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
File chrome/browser/search/instant_service_observer.h (right):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
chrome/browser/search/instant_service_observer.h:26: // Indicates that the
Google URL has changed as a result of searchdomaincheck.
Please also clarify in comments that this only fires after the user accepts the
change by clicking on an infobar.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
File chrome/browser/search/instant_service_unittest.cc (right):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:27: //
TODO(skanuj@chromium.org) : Add more tests.
That's like a TODO: make things better :)

I'm definitely all for it, but I don't think the TODO is buying us much.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
File chrome/browser/search/instant_unittest_base.cc (right):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
chrome/browser/search/instant_unittest_base.cc:66: // (See
oogleURLTrackerFactory::ServiceIsNULLWhileTesting())
s/oogle/Google/

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
chrome/browser/search/instant_unittest_base.cc:67: // For determining
google:baseURL for NTP, following is used:
nit: the following is used

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
File chrome/browser/search/instant_unittest_base.h (right):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
chrome/browser/search/instant_unittest_base.h:14: class InstantUnitTestBase :
public BrowserWithTestWindowTest {
Comments for what this provides.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
chrome/browser/search/instant_unittest_base.h:18: void SetSearchProvider(const
std::string& base_url);
Comments for these two, please.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller_unittest.cc (right):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:41: void
NavigateToPendingEntry(
This is an OVERRIDE, right? (and should be virtual?)

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:54: return validated_
== 1;
What is being validated here? Please use a more descriptive name.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:77: {"Local Embedded
SEHP", chrome::kChromeSearchLocalNtpUrl,
s/Embedded SEHP/NTP

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:97: TabInfo*
SetUpTabReloadTestCases() {
This is way too complex to follow. I shouldn't feel the need to have tests for
your test code :)

Let's get rid of TabInfo and just inline setup and validation in the actual
tests. Yes, you'll repeat it but it'll be more readable overall I think.

Also, just make a FakeWebContentsObserver for each tab and then add a DidReload
helper function to test whether a reload actually happened.

Please also pepper comments throughout about what the setup / verification steps
are.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp_prerenderer.cc (right):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.cc:67: void
InstantNTPPrerenderer::Init() {
Currently this function can be called multiple times: e.g., on mac, if you close
all windows without quitting Chrome and then open a new window. So you need to a
corresponding RemoveObserver call as well.

One option is to rename this to InitNTPContents and then add a RemoveObserver
call in DeleteNTPContents. You would also then need to double-check in the
destructor that ntp_ is NULL and if not, call DeleteNTPContents there.

Alternatively, you can Add in the constructor and Remove in the destructor but I
assume that doesn't work now because you can't look up InstantService in the
constructor (since it's the InstantService constructor that creates this class)?
If that's the case, you can pass in InstantService as an explicit argument to
the constructor here.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp_prerenderer.h (right):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.h:39: // Add itself as observer
on InstantService, preload instant NTP.
nit: Adds itself .. and preloads Instant NTP.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_test_utils.cc (left):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_test_utils.cc:96: FocusOmnibox();
Are you sure this doesn't break any tests? You generally need to register the
observer before you do the action that triggers it.

In any case, this seems unrelated to this CL so please do it in a separate CL if
you do want to make the change.

https://codereview.chromium.org/20388003/diff/37001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

https://codereview.chromium.org/20388003/diff/37001/chrome/chrome_tests.gypi#...
chrome/chrome_tests.gypi:258: 'browser/ui/search/instant_test_utils.h',
Just leave this here. No reason to bloat the unit tests binary with this code.

[Anuj, 2013-08-13 00:18:08]

PTAL.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
File chrome/browser/search/instant_service_observer.h (right):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
chrome/browser/search/instant_service_observer.h:26: // Indicates that the
Google URL has changed as a result of searchdomaincheck.
On 2013/08/12 19:07:28, samarth wrote:
> Please also clarify in comments that this only fires after the user accepts
the
> change by clicking on an infobar.

Done.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
File chrome/browser/search/instant_service_unittest.cc (right):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:27: //
TODO(skanuj@chromium.org) : Add more tests.
On 2013/08/12 19:07:28, samarth wrote:
> That's like a TODO: make things better :)
> 
> I'm definitely all for it, but I don't think the TODO is buying us much.

Done.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
File chrome/browser/search/instant_unittest_base.cc (right):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
chrome/browser/search/instant_unittest_base.cc:66: // (See
oogleURLTrackerFactory::ServiceIsNULLWhileTesting())
On 2013/08/12 19:07:28, samarth wrote:
> s/oogle/Google/

Done.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
chrome/browser/search/instant_unittest_base.cc:67: // For determining
google:baseURL for NTP, following is used:
On 2013/08/12 19:07:28, samarth wrote:
> nit: the following is used

Done.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
File chrome/browser/search/instant_unittest_base.h (right):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
chrome/browser/search/instant_unittest_base.h:14: class InstantUnitTestBase :
public BrowserWithTestWindowTest {
On 2013/08/12 19:07:28, samarth wrote:
> Comments for what this provides.

Done.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/search/ins...
chrome/browser/search/instant_unittest_base.h:18: void SetSearchProvider(const
std::string& base_url);
On 2013/08/12 19:07:28, samarth wrote:
> Comments for these two, please.

Done.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller_unittest.cc (right):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:41: void
NavigateToPendingEntry(
On 2013/08/12 19:07:28, samarth wrote:
> This is an OVERRIDE, right? (and should be virtual?)

Done.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:54: return validated_
== 1;
On 2013/08/12 19:07:28, samarth wrote:
> What is being validated here? Please use a more descriptive name.

Done.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:77: {"Local Embedded
SEHP", chrome::kChromeSearchLocalNtpUrl,
On 2013/08/12 19:07:28, samarth wrote:
> s/Embedded SEHP/NTP

Done.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:97: TabInfo*
SetUpTabReloadTestCases() {
On 2013/08/12 19:07:28, samarth wrote:
> This is way too complex to follow. I shouldn't feel the need to have tests for
> your test code :)
> 
> Let's get rid of TabInfo and just inline setup and validation in the actual
> tests. Yes, you'll repeat it but it'll be more readable overall I think.
> 
> Also, just make a FakeWebContentsObserver for each tab and then add a
DidReload
> helper function to test whether a reload actually happened.
> 
> Please also pepper comments throughout about what the setup / verification
steps
> are.

Done.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp_prerenderer.cc (right):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.cc:67: void
InstantNTPPrerenderer::Init() {
On 2013/08/12 19:07:28, samarth wrote:
> Currently this function can be called multiple times: e.g., on mac, if you
close
> all windows without quitting Chrome and then open a new window. So you need to
a
> corresponding RemoveObserver call as well.
> 
> One option is to rename this to InitNTPContents and then add a RemoveObserver
> call in DeleteNTPContents. You would also then need to double-check in the
> destructor that ntp_ is NULL and if not, call DeleteNTPContents there.
> 
> Alternatively, you can Add in the constructor and Remove in the destructor but
I
> assume that doesn't work now because you can't look up InstantService in the
> constructor (since it's the InstantService constructor that creates this
class)?
> If that's the case, you can pass in InstantService as an explicit argument to
> the constructor here.

Done.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp_prerenderer.h (right):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.h:39: // Add itself as observer
on InstantService, preload instant NTP.
On 2013/08/12 19:07:28, samarth wrote:
> nit: Adds itself .. and preloads Instant NTP.

Done.

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_test_utils.cc (left):

https://codereview.chromium.org/20388003/diff/37001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_test_utils.cc:96: FocusOmnibox();
On 2013/08/12 19:07:28, samarth wrote:
> Are you sure this doesn't break any tests? You generally need to register the
> observer before you do the action that triggers it.
> 
> In any case, this seems unrelated to this CL so please do it in a separate CL
if
> you do want to make the change.

Removed. I was earlier trying to make use of InstantTestBase in the unit tests.
That exercise ran into issues, and I missed reverting this and the gypi change.

https://codereview.chromium.org/20388003/diff/37001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

https://codereview.chromium.org/20388003/diff/37001/chrome/chrome_tests.gypi#...
chrome/chrome_tests.gypi:258: 'browser/ui/search/instant_test_utils.h',
On 2013/08/12 19:07:28, samarth wrote:
> Just leave this here. No reason to bloat the unit tests binary with this code.

Done.

[samarth, 2013-08-14 00:24:13]

lgtm with some nits (and one actual bug below).

Thanks,
Samarth

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:228: ntp_prerenderer_.Destroy();
Init -> InitNTP or PreloadNTP
Destroy -> DestroyNTP or DeleteNTP

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:275: case
chrome::NOTIFICATION_GOOGLE_URL_UPDATED: {
Does this ever fire if google is not your default search provider? If so, you
may want to add a check for that. Otherwise, all the reloads are pointless.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/search/ins...
File chrome/browser/search/instant_service_observer.h (right):

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/search/ins...
chrome/browser/search/instant_service_observer.h:23: // Indicates that the
search provider setting was changed.
nit: Indicates that the default search provider changed.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/search/ins...
File chrome/browser/search/instant_service_unittest.cc (right):

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:72:
EXPECT_TRUE(new_ntp_url.spec().find(new_base_url) == 0);
nit: you could use EXPECT_THAT(..StartsWith(..))

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:44:
instant_service->AddObserver(this);
Call RemoveObserver in the destructor. I'm surprised you're not running into
crashes with this.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller_unittest.cc (right):

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:106:
EXPECT_EQ(test.start_in_instant_process, chrome::IsInstantNTP(contents))
This is only passing because your "Remote Embedded SERP" case is not actually a
SERP. In particular, it's missing search terms. Please add search terms to that
URL and just remove this check here and below.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:114: observers[i] = new
FakeWebContentsObserver(this, contents);
Just use vector<linked_ptr<FakeWebContentsObserver>> and push these onto the
end.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:125:
EXPECT_EQ(test.end_in_instant_process, chrome::IsInstantNTP(contents))
Remove this.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:133:
EXPECT_EQ(test.should_reload ? 1 : 0, observer->num_reloads())
Change num_reloads to ReloadedOnce() so this is just
EXPECT_EQ(test.should_reload, observer->ReloadedOnce()).

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:135: if
(test.should_reload) {
This is testing more detail than necessary. Just skip this section.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp_prerenderer.cc (right):

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.cc:298:
////////////////////////////////////////////////////////////////////////////////
nit: this file isn't really big enough to merit these headers. Just leave it
out.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp_prerenderer.h (right):

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.h:39: // Adds itself as
observer on InstantService, preloads instant NTP.
nit: the first part is just an implementation detail; I would leave it out.
Likewise below.

[Anuj, 2013-08-14 06:38:56]

PTAL.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:228: ntp_prerenderer_.Destroy();
On 2013/08/14 00:24:14, samarth wrote:
> Init -> InitNTP or PreloadNTP
> Destroy -> DestroyNTP or DeleteNTP

Reverted those methods.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:275: case
chrome::NOTIFICATION_GOOGLE_URL_UPDATED: {
On 2013/08/14 00:24:14, samarth wrote:
> Does this ever fire if google is not your default search provider? If so, you
> may want to add a check for that. Otherwise, all the reloads are pointless.

It is guarded by presence of GogoleBaseURLs in default search provider.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/goo...

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/search/ins...
File chrome/browser/search/instant_service_observer.h (right):

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/search/ins...
chrome/browser/search/instant_service_observer.h:23: // Indicates that the
search provider setting was changed.
On 2013/08/14 00:24:14, samarth wrote:
> nit: Indicates that the default search provider changed.

Done.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/search/ins...
File chrome/browser/search/instant_service_unittest.cc (right):

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:72:
EXPECT_TRUE(new_ntp_url.spec().find(new_base_url) == 0);
On 2013/08/14 00:24:14, samarth wrote:
> nit: you could use EXPECT_THAT(..StartsWith(..))

Done.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:44:
instant_service->AddObserver(this);
On 2013/08/14 00:24:14, samarth wrote:
> Call RemoveObserver in the destructor. I'm surprised you're not running into
> crashes with this.

Done. Added BrowserWindowLifeCycle unit test in InstantServiceTest.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller_unittest.cc (right):

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:106:
EXPECT_EQ(test.start_in_instant_process, chrome::IsInstantNTP(contents))
On 2013/08/14 00:24:14, samarth wrote:
> This is only passing because your "Remote Embedded SERP" case is not actually
a
> SERP. In particular, it's missing search terms. Please add search terms to
that
> URL and just remove this check here and below.

Done.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:114: observers[i] = new
FakeWebContentsObserver(this, contents);
On 2013/08/14 00:24:14, samarth wrote:
> Just use vector<linked_ptr<FakeWebContentsObserver>> and push these onto the
> end.

Done.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:125:
EXPECT_EQ(test.end_in_instant_process, chrome::IsInstantNTP(contents))
On 2013/08/14 00:24:14, samarth wrote:
> Remove this.

Done.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:133:
EXPECT_EQ(test.should_reload ? 1 : 0, observer->num_reloads())
On 2013/08/14 00:24:14, samarth wrote:
> Change num_reloads to ReloadedOnce() so this is just
> EXPECT_EQ(test.should_reload, observer->ReloadedOnce()).

ReloadedOnce can technically be false with num_reloads > 2, and that will not
correctly check that other tabs don't reload.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:135: if
(test.should_reload) {
On 2013/08/14 00:24:14, samarth wrote:
> This is testing more detail than necessary. Just skip this section.

Done.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp_prerenderer.cc (right):

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.cc:298:
////////////////////////////////////////////////////////////////////////////////
On 2013/08/14 00:24:14, samarth wrote:
> nit: this file isn't really big enough to merit these headers. Just leave it
> out.

Done.

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp_prerenderer.h (right):

https://codereview.chromium.org/20388003/diff/50001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.h:39: // Adds itself as
observer on InstantService, preloads instant NTP.
On 2013/08/14 00:24:14, samarth wrote:
> nit: the first part is just an implementation detail; I would leave it out.
> Likewise below.

Done.

[Jered, 2013-08-16 00:13:23]

Mostly nits and some confusion about a few comments.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:428: // See
GoogleURLTracker::OnURLFetchComplete.
Add () after OnURLFetchComplete

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:429: // last_prompted_url.is_empty
indicated very first run of Chrome. So there is
Add () after is_empty. I don't understand this comment. What "tampering" would
happen? Please elaborate.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:436: if
(net::StripWWWFromHost(details->first) ==
This condition does not match the comment above it. StripWWWFromHost strips a
leading www., and it does not affect the scheme (http vs. https). Also, it would
help to put in the comment an example of when this can happen.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
File chrome/browser/search/instant_service_observer.cc (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service_observer.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
Why do we need this default implementation? It doesn't seem to do anything.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service_observer.cc:7: void
InstantServiceObserver::ThemeInfoChanged(const ThemeBackgroundInfo&) {}
Here and below, put the closing brace on the next line.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
File chrome/browser/search/instant_service_unittest.cc (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:54:
instant_ntp_contents_observer()->Observe(NULL);
instant_ntp_contents_observer_->

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:58:
MockInstantServiceObserver* instant_service_observer() {
nit: Instead of adding accessors to get raw pointers, just call .get() in the
places where you need them. scoped_ptr has operator* which is *ptr_ so you
shouldn't need to call .get() for the EXPECT_CALL statements below.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:99: TEST_F(InstantServiceTest,
BrowserWindowLifecycle) {
This test probably belongs in browser_instant_controller_unittest.cc

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
File chrome/browser/search/instant_unittest_base.cc (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_unittest_base.cc:65: // GoogleURLTracker is not
initialized is not created in tests.
omit either "is not initialized"  or "is not created"

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
File chrome/browser/search/instant_unittest_base.h (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_unittest_base.h:27: void SetSearchProvider(const
std::string& base_url);
Call this SetDefaultSearchProvider

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_unittest_base.h:34: int
NumObservers(InstantService* instant_service) {
GetNumObservers

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller_unittest.cc (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:98: size_t numTestCases
= arraysize(kTabReloadTestCases);
const size_t num_tests =

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:99:
std::vector<linked_ptr<FakeWebContentsObserver> > observers;
Instead of vector<linked_ptr> use ScopedVector.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:117:
SetSearchProvider("https://bar.com/");
Why are we queuing up a bunch of tabs, doing this, and then checking them all,
instead of testing one case at a time?

https://codereview.chromium.org/20388003/diff/62001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/chrome_browser.gyp...
chrome/chrome_browser.gypi:1810: 'browser/search/instant_service_observer.cc',
Move up one line.

https://codereview.chromium.org/20388003/diff/62001/chrome/chrome_tests_unit....
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/chrome_tests_unit....
chrome/chrome_tests_unit.gypi:163: 'browser/search/instant_unittest_base.cc',
Move up one line.

https://codereview.chromium.org/20388003/diff/62001/chrome/chrome_tests_unit....
chrome/chrome_tests_unit.gypi:1353:
'browser/ui/browser_instant_controller_unittest.cc',
Sort down lower.

[Anuj, 2013-08-22 00:40:45]

PTAL

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:428: // See
GoogleURLTracker::OnURLFetchComplete.
On 2013/08/16 00:13:23, Jered wrote:
> Add () after OnURLFetchComplete

Done.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:429: // last_prompted_url.is_empty
indicated very first run of Chrome. So there is
On 2013/08/16 00:13:23, Jered wrote:
> Add () after is_empty. I don't understand this comment. What "tampering" would
> happen? Please elaborate.

Done.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:436: if
(net::StripWWWFromHost(details->first) ==
On 2013/08/16 00:13:23, Jered wrote:
> This condition does not match the comment above it. StripWWWFromHost strips a
> leading www., and it does not affect the scheme (http vs. https). Also, it
would
> help to put in the comment an example of when this can happen.

StripWWWFromHost looks only at the host component from the url.
(https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_util....)
This is bit of a copy over from GoogleURLTracker::OnURLFetchComplete() as
commented above. I don't think this will really happen, but it is just a measure
to avoid extraneous triggers.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
File chrome/browser/search/instant_service_observer.cc (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service_observer.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2013/08/16 00:13:23, Jered wrote:
> Why do we need this default implementation? It doesn't seem to do anything.

Allows implementors to pick which ones they really want to implement.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service_observer.cc:7: void
InstantServiceObserver::ThemeInfoChanged(const ThemeBackgroundInfo&) {}
On 2013/08/16 00:13:23, Jered wrote:
> Here and below, put the closing brace on the next line.

I blame "git cl format". done.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
File chrome/browser/search/instant_service_unittest.cc (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:54:
instant_ntp_contents_observer()->Observe(NULL);
On 2013/08/16 00:13:23, Jered wrote:
> instant_ntp_contents_observer_->

Done.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:58:
MockInstantServiceObserver* instant_service_observer() {
On 2013/08/16 00:13:23, Jered wrote:
> nit: Instead of adding accessors to get raw pointers, just call .get() in the
> places where you need them. scoped_ptr has operator* which is *ptr_ so you
> shouldn't need to call .get() for the EXPECT_CALL statements below.

Done.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_service_unittest.cc:99: TEST_F(InstantServiceTest,
BrowserWindowLifecycle) {
On 2013/08/16 00:13:23, Jered wrote:
> This test probably belongs in browser_instant_controller_unittest.cc

Done.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
File chrome/browser/search/instant_unittest_base.cc (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_unittest_base.cc:65: // GoogleURLTracker is not
initialized is not created in tests.
On 2013/08/16 00:13:23, Jered wrote:
> omit either "is not initialized"  or "is not created"

Done.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
File chrome/browser/search/instant_unittest_base.h (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_unittest_base.h:27: void SetSearchProvider(const
std::string& base_url);
On 2013/08/16 00:13:23, Jered wrote:
> Call this SetDefaultSearchProvider

Done.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/search/ins...
chrome/browser/search/instant_unittest_base.h:34: int
NumObservers(InstantService* instant_service) {
On 2013/08/16 00:13:23, Jered wrote:
> GetNumObservers

Done.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller_unittest.cc (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:98: size_t numTestCases
= arraysize(kTabReloadTestCases);
On 2013/08/16 00:13:23, Jered wrote:
> const size_t num_tests =

Done.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:99:
std::vector<linked_ptr<FakeWebContentsObserver> > observers;
On 2013/08/16 00:13:23, Jered wrote:
> Instead of vector<linked_ptr> use ScopedVector.

Done.

https://codereview.chromium.org/20388003/diff/62001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller_unittest.cc:117:
SetSearchProvider("https://bar.com/");
On 2013/08/16 00:13:23, Jered wrote:
> Why are we queuing up a bunch of tabs, doing this, and then checking them all,
> instead of testing one case at a time?

I kind of followed process-isolation tests in search_unittest.cc.
There are 2 similar looking tests right now. With one tab per test, I will have
8.

https://codereview.chromium.org/20388003/diff/62001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/chrome_browser.gyp...
chrome/chrome_browser.gypi:1810: 'browser/search/instant_service_observer.cc',
On 2013/08/16 00:13:23, Jered wrote:
> Move up one line.

Done.

https://codereview.chromium.org/20388003/diff/62001/chrome/chrome_tests_unit....
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/20388003/diff/62001/chrome/chrome_tests_unit....
chrome/chrome_tests_unit.gypi:163: 'browser/search/instant_unittest_base.cc',
On 2013/08/16 00:13:23, Jered wrote:
> Move up one line.

Done.

https://codereview.chromium.org/20388003/diff/62001/chrome/chrome_tests_unit....
chrome/chrome_tests_unit.gypi:1353:
'browser/ui/browser_instant_controller_unittest.cc',
On 2013/08/16 00:13:23, Jered wrote:
> Sort down lower.

Done.

[Anuj, 2013-08-22 01:52:08]

I decided to keep the test with the test definition format, as we actually need
to cover at least 3 scenario.
Local NTP : Instant -> Instant (reloads)
Remote NTP/SERP : Instant -> Not Instant (reloads)
Other : Not Instant -> Not Instant (no reload)

I think that much duplication tilts the scale in favor of this style.

[Anuj, 2013-08-26 17:13:08]

Ping.

[samarth, 2013-08-27 22:43:16]

Sorry, did not realize you were still waiting for me. SLGTM.

[commit-bot: I haz the power, 2013-08-28 01:58:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/20388003/72001

[commit-bot: I haz the power, 2013-08-28 03:39:10]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Anuj Sharma, 2013-08-28 05:15:04]

Need OWNERS approval
for chrome/browser/ui/browser_instant_controller_unittest.cc. Who can
provide that?


On Tue, Aug 27, 2013 at 8:39 PM, <commit-bot@chromium.org> wrote:

> Retried try job too often on chromium_presubmit for step(s) presubmit
> http://build.chromium.org/p/**tryserver.chromium/**
>
buildstatus?builder=chromium_**presubmit&number=22694<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=22694>
>
>
https://chromiumcodereview.**appspot.com/20388003/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Anuj, 2013-08-28 17:28:43]

Per discussion with Samarth, I am submitting this TBR=sky@, as the difference is
just a new unittest.

[commit-bot: I haz the power, 2013-08-28 17:43:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/20388003/84001

[commit-bot: I haz the power, 2013-08-28 17:43:41]

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

Patch:       chrome/browser/ui/search/instant_extended_interactive_uitest.cc
Index: chrome/browser/ui/search/instant_extended_interactive_uitest.cc
diff --git a/chrome/browser/ui/search/instant_extended_interactive_uitest.cc
b/chrome/browser/ui/search/instant_extended_interactive_uitest.cc
index
dd6544bf579ccc5e0b25143564ac223fb9fff447..4c3f3580ba2a418b5064652c5f88c4bccaa9e333
100644
--- a/chrome/browser/ui/search/instant_extended_interactive_uitest.cc
+++ b/chrome/browser/ui/search/instant_extended_interactive_uitest.cc
@@ -1538,118 +1538,3 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
   EXPECT_TRUE(UpdateSearchState(active_tab));
   EXPECT_EQ(1, on_most_visited_change_calls_);
 }
-
-// Flaky: crbug.com/267096
-IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
-                       DISABLED_OnDefaultSearchProviderChanged) {
-  InstantService* instant_service =
-      InstantServiceFactory::GetForProfile(browser()->profile());
-  ASSERT_NE(static_cast<InstantService*>(NULL), instant_service);
-
-  // Setup Instant.
-  ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
-  FocusOmniboxAndWaitForInstantNTPSupport();
-  EXPECT_EQ(1, instant_service->GetInstantProcessCount());
-
-  // Navigating to the NTP should use the Instant render process.
-  content::WindowedNotificationObserver new_tab_observer(
-      content::NOTIFICATION_NAV_ENTRY_COMMITTED,
-      content::NotificationService::AllSources());
-  ui_test_utils::NavigateToURLWithDisposition(
-      browser(),
-      GURL(chrome::kChromeUINewTabURL),
-      CURRENT_TAB,
-      ui_test_utils::BROWSER_TEST_NONE);
-  new_tab_observer.Wait();
-
-  content::WebContents* ntp_contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  EXPECT_TRUE(chrome::IsInstantNTP(ntp_contents));
-  EXPECT_TRUE(instant_service->IsInstantProcess(
-      ntp_contents->GetRenderProcessHost()->GetID()));
-  GURL ntp_url = ntp_contents->GetURL();
-
-  AddBlankTabAndShow(browser());
-  content::WebContents* active_tab =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  EXPECT_FALSE(chrome::IsInstantNTP(active_tab));
-  EXPECT_FALSE(instant_service->IsInstantProcess(
-      active_tab->GetRenderProcessHost()->GetID()));
-
-  TemplateURLData data;
-  data.short_name = ASCIIToUTF16("t");
-  data.SetURL("http://defaultturl/q={searchTerms}");
-  data.suggestions_url = "http://defaultturl2/q={searchTerms}";
-  data.instant_url = "http://does/not/exist";
-  data.alternate_urls.push_back(data.instant_url + "#q={searchTerms}");
-  data.search_terms_replacement_key = "strk";
-
-  TemplateURL* template_url = new TemplateURL(browser()->profile(), data);
-  TemplateURLService* service =
-      TemplateURLServiceFactory::GetForProfile(browser()->profile());
-  ui_test_utils::WaitForTemplateURLServiceToLoad(service);
-  service->Add(template_url);  // Takes ownership of |template_url|.
-
-  // Change the default search provider.
-  content::WindowedNotificationObserver observer(
-      content::NOTIFICATION_LOAD_STOP,
-      content::Source<content::NavigationController>(
-          &ntp_contents->GetController()));
-  service->SetDefaultSearchProvider(template_url);
-  observer.Wait();
-
-  // |ntp_contents| should not use the Instant render process.
-  EXPECT_FALSE(chrome::IsInstantNTP(ntp_contents));
-  EXPECT_FALSE(instant_service->IsInstantProcess(
-      ntp_contents->GetRenderProcessHost()->GetID()));
-  // Make sure the URL remains the same.
-  EXPECT_EQ(ntp_url, ntp_contents->GetURL());
-}
-
-IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
-                       ReloadLocalNTPOnSearchProviderChange) {
-  // Setup Instant.
-  ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
-  FocusOmniboxAndWaitForInstantNTPSupport();
-
-  // Navigate to Local NTP.
-  content::WindowedNotificationObserver new_tab_observer(
-      content::NOTIFICATION_NAV_ENTRY_COMMITTED,
-      content::NotificationService::AllSources());
-  ui_test_utils::NavigateToURLWithDisposition(
-      browser(),
-      GURL(chrome::kChromeSearchLocalNtpUrl),
-      CURRENT_TAB,
-      ui_test_utils::BROWSER_TEST_NONE);
-  new_tab_observer.Wait();
-
-  content::WebContents* ntp_contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  GURL ntp_url = ntp_contents->GetURL();
-
-  TemplateURLData data;
-  data.short_name = ASCIIToUTF16("t");
-  data.SetURL("http://defaultturl/q={searchTerms}");
-  data.suggestions_url = "http://defaultturl2/q={searchTerms}";
-  data.instant_url = "http://does/not/exist";
-  data.alternate_urls.push_back(data.instant_url + "#q={searchTerms}");
-  data.search_terms_replacement_key = "strk";
-
-  TemplateURL* template_url = new TemplateURL(browser()->profile(), data);
-  TemplateURLService* service =
-      TemplateURLServiceFactory::GetForProfile(browser()->profile());
-  ui_test_utils::WaitForTemplateURLServiceToLoad(service);
-  service->Add(template_url);  // Takes ownership of |template_url|.
-
-  // Change the default search provider. This will reload the local NTP and the
-  // page URL will remain the same.
-  content::WindowedNotificationObserver observer(
-      content::NOTIFICATION_LOAD_STOP,
-      content::Source<content::NavigationController>(
-          &ntp_contents->GetController()));
-  service->SetDefaultSearchProvider(template_url);
-  observer.Wait();
-
-  // Make sure the URL remains the same.
-  EXPECT_EQ(ntp_url, ntp_contents->GetURL());
-}

[commit-bot: I haz the power, 2013-08-28 20:14:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/20388003/90001

[commit-bot: I haz the power, 2013-08-28 21:19:24]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-08-28 21:39:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/20388003/90001

[commit-bot: I haz the power, 2013-08-28 22:02:38]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-29 00:42:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/20388003/116001

[commit-bot: I haz the power, 2013-08-29 01:35:12]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-08-29 07:08:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/20388003/135001

[commit-bot: I haz the power, 2013-08-29 08:04:20]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-29 09:28:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/20388003/151001

[commit-bot: I haz the power, 2013-08-29 10:23:41]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-08-29 18:04:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/20388003/168001

[commit-bot: I haz the power, 2013-08-29 18:39:01]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-29 22:01:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/20388003/109001

[commit-bot: I haz the power, 2013-08-29 22:51:01]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-05 07:22:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/20388003/199001

[commit-bot: I haz the power, 2013-09-05 08:19:49]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[Jered, 2013-09-17 16:57:25]

On 2013/09/05 08:19:49, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_aura for step(s) unit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

Did you still want to submit this?

[Anuj Sharma, 2013-09-17 18:31:24]

Yes :(

I couldn't figure out the test breakage. Will ping for help sometime today.


On Tue, Sep 17, 2013 at 9:57 AM, <jered@chromium.org> wrote:

> On 2013/09/05 08:19:49, I haz the power (commit-bot) wrote:
>
>> Retried try job too often on linux_aura for step(s) unit_tests
>>
>
> http://build.chromium.org/p/**tryserver.chromium/**
>
buildstatus?builder=linux_**aura&number=74374<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=74374>
>
> Did you still want to submit this?
>
>
https://codereview.chromium.**org/20388003/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-09-24 01:04:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/20388003/224001

[commit-bot: I haz the power, 2013-09-24 07:07:37]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-24 18:13:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/20388003/224001

[commit-bot: I haz the power, 2013-09-24 21:33:44]

Change committed as 225063