Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2166513002:
Create --disable-extensions-except switch (Closed)
|
Created:
4 years, 5 months ago by catmullings Modified:
4 years, 3 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate --disable-extensions-except switch
In response to users/developers using --disable-extensions to disable all
extensions except the ones loaded as components, --load-component-extension.
Create a --disable-extensions-except switch, which disables all extensions that
are not specified, but treats the listed extensions as normal extensions.
BUG=615096
Committed: https://crrev.com/98cd194e53387ee58c845bba224d8999aedb884e
Cr-Commit-Position: refs/heads/master@{#415462}
Patch Set 1 #Patch Set 2 : Added test #
Total comments: 24
Patch Set 3 : Addressed Patch 2 Code Review #
Total comments: 12
Patch Set 4 : Addressed Patch 3 code review; passes test #Patch Set 5 : Patch 4 merged & updated with master; Minor changes as well #Patch Set 6 : Adjusted path to test extensions s.t. fixes trybot failings #
Total comments: 8
Patch Set 7 : Addressed patch 6 code review comments #Patch Set 8 : Fixed ExtensionServiceTest.ExternalUninstall test failing #
Total comments: 3
Patch Set 9 : Addressed code review patch 8 #Patch Set 10 : Comment added to address code review commen in patch 6 #
Total comments: 6
Patch Set 11 : Addressed patch 10 code review #
Messages
Total messages: 43 (28 generated)
Initial patch without any tests. No need to formally review the patch.
catmullings@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Initial patch without any tests. No need to formally review the patch.
Test added.
Looking pretty good! https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:628: std::set<std::string> whitelist_; Whitelisted for what? https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_startup_browsertest.cc (right): https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:317: // DisableExtensionsExceptBrowserTest This line probably isn't necessary. https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:318: // Ensures that the --disable-extesions-except command line flag disables This sounds like a test description, rather than a class description. https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:331: // Adds --disable-extesions-except command line flag. Since this is pretty obvious from the code, I'd omit it. https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:338: "chrome/test/data/extensions/app_dot_com_app/"); Can we test with multiple extensions, too? Also, are we sure this path specification works on all platforms? I could see this having problems on windows where paths use \ instead of /. https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:351: if (extension->location() == extensions::Manifest::COMPONENT) optional fanciness: component_extensions += Manifest::IsComponentLocation(extension->location()) ? 1 : 0; https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:351: if (extension->location() == extensions::Manifest::COMPONENT) prefer Manifest::IsComponentLocation() over comparing directly to Manifest::Component https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:360: EXPECT_EQ(static_cast<uint32_t>(num_expected_extensions), Why not instantiate num_expected_extensions as size_t? https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:361: GetExtensionRegistry()->enabled_extensions().size()); Can we also check that the other extension is the expected extension? https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.cc:267: base::CommandLine::StringType path_list = The fact that the next 10 lines are almost identical to the 10 lines following implies that we could refactor this a bit. ;)
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed patch 2 code review comments https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:628: std::set<std::string> whitelist_; On 2016/07/25 19:10:29, Devlin wrote: > Whitelisted for what? WDYT of enabled_extensions_whitelist_ ? I could be more specific, but then the variable becomes too long: whitelist_of_extensions_overriding_disable_extensions_flag https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_startup_browsertest.cc (right): https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:317: // DisableExtensionsExceptBrowserTest On 2016/07/25 19:10:29, Devlin wrote: > This line probably isn't necessary. Done. https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:318: // Ensures that the --disable-extesions-except command line flag disables On 2016/07/25 19:10:30, Devlin wrote: > This sounds like a test description, rather than a class description. I will just remove it. https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:318: // Ensures that the --disable-extesions-except command line flag disables On 2016/07/25 19:10:30, Devlin wrote: > This sounds like a test description, rather than a class description. Done. https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:331: // Adds --disable-extesions-except command line flag. On 2016/07/25 19:10:29, Devlin wrote: > Since this is pretty obvious from the code, I'd omit it. Done. https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:338: "chrome/test/data/extensions/app_dot_com_app/"); On 2016/07/25 19:10:30, Devlin wrote: > Can we test with multiple extensions, too? > > Also, are we sure this path specification works on all platforms? I could see > this having problems on windows where paths use \ instead of /. Done. https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:351: if (extension->location() == extensions::Manifest::COMPONENT) On 2016/07/25 19:10:30, Devlin wrote: > optional fanciness: > component_extensions += Manifest::IsComponentLocation(extension->location()) ? 1 > : 0; Done. https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:351: if (extension->location() == extensions::Manifest::COMPONENT) On 2016/07/25 19:10:29, Devlin wrote: > prefer Manifest::IsComponentLocation() over comparing directly to > Manifest::Component Done. https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:360: EXPECT_EQ(static_cast<uint32_t>(num_expected_extensions), On 2016/07/25 19:10:29, Devlin wrote: > Why not instantiate num_expected_extensions as size_t? Done. https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:361: GetExtensionRegistry()->enabled_extensions().size()); On 2016/07/25 19:10:30, Devlin wrote: > Can we also check that the other extension is the expected extension? Done. https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.cc:267: base::CommandLine::StringType path_list = On 2016/07/25 19:10:30, Devlin wrote: > The fact that the next 10 lines are almost identical to the 10 lines following > implies that we could refactor this a bit. ;) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:628: std::set<std::string> whitelist_; On 2016/07/27 01:29:45, catmullings wrote: > On 2016/07/25 19:10:29, Devlin wrote: > > Whitelisted for what? > > WDYT of enabled_extensions_whitelist_ ? > > I could be more specific, but then the variable becomes too long: > > whitelist_of_extensions_overriding_disable_extensions_flag enabled_extensions_whitelist still sounds a little vague to me. I agree that we can't be entirely explicit, but maybe something like "disable_exempted_extensions_"? Could even be disable_flag_exempted_extensions_. https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:459: void AddExtensionToWhitelist(const std::string& extensionId) { "Whitelist" here is still pretty vague. But this might be moot with other comments. https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_startup_browsertest.cc (right): https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:331: #if defined(OS_WIN) This kind of platform-specific path logic is handled in base::FilePath I think. We should use that instead here so we don't have to worry about this. https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:345: IN_PROC_BROWSER_TEST_F(DisableExtensionsExceptBrowserTest, This test seems to be failing on the bots. https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:370: for (const scoped_refptr<const extensions::Extension>& extension : Given we're looping over all extensions already, it might be simpler to remove the component_extensions count and just assert that every extension is either a component or one of the expected extensions (while keeping in the assertions that both expected extensions were added). https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_system_impl.cc (left): https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.cc:268: // TODO(yoz): Seems like this should move into ExtensionService::Init. I agree with this fairly-ancient TODO. Can we move extension commandline loading to ExtensionService::Init()? That way we also do away with the public whitelist-affecting methods. https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_system_impl.h (right): https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.h:142: void LoadExtensionsFromCommandLineFlag( Two notes here: If we were to add this method, we should follow the declaration order style: https://google.github.io/styleguide/cppguide.html#Declaration_Order (at least for the most part). Specifically, methods before members. That said, I think we can probably do away with this method. See .cc file.
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed patch 3 code review. Running trybots on this patch to see if it fixed the issue. https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:628: std::set<std::string> whitelist_; On 2016/07/27 17:00:59, Devlin wrote: > On 2016/07/27 01:29:45, catmullings wrote: > > On 2016/07/25 19:10:29, Devlin wrote: > > > Whitelisted for what? > > > > WDYT of enabled_extensions_whitelist_ ? > > > > I could be more specific, but then the variable becomes too long: > > > > whitelist_of_extensions_overriding_disable_extensions_flag > > enabled_extensions_whitelist still sounds a little vague to me. I agree that we > can't be entirely explicit, but maybe something like > "disable_exempted_extensions_"? Could even be > disable_flag_exempted_extensions_. I like disable_flag_exempted_extensions_. I'll go with that. https://codereview.chromium.org/2166513002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:628: std::set<std::string> whitelist_; On 2016/07/27 17:00:59, Devlin wrote: > On 2016/07/27 01:29:45, catmullings wrote: > > On 2016/07/25 19:10:29, Devlin wrote: > > > Whitelisted for what? > > > > WDYT of enabled_extensions_whitelist_ ? > > > > I could be more specific, but then the variable becomes too long: > > > > whitelist_of_extensions_overriding_disable_extensions_flag > > enabled_extensions_whitelist still sounds a little vague to me. I agree that we > can't be entirely explicit, but maybe something like > "disable_exempted_extensions_"? Could even be > disable_flag_exempted_extensions_. Done. https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:459: void AddExtensionToWhitelist(const std::string& extensionId) { On 2016/07/27 17:00:59, Devlin wrote: > "Whitelist" here is still pretty vague. But this might be moot with other > comments. Done. https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_startup_browsertest.cc (right): https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:331: #if defined(OS_WIN) On 2016/07/27 17:00:59, Devlin wrote: > This kind of platform-specific path logic is handled in base::FilePath I think. > We should use that instead here so we don't have to worry about this. Done. https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_startup_browsertest.cc:370: for (const scoped_refptr<const extensions::Extension>& extension : On 2016/07/27 17:01:00, Devlin wrote: > Given we're looping over all extensions already, it might be simpler to remove > the component_extensions count and just assert that every extension is either a > component or one of the expected extensions (while keeping in the assertions > that both expected extensions were added). Done. https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_system_impl.cc (left): https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.cc:268: // TODO(yoz): Seems like this should move into ExtensionService::Init. On 2016/07/27 17:01:00, Devlin wrote: > I agree with this fairly-ancient TODO. Can we move extension commandline > loading to ExtensionService::Init()? That way we also do away with the public > whitelist-affecting methods. Done. https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_system_impl.h (right): https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.h:142: void LoadExtensionsFromCommandLineFlag( On 2016/07/27 17:01:00, Devlin (OOO through aug08) wrote: > Two notes here: > If we were to add this method, we should follow the declaration order style: > https://google.github.io/styleguide/cppguide.html#Declaration_Order (at least > for the most part). Specifically, methods before members. > That said, I think we can probably do away with this method. See .cc file. Moved this definition to extension_service.h. https://codereview.chromium.org/2166513002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.h:142: void LoadExtensionsFromCommandLineFlag( On 2016/07/27 17:01:00, Devlin (OOO through aug08) wrote: > Two notes here: > If we were to add this method, we should follow the declaration order style: > https://google.github.io/styleguide/cppguide.html#Declaration_Order (at least > for the most part). Specifically, methods before members. > That said, I think we can probably do away with this method. See .cc file. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
A few more comments, in addition to the tests that seem to be failing. https://codereview.chromium.org/2166513002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2166513002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:433: if (std::strcmp(switch_name, switches::kDisableExtensionsExcept) == 0) { Any reason to not compare directly? https://codereview.chromium.org/2166513002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:434: AddExtensionToExemptedFromDisableFlag(extension_id); We should probably add it to the whitelist before loading it. https://codereview.chromium.org/2166513002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/2166513002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_service.h:460: void AddExtensionToExemptedFromDisableFlag(const std::string& extensionId) { A few notes here. - These functions are pretty simple, and are only used once. We can probably get rid of them and do the work directly. - If we keep them, they shouldn't be in the public: section, since they're only used internally. - If we keep them, they should go up with the other "regular" functions, rather than in the For Testing section. https://codereview.chromium.org/2166513002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_startup_browsertest.cc (right): https://codereview.chromium.org/2166513002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_startup_browsertest.cc:344: EXPECT_TRUE( hmm... maybe: if (extension->name() == "...") ...enabled = true; else if (extension->name() == "...") ...enabled = true; else EXPECT_TRUE(Manifest::IsComponentLocation(extension->location()); https://codereview.chromium.org/2166513002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_system_impl.h (right): https://codereview.chromium.org/2166513002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.h:10: #include "base/command_line.h" needed?
Patchset #7 (id:120001) has been deleted
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
Addressed patch 6 code review and fixed ExtensionServiceTest.ExternalUninstall test failing. https://codereview.chromium.org/2166513002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2166513002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:433: if (std::strcmp(switch_name, switches::kDisableExtensionsExcept) == 0) { On 2016/08/10 19:22:31, Devlin wrote: > Any reason to not compare directly? Done. https://codereview.chromium.org/2166513002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/2166513002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_service.h:460: void AddExtensionToExemptedFromDisableFlag(const std::string& extensionId) { On 2016/08/10 19:22:31, Devlin wrote: > A few notes here. > - These functions are pretty simple, and are only used once. We can probably > get rid of them and do the work directly. > - If we keep them, they shouldn't be in the public: section, since they're only > used internally. > - If we keep them, they should go up with the other "regular" functions, rather > than in the For Testing section. I'll go with the first option. Done. https://codereview.chromium.org/2166513002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_startup_browsertest.cc (right): https://codereview.chromium.org/2166513002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_startup_browsertest.cc:344: EXPECT_TRUE( On 2016/08/10 19:22:31, Devlin wrote: > hmm... maybe: > if (extension->name() == "...") > ...enabled = true; > else if (extension->name() == "...") > ...enabled = true; > else > EXPECT_TRUE(Manifest::IsComponentLocation(extension->location()); Done.
Nice! I'll wait for the trybot run before fully stamping, but this looks pretty good. https://codereview.chromium.org/2166513002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2166513002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:448: LoadExtensionsFromCommandLineFlag(switches::kDisableExtensionsExcept); nitty nit: Can we put this directly above the block to do the other loads from command line (i.e., line 452 of this patch set)? It makes it a bit cleaner to do similar loads next to each other, and I think the loading order of component -> installed -> commandline makes sense. https://codereview.chromium.org/2166513002/diff/160001/chrome/common/chrome_s... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2166513002/diff/160001/chrome/common/chrome_s... chrome/common/chrome_switches.h:19: }; nit: newline after this.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed code review, particularly a code review comment made in patch 6. https://codereview.chromium.org/2166513002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2166513002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:448: LoadExtensionsFromCommandLineFlag(switches::kDisableExtensionsExcept); On 2016/08/29 20:53:40, Devlin wrote: > nitty nit: Can we put this directly above the block to do the other loads from > command line (i.e., line 452 of this patch set)? It makes it a bit cleaner to > do similar loads next to each other, and I think the loading order of component > -> installed -> commandline makes sense. Done. https://codereview.chromium.org/2166513002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2166513002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:434: // because code is executed asynchronously ... Devlin, I didn't quite understand the asynchronous explanation that you gave. If the code is executed asynchronously, why does that justify the ordering of these bits of code?
lgtm with a few more nits https://codereview.chromium.org/2166513002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2166513002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:421: void ExtensionService::LoadExtensionsFromCommandLineFlag( nit: function definition order in the .cc file should match the function declaration order in the .h. This file's already fairly messed up, but we can try to group this with more private functions. https://codereview.chromium.org/2166513002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:434: // because code is executed asynchronously ... On 2016/08/29 22:28:01, catmullings wrote: > Devlin, I didn't quite understand the asynchronous explanation that you gave. > If the code is executed asynchronously, why does that justify the ordering of > these bits of code? So, if this code were to be executed synchronously, the flow would be: 1. LoadFromCommandLine() 2. AddExtension() - Extension hasn't been added to whitelist; not added to service 3. Add extension id to whitelist However, since it's async, the flow is: 1. LoadExtensionFromCommandLine() starts 2. Add extension id to whitelist 3. AddExtension() - Extension has been added; works as intended Does that make sense? https://codereview.chromium.org/2166513002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:437: } nit: We don't have a strong stylistic rule for brackets if-statements with a single line beyond "match similar code". But I think the majority of extensions code avoids brackets on single-line ifs, so this should be just if (switch_name == ...) disable_flag_exempted... https://codereview.chromium.org/2166513002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:454: if (extensions_enabled_) { ditto re single line ifs https://codereview.chromium.org/2166513002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/2166513002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_service.h:461: void LoadExtensionsFromCommandLineFlag(const char* switch_name); nit: We should add a function comment here describing the behavior.
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
The CQ bit was checked by catmullings@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2166513002/#ps220001 (title: "Addressed patch 10 code review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Create --disable-extensions-except switch In response to users/developers using --disable-extensions to disable all extensions except the ones loaded as components, --load-component-extension. Create a --disable-extensions-except switch, which disables all extensions that are not specified, but treats the listed extensions as normal extensions. BUG=615096 ========== to ========== Create --disable-extensions-except switch In response to users/developers using --disable-extensions to disable all extensions except the ones loaded as components, --load-component-extension. Create a --disable-extensions-except switch, which disables all extensions that are not specified, but treats the listed extensions as normal extensions. BUG=615096 Committed: https://crrev.com/98cd194e53387ee58c845bba224d8999aedb884e Cr-Commit-Position: refs/heads/master@{#415462} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/98cd194e53387ee58c845bba224d8999aedb884e Cr-Commit-Position: refs/heads/master@{#415462} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
