Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1047943002:
Cache --whitelisted-extension-id in SimpleFeature. (Closed)
|
Created:
5 years, 8 months ago by jackhou1 Modified:
5 years, 8 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCache --whitelisted-extension-id in SimpleFeature.
During startup, CommandLine::HasSwitch(kWhitelistedExtensionID) is
called ~1000 times by SimpleFeature. It is the most common switch
lookup, the second being kAppsGalleryUpdateUrl at ~300 times.
Caching this avoids allocating and copying the string each time.
BUG=471976, 476061
Committed: https://crrev.com/16ca320592ce7ef6cd36500da6e670cc645781cd
Cr-Commit-Position: refs/heads/master@{#324583}
Committed: https://crrev.com/c587f309e39c1713a00745f3f73442d93fa4687d
Cr-Commit-Position: refs/heads/master@{#324821}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments. #
Total comments: 2
Patch Set 3 : Sync and rebase #Patch Set 4 : Address comments. #
Total comments: 2
Patch Set 5 : Address comments. #
Total comments: 4
Patch Set 6 : Fix test. #
Total comments: 4
Patch Set 7 : Simplify test. #Patch Set 8 : Fix other test #Patch Set 9 : Set command line switch in SetUp instead of SetUpTestCase. #
Total comments: 4
Patch Set 10 : Address comments #Patch Set 11 : Expose SetWhitelistedExtensionId for tests. #
Total comments: 8
Patch Set 12 : Address comments. #
Total comments: 5
Patch Set 13 : Address comments. #Patch Set 14 : Update tests that fail on memory bots. #Messages
Total messages: 49 (10 generated)
jackhou@chromium.org changed reviewers: + tapted@chromium.org
tapted, could you take a look?
tapted@chromium.org changed reviewers: + kalman@chromium.org
kalman should take a look If this is an identified hotspot, there's a lot of things that are slow around here -- probably nicer for this specifically would be a `bool extensions::Manifest::is_whitelisted_`, but feeding that through is a messier change. Then generally there are hints that features should be precompiled in some way, which would probably make this obsolete eventually. But this change will be pretty neat, and a clear win IMO. Can you make a BUG= to track this stuff? https://codereview.chromium.org/1047943002/diff/1/extensions/common/features/... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/1047943002/diff/1/extensions/common/features/... extensions/common/features/simple_feature.cc:351: extension_id != whitelisted_extension_id()) { I'm not sure we know for certain whether `extension_id` is an empty string here. If it is, and there's no --whitelisted-extension-id, it will get whitelisted. IsIdInWhitelist has a validity check on the id string, so I'm suspicious... https://codereview.chromium.org/1047943002/diff/1/extensions/common/features/... extensions/common/features/simple_feature.cc:580: if (!whitelisted_extension_id_) { use CR_DEFINE_STATIC_LOCAL(std::string, whitelisted_extension_id, ( base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( switches::kWhitelistedExtensionID) )); return whitelisted_extension_id; CR_DEFINE_STATIC_LOCAL(const std::string,... if that works (but it might not) https://codereview.chromium.org/1047943002/diff/1/extensions/common/features/... File extensions/common/features/simple_feature.h (right): https://codereview.chromium.org/1047943002/diff/1/extensions/common/features/... extensions/common/features/simple_feature.h:145: static std::string* whitelisted_extension_id_; this isn't needed (see below)
https://codereview.chromium.org/1047943002/diff/1/extensions/common/features/... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/1047943002/diff/1/extensions/common/features/... extensions/common/features/simple_feature.cc:351: extension_id != whitelisted_extension_id()) { On 2015/03/31 03:26:49, tapted wrote: > I'm not sure we know for certain whether `extension_id` is an empty string here. > If it is, and there's no --whitelisted-extension-id, it will get whitelisted. > IsIdInWhitelist has a validity check on the id string, so I'm suspicious... Added check for empty whitelisted_extension_id. https://codereview.chromium.org/1047943002/diff/1/extensions/common/features/... extensions/common/features/simple_feature.cc:580: if (!whitelisted_extension_id_) { On 2015/03/31 03:26:49, tapted wrote: > use > > CR_DEFINE_STATIC_LOCAL(std::string, whitelisted_extension_id, ( > base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( > switches::kWhitelistedExtensionID) > )); > return whitelisted_extension_id; > > > CR_DEFINE_STATIC_LOCAL(const std::string,... > > if that works (but it might not) Done. https://codereview.chromium.org/1047943002/diff/1/extensions/common/features/... File extensions/common/features/simple_feature.h (right): https://codereview.chromium.org/1047943002/diff/1/extensions/common/features/... extensions/common/features/simple_feature.h:145: static std::string* whitelisted_extension_id_; On 2015/03/31 03:26:49, tapted wrote: > this isn't needed (see below) Done.
https://codereview.chromium.org/1047943002/diff/20001/extensions/common/featu... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/1047943002/diff/20001/extensions/common/featu... extensions/common/features/simple_feature.cc:349: (whitelisted_extension_id().empty() || hm - two function calls isn't quite as nice.. If it's true that an empty extension ID might get here, then maybe instead of whitelisted_extension_id() returning a string, have bool IsWhitelistedByCommandLine(const std::string& extension_id); That probably reads better regardless :/
https://codereview.chromium.org/1047943002/diff/20001/extensions/common/featu... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/1047943002/diff/20001/extensions/common/featu... extensions/common/features/simple_feature.cc:349: (whitelisted_extension_id().empty() || On 2015/03/31 04:46:07, tapted wrote: > hm - two function calls isn't quite as nice.. > > If it's true that an empty extension ID might get here, then maybe instead of > whitelisted_extension_id() returning a string, have > > bool IsWhitelistedByCommandLine(const std::string& extension_id); > > That probably reads better regardless :/ Done.
lgtm with one more change.. sorry for my horrible review https://codereview.chromium.org/1047943002/diff/60001/extensions/common/featu... File extensions/common/features/simple_feature.h (right): https://codereview.chromium.org/1047943002/diff/60001/extensions/common/featu... extensions/common/features/simple_feature.h:177: static bool IsWhitelistedByCommandLine(const std::string& extension_id); dunno why I didn't see this before... but I think this can just be declared in an anonymous namespace. (sorry - my brain really isn't working today. I blame jet lag fugue.)
https://codereview.chromium.org/1047943002/diff/60001/extensions/common/featu... File extensions/common/features/simple_feature.h (right): https://codereview.chromium.org/1047943002/diff/60001/extensions/common/featu... extensions/common/features/simple_feature.h:177: static bool IsWhitelistedByCommandLine(const std::string& extension_id); On 2015/03/31 05:43:30, tapted wrote: > dunno why I didn't see this before... but I think this can just be declared in > an anonymous namespace. > > (sorry - my brain really isn't working today. I blame jet lag fugue.) Nah, I should have noticed it too, sorry.
kalman, please take a look.
lgtm https://codereview.chromium.org/1047943002/diff/80001/extensions/common/featu... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/1047943002/diff/80001/extensions/common/featu... extensions/common/features/simple_feature.cc:231: (base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( why are the parens around the base::CommandLine... call necessary? looks a bit odd.
https://codereview.chromium.org/1047943002/diff/80001/extensions/common/featu... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/1047943002/diff/80001/extensions/common/featu... extensions/common/features/simple_feature.cc:231: (base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( On 2015/03/31 17:16:05, kalman wrote: > why are the parens around the base::CommandLine... call necessary? looks a bit > odd. The macro is just: #define CR_DEFINE_STATIC_LOCAL(type, name, arguments) \ static type& name = *new type arguments so the () in |arguments| becomes part of the constructor syntax.
https://codereview.chromium.org/1047943002/diff/80001/extensions/common/featu... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/1047943002/diff/80001/extensions/common/featu... extensions/common/features/simple_feature.cc:231: (base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( On 2015/03/31 21:53:41, jackhou1 wrote: > On 2015/03/31 17:16:05, kalman wrote: > > why are the parens around the base::CommandLine... call necessary? looks a bit > > odd. > > The macro is just: > #define CR_DEFINE_STATIC_LOCAL(type, name, arguments) \ > static type& name = *new type arguments > > so the () in |arguments| becomes part of the constructor syntax. Well, that's silly. Macros are supposed to put parens around expressions themselves. The CR_DEFINE_STATIC_LOCAL macro should be fixed.
kalman, PTAL, needed to update a test. https://codereview.chromium.org/1047943002/diff/80001/extensions/common/featu... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/1047943002/diff/80001/extensions/common/featu... extensions/common/features/simple_feature.cc:231: (base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( On 2015/03/31 22:10:04, kalman wrote: > On 2015/03/31 21:53:41, jackhou1 wrote: > > On 2015/03/31 17:16:05, kalman wrote: > > > why are the parens around the base::CommandLine... call necessary? looks a > bit > > > odd. > > > > The macro is just: > > #define CR_DEFINE_STATIC_LOCAL(type, name, arguments) \ > > static type& name = *new type arguments > > > > so the () in |arguments| becomes part of the constructor syntax. > > Well, that's silly. Macros are supposed to put parens around expressions > themselves. The CR_DEFINE_STATIC_LOCAL macro should be fixed. Yeah probably, looks like it was originally copied from WebKit, so I can't tell if there was a good reason for it.
https://codereview.chromium.org/1047943002/diff/100001/chrome/common/extensio... File chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc (right): https://codereview.chromium.org/1047943002/diff/100001/chrome/common/extensio... chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc:77: switches::kWhitelistedExtensionID, test_id); This is why I wrote ScopedCommandLineSwitch: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... maybe this should be pulled somewhere like extension test utils.
https://codereview.chromium.org/1047943002/diff/100001/chrome/common/extensio... File chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc (right): https://codereview.chromium.org/1047943002/diff/100001/chrome/common/extensio... chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc:77: switches::kWhitelistedExtensionID, test_id); On 2015/03/31 22:21:51, kalman wrote: > This is why I wrote ScopedCommandLineSwitch: > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... > > maybe this should be pulled somewhere like extension test utils. Unfortunately ScopedCommandLineSwitch doesn't really help in this case because the cache gets set the first time it's used. But I realised the CommandLine only needs to be modified at the start of the test (rather than before).
kalman, PTAL, just want to check you're okay with the test changes before I commit.
Sorry about slow response, had to leave office early yesterday. https://codereview.chromium.org/1047943002/diff/100001/chrome/common/extensio... File chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc (right): https://codereview.chromium.org/1047943002/diff/100001/chrome/common/extensio... chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc:77: switches::kWhitelistedExtensionID, test_id); On 2015/03/31 22:55:38, jackhou1 wrote: > On 2015/03/31 22:21:51, kalman wrote: > > This is why I wrote ScopedCommandLineSwitch: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... > > > > maybe this should be pulled somewhere like extension test utils. > > Unfortunately ScopedCommandLineSwitch doesn't really help in this case because > the cache gets set the first time it's used. But I realised the CommandLine only > needs to be modified at the start of the test (rather than before). It's not so much the cache that I want to reset, rather the commandline. It seems bad to modify the commandline (process global) in a unit test (supposedly without side effects). I meant something like: class WhitelistedPlatformAppsManifestTest : public PlatformAppsManifestTest { public: static void SetUpTestCase() { command_line_.reset( new ScopedCommandLineSwitch(switches::kWhitelistedExtensionID, "ahplfneplbnjcflhdgkkjeiglkkfeelb")); } private: scoped_ptr<ScopedCommandLineSwitch> command_line_; }; I suppose you'd need to add the key-value version for ScopedCommandLineSwitch in that case. Btw should this be in a SetUp method, not SetUpTestCase? I've never seen SetUpTestCase?
https://codereview.chromium.org/1047943002/diff/100001/chrome/common/extensio... File chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc (right): https://codereview.chromium.org/1047943002/diff/100001/chrome/common/extensio... chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc:77: switches::kWhitelistedExtensionID, test_id); On 2015/04/01 17:08:21, kalman wrote: > On 2015/03/31 22:55:38, jackhou1 wrote: > > On 2015/03/31 22:21:51, kalman wrote: > > > This is why I wrote ScopedCommandLineSwitch: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... > > > > > > maybe this should be pulled somewhere like extension test utils. > > > > Unfortunately ScopedCommandLineSwitch doesn't really help in this case because > > the cache gets set the first time it's used. But I realised the CommandLine > only > > needs to be modified at the start of the test (rather than before). > > It's not so much the cache that I want to reset, rather the commandline. It > seems bad to modify the commandline (process global) in a unit test (supposedly > without side effects). I meant something like: > > class WhitelistedPlatformAppsManifestTest : public PlatformAppsManifestTest { > public: > static void SetUpTestCase() { > command_line_.reset( > new ScopedCommandLineSwitch(switches::kWhitelistedExtensionID, > "ahplfneplbnjcflhdgkkjeiglkkfeelb")); > } > > private: > scoped_ptr<ScopedCommandLineSwitch> command_line_; > }; > > I suppose you'd need to add the key-value version for ScopedCommandLineSwitch in > that case. Looks like this requires a bit of a refactor, maybe something like: https://codereview.chromium.org/1054813002 ? Alternatively, I could duplicate ScopedCommandLineSwitch in these tests. WDYT? > Btw should this be in a SetUp method, not SetUpTestCase? I've never seen > SetUpTestCase? Done. SetUpTestCase gets run before the test subclass is instantiated.
The CQ bit was checked by jackhou@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1047943002/#ps160001 (title: "Set command line switch in SetUp instead of SetUpTestCase.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1047943002/160001
The CQ bit was unchecked by commit-bot@chromium.org
This issue passed the CQ dry run.
> Looks like this requires a bit of a refactor, maybe something like: > https://codereview.chromium.org/1054813002 ? > Alternatively, I could duplicate ScopedCommandLineSwitch in these tests. > WDYT? Might as well duplicate it for this patch, no need to block on that refactor, though it would be a good idea to submit the refactor soon after.
lgtm, we should get this in by Friday, but I think there is some follow-up investigation that can be done. https://codereview.chromium.org/1047943002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/permission_message_combinations_unittest.cc (right): https://codereview.chromium.org/1047943002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/permission_message_combinations_unittest.cc:47: ",'key': 'd2hpdGVsaXN0IG1l'}"); Voodoo... I suppose the 'd2hpdGVsaXN0IG1l' corresponds with the "ahplfneplbnjcflhdgkkjeiglkkfeelb" ID? Another testing improvement would be to add a version of MakeExtension which takes an ID. All MakeExtension does is construct a manifest to pass to ExtensionBuilder, which knows how to set the ID manually. https://codereview.chromium.org/1047943002/diff/160001/extensions/common/feat... File extensions/common/features/simple_feature.cc (left): https://codereview.chromium.org/1047943002/diff/160001/extensions/common/feat... extensions/common/features/simple_feature.cc:369: // whitelist. Do you know whether this is actually set outside of tests? It actually seems like a pretty dodgy flag to be exposing to the chrome binary. It's worth looking into, because if it's not, we should just remove command line support entirely and make it in-memory (like add a ScopedWhitelist class which SimpleFeature reads from).
> lgtm, we should get this in by Friday, but I think there is some follow-up > investigation that can be done. Sorry, it was easter weekend. Let me know if you think this is worth merging into M43. https://codereview.chromium.org/1047943002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/permission_message_combinations_unittest.cc (right): https://codereview.chromium.org/1047943002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/permission_message_combinations_unittest.cc:47: ",'key': 'd2hpdGVsaXN0IG1l'}"); On 2015/04/02 14:31:57, kalman wrote: > Voodoo... I suppose the 'd2hpdGVsaXN0IG1l' corresponds with the > "ahplfneplbnjcflhdgkkjeiglkkfeelb" ID? > > Another testing improvement would be to add a version of MakeExtension which > takes an ID. All MakeExtension does is construct a manifest to pass to > ExtensionBuilder, which knows how to set the ID manually. Done. https://codereview.chromium.org/1047943002/diff/160001/extensions/common/feat... File extensions/common/features/simple_feature.cc (left): https://codereview.chromium.org/1047943002/diff/160001/extensions/common/feat... extensions/common/features/simple_feature.cc:369: // whitelist. On 2015/04/02 14:31:57, kalman wrote: > Do you know whether this is actually set outside of tests? It actually seems > like a pretty dodgy flag to be exposing to the chrome binary. It's worth looking > into, because if it's not, we should just remove command line support entirely > and make it in-memory (like add a ScopedWhitelist class which SimpleFeature > reads from). The only non-test use is here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... The other reference is in chrome_content_browser_client.cc when the switch is copied onto the command line of the renderer process. rockot (who's sitting behind me this week) suggested turning tab capture into a BehaviorFeature, then adding an interface to TestExtensionsClient to globally whitelist an extension.
kalman@chromium.org changed reviewers: + mfoltz@chromium.org
Yes it would be nice to update the TabCapture API to use a BehaviorFeature, rather than this hard-coded logic. It does make me wonder how much of the whitelist in the TabCapture API is necessary. Mark, were these added for Cast and are no longer needed? In particular the command line check, as Jack pointed out, would be nice to delete. Btw: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... for general reference.
On 2015/04/07 23:53:55, kalman wrote: > Yes it would be nice to update the TabCapture API to use a BehaviorFeature, > rather than this hard-coded logic. > > It does make me wonder how much of the whitelist in the TabCapture API is > necessary. Mark, were these added for Cast and are no longer needed? In > particular the command line check, as Jack pointed out, would be nice to delete. > > Btw: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > for general reference. We should keep the whitelist, but the command line check can be eliminated if necessary. I agree a BehaviorFeature would be better for this logic.
Unfortunately, PlatformAppsManifestTest.PlatformAppContentSecurityPolicy is still failing. Since unit tests are run in the same process, caching anything from the command line is not compatible with changing the command line in tests. I've changed it so that tests can tell SimpleFeature what the current whitelisted extension id is. PTAL
https://codereview.chromium.org/1047943002/diff/200001/extensions/common/feat... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/1047943002/diff/200001/extensions/common/feat... extensions/common/features/simple_feature.cc:30: std::string* whitelisted_extension_id = NULL; should be g_whitelisted_extension_id; https://codereview.chromium.org/1047943002/diff/200001/extensions/common/feat... extensions/common/features/simple_feature.cc:381: !IsWhitelistedByCommandLine(extension_id)) { "IsWhitelistedByCommandLine" isn't accurate anymore, more "IsWhitelistedGlobally" perhaps. Or, in the hope that these really are only used for testing, "IsWhitelistedForTest"? https://codereview.chromium.org/1047943002/diff/200001/extensions/common/feat... extensions/common/features/simple_feature.cc:581: whitelisted_extension_id = new std::string(id); This will leak in the final unit test that calls SetWhitelistedExtensionId, but, see the comment about Scoped. https://codereview.chromium.org/1047943002/diff/200001/extensions/common/feat... File extensions/common/features/simple_feature.h (right): https://codereview.chromium.org/1047943002/diff/200001/extensions/common/feat... extensions/common/features/simple_feature.h:91: static void SetWhitelistedExtensionId(const std::string& id); I prefer to only provide RAII versions of these set-for-testing flags, in this case, perhaps a "ScopedWhitelist[ForTest]" class similar to the ScopedCurrentChannel class. That would solve the memory leak issue, and really definitely make the unit tests isolated.
https://codereview.chromium.org/1047943002/diff/200001/extensions/common/feat... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/1047943002/diff/200001/extensions/common/feat... extensions/common/features/simple_feature.cc:30: std::string* whitelisted_extension_id = NULL; On 2015/04/08 17:59:22, kalman wrote: > should be g_whitelisted_extension_id; Done. https://codereview.chromium.org/1047943002/diff/200001/extensions/common/feat... extensions/common/features/simple_feature.cc:381: !IsWhitelistedByCommandLine(extension_id)) { On 2015/04/08 17:59:22, kalman wrote: > "IsWhitelistedByCommandLine" isn't accurate anymore, more > "IsWhitelistedGlobally" perhaps. Or, in the hope that these really are only used > for testing, "IsWhitelistedForTest"? Done. https://codereview.chromium.org/1047943002/diff/200001/extensions/common/feat... extensions/common/features/simple_feature.cc:581: whitelisted_extension_id = new std::string(id); On 2015/04/08 17:59:22, kalman wrote: > This will leak in the final unit test that calls SetWhitelistedExtensionId, but, > see the comment about Scoped. Done. https://codereview.chromium.org/1047943002/diff/200001/extensions/common/feat... File extensions/common/features/simple_feature.h (right): https://codereview.chromium.org/1047943002/diff/200001/extensions/common/feat... extensions/common/features/simple_feature.h:91: static void SetWhitelistedExtensionId(const std::string& id); On 2015/04/08 17:59:22, kalman wrote: > I prefer to only provide RAII versions of these set-for-testing flags, in this > case, perhaps a "ScopedWhitelist[ForTest]" class similar to the > ScopedCurrentChannel class. That would solve the memory leak issue, and really > definitely make the unit tests isolated. Done.
lgtm https://codereview.chromium.org/1047943002/diff/220001/extensions/common/feat... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/1047943002/diff/220001/extensions/common/feat... extensions/common/features/simple_feature.cc:231: if (!g_whitelisted_extension_id) { TODO(jackhou): Delete the commandline whitelisting mechanism. https://codereview.chromium.org/1047943002/diff/220001/extensions/common/feat... extensions/common/features/simple_feature.cc:236: return !g_whitelisted_extension_id->empty() && I don't think the .empty() check here is necessary. Hopefully it can never be true, and even if it is, the .empty() check isn't any more efficient than the first iteration of an equality comparison, I would assume.
https://codereview.chromium.org/1047943002/diff/220001/extensions/common/feat... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/1047943002/diff/220001/extensions/common/feat... extensions/common/features/simple_feature.cc:236: return !g_whitelisted_extension_id->empty() && On 2015/04/09 14:36:52, kalman wrote: > I don't think the .empty() check here is necessary. Hopefully it can never be > true, and even if it is, the .empty() check isn't any more efficient than the > first iteration of an equality comparison, I would assume. yeah.. I suggested this nugget of paranoia :) IsIdInWhitelist -> IsIdInList has a check, `if !IsValidExtensionId(extension_id)`. IsWhitelistedForTest doesn't have this check. The worry is that if something managed to get here with an extension id of "", it would get whitelisted by default (since it's indistinguishable from the would match the no-command-line case).
https://codereview.chromium.org/1047943002/diff/220001/extensions/common/feat... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/1047943002/diff/220001/extensions/common/feat... extensions/common/features/simple_feature.cc:236: return !g_whitelisted_extension_id->empty() && On 2015/04/09 14:56:46, tapted wrote: > On 2015/04/09 14:36:52, kalman wrote: > > I don't think the .empty() check here is necessary. Hopefully it can never be > > true, and even if it is, the .empty() check isn't any more efficient than the > > first iteration of an equality comparison, I would assume. > > yeah.. I suggested this nugget of paranoia :) > > IsIdInWhitelist -> IsIdInList has a check, `if > !IsValidExtensionId(extension_id)`. IsWhitelistedForTest doesn't have this > check. > > The worry is that if something managed to get here with an extension id of "", > it would get whitelisted by default (since it's indistinguishable from the would > match the no-command-line case). ok :-) seems like a CHECK (or early-ignore) situation somewhere, but this test is a nice bit of paranoia either way.
https://codereview.chromium.org/1047943002/diff/220001/extensions/common/feat... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/1047943002/diff/220001/extensions/common/feat... extensions/common/features/simple_feature.cc:231: if (!g_whitelisted_extension_id) { On 2015/04/09 14:36:52, kalman wrote: > TODO(jackhou): Delete the commandline whitelisting mechanism. Done.
The CQ bit was checked by jackhou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1047943002/#ps240001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1047943002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/16ca320592ce7ef6cd36500da6e670cc645781cd Cr-Commit-Position: refs/heads/master@{#324583}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/1076393002/ by mlerman@chromium.org. The reason for reverting is: https://code.google.com/p/chromium/issues/detail?id=476061 Causing consistent errors on a variety of memory bots in both InitValueManifestTest.InitFromValueInvalid and LauncherPageManifestTest.ValidLauncherPage tests..
On 2015/04/10 19:58:24, Mike Lerman wrote: > A revert of this CL (patchset #13 id:240001) has been created in > https://codereview.chromium.org/1076393002/ by mailto:mlerman@chromium.org. > > The reason for reverting is: > https://code.google.com/p/chromium/issues/detail?id=476061 > > Causing consistent errors on a variety of memory bots in both > InitValueManifestTest.InitFromValueInvalid and > LauncherPageManifestTest.ValidLauncherPage tests.. Updated those tests to use ScopedWhitelistForTest and confirmed that they pass DrMemory on Windows. Re-committing.
The CQ bit was checked by jackhou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1047943002/#ps260001 (title: "Update tests that fail on memory bots.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1047943002/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/c587f309e39c1713a00745f3f73442d93fa4687d Cr-Commit-Position: refs/heads/master@{#324821} |
