[Hotword] Uninstall and reinstall the extension upon language change.

[Hotword] Uninstall and reinstall the extension upon language change.

There are two places we check to see if we need uninstall-install. The first is on startup when the extension system is ready (in response to the ExtensionSystem::ready() OneShot timer). If the extension is currently pending, then we'll check again when we get the notification that the extension has installed when OnExtensionInstalled is called.

The function MaybeUninstallHotwordExtension does a series of checks to determine if we should uninstall the extension based on pending installs and previous vs. current locale. If an uninstall is necessary, it kicks it off. 

Upon Uninstall, OnExtensionUninstalled is triggered. When OnExtensionUninstalled is triggered for the hotword extension, it will reinstall it and update the previous locale (language). If the user goes in to try to uninstall the hotword extension manually, this change will also force it to be reinstalled.

This will trigger a second call to OnExtensionInstalled. To prevent a loop, we use HotwordService::uninstall_pending_ to prevent another uninstall attempt.

This CL also removes the dependency on the deprecated NOTIFICATION_EXTENSION_INSTALLED as a side effect.

Several unittests were added to the unittest suite to test various parts of the uninstall-install pipeline. In order to have access to the extension system during tests, the HotwordServiceTest now extends ExtensionServiceTestBase.

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

[rpetterson, 2014-06-17 22:52:51]

Yoyo, please take a look to make sure I'm handling the extension aspect
correctly.

Jered, owners, general hotwordiness.

[Yoyo Zhou, 2014-06-18 00:33:22]

I think the extensions stuff looks basically right. Comments inline.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:265: profile_ !=
static_cast<Profile*>(browser_context) ||
Profile::FromBrowserContext

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:271: if
(!ShouldUninstallHotwordExtension())
Does uninstall_pending_ serve as well here?

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:282: installer_ = new
extensions::WebstoreStartupInstaller(
I'm not confident that this is the right choice of installer (there are too many
to choose from =( ), but if it installs silently without prompting the user,
then I think that's what you need.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:308: if (!uninstall_pending_)
What do you think about calling this reinstall_pending_ (that would make more
sense in the context here), and MaybeReinstallHotwordExtension?

It also should be possible to put most of this reinstalling logic into a
separate helper class if HotwordService is getting unwieldy. Up to you.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:321: if (IsHotwordAllowed() &&
This check/disable looks repeated from the constructor.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:502:
profile_->GetPrefs()->HasPrefPath(prefs::kHotwordPreviousLanguage) &&
nit: seems like the no-previous-locale check should come at the beginning of
this function (though I think it works either way)

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:506: void
HotwordService::OnHotwordExtensionInstalled(bool success,
This callback doesn't really do anything. Is it needed?

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.h (right):

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.h:88: // Checks based on locale if the
current version so be uninstalled so that
s/so/should/

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.h:93: // UinstallHotwordExtension returns
true if the extension was uninstalled.
typo: Uninstall

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
File chrome/browser/search/hotword_service_unittest.cc (right):

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service_unittest.cc:209:
SetApplicationLocale(static_cast<Profile*>(profile()), "en");
This static cast shouldn't be needed.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service_unittest.cc:270: // The previous locale
should not be set. No reason to uininstall.
typo: uninstall

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service_unittest.cc:283:
EXPECT_TRUE(service()->GetExtensionById(
Prefer registry()->enabled_extensions()->Contains(id)

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service_unittest.cc:287: // locale.. No reason to
uininstall.
(same typo)

[rpetterson, 2014-06-18 02:08:45]

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:265: profile_ !=
static_cast<Profile*>(browser_context) ||
On 2014/06/18 00:33:21, Yoyo Zhou wrote:
> Profile::FromBrowserContext

Done.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:271: if
(!ShouldUninstallHotwordExtension())
On 2014/06/18 00:33:21, Yoyo Zhou wrote:
> Does uninstall_pending_ serve as well here?

You mean just replace the check ShouldUninstallHotwordExtension() with
uninstall_pending_? Yes, that should work since uninstall_pending_ should be set
if we've decided an uninstall is needed.

I've updated it.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:282: installer_ = new
extensions::WebstoreStartupInstaller(
On 2014/06/18 00:33:21, Yoyo Zhou wrote:
> I'm not confident that this is the right choice of installer (there are too
many
> to choose from =( ), but if it installs silently without prompting the user,
> then I think that's what you need.

This is the same one used by ChromeOS to install the hotword extension for the
app_list which is why I used it since it's already been in production in a
similar context. It does silently install without prompting the user from what I
can tell.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:308: if (!uninstall_pending_)
On 2014/06/18 00:33:21, Yoyo Zhou wrote:
> What do you think about calling this reinstall_pending_ (that would make more
> sense in the context here), and MaybeReinstallHotwordExtension?
> 

sgtm. done.

> It also should be possible to put most of this reinstalling logic into a
> separate helper class if HotwordService is getting unwieldy. Up to you.

I'd rather do that as a separate CL for now. I've added a tracking bug for it:
crbug.com/385969

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:321: if (IsHotwordAllowed() &&
On 2014/06/18 00:33:21, Yoyo Zhou wrote:
> This check/disable looks repeated from the constructor.

Yes, but we have to repeat it because the HotwordService constructor can finish
before the external component loader finishes installing the extensions. In that
case, we catch it here and disable it.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:502:
profile_->GetPrefs()->HasPrefPath(prefs::kHotwordPreviousLanguage) &&
On 2014/06/18 00:33:21, Yoyo Zhou wrote:
> nit: seems like the no-previous-locale check should come at the beginning of
> this function (though I think it works either way)

Done.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:506: void
HotwordService::OnHotwordExtensionInstalled(bool success,
On 2014/06/18 00:33:21, Yoyo Zhou wrote:
> This callback doesn't really do anything. Is it needed?

I agree. Removed.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.h (right):

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.h:88: // Checks based on locale if the
current version so be uninstalled so that
On 2014/06/18 00:33:21, Yoyo Zhou wrote:
> s/so/should/

Done.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service.h:93: // UinstallHotwordExtension returns
true if the extension was uninstalled.
On 2014/06/18 00:33:22, Yoyo Zhou wrote:
> typo: Uninstall

Done.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
File chrome/browser/search/hotword_service_unittest.cc (right):

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service_unittest.cc:209:
SetApplicationLocale(static_cast<Profile*>(profile()), "en");
On 2014/06/18 00:33:22, Yoyo Zhou wrote:
> This static cast shouldn't be needed.

Ah, you're right. Copy/paste . . .

Fixed.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service_unittest.cc:270: // The previous locale
should not be set. No reason to uininstall.
On 2014/06/18 00:33:22, Yoyo Zhou wrote:
> typo: uninstall

Done.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service_unittest.cc:283:
EXPECT_TRUE(service()->GetExtensionById(
On 2014/06/18 00:33:22, Yoyo Zhou wrote:
> Prefer registry()->enabled_extensions()->Contains(id)

Done.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service_unittest.cc:287: // locale.. No reason to
uininstall.
On 2014/06/18 00:33:22, Yoyo Zhou wrote:
> (same typo)

Done.

[Yoyo Zhou, 2014-06-18 02:15:49]

Cool, LGTM.

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
File chrome/browser/search/hotword_service_unittest.cc (right):

https://codereview.chromium.org/330193005/diff/100001/chrome/browser/search/h...
chrome/browser/search/hotword_service_unittest.cc:283:
EXPECT_TRUE(service()->GetExtensionById(
On 2014/06/18 02:08:44, rpetterson wrote:
> On 2014/06/18 00:33:22, Yoyo Zhou wrote:
> > Prefer registry()->enabled_extensions()->Contains(id)
> 
> Done.

Oops, yes, disabled.

[Jered, 2014-06-18 15:04:19]

I think I see what this is trying to do, but am a little but confused in a
couple places.

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:152: // static
delete this comment

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:302: // However, if we arrived her via
an uninstall-triggered-install (and in
her -> here

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:306: MaybeReinstallHotwordExtension();
Huh? Why would we ever want to reinstall the hotword extension just after it got
installed?

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:308: reinstall_pending_ = false;
Just unconditionally set this to false.

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:353: reinstall_pending_ = true;
Should this be set to the return value of UninstallHotwordExtension, and then be
returned?

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:485: void
HotwordService::SetPreviousLocalePref() {
nit: The pref is called previous language, so should this be previous language
rather than previous locale?

[Yoyo Zhou, 2014-06-18 19:10:31]

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:306: MaybeReinstallHotwordExtension();
On 2014/06/18 15:04:19, Jered wrote:
> Huh? Why would we ever want to reinstall the hotword extension just after it
got
> installed?

I believe this is for the scenario where the locale changes and the extension
updates at the same time. We hit the IsIdPending at startup, so we wait for the
update to finish. When it has done so, we get here.

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:308: reinstall_pending_ = false;
On 2014/06/18 15:04:19, Jered wrote:
> Just unconditionally set this to false.

I don't think that works with the previous.

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:353: reinstall_pending_ = true;
On 2014/06/18 15:04:19, Jered wrote:
> Should this be set to the return value of UninstallHotwordExtension, and then
be
> returned?

That seems right.

[rpetterson, 2014-06-18 20:48:33]

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:152: // static
On 2014/06/18 15:04:19, Jered wrote:
> delete this comment

Done.

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:302: // However, if we arrived her via
an uninstall-triggered-install (and in
On 2014/06/18 15:04:19, Jered wrote:
> her -> here

Done.

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:306: MaybeReinstallHotwordExtension();
On 2014/06/18 19:10:31, Yoyo Zhou wrote:
> On 2014/06/18 15:04:19, Jered wrote:
> > Huh? Why would we ever want to reinstall the hotword extension just after it
> got
> > installed?
> 
> I believe this is for the scenario where the locale changes and the extension
> updates at the same time. We hit the IsIdPending at startup, so we wait for
the
> update to finish. When it has done so, we get here.

That's correct. Typically, no, we would not. If we've just installed it, chances
are good that we installed it in the correct language. However, this callback is
also triggered when the extension has been installed from an update. So here's a
hypothetical situation:

- Select new language
- Restart Chrome to let new language take effect
- Upon Restart, external component loader decides it's time to update the
extensions
- Hotword has an update so we download the new one
- Meanwhile, the extension system says it's ready so we do a check
- Our correct language check tells us that the hotword extension is pending
(because it's downloading) so we'll wait until the OnExtensionInstalled trigger
- OnExtensionInstalled triggers and we realize that the updated extension is the
wrong language
- Proceed to uninstall and reinstall in the correct language

Would it be simpler to do this check before the external component loader does
it's updates so that we don't download the extension twice sometimes? Yes.
However, that requires more complicated hooks into the extension system.
Eventually the extension folks plan to build in forced updates which will serve
our purpose as well, but until then this is less invasive. We're covering edge
cases with this check that should rarely happen so extra downloads in general
shouldn't be an issue.

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:308: reinstall_pending_ = false;
On 2014/06/18 19:10:31, Yoyo Zhou wrote:
> On 2014/06/18 15:04:19, Jered wrote:
> > Just unconditionally set this to false.
> 
> I don't think that works with the previous.


I thought about this, but it needs to be in an else. Otherwise it might be set
to false before the OnExtensionUninstalled triggers which puts it to false
inside that call and that would be incorrect (I've seen this in manual testing).
Only after we know the reinstall has completed should it be set back to false.

I'm also not sure it's "better" since without the else we go through all the
ShouldReinstallHotwordExtension checks after the extension is reinstalled rather
than just checking the one boolean.

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:353: reinstall_pending_ = true;
On 2014/06/18 19:10:31, Yoyo Zhou wrote:
> On 2014/06/18 15:04:19, Jered wrote:
> > Should this be set to the return value of UninstallHotwordExtension, and
then
> be
> > returned?
> 
> That seems right.

Sorta. I still need to set reinstall_pending_ to true before the call to
UninstallHotwordExtension. Otherwise it's still false when we get into
OnExtensionUninstalled and we never reinstall since it might be the result of a
different kind of uninstall. But I can set the ultimate value of
reinstall_pending_ to be the return value of UninstallHotwordExtension and
return that.

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:485: void
HotwordService::SetPreviousLocalePref() {
On 2014/06/18 15:04:19, Jered wrote:
> nit: The pref is called previous language, so should this be previous language
> rather than previous locale?

Done.

[Jered, 2014-06-18 21:06:04]

lgtm

Some nits. This is a little subtle still so I'm not 100% sure I've not missed
something, but it sounds like you've tested it thoroughly.

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:306: MaybeReinstallHotwordExtension();
On 2014/06/18 20:48:33, rpetterson wrote:
> On 2014/06/18 19:10:31, Yoyo Zhou wrote:
> > On 2014/06/18 15:04:19, Jered wrote:
> > > Huh? Why would we ever want to reinstall the hotword extension just after
it
> > got
> > > installed?
> > 
> > I believe this is for the scenario where the locale changes and the
extension
> > updates at the same time. We hit the IsIdPending at startup, so we wait for
> the
> > update to finish. When it has done so, we get here.
> 
> That's correct. Typically, no, we would not. If we've just installed it,
chances
> are good that we installed it in the correct language. However, this callback
is
> also triggered when the extension has been installed from an update. So here's
a
> hypothetical situation:
> 
> - Select new language
> - Restart Chrome to let new language take effect
> - Upon Restart, external component loader decides it's time to update the
> extensions
> - Hotword has an update so we download the new one
> - Meanwhile, the extension system says it's ready so we do a check
> - Our correct language check tells us that the hotword extension is pending
> (because it's downloading) so we'll wait until the OnExtensionInstalled
trigger
> - OnExtensionInstalled triggers and we realize that the updated extension is
the
> wrong language
> - Proceed to uninstall and reinstall in the correct language
> 
> Would it be simpler to do this check before the external component loader does
> it's updates so that we don't download the extension twice sometimes? Yes.
> However, that requires more complicated hooks into the extension system.
> Eventually the extension folks plan to build in forced updates which will
serve
> our purpose as well, but until then this is less invasive. We're covering edge
> cases with this check that should rarely happen so extra downloads in general
> shouldn't be an issue.

Ok, thanks for explaining.

https://codereview.chromium.org/330193005/diff/140001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:308: reinstall_pending_ = false;
On 2014/06/18 20:48:33, rpetterson wrote:
> On 2014/06/18 19:10:31, Yoyo Zhou wrote:
> > On 2014/06/18 15:04:19, Jered wrote:
> > > Just unconditionally set this to false.
> > 
> > I don't think that works with the previous.
> 
> 
> I thought about this, but it needs to be in an else. Otherwise it might be set
> to false before the OnExtensionUninstalled triggers which puts it to false
> inside that call and that would be incorrect (I've seen this in manual
testing).
> Only after we know the reinstall has completed should it be set back to false.
> 
> I'm also not sure it's "better" since without the else we go through all the
> ShouldReinstallHotwordExtension checks after the extension is reinstalled
rather
> than just checking the one boolean.

sgtm.

https://codereview.chromium.org/330193005/diff/160001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/330193005/diff/160001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:359: return reinstall_pending_ =
UninstallHotwordExtension(extension_service);
nit: Can we write this in a less clever way? e.g.
reinstall_pending_ = true;
if (!UninstallHotwordExtension(extension_service))
  reinstall_pending_ = false;
return reinstall_pending_;

https://codereview.chromium.org/330193005/diff/160001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.h (right):

https://codereview.chromium.org/330193005/diff/160001/chrome/browser/search/h...
chrome/browser/search/hotword_service.h:97: // Sets the pref value of the
previous locale.
locale -> language

[rpetterson, 2014-06-18 23:05:57]

https://codereview.chromium.org/330193005/diff/160001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/330193005/diff/160001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:359: return reinstall_pending_ =
UninstallHotwordExtension(extension_service);
On 2014/06/18 21:06:04, Jered wrote:
> nit: Can we write this in a less clever way? e.g.
> reinstall_pending_ = true;
> if (!UninstallHotwordExtension(extension_service))
>   reinstall_pending_ = false;
> return reinstall_pending_;

So this doesn't actually work. Which is pretty interesting since I would expect
it to as well. What's happening is that we make the call to
UninstallHotwordExtension(..) and before we get to the next line to return
reinstall_pending_, we've already triggered the OnExtensionInstalled which sets
reinstall_pending_ to false.

So the options are either the above or simply:
return UninstallHotwordExtension(extension_service);

I personally favor the latter since this return is only used in tests.

[Jered, 2014-06-18 23:12:17]

https://codereview.chromium.org/330193005/diff/160001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/330193005/diff/160001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:359: return reinstall_pending_ =
UninstallHotwordExtension(extension_service);
On 2014/06/18 23:05:57, rpetterson wrote:
> On 2014/06/18 21:06:04, Jered wrote:
> > nit: Can we write this in a less clever way? e.g.
> > reinstall_pending_ = true;
> > if (!UninstallHotwordExtension(extension_service))
> >   reinstall_pending_ = false;
> > return reinstall_pending_;
> 
> So this doesn't actually work. Which is pretty interesting since I would
expect
> it to as well. What's happening is that we make the call to
> UninstallHotwordExtension(..) and before we get to the next line to return
> reinstall_pending_, we've already triggered the OnExtensionInstalled which
sets
> reinstall_pending_ to false.
> 
> So the options are either the above or simply:
> return UninstallHotwordExtension(extension_service);
> 
> I personally favor the latter since this return is only used in tests.

I was concerned that if UninstallHotwordExtension() fails, this might be left
set to true and mess up the state machine for later. Consider setting it to
false below, inside the LOG(WARNING) block in UninstallHotwordExtension.

[rpetterson, 2014-06-18 23:45:35]

https://codereview.chromium.org/330193005/diff/160001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/330193005/diff/160001/chrome/browser/search/h...
chrome/browser/search/hotword_service.cc:359: return reinstall_pending_ =
UninstallHotwordExtension(extension_service);
On 2014/06/18 23:12:17, Jered wrote:
> On 2014/06/18 23:05:57, rpetterson wrote:
> > On 2014/06/18 21:06:04, Jered wrote:
> > > nit: Can we write this in a less clever way? e.g.
> > > reinstall_pending_ = true;
> > > if (!UninstallHotwordExtension(extension_service))
> > >   reinstall_pending_ = false;
> > > return reinstall_pending_;
> > 
> > So this doesn't actually work. Which is pretty interesting since I would
> expect
> > it to as well. What's happening is that we make the call to
> > UninstallHotwordExtension(..) and before we get to the next line to return
> > reinstall_pending_, we've already triggered the OnExtensionInstalled which
> sets
> > reinstall_pending_ to false.
> > 
> > So the options are either the above or simply:
> > return UninstallHotwordExtension(extension_service);
> > 
> > I personally favor the latter since this return is only used in tests.
> 
> I was concerned that if UninstallHotwordExtension() fails, this might be left
> set to true and mess up the state machine for later. Consider setting it to
> false below, inside the LOG(WARNING) block in UninstallHotwordExtension.

A valid concern. This looks like a good solution to me. Thank you!

https://codereview.chromium.org/330193005/diff/160001/chrome/browser/search/h...
File chrome/browser/search/hotword_service.h (right):

https://codereview.chromium.org/330193005/diff/160001/chrome/browser/search/h...
chrome/browser/search/hotword_service.h:97: // Sets the pref value of the
previous locale.
On 2014/06/18 21:06:04, Jered wrote:
> locale -> language

Done.

[rpetterson, 2014-06-20 00:01:05]

The CQ bit was checked by rlp@chromium.org

[commit-bot: I haz the power, 2014-06-20 00:02:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlp@chromium.org/330193005/220001

[commit-bot: I haz the power, 2014-06-20 01:06:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-06-20 01:10:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-20 01:10:17]

Try jobs failed on following builders:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[rpetterson, 2014-06-20 06:44:45]

The CQ bit was checked by rlp@chromium.org

[commit-bot: I haz the power, 2014-06-20 06:47:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlp@chromium.org/330193005/220001

[commit-bot: I haz the power, 2014-06-20 07:51:41]

Change committed as 278644