Handle TemplateURLService load failure better, and make some test correctness fixes that will be ne…

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

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

BUG=364183
TEST=none
R=jered@chromium.org, pkasting@chromium.org
TBR=engedy

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

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

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

pkasting, jered: PTAL. This CL should be considered the official one now as I
have fixed all of the test failures in Peter's.

zea: This is not functionally changed from the most recent version of the CL you
LGTM'd (http://crrev.com/263823007/). It may have changed from the patchset you
LGTM'd, though.

[erikwright (departed), 2014-05-08 14:10:55]

engedy: FYI. This will now do a more thorough check to see if the TemplateURL
reference Google base URLs.

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

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

https://codereview.chromium.org/272573004/diff/80001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:59:
template_url_service_(TemplateURLServiceFactory::GetForProfile(profile_)),
I purposefully didn't do this in order to decouple the lifetime of
InstantService and TemplateURLService.  This assumes that the
TemplateURLService, if it exists, will outlive the InstantService.  It seemed
better to instead attempt to get it anew whenever we needed it, so it could go
away at any time.

I assume you changed this because it was the source of the refcounting issues. 
And I assume that in turn was because the GetForProfile() call in
~InstantService() caused the service to be recreated after it was supposed to
have been torn down?

If that's the case, then this change is broken, because the saved member pointer
is going to point to a destroyed service by the time ~InstantService() runs.  In
such a case, I think we'd want TemplateURLServiceFactory to have a method like
HistoryServiceFactory's GetForProfileWithoutCreating(), and call that in the
destructor.  Or maybe, because InstantServiceFactory says this depends on the
TemplateURLService, we're ensured that there will _never_ be a
TemplateURLService by the time our destructor runs, so we should omit the
RemoveObserver() call (and add a comment about why doing so is not just safe but
necessary).

https://codereview.chromium.org/272573004/diff/80001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:93: instant_io_context_ = new
InstantIOContext();
If this line needs to be here rather than in the initializer list (since you
moved it back), could you add a comment about why?

https://codereview.chromium.org/272573004/diff/80001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:412: if
(!chrome::ShouldPrefetchSearchResults())
This check wasn't in the old code, and it's not clear why it's correct.

https://codereview.chromium.org/272573004/diff/80001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:413: return;
In this case, I'd prefer to either reverse the conditional and move the FOR_EACH
call into an arm of it, or else reverse the outer conditional here and have only
top-level early-returns.  Early-returns inside outer conditionals are kinda
asking for later problems when someone adds code to the tail end of this
function.

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

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

https://codereview.chromium.org/272573004/diff/80001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:59:
template_url_service_(TemplateURLServiceFactory::GetForProfile(profile_)),
On 2014/05/08 18:23:37, Peter Kasting wrote:
> I purposefully didn't do this in order to decouple the lifetime of
> InstantService and TemplateURLService.  This assumes that the
> TemplateURLService, if it exists, will outlive the InstantService.  It seemed
> better to instead attempt to get it anew whenever we needed it, so it could go
> away at any time.
> 
> I assume you changed this because it was the source of the refcounting issues.

> And I assume that in turn was because the GetForProfile() call in
> ~InstantService() caused the service to be recreated after it was supposed to
> have been torn down?
> 
> If that's the case, then this change is broken, because the saved member
pointer
> is going to point to a destroyed service by the time ~InstantService() runs. 
In
> such a case, I think we'd want TemplateURLServiceFactory to have a method like
> HistoryServiceFactory's GetForProfileWithoutCreating(), and call that in the
> destructor.  Or maybe, because InstantServiceFactory says this depends on the
> TemplateURLService, we're ensured that there will _never_ be a
> TemplateURLService by the time our destructor runs, so we should omit the
> RemoveObserver() call (and add a comment about why doing so is not just safe
but
> necessary).

No. InstantServiceFactory declares a dependency on TemplateURLService. As a
result, the destruction order is defined. That is precisely for this scenario,
AFAIU.

[Peter Kasting, 2014-05-08 18:43:04]

On 2014/05/08 18:26:00, erikwright wrote:
>
https://codereview.chromium.org/272573004/diff/80001/chrome/browser/search/in...
> File chrome/browser/search/instant_service.cc (right):
> 
>
https://codereview.chromium.org/272573004/diff/80001/chrome/browser/search/in...
> chrome/browser/search/instant_service.cc:59:
> template_url_service_(TemplateURLServiceFactory::GetForProfile(profile_)),
> On 2014/05/08 18:23:37, Peter Kasting wrote:
> > I purposefully didn't do this in order to decouple the lifetime of
> > InstantService and TemplateURLService.  This assumes that the
> > TemplateURLService, if it exists, will outlive the InstantService.  It
seemed
> > better to instead attempt to get it anew whenever we needed it, so it could
go
> > away at any time.
> > 
> > I assume you changed this because it was the source of the refcounting
issues.
> 
> > And I assume that in turn was because the GetForProfile() call in
> > ~InstantService() caused the service to be recreated after it was supposed
to
> > have been torn down?
> > 
> > If that's the case, then this change is broken, because the saved member
> pointer
> > is going to point to a destroyed service by the time ~InstantService() runs.

> In
> > such a case, I think we'd want TemplateURLServiceFactory to have a method
like
> > HistoryServiceFactory's GetForProfileWithoutCreating(), and call that in the
> > destructor.  Or maybe, because InstantServiceFactory says this depends on
the
> > TemplateURLService, we're ensured that there will _never_ be a
> > TemplateURLService by the time our destructor runs, so we should omit the
> > RemoveObserver() call (and add a comment about why doing so is not just safe
> but
> > necessary).
> 
> No. InstantServiceFactory declares a dependency on TemplateURLService. As a
> result, the destruction order is defined. That is precisely for this scenario,
> AFAIU.

Oops, yeah, I had the destruction order mentally reversed.

But in that case, why was there a problem with my code that always got this
dynamically?  It seems like that also should have worked fine, albeit maybe the
complexity was unnecessary.

In any case, you should consider putting in the .h comment on
|template_url_service_| something like "The dependency set in
InstantServiceFactory guarantees that, if this exists, it outlives us."  Also,
doesn't that dependency also mean there should _always_ be a TemplateURLService,
so we don't need to null-check it?

[erikwright (departed), 2014-05-08 18:54:07]

>
> No. InstantServiceFactory declares a dependency on TemplateURLService. As a
>>
> result, the destruction order is defined. That is precisely for this
>> scenario,
>> AFAIU.
>>
>
> Oops, yeah, I had the destruction order mentally reversed.
>
> But in that case, why was there a problem with my code that always got this
> dynamically?  It seems like that also should have worked fine, albeit
> maybe the
> complexity was unnecessary.
>

TemplateURLService is declared NULL for tests (although some tests
explicitly say it should be used).

There seems to be some kind of bug in the way it is all set up, such that
the NULL-ness is forgotten once you get to the shutdown phase. I could
hypothesize, but it's irrelevant. What I observed is that it is NULL as
expected in the constructor, but the GetForProfile call in the destructor
actually triggered an initialization.

In any case, you should consider putting in the .h comment on
> |template_url_service_| something like "The dependency set in
> InstantServiceFactory guarantees that, if this exists, it outlives us."
>  Also,
> doesn't that dependency also mean there should _always_ be a
> TemplateURLService,
> so we don't need to null-check it?


I don't really find the comment meaningful, since that's precisely the
point of KeyedService dependencies. But I don't mind adding it.

The dependency is _only_ used to order destruction. It doesn't actually
mean anything with regards to initialization (which is lazy) or a
guaranteeing availability. I agree it's surprising.

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

[erikwright (departed), 2014-05-08 19:23:11]

pkasting: PTAL.

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

https://codereview.chromium.org/272573004/diff/80001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:93: instant_io_context_ = new
InstantIOContext();
On 2014/05/08 18:23:37, Peter Kasting wrote:
> If this line needs to be here rather than in the initializer list (since you
> moved it back), could you add a comment about why?

Done.

https://codereview.chromium.org/272573004/diff/80001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:412: if
(!chrome::ShouldPrefetchSearchResults())
On 2014/05/08 18:23:37, Peter Kasting wrote:
> This check wasn't in the old code, and it's not clear why it's correct.

Someone who contributed to this CL (there have been 3 of us) edited
InstantServiceTest.DispatchGoogleURLUpdated to not expect a notification when
the experiment is disabled. The comment said:

  // In this test, the default search engine does not use instant, so changing
  // the Google base URL should not result in the instant service observer being
  // notified.

I don't know why, because that diverges from the prior implementation, which did
send GoogleURLUpdated events in this scenario.

I blindly fixed the implementation, but as of now I am restoring the test
expectations.

https://codereview.chromium.org/272573004/diff/80001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:413: return;
On 2014/05/08 18:23:37, Peter Kasting wrote:
> In this case, I'd prefer to either reverse the conditional and move the
FOR_EACH
> call into an arm of it, or else reverse the outer conditional here and have
only
> top-level early-returns.  Early-returns inside outer conditionals are kinda
> asking for later problems when someone adds code to the tail end of this
> function.

N/A.

[Peter Kasting, 2014-05-08 20:17:40]

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

https://codereview.chromium.org/272573004/diff/80001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:412: if
(!chrome::ShouldPrefetchSearchResults())
On 2014/05/08 19:23:11, erikwright wrote:
> On 2014/05/08 18:23:37, Peter Kasting wrote:
> > This check wasn't in the old code, and it's not clear why it's correct.
> 
> Someone who contributed to this CL (there have been 3 of us) edited
> InstantServiceTest.DispatchGoogleURLUpdated to not expect a notification when
> the experiment is disabled. The comment said:
> 
>   // In this test, the default search engine does not use instant, so changing
>   // the Google base URL should not result in the instant service observer
being
>   // notified.
> 
> I don't know why, because that diverges from the prior implementation, which
did
> send GoogleURLUpdated events in this scenario.
> 
> I blindly fixed the implementation, but as of now I am restoring the test
> expectations.

I made that change, and this code should work with it.  The test originally
passed for me locally as modified.  If it's not passing, do not change the test
expectations, and don't add the line of code here -- instead,
|default_search_provider_changed| should not be set, so we shouldn't reach here.

I suspect this may be failing because, while the DSE in that test wasn't using
the Google base URL in its instant URL, it was using it somewhere else.  We
should instead change the test to not refer to the Google base URL anywhere, so
that it can then verify that changing the Google base URL does not send
notifications for TURLs that don't refer to it (which was the intent of the
modification).

[Jered, 2014-05-08 20:20:04]

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

https://codereview.chromium.org/272573004/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:68: if (template_url_service_) {
When would this be null? Should this crash, instead? Add a comment to explain.

https://codereview.chromium.org/272573004/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:409: if (template_url &&
template_url->HasGoogleBaseURLs())
Why is a change not a change when !HasGoogleBaseURLs()? Remove or add a comment
about this.

https://codereview.chromium.org/272573004/diff/100001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_instant_controller.h (left):

https://codereview.chromium.org/272573004/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser_instant_controller.h:59: // Reloads the tabs in
instant process to ensure that their privileged status
This comment is useful because it describes why we're doing the work this method
does. Keep it in the .cc file?

[Peter Kasting, 2014-05-08 20:40:04]

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

https://codereview.chromium.org/272573004/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:68: if (template_url_service_) {
On 2014/05/08 20:20:05, Jered wrote:
> When would this be null? Should this crash, instead? Add a comment to explain.

Yeah, it sounds like this can be NULL in tests, and if so we should say so.

https://codereview.chromium.org/272573004/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:409: if (template_url &&
template_url->HasGoogleBaseURLs())
On 2014/05/08 20:20:05, Jered wrote:
> Why is a change not a change when !HasGoogleBaseURLs()? Remove or add a
comment
> about this.

We already checked above that nothing about the default search provider changed.
 So the only possible thing that can affect us is if the default search provider
referred to the Google base URL, and _that_ changed.  That's precisely what the
comment above this block already says.  Was it unclear?

[erikwright (departed), 2014-05-08 20:52:58]

Replied to review comments. PTAL.

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

https://codereview.chromium.org/272573004/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:68: if (template_url_service_) {
On 2014/05/08 20:40:04, Peter Kasting wrote:
> On 2014/05/08 20:20:05, Jered wrote:
> > When would this be null? Should this crash, instead? Add a comment to
explain.
> 
> Yeah, it sounds like this can be NULL in tests, and if so we should say so.

Done.

https://codereview.chromium.org/272573004/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:409: if (template_url &&
template_url->HasGoogleBaseURLs())
On 2014/05/08 20:20:05, Jered wrote:
> Why is a change not a change when !HasGoogleBaseURLs()? Remove or add a
comment
> about this.

There were two comments already in the method, but I realize that they are a bit
opaque if you don't understand the context of when this observer method will be
called. So I am adding an implementation comment to the method.

https://codereview.chromium.org/272573004/diff/100001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_instant_controller.h (left):

https://codereview.chromium.org/272573004/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser_instant_controller.h:59: // Reloads the tabs in
instant process to ensure that their privileged status
On 2014/05/08 20:20:05, Jered wrote:
> This comment is useful because it describes why we're doing the work this
method
> does. Keep it in the .cc file?

Done.

[Jered, 2014-05-08 21:02:24]

lgtm

stamp for search--but please address Peter's test comment before submitting.

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

https://codereview.chromium.org/272573004/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:409: if (template_url &&
template_url->HasGoogleBaseURLs())
On 2014/05/08 20:52:59, erikwright wrote:
> On 2014/05/08 20:20:05, Jered wrote:
> > Why is a change not a change when !HasGoogleBaseURLs()? Remove or add a
> comment
> > about this.
> 
> There were two comments already in the method, but I realize that they are a
bit
> opaque if you don't understand the context of when this observer method will
be
> called. So I am adding an implementation comment to the method.

Gotcha. So MatchesData() implies that old_template_url->HasGoogleBaseURLs() ==
template_url->HasGoogleBaseURLs(). This is fine.

[Peter Kasting, 2014-05-08 21:04:40]

LGTM

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

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:62: // Stub for unit tests.
Nit: While here: This comment is rather vague...

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:65: previous_google_base_url_ =
Nit: Blank line above this?

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:96: // is only instantiated here (after
the check for a UI thread above).
OK... if this only depends on being on the UI thread, can we move it up to being
initialized just above |previous_google_base_url_|?  As it stands it's kinda
right in the middle of the registrar_-mucking bits.

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:397: // Provider.
Nit: I would put this first paragraph in the .h.

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:401: // but it's up to us to do the
work to check both.
Nit: And I would move this second paragraph (with minor tweaks) into the method,
just above the Google base URL check, to replace the briefer comment below.

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser_instant_controller.cc:145: // is still valid.
Nit: Instead of putting this here, I'd try to just improve the almost-identical
comment within the method as necessary.

Not only will that be less comment duplication, but since this method clearly
does more than just this sentence, putting such a comment here is somewhat
misleading.

[erikwright (departed), 2014-05-08 21:07:23]

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser_instant_controller.cc:145: // is still valid.
On 2014/05/08 21:04:41, Peter Kasting wrote:
> Nit: Instead of putting this here, I'd try to just improve the
almost-identical
> comment within the method as necessary.
> 
> Not only will that be less comment duplication, but since this method clearly
> does more than just this sentence, putting such a comment here is somewhat
> misleading.

jered: Can you suggest a way that the comments in this method could be reworked
to reflect its intent to someone with less intimate knowledge of
TemplateURLService?

[Jered, 2014-05-08 21:12:37]

slgtm

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser_instant_controller.cc:145: // is still valid.
On 2014/05/08 21:07:24, erikwright wrote:
> On 2014/05/08 21:04:41, Peter Kasting wrote:
> > Nit: Instead of putting this here, I'd try to just improve the
> almost-identical
> > comment within the method as necessary.
> > 
> > Not only will that be less comment duplication, but since this method
clearly
> > does more than just this sentence, putting such a comment here is somewhat
> > misleading.
> 
> jered: Can you suggest a way that the comments in this method could be
reworked
> to reflect its intent to someone with less intimate knowledge of
> TemplateURLService?

I missed the comment below. Given that comment, I agree this is redundant, and
vote we just delete the one above. Diving into this method more feels outside
the scope of this CL.

[erikwright (departed), 2014-05-09 00:46:14]

The CQ bit was checked by erikwright@chromium.org

[erikwright (departed), 2014-05-09 00:46:42]

FYI: Nits addressed. In CQ.

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

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:62: // Stub for unit tests.
On 2014/05/08 21:04:41, Peter Kasting wrote:
> Nit: While here: This comment is rather vague...

Done.

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:65: previous_google_base_url_ =
On 2014/05/08 21:04:41, Peter Kasting wrote:
> Nit: Blank line above this?

Done.

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:96: // is only instantiated here (after
the check for a UI thread above).
On 2014/05/08 21:04:41, Peter Kasting wrote:
> OK... if this only depends on being on the UI thread, can we move it up to
being
> initialized just above |previous_google_base_url_|?  As it stands it's kinda
> right in the middle of the registrar_-mucking bits.

Done.

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:397: // Provider.
On 2014/05/08 21:04:41, Peter Kasting wrote:
> Nit: I would put this first paragraph in the .h.

Done.

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:401: // but it's up to us to do the
work to check both.
On 2014/05/08 21:04:41, Peter Kasting wrote:
> Nit: And I would move this second paragraph (with minor tweaks) into the
method,
> just above the Google base URL check, to replace the briefer comment below.

Done.

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/272573004/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser_instant_controller.cc:145: // is still valid.
On 2014/05/08 21:12:38, Jered wrote:
> On 2014/05/08 21:07:24, erikwright wrote:
> > On 2014/05/08 21:04:41, Peter Kasting wrote:
> > > Nit: Instead of putting this here, I'd try to just improve the
> > almost-identical
> > > comment within the method as necessary.
> > > 
> > > Not only will that be less comment duplication, but since this method
> clearly
> > > does more than just this sentence, putting such a comment here is somewhat
> > > misleading.
> > 
> > jered: Can you suggest a way that the comments in this method could be
> reworked
> > to reflect its intent to someone with less intimate knowledge of
> > TemplateURLService?
> 
> I missed the comment below. Given that comment, I agree this is redundant, and
> vote we just delete the one above. Diving into this method more feels outside
> the scope of this CL.

Done.

[erikwright (departed), 2014-05-09 00:48:39]

TBR engedy@ who already reviewed this content in a different CL.

[commit-bot: I haz the power, 2014-05-09 00:50:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/272573004/160001

[erikwright (departed), 2014-05-09 16:33:54]

Committed patchset #7 manually as r269329 (presubmit successful).

[Alpha Left Google, 2014-05-10 21:06:06]

A revert of this CL has been created in
https://codereview.chromium.org/273153004/ by hclam@chromium.org.

The reason for reverting is: Seems to be causing use after free on Mac:
http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%....

[erikwright (departed), 2014-05-11 18:01:59]

On 2014/05/10 21:06:06, Alpha wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/273153004/ by mailto:hclam@chromium.org.
> 
> The reason for reverting is: Seems to be causing use after free on Mac:
>
http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%....

The test failures have been addressed in
https://codereview.chromium.org/276063003/ .

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

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-05-11 18:05:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/272573004/180001

[commit-bot: I haz the power, 2014-05-11 19:48:55]

Change committed as 269716