Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2680883003:
Temporarily re-introduce load-component-extension switch. (Closed)
DescriptionTemporarily re-introduce load-component-extension switch.
BUG=chromedriver:1625, 615096
Review-Url: https://codereview.chromium.org/2680883003
Cr-Commit-Position: refs/heads/master@{#450581}
Committed: https://chromium.googlesource.com/chromium/src/+/5da9a9aed98a274658b923478a68968f348ec5b5
Patch Set 1 #Patch Set 2 : --enable-chromedriver -> --enable-automation #
Total comments: 7
Patch Set 3 : address review comments #
Total comments: 4
Patch Set 4 : fix final review comments #Messages
Total messages: 23 (11 generated)
samuong@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
This was split out of https://codereview.chromium.org/2564973002/ as requested.
sschacon5994@gmail.com changed reviewers: + SSChacon5994@gmail.com
lgtm
lgtm
samuong@chromium.org changed reviewers: + sschacon5994@gmail.com - SSChacon5994@gmail.com
I've updated this to use --enable-automation, since that's what we're calling the switch now. Devlin, could you please take another look?
rdevlin.cronin@chromium.org changed reviewers: - sschacon5994@gmail.com
+catmullings@ fyi. We should also link crbug.com/615096 to track this. https://codereview.chromium.org/2680883003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2680883003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:445: // stops supporting Chrome 56. ETA? https://codereview.chromium.org/2680883003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:445: // stops supporting Chrome 56. Clarify that this only loads the extension as a regular extension, *not* as a component extension, and is thus safe. https://codereview.chromium.org/2680883003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:446: if (command_line_->HasSwitch(switches::kEnableAutomation)) Maybe something like: if (command_line_->HasSwitch(switches::kEnableAutomation) && command_line_->HasSwitch(kLoadComponentExtension)) { LOG(WARNING) << "Loading extension specified by --load-component-extension as a regular extension. Extensions specified by --load-component-extension will not be loaded starting in M<NN>. Use --load-extension or --disable-extensions-except." LoadExtensionFromCommandLineFlag(kLoadComponentExtension); } https://codereview.chromium.org/2680883003/diff/20001/chrome/common/chrome_sw... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2680883003/diff/20001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:167: extern const char kLoadComponentExtension[]; Let's isolate this definition to extension_service.cc. Things specified in switches have a nasty habit of being used.
Description was changed from ========== Temporarily re-introduce load-component-extension switch. BUG=chromedriver:1625 ========== to ========== Temporarily re-introduce load-component-extension switch. BUG=chromedriver:1625,615096 ==========
Thanks Devlin! I've included 615096 in the CL description. https://codereview.chromium.org/2680883003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2680883003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:445: // stops supporting Chrome 56. On 2017/02/14 20:44:41, Devlin wrote: > Clarify that this only loads the extension as a regular extension, *not* as a > component extension, and is thus safe. Done. ChromeDriver supports the current and previous stable releases, so we won't need this flag in Chrome 58+. I've updated the comment to mention this. https://codereview.chromium.org/2680883003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:446: if (command_line_->HasSwitch(switches::kEnableAutomation)) On 2017/02/14 20:44:41, Devlin wrote: > Maybe something like: > if (command_line_->HasSwitch(switches::kEnableAutomation) && > command_line_->HasSwitch(kLoadComponentExtension)) { > LOG(WARNING) << "Loading extension specified by --load-component-extension as > a regular extension. Extensions specified by --load-component-extension will not > be loaded starting in M<NN>. Use --load-extension or > --disable-extensions-except." > LoadExtensionFromCommandLineFlag(kLoadComponentExtension); > } Done. https://codereview.chromium.org/2680883003/diff/20001/chrome/common/chrome_sw... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2680883003/diff/20001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:167: extern const char kLoadComponentExtension[]; On 2017/02/14 20:44:41, Devlin wrote: > Let's isolate this definition to extension_service.cc. Things specified in > switches have a nasty habit of being used. Done.
lgtm; thanks Sam! https://codereview.chromium.org/2680883003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2680883003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:140: // Comma-separated list of directories with extensions to load. TODO(samuong): Remove this. See also ExtensionService::Init() https://codereview.chromium.org/2680883003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:141: const char kLoadComponentExtension[] = "load-component-extension"; s/kLoadComponentExtension/kDeprecatedLoadComponentExtension
https://codereview.chromium.org/2680883003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2680883003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:140: // Comma-separated list of directories with extensions to load. On 2017/02/14 23:57:53, Devlin wrote: > TODO(samuong): Remove this. See also ExtensionService::Init() Done. https://codereview.chromium.org/2680883003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:141: const char kLoadComponentExtension[] = "load-component-extension"; On 2017/02/14 23:57:53, Devlin wrote: > s/kLoadComponentExtension/kDeprecatedLoadComponentExtension Done.
The CQ bit was checked by samuong@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from SSChacon5994@gmail.com, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2680883003/#ps60001 (title: "fix final review comments")
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by samuong@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487133234435680,
"parent_rev": "250eb5097bfcc2d24bf814ece2b48946b90cdec8", "commit_rev":
"5da9a9aed98a274658b923478a68968f348ec5b5"}
Message was sent while issue was closed.
Description was changed from ========== Temporarily re-introduce load-component-extension switch. BUG=chromedriver:1625,615096 ========== to ========== Temporarily re-introduce load-component-extension switch. BUG=chromedriver:1625,615096 Review-Url: https://codereview.chromium.org/2680883003 Cr-Commit-Position: refs/heads/master@{#450581} Committed: https://chromium.googlesource.com/chromium/src/+/5da9a9aed98a274658b923478a68... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/5da9a9aed98a274658b923478a68... |
