Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
331743004:
Always enable enhanced bookmarks external component extensions for incognito mode (Closed)
|
Created:
6 years, 6 months ago by Mike Wittman Modified:
6 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, asargent_no_longer_on_chrome Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Visibility:
Public. |
DescriptionAlways enable enhanced bookmarks external component extensions for incognito mode
BUG=384656
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278469
Patch Set 1 #Patch Set 2 : set incognito pref on install #
Total comments: 2
Patch Set 3 : move code to extension_util #Patch Set 4 : rebase #
Total comments: 8
Patch Set 5 : address comments #
Total comments: 4
Patch Set 6 : fix nits #Messages
Total messages: 16 (0 generated)
PTAL This appeared to proper place to do the incognito check, rather than Component Loader.
where is the code which actually sets up these extensions? can't that have some kind of flag to load in incognito?
On 2014/06/16 23:58:16, kalman wrote: > where is the code which actually sets up these extensions? can't that have some > kind of flag to load in incognito? the idea being that this would set the incognito pref on install? I can look into that.
Yeah well I don't know how this code works really, but I presume these external extensions are registered somewhere and I was imagining that registration code to enable in incognito.
On 2014/06/17 19:37:19, kalman wrote: > Yeah well I don't know how this code works really, but I presume these external > extensions are registered somewhere and I was imagining that registration code > to enable in incognito. As far as I can tell, there's nothing special about external component extensions outside of some external_update_url pref setting in ExternalComponentLoader::StartLoading. They're stored alongside all the other extensions in ExtensionRegistry, via ExtensionService. I'll hang the incognito pref setting off ExtensionService::FinishInstallation unless you're aware of a better location.
On 2014/06/17 23:28:52, Mike Wittman wrote: > On 2014/06/17 19:37:19, kalman wrote: > > Yeah well I don't know how this code works really, but I presume these > external > > extensions are registered somewhere and I was imagining that registration code > > to enable in incognito. > > As far as I can tell, there's nothing special about external component > extensions outside of some external_update_url pref setting in > ExternalComponentLoader::StartLoading. They're stored alongside all the other > extensions in ExtensionRegistry, via ExtensionService. I'll hang the incognito > pref setting off ExtensionService::FinishInstallation unless you're aware of a > better location. Patch set 2 implements this approach.
https://codereview.chromium.org/331743004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/331743004/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:2555: "3F65507A3B39259B38C8173C6FFA3D12DF64CCE9" // http://crbug.com/371562 This code would be better contained in here, is where we whitelist component extensions: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
https://codereview.chromium.org/331743004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/331743004/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:2555: "3F65507A3B39259B38C8173C6FFA3D12DF64CCE9" // http://crbug.com/371562 On 2014/06/18 00:42:02, kalman wrote: > This code would be better contained in here, is where we whitelist component > extensions: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... Moved the hashes and logic to extension_utils.{cc,h}.
Ping?
https://codereview.chromium.org/331743004/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/331743004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:1988: extension_prefs_->SetIsIncognitoEnabled(extension->id(), true); i don't think you need this code anymore https://codereview.chromium.org/331743004/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/331743004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_util.cc:60: } see code here: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... might help https://codereview.chromium.org/331743004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_util.cc:75: return true; like here: if (IsWhitelistedForIncognito(..)) return true; is it because you want users to be able to turn *off* incognito mode later? that doesn't seem necessary. https://codereview.chromium.org/331743004/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_util.h (right): https://codereview.chromium.org/331743004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_util.h:34: bool EnableIncognitoByDefault(const Extension* extension); rather than making this a separate function I was imagining just calling it from IsIncognitoEnabled
https://codereview.chromium.org/331743004/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/331743004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:1988: extension_prefs_->SetIsIncognitoEnabled(extension->id(), true); On 2014/06/18 23:49:19, kalman wrote: > i don't think you need this code anymore Done. https://codereview.chromium.org/331743004/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/331743004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_util.cc:60: } On 2014/06/18 23:49:20, kalman wrote: > see code here: > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... > > might help Done. https://codereview.chromium.org/331743004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_util.cc:75: return true; On 2014/06/18 23:49:20, kalman wrote: > like here: > > if (IsWhitelistedForIncognito(..)) > return true; > > is it because you want users to be able to turn *off* incognito mode later? that > doesn't seem necessary. Either way works fine for my use case. I assumed the concern was over the cost of checking the whitelist on every call to IsIncognitoEnabled. https://codereview.chromium.org/331743004/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_util.h (right): https://codereview.chromium.org/331743004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_util.h:34: bool EnableIncognitoByDefault(const Extension* extension); On 2014/06/18 23:49:20, kalman wrote: > rather than making this a separate function I was imagining just calling it from > IsIncognitoEnabled Done.
lgtm https://codereview.chromium.org/331743004/diff/70001/chrome/browser/extension... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/331743004/diff/70001/chrome/browser/extension... chrome/browser/extensions/extension_util.cc:44: "D57DE394F36DC1C3220E7604C575D29C51A6C495", // http://crbug.com/319444 nit: 2 spaces before comment https://codereview.chromium.org/331743004/diff/70001/chrome/browser/extension... chrome/browser/extensions/extension_util.cc:69: return true; nit: curlies on body with multi-line condition.
https://codereview.chromium.org/331743004/diff/70001/chrome/browser/extension... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/331743004/diff/70001/chrome/browser/extension... chrome/browser/extensions/extension_util.cc:44: "D57DE394F36DC1C3220E7604C575D29C51A6C495", // http://crbug.com/319444 On 2014/06/19 01:27:34, kalman wrote: > nit: 2 spaces before comment Done. https://codereview.chromium.org/331743004/diff/70001/chrome/browser/extension... chrome/browser/extensions/extension_util.cc:69: return true; On 2014/06/19 01:27:34, kalman wrote: > nit: curlies on body with multi-line condition. Done.
The CQ bit was checked by wittman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wittman@chromium.org/331743004/90001
Message was sent while issue was closed.
Change committed as 278469 |
