Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
196753002:
[Hotword] Making enabling/disabling the setting enable/disable the hotword extension. (Closed)
|
Created:
6 years, 9 months ago by rpetterson Modified:
6 years, 9 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, dominich, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, chromium-apps-reviews_chromium.org, kmadhusu+watch_chromium.org, Jered, samarth Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Hotword] Making enabling/disabling the setting enable/disable the hotword extension.
This changes also allows external component extensions to be user modifiable. However, currently hotwording is the only such component which this affects since others are not typically accessible for changing.
BUG=349573
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258106
Patch Set 1 #
Total comments: 5
Patch Set 2 : update comment #
Total comments: 5
Patch Set 3 : refactor, update comment #Patch Set 4 : remove unneeded includes #
Total comments: 4
Patch Set 5 : cleanup #Patch Set 6 : fixing unittests #Patch Set 7 : rebase because try bots are hanging #
Total comments: 2
Patch Set 8 : rebasing for keyed service change #Patch Set 9 : undo profile resetter unittest change, but modify behavior when disabling extensions #
Total comments: 7
Patch Set 10 : fixing nits, adding comment about filed bug #Patch Set 11 : adding check in ShouldEnableOnInstall #Patch Set 12 : remove extra blank lines #Patch Set 13 : adding debug lines since I can't repro locally, debug or release #Patch Set 14 : compile error #Patch Set 15 : remove is_app and test #
Messages
Total messages: 38 (0 generated)
miket@chromium.org: Please review changes in browser/search samarth@chromium.org: Please review changes in extensions
switching from samarth to jered since samarth is OOO.
Quick question for search/ change https://codereview.chromium.org/196753002/diff/1/chrome/browser/search/hotwor... File chrome/browser/search/hotword_service.cc (right): https://codereview.chromium.org/196753002/diff/1/chrome/browser/search/hotwor... chrome/browser/search/hotword_service.cc:95: if (profile_->GetPrefs()->HasPrefPath(prefs::kHotwordSearchEnabled)) { Do you also need to call OnHotwordSearchEnabledChanged() here, on initialization, or is that handled somewhere else? https://codereview.chromium.org/196753002/diff/1/chrome/browser/search/hotwor... chrome/browser/search/hotword_service.cc:147: // Do not include disabled extension (false parameter) because if the nit: Update this comment.
https://codereview.chromium.org/196753002/diff/1/chrome/browser/search/hotwor... File chrome/browser/search/hotword_service.cc (right): https://codereview.chromium.org/196753002/diff/1/chrome/browser/search/hotwor... chrome/browser/search/hotword_service.cc:95: if (profile_->GetPrefs()->HasPrefPath(prefs::kHotwordSearchEnabled)) { On 2014/03/12 20:43:45, Jered wrote: > Do you also need to call OnHotwordSearchEnabledChanged() here, on > initialization, or is that handled somewhere else? No we don't. By default, extensions are enabled. So the only case we need to worry about is if they haven't opted in or out in which case the extension should be disabled. This is handled during on line 103. Otherwise, the state of both the preference and whether the extension is enabled are carried over. https://codereview.chromium.org/196753002/diff/1/chrome/browser/search/hotwor... chrome/browser/search/hotword_service.cc:147: // Do not include disabled extension (false parameter) because if the On 2014/03/12 20:43:45, Jered wrote: > nit: Update this comment. Done.
https://codereview.chromium.org/196753002/diff/1/chrome/browser/search/hotwor... File chrome/browser/search/hotword_service.cc (right): https://codereview.chromium.org/196753002/diff/1/chrome/browser/search/hotwor... chrome/browser/search/hotword_service.cc:95: if (profile_->GetPrefs()->HasPrefPath(prefs::kHotwordSearchEnabled)) { Let me clarify just a bit more. When the preference actually changes, that's when we handle it and then *that* state carries over. On 2014/03/12 20:51:25, rpetterson wrote: > On 2014/03/12 20:43:45, Jered wrote: > > Do you also need to call OnHotwordSearchEnabledChanged() here, on > > initialization, or is that handled somewhere else? > > No we don't. By default, extensions are enabled. So the only case we need to > worry about is if they haven't opted in or out in which case the extension > should be disabled. This is handled during on line 103. Otherwise, the state of > both the preference and whether the extension is enabled are carried over.
lgtm for search
LGTM with some nits including one easy refactoring opportunity. https://codereview.chromium.org/196753002/diff/20001/chrome/browser/search/ho... File chrome/browser/search/hotword_service.cc (right): https://codereview.chromium.org/196753002/diff/20001/chrome/browser/search/ho... chrome/browser/search/hotword_service.cc:189: extensions::Extension::DISABLE_USER_ACTION); I agree this isn't sufficiently distinct from a typical user-initiated disable action to enumerate it separately. https://codereview.chromium.org/196753002/diff/20001/chrome/browser/search/ho... chrome/browser/search/hotword_service.cc:209: EnableHotwordExtension(); It would be way cooler to refactor the work to grab ExtensionService into this method. Otherwise it's needless duplication above. https://codereview.chromium.org/196753002/diff/20001/extensions/browser/admin... File extensions/browser/admin_policy.cc (right): https://codereview.chromium.org/196753002/diff/20001/extensions/browser/admin... extensions/browser/admin_policy.cc:19: extension->location() != extensions::Manifest::COMPONENT && Please add a comment indicating that this test implicitly treats COMPONENT and EXTERNAL_COMPONENT differently. It's not at all apparent from a quick read by someone who might have forgotten that there are now two kinds of COMPONENT types.
Jered, there was some refactoring in hotword_service.cc, if you want to take another look. https://codereview.chromium.org/196753002/diff/20001/chrome/browser/search/ho... File chrome/browser/search/hotword_service.cc (right): https://codereview.chromium.org/196753002/diff/20001/chrome/browser/search/ho... chrome/browser/search/hotword_service.cc:209: EnableHotwordExtension(); On 2014/03/12 21:54:46, miket wrote: > It would be way cooler to refactor the work to grab ExtensionService into this > method. Otherwise it's needless duplication above. Done. But factored into a separate method since DisableHotwordExtension is used in the constructor as well. But I was also able to pull it out of RetryHotwordExtension so cleaner overall. https://codereview.chromium.org/196753002/diff/20001/extensions/browser/admin... File extensions/browser/admin_policy.cc (right): https://codereview.chromium.org/196753002/diff/20001/extensions/browser/admin... extensions/browser/admin_policy.cc:19: extension->location() != extensions::Manifest::COMPONENT && On 2014/03/12 21:54:46, miket wrote: > Please add a comment indicating that this test implicitly treats COMPONENT and > EXTERNAL_COMPONENT differently. It's not at all apparent from a quick read by > someone who might have forgotten that there are now two kinds of COMPONENT > types. Done.
slgtm https://codereview.chromium.org/196753002/diff/60001/chrome/browser/search/ho... File chrome/browser/search/hotword_service.cc (right): https://codereview.chromium.org/196753002/diff/60001/chrome/browser/search/ho... chrome/browser/search/hotword_service.cc:69: if (!extension_system || !extension_system->extension_service()) nit: simpler if written if (extension_system) return extension_system->extension_service(); return NULL; https://codereview.chromium.org/196753002/diff/60001/chrome/browser/search/ho... chrome/browser/search/hotword_service.cc:183: if (extension_service) nit: prefer if wrapped lines go in {} but up to you
https://codereview.chromium.org/196753002/diff/60001/chrome/browser/search/ho... File chrome/browser/search/hotword_service.cc (right): https://codereview.chromium.org/196753002/diff/60001/chrome/browser/search/ho... chrome/browser/search/hotword_service.cc:69: if (!extension_system || !extension_system->extension_service()) On 2014/03/12 23:30:49, Jered wrote: > nit: simpler if written > if (extension_system) > return extension_system->extension_service(); > return NULL; Done. https://codereview.chromium.org/196753002/diff/60001/chrome/browser/search/ho... chrome/browser/search/hotword_service.cc:183: if (extension_service) On 2014/03/12 23:30:49, Jered wrote: > nit: prefer if wrapped lines go in {} but up to you I agree. I just missed that when doing the refactor. Thanks!
battre@chromium.org: Please review changes in profile_resetter Mike & Jered, I had to update unittests to take into account the thread checks from disabling/enabling extensions. Dominic, this change affected how external component extensions are treated with respect to being modifiable which affects reset and thus the test in profile resetter.
search slgtm
OK
https://codereview.chromium.org/196753002/diff/120001/chrome/browser/profile_... File chrome/browser/profile_resetter/profile_resetter_unittest.cc (right): https://codereview.chromium.org/196753002/diff/120001/chrome/browser/profile_... chrome/browser/profile_resetter/profile_resetter_unittest.cc:621: // and should not be re-enabled on reset if they have been disabled. Could you explain me how these external component extensions are registered? Are their locations on the harddrive and update urls hardcoded?
https://codereview.chromium.org/196753002/diff/120001/chrome/browser/profile_... File chrome/browser/profile_resetter/profile_resetter_unittest.cc (right): https://codereview.chromium.org/196753002/diff/120001/chrome/browser/profile_... chrome/browser/profile_resetter/profile_resetter_unittest.cc:621: // and should not be re-enabled on reset if they have been disabled. On 2014/03/14 09:23:15, battre wrote: > Could you explain me how these external component extensions are registered? Are > their locations on the harddrive and update urls hardcoded? The update urls are hard coded (most as constants) and the directories on the hard drive are the same as "normal" extensions. The loading occurs here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... AFAICT the only three extensions/apps which currently use this framework are hotwording, InAppPayments and the enhanced bookmark experiment.
Dominic, ping? This is for a stable blocker.
Mike, In talking with Balazs and Dominic, we uncovered an issue with the profile resetter (it would reset external component extensions as well which was an undesirable side effect). I've fixed this after discussion with Balazs with the changes in extension_service.cc::DisableUserExtensions. This function only seems to be used by the profile_resetter AFAICT. PTAL? Thanks!
lgtm https://codereview.chromium.org/196753002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/196753002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1079: extension->get()->location() != Manifest::EXTERNAL_COMPONENT) This is starting to look ugly. What about wrapping this test into a helper method? bool ShouldBeDisabled(const Extension* extension) { if (...conditions...) return true; else if (...future conditions that someone else might add...) { return true; } return false; } As-is, it's susceptible to copy-paste errors in the future. Also, with boolean tests that grow large like this, it's not really clear what's going on. Bonus points if you put the helper method somewhere that others are more likely to find and use it for other purposes in the future. https://codereview.chromium.org/196753002/diff/160001/chrome/browser/search/h... File chrome/browser/search/hotword_service.cc (right): https://codereview.chromium.org/196753002/diff/160001/chrome/browser/search/h... chrome/browser/search/hotword_service.cc:181: void HotwordService::DisableHotwordExtension() { Curious: why not pass in extension_service as a parameter to these two methods? That's the final version of the refactoring that I had in mind. If it's impossible for some reason, then leave it. But it seems like it's doable. https://codereview.chromium.org/196753002/diff/160001/chrome/browser/search/h... File chrome/browser/search/hotword_service.h (right): https://codereview.chromium.org/196753002/diff/160001/chrome/browser/search/h... chrome/browser/search/hotword_service.h:56: void EnableHotwordExtension(); Teeny nit: Enable usually comes first in the pair. https://codereview.chromium.org/196753002/diff/160001/extensions/browser/admi... File extensions/browser/admin_policy.cc (right): https://codereview.chromium.org/196753002/diff/160001/extensions/browser/admi... extensions/browser/admin_policy.cc:22: // extensions when the proper command line flag is passed. Great! Thanks.
https://codereview.chromium.org/196753002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/196753002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1079: extension->get()->location() != Manifest::EXTERNAL_COMPONENT) On 2014/03/17 20:10:51, miket wrote: > This is starting to look ugly. What about wrapping this test into a helper > method? > > bool ShouldBeDisabled(const Extension* extension) { > if (...conditions...) > return true; > else if (...future conditions that someone else might add...) { > return true; > } > return false; > } > > As-is, it's susceptible to copy-paste errors in the future. Also, with boolean > tests that grow large like this, it's not really clear what's going on. > > Bonus points if you put the helper method somewhere that others are more likely > to find and use it for other purposes in the future. Because this needs to be merged, I'd rather keep the change small at this point. I've filed a bug (crbug.com/353266) to cover this and will clean up right after this. Does that seem reasonable? https://codereview.chromium.org/196753002/diff/160001/chrome/browser/search/h... File chrome/browser/search/hotword_service.cc (right): https://codereview.chromium.org/196753002/diff/160001/chrome/browser/search/h... chrome/browser/search/hotword_service.cc:181: void HotwordService::DisableHotwordExtension() { On 2014/03/17 20:10:51, miket wrote: > Curious: why not pass in extension_service as a parameter to these two methods? > That's the final version of the refactoring that I had in mind. If it's > impossible for some reason, then leave it. But it seems like it's doable. Fixed. I didn't originally pass it in because I still have to call the GetExtensionService(..) function twice -- once here and once where I call it on line 110 above. https://codereview.chromium.org/196753002/diff/160001/chrome/browser/search/h... File chrome/browser/search/hotword_service.h (right): https://codereview.chromium.org/196753002/diff/160001/chrome/browser/search/h... chrome/browser/search/hotword_service.h:56: void EnableHotwordExtension(); On 2014/03/17 20:10:51, miket wrote: > Teeny nit: Enable usually comes first in the pair. Done.
> Because this needs to be merged, I'd rather keep the change small at this point. > I've filed a bug (crbug.com/353266) to cover this and will clean up right after > this. Does that seem reasonable? Okey dokey.
The CQ bit was checked by rlp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rlp@chromium.org/196753002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by rlp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rlp@chromium.org/196753002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by rlp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rlp@chromium.org/196753002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
On 2014/03/18 11:47:57, I haz the power (commit-bot) wrote: > Retried try job too often on win_rel for step(s) unit_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Here's the error: ExternalProviderImplTest.InAppPayments (run #1): [ RUN ] ExternalProviderImplTest.InAppPayments e:\b\build\slave\win\build\src\chrome\browser\extensions\external_provider_impl_unittest.cc(152): error: Value of: service_->IsExtensionEnabled( extension_misc::kInAppPaymentsSupportAppId) Actual: false Expected: true [ FAILED ] ExternalProviderImplTest.InAppPayments (62 ms) Anyone (Mike or Antony in particular) have any idea why this would fail on the commit bots but not the try bots? What might be different in the setup? It's clearly caused by my change somehow but I can't repro locally or on trybots -- only when I try to commit.
On 2014/03/18 16:28:41, rpetterson wrote: > On 2014/03/18 11:47:57, I haz the power (commit-bot) wrote: > > Retried try job too often on win_rel for step(s) unit_tests > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... > > Here's the error: > > ExternalProviderImplTest.InAppPayments (run #1): > [ RUN ] ExternalProviderImplTest.InAppPayments > e:\b\build\slave\win\build\src\chrome\browser\extensions\external_provider_impl_unittest.cc(152): > error: Value of: service_->IsExtensionEnabled( > extension_misc::kInAppPaymentsSupportAppId) > Actual: false > Expected: true > [ FAILED ] ExternalProviderImplTest.InAppPayments (62 ms) > > Anyone (Mike or Antony in particular) have any idea why this would fail on the > commit bots but not the try bots? What might be different in the setup? It's > clearly caused by my change somehow but I can't repro locally or on trybots -- > only when I try to commit. Any chance the commit bots are the only ones that run in Release mode, and both you and the try bots are only trying Debug? You might also touch base with mek@ who I think wrote those particular tests.
On 2014/03/18 23:14:11, asargent(VACATION until 3_18) wrote: > On 2014/03/18 16:28:41, rpetterson wrote: > > On 2014/03/18 11:47:57, I haz the power (commit-bot) wrote: > > > Retried try job too often on win_rel for step(s) unit_tests > > > > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... > > > > Here's the error: > > > > ExternalProviderImplTest.InAppPayments (run #1): > > [ RUN ] ExternalProviderImplTest.InAppPayments > > > e:\b\build\slave\win\build\src\chrome\browser\extensions\external_provider_impl_unittest.cc(152): > > error: Value of: service_->IsExtensionEnabled( > > extension_misc::kInAppPaymentsSupportAppId) > > Actual: false > > Expected: true > > [ FAILED ] ExternalProviderImplTest.InAppPayments (62 ms) > > > > Anyone (Mike or Antony in particular) have any idea why this would fail on the > > commit bots but not the try bots? What might be different in the setup? It's > > clearly caused by my change somehow but I can't repro locally or on trybots -- > > only when I try to commit. > > Any chance the commit bots are the only ones that run in Release mode, and both > you and the try bots are only trying Debug? > > You might also touch base with mek@ who I think wrote those particular tests. I did touch base with mek@. I'm trying some of his recommendations now. But I'll test the release thing, too. Thanks!
The CQ bit was checked by rlp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rlp@chromium.org/196753002/260001
Message was sent while issue was closed.
Change committed as 258106 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
