Handle TemplateURLService load failure better

Handle TemplateURLService load failure better, and make some test correctness fixes that will be needed later.

The test fixes in this were generally obtained from https://codereview.chromium.org/268643002/ .

This also does a variety of miscellaneous cleanups to the modified files.

BUG=364183
TEST=none

[Peter Kasting, 2014-05-02 22:58:16]

For a sample of the weirdness this addresses, see
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...
, which was triggered by changing the default Google base URL from HTTP to HTTPS
-- something that seemingly has no connection to the test.

[erikwright (departed), 2014-05-03 01:01:54]

https://codereview.chromium.org/263823007/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_service.cc (left):

https://codereview.chromium.org/263823007/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.cc:633: return ((!loaded_ ||
load_failed_) &&
Now that we copied initial_default_search_provider_ over, isn't it inappropriate
to reference it if load_failed_?

(Also futile, since initial... is NULL).

Here and the next few methods.

https://codereview.chromium.org/263823007/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.cc:861: if (loaded_ &&
!load_failed_)
Ditto here.

In fact, load_failed_ basically seems useless now since there is also no
accessor for it...

https://codereview.chromium.org/263823007/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/263823007/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.cc:1003: load_failed_ = true;
Shouldn't this also be set in the '!service_' case in TUS::Load()?

[Peter Kasting, 2014-05-05 18:46:24]

+zea as a reviewer: Please review change to
TemplateURLService::MergeDataAndStartSyncing() to ensure this is the correct way
to locally disable search engine sync for the remainder of this run (but not
permanently).

erikwright: You're right, the usage of |load_failed_| made no sense.

I tried just setting |load_failed_| from Load() the way
OnWebDataServiceRequestDone() was already doing, and nuking most of the rest of
the change.  That makes some unittests work, but others break because setting
that variable means that further, manual attempts to modify the default search
engine will be ignored.  Tests really assume that "Load()" puts the service into
a fully-functional state.

The solution in this patch set is the result of trying to make this more sane
and make tests pass.  More specific comments on these changes below.

https://codereview.chromium.org/263823007/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/263823007/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.cc:1003: load_failed_ = true;
On 2014/05/03 01:01:54, erikwright wrote:
> Shouldn't this also be set in the '!service_' case in TUS::Load()?

Yes.  Moved to OnFailedLoad().

https://codereview.chromium.org/263823007/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/263823007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:421: base::string16
TemplateURLService::GenerateKeyword(const GURL& url) {
This apparent diff is just fallout from moving
SaveDefaultSearchProviderToPrefs() lower down within the file.

https://codereview.chromium.org/263823007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:633: return (!loaded_ &&
initial_default_search_provider_.get() &&
Leaving the "load_failed_" checks in these next three functions is actively
wrong, since it means that upon failed load, we'll continue to try to reference
|initial_default_search_provider_|, which we don't want to pay attention to
anymore.

That said, in a patch set I haven't yet uploaded, I also tweaked OnFailedLoad()
to ensure |initial_default_search_provider_| is reset in all cases.

https://codereview.chromium.org/263823007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:811:
service_->UpdateKeyword(url->data());
I removed some unnecessary get()s here and below while I was touching the file. 
No functional change in these sorts of cases, I just noticed them and wanted to
clean them up.

https://codereview.chromium.org/263823007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:858: return loaded_ ?
Another case where examining |load_failed_| is actually wrong.

https://codereview.chromium.org/263823007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:1274: // Disable sync if
we failed to load.
Because the in-memory state won't match the on-disk database (and thus the
server-side state), I don't think we want to try to sync anything -- we could
wind up sending the server undesirable changes.

https://codereview.chromium.org/263823007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:1791: if
(!GetDefaultSearchProvider()) {
Some tests set a new default search provider before reaching this point.  This
conditional causes us to respect those changes.

https://codereview.chromium.org/263823007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:1796:
default_search_provider->id() == kInvalidTemplateURLID);
We want AddNoNotify() to ensure the provided TemplateURL gets an ID, since
various pieces of the TemplateURL system rely on this, so we can't pass "false"
here.  However, |initial_default_search_provider_| may already have an ID if
it's being loaded from prefs on disk instead of initialized directly from the
prepopulate data during first run, so we can't pass "true" either.  Instead we
tell AddNoNotify() to give the URL an ID iff it's necessary.

https://codereview.chromium.org/263823007/diff/20001/chrome/browser/ui/search...
File chrome/browser/ui/search_engines/keyword_editor_controller_unittest.cc
(right):

https://codereview.chromium.org/263823007/diff/20001/chrome/browser/ui/search...
chrome/browser/ui/search_engines/keyword_editor_controller_unittest.cc:128: int
original_row_count = table_model()->RowCount();
This file comes directly from your CL you sent me for review -- turns out I need
many of the same things in order for my tests to pass, and these changes are
just more correct in general than the existing code anyway.

[Peter Kasting, 2014-05-05 21:22:02]

Patch set 3 mainly differs from patch set 2 by adding a bunch more code from
https://codereview.chromium.org/268643002/ to fix instant-related tests.  The
instant service really should have been observing the TemplateURLService, and
not the prefs, to begin with.

This also clears |initial_default_search_provider_| unconditionally in
TemplateURLService::OnFailedLoad().

[Nicolas Zea, 2014-05-05 21:23:59]

https://codereview.chromium.org/263823007/diff/40001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/263823007/diff/40001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:1275: if (load_failed_) {
Under what conditions is the load expected to fail, and is that meant to be a
permanent error? This will prevent search engine sync from working until the
next restart of Chrome.

Another option might be delaying the startup of the search engines datatype
until some condition is met. See for example
search_engine_data_type_controller.cc's StartModels method, which waits for the
TemplateURLService to load.

[erikwright (departed), 2014-05-05 21:26:40]

On 2014/05/05 21:23:59, Nicolas Zea wrote:
>
https://codereview.chromium.org/263823007/diff/40001/chrome/browser/search_en...
> File chrome/browser/search_engines/template_url_service.cc (right):
> 
>
https://codereview.chromium.org/263823007/diff/40001/chrome/browser/search_en...
> chrome/browser/search_engines/template_url_service.cc:1275: if (load_failed_)
{
> Under what conditions is the load expected to fail, and is that meant to be a
> permanent error? This will prevent search engine sync from working until the
> next restart of Chrome.
> 
> Another option might be delaying the startup of the search engines datatype
> until some condition is met. See for example
> search_engine_data_type_controller.cc's StartModels method, which waits for
the
> TemplateURLService to load.

A lot of this CL seems to be obviated by my own CL. Is it correct to say that
the main thing added is not sync'ing when we fail to load?

[Peter Kasting, 2014-05-05 21:33:31]

https://codereview.chromium.org/263823007/diff/40001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/263823007/diff/40001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:1275: if (load_failed_) {
On 2014/05/05 21:23:59, Nicolas Zea wrote:
> Under what conditions is the load expected to fail, and is that meant to be a
> permanent error? This will prevent search engine sync from working until the
> next restart of Chrome.

(1) Various tests
(2) Disk corruption preventing us from correctly loading the search engines from
disk

In both these cases, the TemplateURLService won't "recover" from this state, so
halting search engine sync until the next restart is the desired behavior.  My
concern was whether this would halt search engine sync indefinitely (across
restarts) until the user re-enabled the datatype.  If that's not true, I'm
happy.

> Another option might be delaying the startup of the search engines datatype
> until some condition is met. See for example
> search_engine_data_type_controller.cc's StartModels method, which waits for
the
> TemplateURLService to load.

We could instead add more code to that class to check whether the load failed
and just never start model association.  This would require me to expose an
accessor for TemplateURLService::load_failed_, which isn't a big deal.  It seems
like both methods would work; tell me which you prefer and I'll comply.

[Nicolas Zea, 2014-05-05 21:36:17]

Sounds like this has the intended effect, so LGTM. I'm fine with the current
approach rather than modifying the datatype controller.

[Peter Kasting, 2014-05-05 21:56:51]

On 2014/05/05 21:26:40, erikwright wrote:
> A lot of this CL seems to be obviated by my own CL. Is it correct to say that
> the main thing added is not sync'ing when we fail to load?

Your CL accomplishes the main purpose of this, right?  (Namely, that we continue
to report the correct prepopulated engine as default after a direct call to
TemplateURLService::Load(), e.g. in a test, which today will cause the service
to erroneously begin reporting NULL since that initial value is never copied
into the "real" memory model anywhere.)

Besides that purpose, this change only does the following:
(1) Test changes I copied from you which are correct anyway even before the rest
of either of our changes
(2) Disable sync and NULL out |service_| on a failed load, so we don't persist
any changes anywhere
(3) Some non-functional cleanups, e.g. removing get()s
(4) Some stuff that doesn't need to happen once your change lands (making the
"else" case in Load() be treated like failure, the changes in
template_url_service_sync_unittest.cc)

We could try and land (1) to break your change into pieces a little bit,
especially since there's a chance it might get reverted.  (2) and (3) could be
incorporated into your change or I could land them first and you could rebase. 
(4) would just get thrown away.

What would you like to do?

[erikwright (departed), 2014-05-05 22:16:18]

I think Robert's dream is that I solve this remaining Sync problem tonight
and land my CL. So anything that would delay that (rebasing, splitting it
up) he would be opposed to.

While the sync issue might be hard to resolve tonight, I can't imagine a
world where you don't have my best possible shot at it in your queue
tomorrow morning at the latest, so I think what I would prefer is that you
give me all of your feedback on the big CL before leaving tonight, and
hopefully there is only that little bit to look at in the morning.

Then the contents of your CL that remain relevant can be landed afterwards.
I will offer to do the rebasing if you like.


On Mon, May 5, 2014 at 5:56 PM, <pkasting@chromium.org> wrote:

> On 2014/05/05 21:26:40, erikwright wrote:
>
>> A lot of this CL seems to be obviated by my own CL. Is it correct to say
>> that
>> the main thing added is not sync'ing when we fail to load?
>>
>
> Your CL accomplishes the main purpose of this, right?  (Namely, that we
> continue
> to report the correct prepopulated engine as default after a direct call to
> TemplateURLService::Load(), e.g. in a test, which today will cause the
> service
> to erroneously begin reporting NULL since that initial value is never
> copied
> into the "real" memory model anywhere.)
>
> Besides that purpose, this change only does the following:
> (1) Test changes I copied from you which are correct anyway even before
> the rest
> of either of our changes
> (2) Disable sync and NULL out |service_| on a failed load, so we don't
> persist
> any changes anywhere
> (3) Some non-functional cleanups, e.g. removing get()s
> (4) Some stuff that doesn't need to happen once your change lands (making
> the
> "else" case in Load() be treated like failure, the changes in
> template_url_service_sync_unittest.cc)
>
> We could try and land (1) to break your change into pieces a little bit,
> especially since there's a chance it might get reverted.  (2) and (3)
> could be
> incorporated into your change or I could land them first and you could
> rebase.
> (4) would just get thrown away.
>
> What would you like to do?
>
> https://codereview.chromium.org/263823007/
>

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

[Peter Kasting, 2014-05-05 22:22:19]

On Mon, May 5, 2014 at 3:16 PM, Erik Wright <erikwright@chromium.org> wrote:

> I think Robert's dream is that I solve this remaining Sync problem tonight
> and land my CL.
>

I don't think that's going to happen.  This stuff is too complicated and
messy.

Furthermore, your change is so big and comparatively risky that it has a
reasonable chance of being reverted one or more times before it sticks.
 Splitting out pieces of it ought to reduce that risk (and the difficulty
of getting relands re-reviewed) rather than increase it.

PK

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

[robertshield, 2014-05-05 22:29:06]

On Mon, May 5, 2014 at 6:22 PM, Peter Kasting <pkasting@chromium.org> wrote:

> On Mon, May 5, 2014 at 3:16 PM, Erik Wright <erikwright@chromium.org>wrote:
>
>> I think Robert's dream is that I solve this remaining Sync problem
>> tonight and land my CL.
>>
>
> I don't think that's going to happen.  This stuff is too complicated and
> messy.
>

Agreed. https://codereview.chromium.org/268643002/ is a complicated CL and
I don't see a consensus emerging on some of the key sync points just yet.


>
> Furthermore, your change is so big and comparatively risky that it has a
> reasonable chance of being reverted one or more times before it sticks.
>  Splitting out pieces of it ought to reduce that risk (and the difficulty
> of getting relands re-reviewed) rather than increase it.
>

Also completely agreed. Landing as many of the uncontroversial parts of the
mega CL is the best way to reduce its risk and is more likely to accelerate
landing the whole.



>
> PK
>

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

[Peter Kasting, 2014-05-06 01:23:23]

Patch set 4 aims to fix another test failure that occurred with both patch set 3
and Erik's CL.

This rips out InstantServiceObserver::GoogleURLUpdated() entirely.  If the
Google URL update affected any template URLs,
InstantService::OnTemplateURLServiceChanged() will get called (as a result of
the earlier changes here to observe TemplateURLService instead of the default
browser prefs).  If it didn't, the InstantService doesn't need to do anything
anyway.

This also removes a bunch of unnecessary #includes in instant_service.* and does
some misc. other cleanup while I was touching them.

[Peter Kasting, 2014-05-06 18:12:58]

OK, patch set 5 is hopefully a candidate to land.

This removes the original reason for this patch set -- copying the initial
default search engine from the before-load to the after-load state -- in favor
of letting https://codereview.chromium.org/268643002/ solve that problem.

This makes the primary purposes of this patch:
(1) Split out test fixes from that CL to make it simpler, on the assumption that
this change will land first.
(2) Cleanup.
(3) On a failed load, don't persist anything further to prefs, sync, or the web
data service.  There aren't known outstanding bugs about this, it just seems
like the sane thing to do.

Erik: Please review everything.
Samarth: OWNERS for c/b/search/.

[erikwright (departed), 2014-05-06 19:48:02]

LGTM.

The not writing to Prefs thing is up to you, I guess. It's not clear to me that
it's right.

https://codereview.chromium.org/263823007/diff/80001/chrome/browser/search/in...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/263823007/diff/80001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:75:
template_url_service->AddObserver(this);
Should we do a RemoveObserver in the destructor?

https://codereview.chromium.org/263823007/diff/80001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/263823007/diff/80001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:501: if (!prefs ||
load_failed_)
This seems strange to me. Wouldn't it be better to write to prefs in this case?
There are two possibilities:

(1) You load Web Data correctly next time. Great! When we boot up, we will
initially use the value that we shut down with, but then we will restore the
value from Web Data shortly.
(2) We are never able to load Web Data. In that case, at least the user is able
to persist a DSE choice (even if keywords cannot be persisted).

I know that historically some clients have gotten into state (2), at least in
the case of the cookie store. While shess@ has implemented some recovery logic
for many cases I would not assume that it never happens.

[Peter Kasting, 2014-05-06 19:58:03]

https://codereview.chromium.org/263823007/diff/80001/chrome/browser/search/in...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/263823007/diff/80001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:75:
template_url_service->AddObserver(this);
On 2014/05/06 19:48:02, erikwright wrote:
> Should we do a RemoveObserver in the destructor?

Yes.  Good catch.

https://codereview.chromium.org/263823007/diff/80001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/263823007/diff/80001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:501: if (!prefs ||
load_failed_)
On 2014/05/06 19:48:02, erikwright wrote:
> This seems strange to me. Wouldn't it be better to write to prefs in this
case?
> There are two possibilities:
> 
> (1) You load Web Data correctly next time. Great! When we boot up, we will
> initially use the value that we shut down with, but then we will restore the
> value from Web Data shortly.
> (2) We are never able to load Web Data. In that case, at least the user is
able
> to persist a DSE choice (even if keywords cannot be persisted).
> 
> I know that historically some clients have gotten into state (2), at least in
> the case of the cookie store. While shess@ has implemented some recovery logic
> for many cases I would not assume that it never happens.

For a world where prefs are just a cache until the real system starts up, I
think it's more arguable that writing to prefs in a failed load state is OK. 
But since we're changing the prefs to be authoritative, I don't think we want to
write to them.  Basically, when the load fails, our system state is
inconsistent, and at that point I'm more comfortable locking out all changes
than I am with the idea that we could persist some of them to one of our data
stores.

I do think you're right that some people may be in a hosed state for a long
period of time, but in that case I'd almost rather be more broken than less
broken, to try and make it as clear as possible that the user needs to pay
attention to those "your profile could not be opened correctly" dialogs and fix
the issue.

[erikwright (departed), 2014-05-06 20:04:47]

Hmm. Then why allow the user to change it at all?

It will actually be burdensome to implement the logic you are describing in
my CL. Once we realize the Web DB load failed, we will need to enter a
state where we respect the value we currently have from prefs, but no
permit future writing of that value to prefs. But we still presumably want
to respect changes to the pref from policy. And also have to allow the user
to set a DSE, but only if extensions or policy have not overridden it...


On Tue, May 6, 2014 at 3:58 PM, <pkasting@chromium.org> wrote:

>
> https://codereview.chromium.org/263823007/diff/80001/
> chrome/browser/search/instant_service.cc
> File chrome/browser/search/instant_service.cc (right):
>
> https://codereview.chromium.org/263823007/diff/80001/
> chrome/browser/search/instant_service.cc#newcode75
> chrome/browser/search/instant_service.cc:75:
> template_url_service->AddObserver(this);
> On 2014/05/06 19:48:02, erikwright wrote:
>
>> Should we do a RemoveObserver in the destructor?
>>
>
> Yes.  Good catch.
>
>
> https://codereview.chromium.org/263823007/diff/80001/
> chrome/browser/search_engines/template_url_service.cc
> File chrome/browser/search_engines/template_url_service.cc (right):
>
> https://codereview.chromium.org/263823007/diff/80001/
> chrome/browser/search_engines/template_url_service.cc#newcode501
> chrome/browser/search_engines/template_url_service.cc:501: if (!prefs ||
> load_failed_)
> On 2014/05/06 19:48:02, erikwright wrote:
>
>> This seems strange to me. Wouldn't it be better to write to prefs in
>>
> this case?
>
>> There are two possibilities:
>>
>
>  (1) You load Web Data correctly next time. Great! When we boot up, we
>>
> will
>
>> initially use the value that we shut down with, but then we will
>>
> restore the
>
>> value from Web Data shortly.
>> (2) We are never able to load Web Data. In that case, at least the
>>
> user is able
>
>> to persist a DSE choice (even if keywords cannot be persisted).
>>
>
>  I know that historically some clients have gotten into state (2), at
>>
> least in
>
>> the case of the cookie store. While shess@ has implemented some
>>
> recovery logic
>
>> for many cases I would not assume that it never happens.
>>
>
> For a world where prefs are just a cache until the real system starts
> up, I think it's more arguable that writing to prefs in a failed load
> state is OK.  But since we're changing the prefs to be authoritative, I
> don't think we want to write to them.  Basically, when the load fails,
> our system state is inconsistent, and at that point I'm more comfortable
> locking out all changes than I am with the idea that we could persist
> some of them to one of our data stores.
>
> I do think you're right that some people may be in a hosed state for a
> long period of time, but in that case I'd almost rather be more broken
> than less broken, to try and make it as clear as possible that the user
> needs to pay attention to those "your profile could not be opened
> correctly" dialogs and fix the issue.
>
> https://codereview.chromium.org/263823007/
>

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

[Peter Kasting, 2014-05-06 20:09:04]

On 2014/05/06 20:04:47, erikwright wrote:
> Hmm. Then why allow the user to change it at all?
> 
> It will actually be burdensome to implement the logic you are describing in
> my CL. Once we realize the Web DB load failed, we will need to enter a
> state where we respect the value we currently have from prefs, but no
> permit future writing of that value to prefs. But we still presumably want
> to respect changes to the pref from policy. And also have to allow the user
> to set a DSE, but only if extensions or policy have not overridden it...

We shouldn't need to do any checks anywhere to see if policy or the user is
allowed to change the DSE.  The only thing we want to do is, when the load
fails, disable all writing to everywhere.  If that breaks something visible --
e.g. if the user tries to change the DSE, but that's only reflected if we can
actually write that to a pref somewhere, and since we can't write it, it appears
to fail -- that's fine.  We don't care about broken-looking behavior.  We only
care about not touching any state on disk.

[erikwright (departed), 2014-05-06 20:11:02]

OK. Does this also imply that no sync events should be sent either, in that
scenario?


On Tue, May 6, 2014 at 4:09 PM, <pkasting@chromium.org> wrote:

> On 2014/05/06 20:04:47, erikwright wrote:
>
>> Hmm. Then why allow the user to change it at all?
>>
>
>  It will actually be burdensome to implement the logic you are describing
>> in
>> my CL. Once we realize the Web DB load failed, we will need to enter a
>> state where we respect the value we currently have from prefs, but no
>> permit future writing of that value to prefs. But we still presumably want
>> to respect changes to the pref from policy. And also have to allow the
>> user
>> to set a DSE, but only if extensions or policy have not overridden it...
>>
>
> We shouldn't need to do any checks anywhere to see if policy or the user is
> allowed to change the DSE.  The only thing we want to do is, when the load
> fails, disable all writing to everywhere.  If that breaks something
> visible --
> e.g. if the user tries to change the DSE, but that's only reflected if we
> can
> actually write that to a pref somewhere, and since we can't write it, it
> appears
> to fail -- that's fine.  We don't care about broken-looking behavior.  We
> only
> care about not touching any state on disk.
>
> https://codereview.chromium.org/263823007/
>

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

[Peter Kasting, 2014-05-06 20:12:12]

On 2014/05/06 20:11:02, erikwright wrote:
> OK. Does this also imply that no sync events should be sent either, in that
> scenario?

Correct, and they won't be, because we've never successfully associated the
models, so all the sync-handling code will automatically no-op.  That's the
purpose of the change in MergeDataAndStartSyncing().

[Peter Kasting, 2014-05-06 22:51:06]

+Jered as he may be able to review this sooner than Samarth

[Jered, 2014-05-06 23:01:18]

Thanks for the cleanup, I have a couple questions so will need to study a bit
more before I'm sure this is ok, perhaps you can answer.

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (left):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:409: void
InstantService::OnGoogleURLUpdated(
Is this notification now totally unused? What about, for example, search domain
check?

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:41: // Helpers
--------------------------------------------------------------------
Please omit these section comments, they don't really help with readability imo.

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:406: if
(new_search_result_prefetch_base_url ==
This test prevents notifying observers about the change whenever the prefetch
base url hasn't changed. That seems not correct.

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.h (right):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.h:116: 
nit: I think you can omit this newline

[Peter Kasting, 2014-05-06 23:17:22]

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (left):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:409: void
InstantService::OnGoogleURLUpdated(
On 2014/05/06 23:01:18, Jered wrote:
> Is this notification now totally unused? What about, for example, search
domain
> check?

Do you mean, will searchdomaincheck changes cause the InstantService to be
updated?  Yes; they will cause the TemplateURLService to update, which will then
notify OnTemplateURLServiceChanged().  This is verified by unittests.

If you mean, is any code using NOTIFICATION_GOOGLE_URL_UPDATED other than
InstantService, yes, there are still some uses.

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:406: if
(new_search_result_prefetch_base_url ==
On 2014/05/06 23:01:18, Jered wrote:
> This test prevents notifying observers about the change whenever the prefetch
> base url hasn't changed. That seems not correct.

When isn't it correct?

(We have to filter _some_ notifications, because this function is now reached
every time the TemplateURLService changes in _any_ way, so the question is what
the correct filter is.  This code makes tests pass, but perhaps it is too
restrictive?)

[Jered, 2014-05-07 00:08:26]

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (left):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:409: void
InstantService::OnGoogleURLUpdated(
On 2014/05/06 23:17:23, Peter Kasting wrote:
> On 2014/05/06 23:01:18, Jered wrote:
> > Is this notification now totally unused? What about, for example, search
> domain
> > check?
> 
> Do you mean, will searchdomaincheck changes cause the InstantService to be
> updated?  Yes; they will cause the TemplateURLService to update, which will
then
> notify OnTemplateURLServiceChanged().  This is verified by unittests.
The unit test helper seems to not use GoogleURLTracker, so I'm not sure I trust
it.
See InstantUnitTestBase::NotifyGoogleBaseURLUpdate().

> 
> If you mean, is any code using NOTIFICATION_GOOGLE_URL_UPDATED other than
> InstantService, yes, there are still some uses.

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:406: if
(new_search_result_prefetch_base_url ==
On 2014/05/06 23:17:23, Peter Kasting wrote:
> On 2014/05/06 23:01:18, Jered wrote:
> > This test prevents notifying observers about the change whenever the
prefetch
> > base url hasn't changed. That seems not correct.
> 
> When isn't it correct?
> 
> (We have to filter _some_ notifications, because this function is now reached
> every time the TemplateURLService changes in _any_ way, so the question is
what
> the correct filter is.  This code makes tests pass, but perhaps it is too
> restrictive?)

BrowserInstantController::DefaultSearchProviderChanged() expects to be notified
when the DSP changes so that it can reload tabs in the instant process. That's
done by the FOR_EACH_OBSERVER statement, below, which this early return skips
when there's no change in prefetch_base_url, which is a semantically unrelated
field. (e.g. imagine two search engines implement an NTP but both have empty
prefetch URLs; this early return would not dispatch the message we need.)

Instead, how about moving this if-guarding into ResetInstantSearchPrerenderer
itself, or just wrapping the ResetInstantSearchProvider() call inside the if.

[Peter Kasting, 2014-05-07 00:20:17]

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (left):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:409: void
InstantService::OnGoogleURLUpdated(
On 2014/05/07 00:08:27, Jered wrote:
> On 2014/05/06 23:17:23, Peter Kasting wrote:
> > On 2014/05/06 23:01:18, Jered wrote:
> > > Is this notification now totally unused? What about, for example, search
> > domain
> > > check?
> > 
> > Do you mean, will searchdomaincheck changes cause the InstantService to be
> > updated?  Yes; they will cause the TemplateURLService to update, which will
> then
> > notify OnTemplateURLServiceChanged().  This is verified by unittests.
> The unit test helper seems to not use GoogleURLTracker, so I'm not sure I
trust
> it.

Why not?  It's firing the same notification.  There's no reason to spin up a
whole GoogleURLTracker object in the test and muck with it just to get it to
fire a notification we can send directly.

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:406: if
(new_search_result_prefetch_base_url ==
On 2014/05/07 00:08:27, Jered wrote:
> On 2014/05/06 23:17:23, Peter Kasting wrote:
> > On 2014/05/06 23:01:18, Jered wrote:
> > > This test prevents notifying observers about the change whenever the
> prefetch
> > > base url hasn't changed. That seems not correct.
> > 
> > When isn't it correct?
> > 
> > (We have to filter _some_ notifications, because this function is now
reached
> > every time the TemplateURLService changes in _any_ way, so the question is
> what
> > the correct filter is.  This code makes tests pass, but perhaps it is too
> > restrictive?)
> 
> BrowserInstantController::DefaultSearchProviderChanged() expects to be
notified
> when the DSP changes so that it can reload tabs in the instant process. That's
> done by the FOR_EACH_OBSERVER statement, below, which this early return skips
> when there's no change in prefetch_base_url, which is a semantically unrelated
> field. (e.g. imagine two search engines implement an NTP but both have empty
> prefetch URLs; this early return would not dispatch the message we need.)
> 
> Instead, how about moving this if-guarding into ResetInstantSearchPrerenderer
> itself, or just wrapping the ResetInstantSearchProvider() call inside the if.

If observers care about all DSP changes, not just ones that affect the search
prerenderer, shouldn't they be observing TemplateURLService directly?

The only thing InstantService does in response to _any_ changes to the
TemplateURLService is to reset the search prerenderer when it's affected.  So it
stands to reason that observers should get called back iff that's actually
happened.

[Jered, 2014-05-07 00:47:03]

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (left):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:409: void
InstantService::OnGoogleURLUpdated(
On 2014/05/07 00:20:18, Peter Kasting wrote:
> On 2014/05/07 00:08:27, Jered wrote:
> > On 2014/05/06 23:17:23, Peter Kasting wrote:
> > > On 2014/05/06 23:01:18, Jered wrote:
> > > > Is this notification now totally unused? What about, for example, search
> > > domain
> > > > check?
> > > 
> > > Do you mean, will searchdomaincheck changes cause the InstantService to be
> > > updated?  Yes; they will cause the TemplateURLService to update, which
will
> > then
> > > notify OnTemplateURLServiceChanged().  This is verified by unittests.
> > The unit test helper seems to not use GoogleURLTracker, so I'm not sure I
> trust
> > it.
> 
> Why not?  It's firing the same notification.  There's no reason to spin up a
> whole GoogleURLTracker object in the test and muck with it just to get it to
> fire a notification we can send directly.

Ok. I've convinced myself that TemplateURLService::GoogleBaseURLChanged() gets
called, which should call GoogleBaseURLChanged() and then NotifyObservers().
This is fine.

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:406: if
(new_search_result_prefetch_base_url ==
On 2014/05/07 00:20:18, Peter Kasting wrote:
> On 2014/05/07 00:08:27, Jered wrote:
> > On 2014/05/06 23:17:23, Peter Kasting wrote:
> > > On 2014/05/06 23:01:18, Jered wrote:
> > > > This test prevents notifying observers about the change whenever the
> > prefetch
> > > > base url hasn't changed. That seems not correct.
> > > 
> > > When isn't it correct?
> > > 
> > > (We have to filter _some_ notifications, because this function is now
> reached
> > > every time the TemplateURLService changes in _any_ way, so the question is
> > what
> > > the correct filter is.  This code makes tests pass, but perhaps it is too
> > > restrictive?)
> > 
> > BrowserInstantController::DefaultSearchProviderChanged() expects to be
> notified
> > when the DSP changes so that it can reload tabs in the instant process.
That's
> > done by the FOR_EACH_OBSERVER statement, below, which this early return
skips
> > when there's no change in prefetch_base_url, which is a semantically
unrelated
> > field. (e.g. imagine two search engines implement an NTP but both have empty
> > prefetch URLs; this early return would not dispatch the message we need.)
> > 
> > Instead, how about moving this if-guarding into
ResetInstantSearchPrerenderer
> > itself, or just wrapping the ResetInstantSearchProvider() call inside the
if.
> 
> If observers care about all DSP changes, not just ones that affect the search
> prerenderer, shouldn't they be observing TemplateURLService directly?
Observers care about any DSP changes that affect instant. It makes sense to me
to express that by implementing
InstantServiceObserver::DefaultSearchProviderChanged(), but see below.

> 
> The only thing InstantService does in response to _any_ changes to the
> TemplateURLService is to reset the search prerenderer when it's affected.  So
it
> stands to reason that observers should get called back iff that's actually
> happened.

This argument also seems reasonable. If you prefer to do that, though, please
change the message name below to "InstantSearchPrerendererChanged" and update
the other observers to observe the upstream DefaultSearchProviderChanged
message, instead.

[Peter Kasting, 2014-05-07 00:50:27]

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:406: if
(new_search_result_prefetch_base_url ==
On 2014/05/07 00:47:04, Jered wrote:
> On 2014/05/07 00:20:18, Peter Kasting wrote:
> > The only thing InstantService does in response to _any_ changes to the
> > TemplateURLService is to reset the search prerenderer when it's affected. 
So
> it
> > stands to reason that observers should get called back iff that's actually
> > happened.
> 
> This argument also seems reasonable. If you prefer to do that, though, please
> change the message name below to "InstantSearchPrerendererChanged" and update
> the other observers to observe the upstream DefaultSearchProviderChanged
> message, instead.

The only observer of this is BrowserInstantController, and the only thing it
does in response is call ReloadTabsInInstantProcess().  Isn't that only
necessary when the instant search prerenderer has been reset?  Or are there
other cases where that has to happen, and if so, what are they?

(I'm pretty sure we wouldn't want to call that on _every_ change to the DSP.)

[Jered, 2014-05-07 00:53:07]

On 2014/05/07 00:50:27, Peter Kasting wrote:
>
https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
> File chrome/browser/search/instant_service.cc (right):
> 
>
https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
> chrome/browser/search/instant_service.cc:406: if
> (new_search_result_prefetch_base_url ==
> On 2014/05/07 00:47:04, Jered wrote:
> > On 2014/05/07 00:20:18, Peter Kasting wrote:
> > > The only thing InstantService does in response to _any_ changes to the
> > > TemplateURLService is to reset the search prerenderer when it's affected. 
> So
> > it
> > > stands to reason that observers should get called back iff that's actually
> > > happened.
> > 
> > This argument also seems reasonable. If you prefer to do that, though,
please
> > change the message name below to "InstantSearchPrerendererChanged" and
update
> > the other observers to observe the upstream DefaultSearchProviderChanged
> > message, instead.
> 
> The only observer of this is BrowserInstantController, and the only thing it
> does in response is call ReloadTabsInInstantProcess().  Isn't that only
> necessary when the instant search prerenderer has been reset?  Or are there
> other cases where that has to happen, and if so, what are they?
> 
> (I'm pretty sure we wouldn't want to call that on _every_ change to the DSP.)

This is actually much older code. When you change your DSP, we kick your old
DSP's page out of the priveleged instant process. This also has the effect of
closing old Google NTPs if you, say, switch to bing, which makes some amount of
sense.

[Peter Kasting, 2014-05-07 00:57:14]

On 2014/05/07 00:53:07, Jered wrote:
> This is actually much older code. When you change your DSP, we kick your old
> DSP's page out of the priveleged instant process. This also has the effect of
> closing old Google NTPs if you, say, switch to bing, which makes some amount
of
> sense.

Fine, but that doesn't really answer my question.  Precisely what types of
changes to the DSP should result in calling this ReloadTabsInInstantProcess()
method?  For example, clearly changes to the DSP name or keyword fields
shouldn't.  Probably changes to the search_url shouldn't either.  What fields do
we care about?  instant_url and/or ntp_url?  And what kinds of changes within
them?  Any change?  A change in hostname?  etc.

[Jered, 2014-05-07 00:58:38]

On 2014/05/07 00:57:14, Peter Kasting wrote:
> On 2014/05/07 00:53:07, Jered wrote:
> > This is actually much older code. When you change your DSP, we kick your old
> > DSP's page out of the priveleged instant process. This also has the effect
of
> > closing old Google NTPs if you, say, switch to bing, which makes some amount
> of
> > sense.
> 
> Fine, but that doesn't really answer my question.  Precisely what types of
> changes to the DSP should result in calling this ReloadTabsInInstantProcess()
> method?  For example, clearly changes to the DSP name or keyword fields
> shouldn't.  Probably changes to the search_url shouldn't either.  What fields
do
> we care about?  instant_url and/or ntp_url?  And what kinds of changes within
> them?  Any change?  A change in hostname?  etc.

Answering this requires me to exhaustively understand the contents of a template
url
and evaluate what the side-effects are of each one for instant. That's a lot of
work,
so it's going to take a while.

[Peter Kasting, 2014-05-07 01:01:39]

On 2014/05/07 00:58:38, Jered wrote:
> On 2014/05/07 00:57:14, Peter Kasting wrote:
> > On 2014/05/07 00:53:07, Jered wrote:
> > > This is actually much older code. When you change your DSP, we kick your
old
> > > DSP's page out of the priveleged instant process. This also has the effect
> of
> > > closing old Google NTPs if you, say, switch to bing, which makes some
amount
> > of
> > > sense.
> > 
> > Fine, but that doesn't really answer my question.  Precisely what types of
> > changes to the DSP should result in calling this
ReloadTabsInInstantProcess()
> > method?  For example, clearly changes to the DSP name or keyword fields
> > shouldn't.  Probably changes to the search_url shouldn't either.  What
fields
> do
> > we care about?  instant_url and/or ntp_url?  And what kinds of changes
within
> > them?  Any change?  A change in hostname?  etc.
> 
> Answering this requires me to exhaustively understand the contents of a
template
> url
> and evaluate what the side-effects are of each one for instant. That's a lot
of
> work,
> so it's going to take a while.

In fairness, what this really says is that when the previous version of the code
got written, someone didn't make clear in code or comments what they were
actually trying to do.

Here's another question.  Does
BrowserInstantController::ReloadTabsInInstantProcess() have to happen after
InstantService::ResetInstantSearchPrerenderer() gets called?  If so, then we
can't just make BrowserInstantController blindly observe TemplateURLService,
because that ordering won't be guaranteed.

[erikwright (departed), 2014-05-07 18:40:01]

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:406: if
(new_search_result_prefetch_base_url ==
On 2014/05/07 00:50:27, Peter Kasting wrote:
> On 2014/05/07 00:47:04, Jered wrote:
> > On 2014/05/07 00:20:18, Peter Kasting wrote:
> > > The only thing InstantService does in response to _any_ changes to the
> > > TemplateURLService is to reset the search prerenderer when it's affected. 
> So
> > it
> > > stands to reason that observers should get called back iff that's actually
> > > happened.
> > 
> > This argument also seems reasonable. If you prefer to do that, though,
please
> > change the message name below to "InstantSearchPrerendererChanged" and
update
> > the other observers to observe the upstream DefaultSearchProviderChanged
> > message, instead.
> 
> The only observer of this is BrowserInstantController, and the only thing it
> does in response is call ReloadTabsInInstantProcess().  Isn't that only
> necessary when the instant search prerenderer has been reset?  Or are there
> other cases where that has to happen, and if so, what are they?
> 
> (I'm pretty sure we wouldn't want to call that on _every_ change to the DSP.)

The BrowserInstantController calls InstantService::SendSearchURLSToRenderer,
which sends the values of all of the URLs from the default TemplateURL to the
renderer.

In other words, this does not appear to be correct at the moment.

I think the safest thing is to just trigger the event on every notification from
the TemplateURLService. In the future it would be great if the service made a
distinction between changes to any keyword and changes to the DSE.

[Peter Kasting, 2014-05-07 18:43:36]

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:406: if
(new_search_result_prefetch_base_url ==
On 2014/05/07 18:40:01, erikwright wrote:
> The BrowserInstantController calls InstantService::SendSearchURLSToRenderer,
> which sends the values of all of the URLs from the default TemplateURL to the
> renderer.
> 
> In other words, this does not appear to be correct at the moment.
> 
> I think the safest thing is to just trigger the event on every notification
from
> the TemplateURLService. In the future it would be great if the service made a
> distinction between changes to any keyword and changes to the DSE.

I think we can detect whether the changes in question are changes to the DSE
here.  Basically, we copy the DSE at class construction time, and then when
we're notified here, we grab the current DSE and compare it to the saved value.

We'll need to be careful about this comparison to ensure we catch cases like
"nothing about the engine changed, but the Google base URL changed."

[erikwright (departed), 2014-05-07 18:48:10]

On 2014/05/07 18:43:36, Peter Kasting wrote:
>
https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
> File chrome/browser/search/instant_service.cc (right):
> 
>
https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
> chrome/browser/search/instant_service.cc:406: if
> (new_search_result_prefetch_base_url ==
> On 2014/05/07 18:40:01, erikwright wrote:
> > The BrowserInstantController calls InstantService::SendSearchURLSToRenderer,
> > which sends the values of all of the URLs from the default TemplateURL to
the
> > renderer.
> > 
> > In other words, this does not appear to be correct at the moment.
> > 
> > I think the safest thing is to just trigger the event on every notification
> from
> > the TemplateURLService. In the future it would be great if the service made
a
> > distinction between changes to any keyword and changes to the DSE.
> 
> I think we can detect whether the changes in question are changes to the DSE
> here.  Basically, we copy the DSE at class construction time, and then when
> we're notified here, we grab the current DSE and compare it to the saved
value.
> 
> We'll need to be careful about this comparison to ensure we catch cases like
> "nothing about the engine changed, but the Google base URL changed."

Yeah, I was looking at that. But it seems hard to know if the DSE contains
Google URLs. There is IsGoogleSearchURLWithReplaceableKeyword() but it doesn't
actually check all of the URLs. So you would have to check each of the url_refs.

Or you could trigger the event whenever the TemplateURLData changes OR the
Google URL changes. There would be some false positives in this case.

[Peter Kasting, 2014-05-07 18:51:45]

On 2014/05/07 18:48:10, erikwright wrote:
> Yeah, I was looking at that. But it seems hard to know if the DSE contains
> Google URLs. There is IsGoogleSearchURLWithReplaceableKeyword() but it doesn't
> actually check all of the URLs. So you would have to check each of the
url_refs.

Interestingly, TemplateURLService::GoogleBaseURLChanged() doesn't actually check
all the url_refs either.

I think the easiest route here is to make a TemplateURL::HasGoogleBaseURLs()
function that checks all the url_refs, and call that from everywhere necessary.

[Peter Kasting, 2014-05-07 22:24:37]

New patch set up.  This makes InstantService attempt to track the default search
engine and Google base URL and notify its observers when anything relevant
changes.

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:41: // Helpers
--------------------------------------------------------------------
On 2014/05/06 23:01:18, Jered wrote:
> Please omit these section comments, they don't really help with readability
imo.

Just trying to stay consistent with the headers used by other Chromium files. 
Not a big deal, removed.

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.h (right):

https://codereview.chromium.org/263823007/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.h:116: 
On 2014/05/06 23:01:18, Jered wrote:
> nit: I think you can omit this newline

Done.

[Peter Kasting, 2014-05-08 01:24:58]

The latest patch set fixes some of the previous set's test failures, especially
relating to improperly using GoogleURLTracker::GoogleURL() instead of
UIThreadSearchTermsData::GoogleBaseURL().

However, this doesn't fix some incognito service-shutdown crashes.  I don't know
what those crashes mean.  I pinged erg for help.  Erik, if you have brilliant
insight, feel free to share it.

[erikwright (departed), 2014-05-08 02:00:00]

https://codereview.chromium.org/263823007/diff/140001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/263823007/diff/140001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:75:
previous_default_search_provider_.reset(
I'm going to guess that the test failures due to too many callbacks are because
something is not properly initialized before this line here. So the DSP is
changing when the TemplateURLService finishes loading.

Maybe that is actually more realistic, or maybe not. But either you have to
update the tests to initialize the TUS as is expected or you need to update the
expectations.

[erikwright (departed), 2014-05-08 03:11:50]

I uploaded a CL identical to this but with the incognito bugs fixed.

https://codereview.chromium.org/272573004

[samarth, 2014-05-08 04:14:35]

Looks like Jered's on top of this.  Removing myself.

Thanks,
Samarth

[erikwright (departed), 2014-05-08 05:46:48]

OK, There are still a few test failures with my version of this CL, but they
seem less gnarly than the incognito one.

[erikwright (departed), 2014-05-08 14:06:51]

This CL should be considered deprecated in favor of
https://codereview.chromium.org/272573004 .

[Peter Kasting, 2014-05-08 18:23:56]

On 2014/05/08 14:06:51, erikwright wrote:
> This CL should be considered deprecated in favor of
> https://codereview.chromium.org/272573004 .

OK.  Closing.