Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
499313002:
Add ExtensionManagement class (Closed)
DescriptionInitial ExtensionManagement class implementation, with legacy preference parsing and interface for obtaining basic structure of extension management settings.
BUG=177351
TEST=new unit test
Committed: https://crrev.com/5f405efcf1951900430434e77f046423ac4910cf
Cr-Commit-Position: refs/heads/master@{#293198}
Patch Set 1 #Patch Set 2 : add unit_tests #Patch Set 3 : rebase minor robustness improvement #
Total comments: 35
Patch Set 4 : reject default pref value #Patch Set 5 : fixes to comments from #3 #Patch Set 6 : use std::find() #
Total comments: 5
Patch Set 7 : fixes to #5 #
Total comments: 20
Patch Set 8 : fixes to #11 #
Total comments: 1
Patch Set 9 : fixes to #13 #
Messages
Total messages: 27 (8 generated)
binjin@chromium.org changed reviewers: + joaodasilva@chromium.org
Hello Joao, I re-uploaded this CL as it's been a while from last code reviewing. I also added new unit tests. PTAL once you have time Bin
https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.cc:33: : pref_service_(pref_service) { Shouldn't this class observe the prefs in the pref_service? Or is that part of another CL? https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.cc:90: const base::DictionaryValue* dict_value; = NULL https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.cc:161: if (value && value->GetType() == expected_type) It's also possible to do value->IsType(expected_type) but I don't see a reason to favour either, so it's your choice https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_management.h (right): https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.h:23: // provide interfaces for monitoring and obtaining these settings. This comment is very generic and a bit misleading. Here's a suggestion: "Tracks the management policies that affect extensions and provides interfaces for observing and obtaining the global settings for all extensions, as well as per-extension settings." https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.h:58: scoped_ptr<URLPatternSet> install_sources; Why is this in a scoped_ptr and not just a URLPatternSet? If it's meant to indicate that there is a restricted set of install sources then it's clearer to just add a bool for that (something like has_restricted_install_sources) https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.h:59: scoped_ptr<std::vector<Manifest::Type> > allowed_types; Same here: why isn't this just a vector? If it's meant to distinguish from "empty vector because no type is allowed" vs "empty vector because policy not set and all types are allowed" then I think a boolean is clearer again. Something like has_restricted_allowed_types. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.h:69: void Refresh(); I don't think this should be public. The ExtensionManagement class should observe the pref_service_ and update automatically. Or is that part of another CL? https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.h:76: const GlobalSettings& ReadGlobalSettings() const; I think it would be simpler to have accessors for |install_sources| and |allowed_types| directly here, and have those fields as members of the ExtensionManagement class directly. The GlobalSettings structure won't be needed after that. But I haven't seen other CLs after this one, so maybe there's a reason for this. WDYT? https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.h:84: bool force_managed, Why do we force managed for some prefs only? https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.h:105: nit: single newline here https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_management_unittest.cc (right): https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management_unittest.cc:20: const std::string kTargetExtension = "abcdefghijklmnopabcdefghijklmnop"; Make this const char kTargetExtension[] = "..."; (same for the other strings). Having global std::strings will require static initializers and this is discouraged. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management_unittest.cc:23: }; nit: no ";" when closing a namespace. Add a comment: } // namespace https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management_unittest.cc:32: pref_service_.reset(new TestingPrefServiceSimple()); Why is this in a scoped_ptr and not just directly used as a value? https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management_unittest.cc:40: extension_management_.reset(new ExtensionManagement(pref_service_.get())); Same for this one, can't it be directly used as a value without the scoped_ptr? https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management_unittest.cc:45: pref_service_.reset(); Is this TearDown needed? https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management_unittest.cc:105: EXPECT_FALSE(find(allowed_types->begin(), Use Contains from base/stl_util.h
Hello Joao, I also have some changes as patchset 4, PTAL -bjin https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.cc:33: : pref_service_(pref_service) { On 2014/09/02 19:34:01, Joao da Silva wrote: > Shouldn't this class observe the prefs in the pref_service? Or is that part of > another CL? Yes, it's part of the following CL. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.cc:90: const base::DictionaryValue* dict_value; On 2014/09/02 19:34:02, Joao da Silva wrote: > = NULL Done. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.cc:161: if (value && value->GetType() == expected_type) On 2014/09/02 19:34:01, Joao da Silva wrote: > It's also possible to do value->IsType(expected_type) but I don't see a reason > to favour either, so it's your choice Done. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_management.h (right): https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.h:23: // provide interfaces for monitoring and obtaining these settings. On 2014/09/02 19:34:02, Joao da Silva wrote: > This comment is very generic and a bit misleading. Here's a suggestion: > > "Tracks the management policies that affect extensions and provides interfaces > for observing and obtaining the global settings for all extensions, as well as > per-extension settings." Done. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.h:58: scoped_ptr<URLPatternSet> install_sources; On 2014/09/02 19:34:02, Joao da Silva wrote: > Why is this in a scoped_ptr and not just a URLPatternSet? > > If it's meant to indicate that there is a restricted set of install sources then > it's clearer to just add a bool for that (something like > has_restricted_install_sources) Done. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.h:59: scoped_ptr<std::vector<Manifest::Type> > allowed_types; On 2014/09/02 19:34:02, Joao da Silva wrote: > Same here: why isn't this just a vector? > > If it's meant to distinguish from "empty vector because no type is allowed" vs > "empty vector because policy not set and all types are allowed" then I think a > boolean is clearer again. Something like has_restricted_allowed_types. Done. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.h:69: void Refresh(); On 2014/09/02 19:34:02, Joao da Silva wrote: > I don't think this should be public. The ExtensionManagement class should > observe the pref_service_ and update automatically. Or is that part of another > CL? Yes, it's part of the following CL, I will remove this to private part then. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.h:76: const GlobalSettings& ReadGlobalSettings() const; On 2014/09/02 19:34:02, Joao da Silva wrote: > I think it would be simpler to have accessors for |install_sources| and > |allowed_types| directly here, and have those fields as members of the > ExtensionManagement class directly. The GlobalSettings structure won't be needed > after that. > > But I haven't seen other CLs after this one, so maybe there's a reason for this. > WDYT? I prefer to keep this structure, there are plenty many other global settings from design doc. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.h:84: bool force_managed, On 2014/09/02 19:34:02, Joao da Silva wrote: > Why do we force managed for some prefs only? I think it might be useful for user (not admin) to manually set blocked extensions. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.h:105: On 2014/09/02 19:34:02, Joao da Silva wrote: > nit: single newline here Done. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_management_unittest.cc (right): https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management_unittest.cc:20: const std::string kTargetExtension = "abcdefghijklmnopabcdefghijklmnop"; On 2014/09/02 19:34:02, Joao da Silva wrote: > Make this const char kTargetExtension[] = "..."; (same for the other strings). > > Having global std::strings will require static initializers and this is > discouraged. Done. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management_unittest.cc:23: }; On 2014/09/02 19:34:02, Joao da Silva wrote: > nit: no ";" when closing a namespace. > > Add a comment: > > } // namespace Done. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management_unittest.cc:32: pref_service_.reset(new TestingPrefServiceSimple()); On 2014/09/02 19:34:02, Joao da Silva wrote: > Why is this in a scoped_ptr and not just directly used as a value? See below. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management_unittest.cc:40: extension_management_.reset(new ExtensionManagement(pref_service_.get())); On 2014/09/02 19:34:02, Joao da Silva wrote: > Same for this one, can't it be directly used as a value without the scoped_ptr? See below. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management_unittest.cc:45: pref_service_.reset(); On 2014/09/02 19:34:02, Joao da Silva wrote: > Is this TearDown needed? Because extension_management_ doesn't own pref_service_ and actually the destruction order matters here. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management_unittest.cc:105: EXPECT_FALSE(find(allowed_types->begin(), On 2014/09/02 19:34:02, Joao da Silva wrote: > Use Contains from base/stl_util.h stl_util.h only provide ContainsKey for containers like map/set, no Contains for vector I think.
Getting almost there. Let's discuss GlobalSettings offline. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_management.h (right): https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management.h:76: const GlobalSettings& ReadGlobalSettings() const; On 2014/09/02 20:02:56, bjin wrote: > On 2014/09/02 19:34:02, Joao da Silva wrote: > > I think it would be simpler to have accessors for |install_sources| and > > |allowed_types| directly here, and have those fields as members of the > > ExtensionManagement class directly. The GlobalSettings structure won't be > needed > > after that. > > > > But I haven't seen other CLs after this one, so maybe there's a reason for > this. > > WDYT? > I prefer to keep this structure, there are plenty many other global settings > from design doc. Right, and they are all managed by this class. So why don't have just have direct acessors in ExtensionManagement? It seems like the GlobalSettings struct is adding one more indirection that is not necessary. Let's discuss this offline. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_management_unittest.cc (right): https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management_unittest.cc:45: pref_service_.reset(); On 2014/09/02 20:02:56, bjin wrote: > On 2014/09/02 19:34:02, Joao da Silva wrote: > > Is this TearDown needed? > > Because extension_management_ doesn't own pref_service_ and actually the > destruction order matters here. They will be destroyed in the reverse order of declaration. So if you have: public: ExtensionManagementTest() : extension_management_(&pref_service_) {} protected: TestingPrefServiceSimple pref_service_; ExtensionManagement extension_management_; Then |extension_management_| gets destroyed first and |pref_service_| gets destroyed after. https://codereview.chromium.org/499313002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/499313002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:54: pref_names::kAllowedInstallSites, false, base::Value::TYPE_LIST)); Shouldn't this be force_managed too? https://codereview.chromium.org/499313002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:57: pref_names::kAllowedTypes, false, base::Value::TYPE_LIST)); Shouldn't this be force_managed too?
Patchset #7 (id:120001) has been deleted
As we discussed offline, I still prefer to keep the GlobalSettings structure in order to keep a minimal interface in a service class. PTAL at the other changes. https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_management_unittest.cc (right): https://codereview.chromium.org/499313002/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_management_unittest.cc:45: pref_service_.reset(); On 2014/09/03 09:20:40, Joao da Silva wrote: > On 2014/09/02 20:02:56, bjin wrote: > > On 2014/09/02 19:34:02, Joao da Silva wrote: > > > Is this TearDown needed? > > > > Because extension_management_ doesn't own pref_service_ and actually the > > destruction order matters here. > > They will be destroyed in the reverse order of declaration. So if you have: > > public: > ExtensionManagementTest() : extension_management_(&pref_service_) {} > > protected: > TestingPrefServiceSimple pref_service_; > ExtensionManagement extension_management_; > > Then |extension_management_| gets destroyed first and |pref_service_| gets > destroyed after. I see. But in the following CL I need to reset both pointer, so probably it's better to keep it as is now. TearDown() is removed. https://codereview.chromium.org/499313002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/499313002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:54: pref_names::kAllowedInstallSites, false, base::Value::TYPE_LIST)); On 2014/09/03 09:20:40, Joao da Silva wrote: > Shouldn't this be force_managed too? A user-set preference might be helpful for AllowedInstallSites as well, in case that a managed preference is not specified. But yes, it's rather useless, will change it to forced_managed. https://codereview.chromium.org/499313002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:57: pref_names::kAllowedTypes, false, base::Value::TYPE_LIST)); On 2014/09/03 09:20:40, Joao da Silva wrote: > Shouldn't this be force_managed too? Same as above
lgtm This needs to be reviewed by an owner of chrome/browser/extensions/ now. https://codereview.chromium.org/499313002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/499313002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:54: pref_names::kAllowedInstallSites, false, base::Value::TYPE_LIST)); On 2014/09/03 11:28:21, bjin wrote: > On 2014/09/03 09:20:40, Joao da Silva wrote: > > Shouldn't this be force_managed too? > > A user-set preference might be helpful for AllowedInstallSites as well, in case > that a managed preference is not specified. But yes, it's rather useless, will > change it to forced_managed. My concern was that malware could override this in the prefs, to get 3rd party websites whitelisted to install malicious extensions.
binjin@chromium.org changed reviewers: + finnur@chromium.org, rockot@chromium.org
rockot, funnur: Please take a look at chrome/browser/extensions/*
It is a little weird to be reviewing a class that is only used in a test... :) https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:22: } |has_restricted_install_sources| and |has_restricted_allowed_types| are uninitialized when this completes. Call Reset()? https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:36: } |default_settings_| is uninitialized when this completes. Call default_settings_.Reset()? https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:48: pref_names::kInstallDenyList, false, base::Value::TYPE_LIST)); Why is this the only one with false? Seems worthy of documentation. https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:64: // Parse defaults settings. s/defaults/default/ https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:108: global_settings_.has_restricted_install_sources = true; Can you explain this a little. Does the presence of kAllowedInstallSites mean that all other install sources are restricted? If so, should this be named |has_restricted_install_source_exception|? https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:118: << pref_names::kAllowedInstallSites << "."; Why not list the value too? https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:125: global_settings_.has_restricted_allowed_types = true; Same question with this variable. https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_management.h (right): https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.h:49: std::string update_url; nit: document. https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.h:63: bool has_restricted_allowed_types; nit: document all these. https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.h:100: GlobalSettings global_settings_; nit: Document. In my experience, this only invites headaches down the road when people see this list and think "well, I guess I don't need to document the subtle member *I'm* adding, because all these members I have no knowledge about are not documented either".
https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:22: } On 2014/09/03 13:04:01, Finnur wrote: > |has_restricted_install_sources| and |has_restricted_allowed_types| are > uninitialized when this completes. Call Reset()? Done. https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:36: } On 2014/09/03 13:04:00, Finnur wrote: > |default_settings_| is uninitialized when this completes. Call > default_settings_.Reset()? In the following CL, Refresh() will be called here. https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:48: pref_names::kInstallDenyList, false, base::Value::TYPE_LIST)); On 2014/09/03 13:04:01, Finnur wrote: > Why is this the only one with false? Seems worthy of documentation. Done. https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:64: // Parse defaults settings. On 2014/09/03 13:04:01, Finnur wrote: > s/defaults/default/ Done. https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:108: global_settings_.has_restricted_install_sources = true; On 2014/09/03 13:04:01, Finnur wrote: > Can you explain this a little. Does the presence of kAllowedInstallSites mean > that all other install sources are restricted? If so, should this be named > |has_restricted_install_source_exception|? Done. See comments in header file. https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:118: << pref_names::kAllowedInstallSites << "."; On 2014/09/03 13:04:00, Finnur wrote: > Why not list the value too? Done. https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:125: global_settings_.has_restricted_allowed_types = true; On 2014/09/03 13:04:01, Finnur wrote: > Same question with this variable. Done. See comments in header file. https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_management.h (right): https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.h:49: std::string update_url; On 2014/09/03 13:04:01, Finnur wrote: > nit: document. Done. https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.h:63: bool has_restricted_allowed_types; On 2014/09/03 13:04:01, Finnur wrote: > nit: document all these. Done. https://codereview.chromium.org/499313002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_management.h:100: GlobalSettings global_settings_; On 2014/09/03 13:04:01, Finnur wrote: > nit: Document. > > In my experience, this only invites headaches down the road when people see this > list and think "well, I guess I don't need to document the subtle member *I'm* > adding, because all these members I have no knowledge about are not documented > either". Done.
LGTM, with nit. https://codereview.chromium.org/499313002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_management.h (right): https://codereview.chromium.org/499313002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_management.h:115: // extension ID) or a group of extension(with specified extension update URL), nit: space before ( (two occurrences)
The CQ bit was checked by binjin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/499313002/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by binjin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/499313002/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by binjin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/499313002/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as 517f79487405ecc7fa1b72fa8c55a5f814f408b2
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/5f405efcf1951900430434e77f046423ac4910cf Cr-Commit-Position: refs/heads/master@{#293198} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
