Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23513049:
Implement install module verification metric. (Closed)
|
Created:
7 years, 3 months ago by erikwright (departed) Modified:
7 years, 2 months ago CC:
chromium-reviews, Alexei Svitkine (slow) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement install module verification metric.
BUG=297675
NOTRY=True
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227542
Patch Set 1 #
Total comments: 22
Patch Set 2 : Reviewer comments. #
Total comments: 8
Patch Set 3 : Reviewer comments. #
Total comments: 6
Patch Set 4 : Reviewer comments. #Patch Set 5 : Reviewer comments. #Patch Set 6 : Reviewer comments. #
Total comments: 6
Patch Set 7 : Merge and final nits. #
Total comments: 22
Patch Set 8 : Refactor, wstring to string16 #Patch Set 9 : Comments. #Patch Set 10 : Comments. #
Total comments: 18
Patch Set 11 : Reviewer comments. #
Total comments: 4
Patch Set 12 : Reviewer comments. #Patch Set 13 : Convert to store name digests rather than names. #Patch Set 14 : Remove obsolete file. #Patch Set 15 : Convert to a resource-based solution. #Patch Set 16 : Additional tests. #
Total comments: 7
Patch Set 17 : Reviewer comments. #Patch Set 18 : Remove legacy code/includes. IWYU. #Patch Set 19 : Change file location. #Patch Set 20 : Update to support varied EOLs. #Patch Set 21 : Line length. #Messages
Total messages: 52 (0 generated)
PTAL. I added a few comments for things I already need to do. MAD, I'll incorporate your suggestions sent via email. https://codereview.chromium.org/23513049/diff/1/chrome/browser/chrome_browser... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/chrome_browser... chrome/browser/chrome_browser_main_win.cc:232: remove one blank line https://codereview.chromium.org/23513049/diff/1/chrome/browser/expected_insta... File chrome/browser/expected_install_modules_win.h (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/expected_insta... chrome/browser/expected_install_modules_win.h:10: std::string CalculateInstalledModuleDigest(const std::string& module_name); Add comments https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... File chrome/browser/install_module_verifier_win.h (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... chrome/browser/install_module_verifier_win.h:14: void InitiateInstalledModuleVerification(); Add comments.
https://codereview.chromium.org/23513049/diff/1/chrome/browser/expected_insta... File chrome/browser/expected_install_modules_win.cc (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/expected_insta... chrome/browser/expected_install_modules_win.cc:9: } minor nit, move kExpectedInstallModuleDigests up https://codereview.chromium.org/23513049/diff/1/chrome/browser/expected_insta... File chrome/browser/expected_install_modules_win.h (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/expected_insta... chrome/browser/expected_install_modules_win.h:11: extern const char* kExpectedInstallModuleDigests[]; minor nit: move the constant up https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... chrome/browser/install_module_verifier_win.cc:34: content::NotificationService::AllSources()); <ignore-this-comment>ugh, notifications</ignore-this-comment> https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... chrome/browser/install_module_verifier_win.cc:92: for (size_t i = 0; kExpectedInstallModuleDigests[i] != 0; ++i) { nit: i -> module_id https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... File chrome/browser/install_module_verifier_win.h (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... chrome/browser/install_module_verifier_win.h:14: void InitiateInstalledModuleVerification(); naming bikeshedding: how about "BeginModuleVerification" and "VerifyModules" as shorter-and-as-descriptive names?
Also, code looks awesome :-) (hit publish too quickly)
I have a few comments, but I'm not done yet... I was interrupted with a high priority issue... So I might as well send you this now and I'll continue later on... Sorry about that... BYE MAD https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... File chrome/browser/install_module_verifier_unittest_win.cc (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... chrome/browser/install_module_verifier_unittest_win.cc:45: ASSERT_TRUE(reported_ids.empty()); These tests that we properly ignore module types, but it doesn't test that we ignore DLL names not in the array. We should add a string unlikely to be in the array (and maybe even add an ASSERT that it isn't) and make sure it doesn't get recorded... https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... chrome/browser/install_module_verifier_win.cc:92: for (size_t i = 0; kExpectedInstallModuleDigests[i] != 0; ++i) { You could avoid having a 0 at the end if you compared with arraysize(kExpectedInstallModuleDigests).
https://codereview.chromium.org/23513049/diff/1/chrome/browser/chrome_browser... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/chrome_browser... chrome/browser/chrome_browser_main_win.cc:232: On 2013/09/13 14:47:07, erikwright wrote: > remove one blank line Done. https://codereview.chromium.org/23513049/diff/1/chrome/browser/expected_insta... File chrome/browser/expected_install_modules_win.cc (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/expected_insta... chrome/browser/expected_install_modules_win.cc:9: } On 2013/09/13 17:26:05, robertshield wrote: > minor nit, move kExpectedInstallModuleDigests up Done. https://codereview.chromium.org/23513049/diff/1/chrome/browser/expected_insta... File chrome/browser/expected_install_modules_win.h (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/expected_insta... chrome/browser/expected_install_modules_win.h:10: std::string CalculateInstalledModuleDigest(const std::string& module_name); On 2013/09/13 14:47:07, erikwright wrote: > Add comments Done. https://codereview.chromium.org/23513049/diff/1/chrome/browser/expected_insta... chrome/browser/expected_install_modules_win.h:11: extern const char* kExpectedInstallModuleDigests[]; On 2013/09/13 17:26:05, robertshield wrote: > minor nit: move the constant up Done. https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... chrome/browser/install_module_verifier_win.cc:92: for (size_t i = 0; kExpectedInstallModuleDigests[i] != 0; ++i) { On 2013/09/13 20:15:33, MAD wrote: > You could avoid having a 0 at the end if you compared with > arraysize(kExpectedInstallModuleDigests). arraysize doesn't work with pointers like this. It might work if the header defined the size of the array, but that's not practical here. See basictypes.h and note that arraysize is a template that requires the size of the array to be inferred from its type at compile time.
https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... chrome/browser/install_module_verifier_win.cc:92: for (size_t i = 0; kExpectedInstallModuleDigests[i] != 0; ++i) { I don't understand. Why do you say that the size is not known at compile time? I use arraysize for const char* arrays like this and it works fine... What am I missing?
On 2013/09/13 20:34:38, MAD wrote: > https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... > File chrome/browser/install_module_verifier_win.cc (right): > > https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... > chrome/browser/install_module_verifier_win.cc:92: for (size_t i = 0; > kExpectedInstallModuleDigests[i] != 0; ++i) { > I don't understand. Why do you say that the size is not known at compile time? I > use arraysize for const char* arrays like this and it works fine... What am I > missing? Can you show me an example where it is not declared, defined, and passed to arraysize in a single compilation unit?
D'Ho! Sorry about that... I forgot about the extern declaration... My goof... Move along.. :-) On Fri, Sep 13, 2013 at 4:39 PM, <erikwright@chromium.org> wrote: > On 2013/09/13 20:34:38, MAD wrote: > > https://codereview.chromium.**org/23513049/diff/1/chrome/** > browser/install_module_**verifier_win.cc<https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module_verifier_win.cc> > >> File chrome/browser/install_module_**verifier_win.cc (right): >> > > > https://codereview.chromium.**org/23513049/diff/1/chrome/** > browser/install_module_**verifier_win.cc#newcode92<https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module_verifier_win.cc#newcode92> > >> chrome/browser/install_module_**verifier_win.cc:92: for (size_t i = 0; >> kExpectedInstallModuleDigests[**i] != 0; ++i) { >> I don't understand. Why do you say that the size is not known at compile >> time? >> > I > >> use arraysize for const char* arrays like this and it works fine... What >> am I >> missing? >> > > Can you show me an example where it is not declared, defined, and passed to > arraysize in a single compilation unit? > > https://codereview.chromium.**org/23513049/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Some more minor comments... Very well done... Thanks! BYE MAD https://codereview.chromium.org/23513049/diff/12001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/12001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:72: std::set<std::string> module_name_digests; digest -> canonical? https://codereview.chromium.org/23513049/diff/12001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:88: std::string module_name_digest = CanonicalizeModuleName(module_name); digest -> canonical?
One more small thing that I just thought about... BYE MAD https://codereview.chromium.org/23513049/diff/12001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_win.h (right): https://codereview.chromium.org/23513049/diff/12001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.h:16: const base::Callback<void(unsigned int)>& delegate); Why not size_t instead of unsigned int?
One more thing... https://codereview.chromium.org/23513049/diff/12001/chrome/browser/expected_i... File chrome/browser/expected_install_modules_win.cc (right): https://codereview.chromium.org/23513049/diff/12001/chrome/browser/expected_i... chrome/browser/expected_install_modules_win.cc:32: size_t slash = module_name.rfind('/'); On windows, you should be looking for '\'... And why not use FilePath::BaseName instead?
https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... File chrome/browser/install_module_verifier_win.h (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... chrome/browser/install_module_verifier_win.h:14: void InitiateInstalledModuleVerification(); On 2013/09/13 17:26:05, robertshield wrote: > naming bikeshedding: how about "BeginModuleVerification" and "VerifyModules" as > shorter-and-as-descriptive names? No bites on the bikeshedding? https://codereview.chromium.org/23513049/diff/27001/chrome/browser/expected_i... File chrome/browser/expected_install_modules_win.cc (right): https://codereview.chromium.org/23513049/diff/27001/chrome/browser/expected_i... chrome/browser/expected_install_modules_win.cc:9: // reporting. Would an array of (module, id) pairs be less prone to accidental reordering? Something like: static const struct { const char* name; const int id; } kExpectedInstallModules[] = { { "chrome.dll", 0 }, ... }; https://codereview.chromium.org/23513049/diff/27001/chrome/browser/expected_i... chrome/browser/expected_install_modules_win.cc:32: size_t slash = module_name.rfind('/'); Where does this get its module names from? This looks un-windowsy. Also, FilePath::BaseName?
PTAL. https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... File chrome/browser/install_module_verifier_unittest_win.cc (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... chrome/browser/install_module_verifier_unittest_win.cc:45: ASSERT_TRUE(reported_ids.empty()); On 2013/09/13 20:15:33, MAD wrote: > These tests that we properly ignore module types, but it doesn't test that we > ignore DLL names not in the array. We should add a string unlikely to be in the > array (and maybe even add an ASSERT that it isn't) and make sure it doesn't get > recorded... Done. https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... chrome/browser/install_module_verifier_win.cc:34: content::NotificationService::AllSources()); On 2013/09/13 17:26:05, robertshield wrote: > <ignore-this-comment>ugh, notifications</ignore-this-comment> Done. https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... chrome/browser/install_module_verifier_win.cc:92: for (size_t i = 0; kExpectedInstallModuleDigests[i] != 0; ++i) { On 2013/09/13 17:26:05, robertshield wrote: > nit: i -> module_id Done. https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... File chrome/browser/install_module_verifier_win.h (right): https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... chrome/browser/install_module_verifier_win.h:14: void InitiateInstalledModuleVerification(); On 2013/09/13 17:26:05, robertshield wrote: > naming bikeshedding: how about "BeginModuleVerification" and "VerifyModules" as > shorter-and-as-descriptive names? Done. https://codereview.chromium.org/23513049/diff/1/chrome/browser/install_module... chrome/browser/install_module_verifier_win.h:14: void InitiateInstalledModuleVerification(); On 2013/09/16 15:07:21, robertshield wrote: > On 2013/09/13 17:26:05, robertshield wrote: > > naming bikeshedding: how about "BeginModuleVerification" and "VerifyModules" > as > > shorter-and-as-descriptive names? > > No bites on the bikeshedding? Done. https://codereview.chromium.org/23513049/diff/12001/chrome/browser/expected_i... File chrome/browser/expected_install_modules_win.cc (right): https://codereview.chromium.org/23513049/diff/12001/chrome/browser/expected_i... chrome/browser/expected_install_modules_win.cc:32: size_t slash = module_name.rfind('/'); On 2013/09/16 14:42:43, MAD wrote: > On windows, you should be looking for '\'... > > And why not use FilePath::BaseName instead? Done. https://codereview.chromium.org/23513049/diff/12001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/12001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:72: std::set<std::string> module_name_digests; On 2013/09/16 00:26:16, MAD wrote: > digest -> canonical? Done. https://codereview.chromium.org/23513049/diff/12001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:88: std::string module_name_digest = CanonicalizeModuleName(module_name); On 2013/09/16 00:26:16, MAD wrote: > digest -> canonical? Done. https://codereview.chromium.org/23513049/diff/12001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_win.h (right): https://codereview.chromium.org/23513049/diff/12001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.h:16: const base::Callback<void(unsigned int)>& delegate); On 2013/09/16 13:47:01, MAD wrote: > Why not size_t instead of unsigned int? Done. https://codereview.chromium.org/23513049/diff/27001/chrome/browser/expected_i... File chrome/browser/expected_install_modules_win.cc (right): https://codereview.chromium.org/23513049/diff/27001/chrome/browser/expected_i... chrome/browser/expected_install_modules_win.cc:32: size_t slash = module_name.rfind('/'); On 2013/09/16 15:07:21, robertshield wrote: > Where does this get its module names from? This looks un-windowsy. Also, > FilePath::BaseName? Done.
MAD/Robert: Ping.
OK, LGTM with a last set of small potential nits... Thanks! BYE MAD https://codereview.chromium.org/23513049/diff/16001/chrome/browser/expected_i... File chrome/browser/expected_install_modules_win.cc (right): https://codereview.chromium.org/23513049/diff/16001/chrome/browser/expected_i... chrome/browser/expected_install_modules_win.cc:28: 0 0 -> NULL? https://codereview.chromium.org/23513049/diff/16001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_unittest_win.cc (right): https://codereview.chromium.org/23513049/diff/16001/chrome/browser/install_mo... chrome/browser/install_module_verifier_unittest_win.cc:15: unnamed namespace? https://codereview.chromium.org/23513049/diff/16001/chrome/browser/install_mo... chrome/browser/install_module_verifier_unittest_win.cc:31: std::vector<unsigned int> reported_ids; unsigned int -> size_t? https://codereview.chromium.org/23513049/diff/16001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/16001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:71: const base::Callback<void(size_t)>& delegate) { A DCHECK that module_list is not NULL? https://codereview.chromium.org/23513049/diff/16001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:92: for (size_t module_index = 0; kExpectedInstallModules[module_index] != 0; 0 -> NULL?
LGTM, please address comment from earlier revision. https://codereview.chromium.org/23513049/diff/27001/chrome/browser/expected_i... File chrome/browser/expected_install_modules_win.cc (right): https://codereview.chromium.org/23513049/diff/27001/chrome/browser/expected_i... chrome/browser/expected_install_modules_win.cc:9: // reporting. On 2013/09/16 15:07:21, robertshield wrote: > Would an array of (module, id) pairs be less prone to accidental reordering? > Something like: > > static const struct { > const char* name; > const int id; > } kExpectedInstallModules[] = { > { "chrome.dll", 0 }, > ... > }; Let me know what you think of the above. https://codereview.chromium.org/23513049/diff/16001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/16001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:62: base::Bind(&OnModuleMatch)); nit: indent
https://codereview.chromium.org/23513049/diff/27001/chrome/browser/expected_i... File chrome/browser/expected_install_modules_win.cc (right): https://codereview.chromium.org/23513049/diff/27001/chrome/browser/expected_i... chrome/browser/expected_install_modules_win.cc:9: // reporting. On 2013/09/17 17:06:59, robertshield wrote: > On 2013/09/16 15:07:21, robertshield wrote: > > Would an array of (module, id) pairs be less prone to accidental reordering? > > Something like: > > > > static const struct { > > const char* name; > > const int id; > > } kExpectedInstallModules[] = { > > { "chrome.dll", 0 }, > > ... > > }; > > Let me know what you think of the above. Sorry for forgetting to reply here. I'm not convinced it's any better. You have the risk of copy-paste error in that case. I'll do it if you feel strongly it's better. Seems mildly more complex for unclear gain to me.
still LGTM https://codereview.chromium.org/23513049/diff/27001/chrome/browser/expected_i... File chrome/browser/expected_install_modules_win.cc (right): https://codereview.chromium.org/23513049/diff/27001/chrome/browser/expected_i... chrome/browser/expected_install_modules_win.cc:9: // reporting. On 2013/09/17 17:20:52, erikwright wrote: > On 2013/09/17 17:06:59, robertshield wrote: > > On 2013/09/16 15:07:21, robertshield wrote: > > > Would an array of (module, id) pairs be less prone to accidental reordering? > > > Something like: > > > > > > static const struct { > > > const char* name; > > > const int id; > > > } kExpectedInstallModules[] = { > > > { "chrome.dll", 0 }, > > > ... > > > }; > > > > Let me know what you think of the above. > > Sorry for forgetting to reply here. > > I'm not convinced it's any better. You have the risk of copy-paste error in that > case. > > I'll do it if you feel strongly it's better. Seems mildly more complex for > unclear gain to me. Fair enough. My theory is that the copy paste error will seem wrong to someone whose OCD triggers an allergic reaction at the sight of {xx, 0}, {xx, 2}, {xx, 1}, {xx, 3}. i.e. most of us :-) I can see the complexity argument though, so it's fine to leave it as is, but in that case please make THE COMMENT SHOUT a bit to make it slightly harder to miss.
Scott, PTAL. Finnur, FYI for usage of EnumerateModulesModel.
https://codereview.chromium.org/23513049/diff/8001/chrome/browser/expected_in... File chrome/browser/expected_install_modules_win.cc (right): https://codereview.chromium.org/23513049/diff/8001/chrome/browser/expected_in... chrome/browser/expected_install_modules_win.cc:9: // DO NOT REORDER THESE ELEMENTS. Their index is used as an ID in metrics Can we use strings in the reporting to avoid having to worry about indices or name changes? https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:71: void VerifyModules(base::ListValue* module_list, Can this take a const base::ListValue& ? https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:75: std::set<std::wstring> canonical_module_names; This code would be clearer if you broke this into two steps. One to build the names the delegate is to run with, and then another to run on the delegate. And a description here would be nice.
General comments: Guess you didn't need to modify the EnumerateModulesModel classes? Neat. You are missing a BUG= in your description. More specific comments: https://codereview.chromium.org/23513049/diff/8001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/23513049/diff/8001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main_win.cc:230: BeginModuleVerification(); So... we decided (when I changed the module enumerator to run always on XP at startup) to start that on a delay, so as to not let the ModuleEnumerator affected startup. The delay in that case was supposed to be 45 seconds (see comment from cpu): https://codereview.chromium.org/15969017/ This will start the module enumeration much sooner, no? My worry is that this could be right in the middle of the startup whirlwind and adversely affect it. https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:40: const content::NotificationDetails& details) OVERRIDE { Maybe DCHECK that you got back type == NOTIFICATION_MODULE_LIST_ENUMERATED? https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:47: } nit: No braces around single line ifs. https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:53: DISALLOW_COPY_AND_ASSIGN(InstallModuleVerifier); nit: Space before this line. https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:90: std::wstring module_name = base::UTF8ToWide(utf8_module_name); nit: s/std::wstring/string16/g Yes, it is the same thing on Windows, but more readable in my opinion (and more portable, in case this gets copied somewhere). https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:100: } Nit: Single line -- no brace. Also, don't you want EndsWith instead of find? https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... File chrome/browser/install_module_verifier_win.h (right): https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.h:20: // expected_install_modules_win.h . nit: space before period.
PTAL. +asvitkine, FYI. https://codereview.chromium.org/23513049/diff/8001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/23513049/diff/8001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main_win.cc:230: BeginModuleVerification(); On 2013/09/20 14:23:14, Finnur wrote: > So... we decided (when I changed the module enumerator to run always on XP at > startup) to start that on a delay, so as to not let the ModuleEnumerator > affected startup. The delay in that case was supposed to be 45 seconds (see > comment from cpu): https://codereview.chromium.org/15969017/ > > This will start the module enumeration much sooner, no? My worry is that this > could be right in the middle of the startup whirlwind and adversely affect it. I added a delay here. https://codereview.chromium.org/23513049/diff/8001/chrome/browser/expected_in... File chrome/browser/expected_install_modules_win.cc (right): https://codereview.chromium.org/23513049/diff/8001/chrome/browser/expected_in... chrome/browser/expected_install_modules_win.cc:9: // DO NOT REORDER THESE ELEMENTS. Their index is used as an ID in metrics On 2013/09/19 20:54:36, sky wrote: > Can we use strings in the reporting to avoid having to worry about indices or > name changes? These are UMA metrics we are talking about. To send strings we would (AFAIU) have to use a different UMA metric per entry. https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:40: const content::NotificationDetails& details) OVERRIDE { On 2013/09/20 14:23:14, Finnur wrote: > Maybe DCHECK that you got back type == NOTIFICATION_MODULE_LIST_ENUMERATED? Done. https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:47: } On 2013/09/20 14:23:14, Finnur wrote: > nit: No braces around single line ifs. Done. https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:53: DISALLOW_COPY_AND_ASSIGN(InstallModuleVerifier); On 2013/09/20 14:23:14, Finnur wrote: > nit: Space before this line. Done. https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:71: void VerifyModules(base::ListValue* module_list, On 2013/09/19 20:54:36, sky wrote: > Can this take a const base::ListValue& ? Done. https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:75: std::set<std::wstring> canonical_module_names; On 2013/09/19 20:54:36, sky wrote: > This code would be clearer if you broke this into two steps. One to build the > names the delegate is to run with, and then another to run on the delegate. > > And a description here would be nice. Let me know if the new factoring corresponds to what you had in mind. I added header comments and an implementation comment. Let me know if you were looking for something more. https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:90: std::wstring module_name = base::UTF8ToWide(utf8_module_name); On 2013/09/20 14:23:14, Finnur wrote: > nit: s/std::wstring/string16/g > Yes, it is the same thing on Windows, but more readable in my opinion (and more > portable, in case this gets copied somewhere). Done. https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:100: } On 2013/09/20 14:23:14, Finnur wrote: > Nit: Single line -- no brace. My understanding is that, if the condition is more than one line, we use braces. > Also, don't you want EndsWith instead of find? No. The names are already converted to a canonical form ignoring path, case, etc. So we have put them all in a set and for each expected value we check if it's in the set. https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... File chrome/browser/install_module_verifier_win.h (right): https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.h:20: // expected_install_modules_win.h . On 2013/09/20 14:23:14, Finnur wrote: > nit: space before period. Done.
LGTM with a nit https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:28: void OnModuleMatch(size_t module_id) { Nit: I'd call this module_index for consistency with what's passed in.
https://codereview.chromium.org/23513049/diff/73001/chrome/browser/chrome_bro... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/23513049/diff/73001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_main_win.cc:232: content::BrowserThread::GetMessageLoopProxyForThread( Why do we need to delay here? Assuming we do, document why and why 45 seconds. https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_unittest_win.cc (right): https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... chrome/browser/install_module_verifier_unittest_win.cc:10: #include "base/bind.h" nit: newline between 9/10 https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:9: #include "base/basictypes.h" nit: newline between 8/9 https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:38: class InstallModuleVerifier : public content::NotificationObserver { Description, in fact all your private functions should have descriptions too. https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:60: content::NotificationRegistrar notification_registrar_; private destructor to make it class this object owns its lifetime. https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_win.h (right): https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.h:9: #include "base/callback_forward.h" nit: newline between 8/9 https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.h:29: const base::Callback<void(size_t)>& delegate); Why the callback here? Is the plan that it'll be async soon?
https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:90: std::wstring module_name = base::UTF8ToWide(utf8_module_name); Hmmm, I didn't realize we had started using the base:: prefix everywhere for string16. :/ It was also a bigger change than I thought it would be when I made the request. Anyway... Thanks for indulging me. On 2013/09/24 17:33:51, erikwright wrote: > On 2013/09/20 14:23:14, Finnur wrote: > > nit: s/std::wstring/string16/g > > Yes, it is the same thing on Windows, but more readable in my opinion (and > more > > portable, in case this gets copied somewhere). > > Done. https://codereview.chromium.org/23513049/diff/8001/chrome/browser/install_mod... chrome/browser/install_module_verifier_win.cc:100: } Oh, it is a *set*. I overlooked that part. I thought it was the string::find function, not set::find. I was worried about "e.dll" matching with "chrome.dll" (and I now realize I also meant to say compare() not EndsWith()...) :) But, looks like we are good here. As for the other question: Yes, that's a very common misunderstanding. The rule said: "Curly braces are not required for single-line conditionals or loop bodies, but are required for single-statement bodies that span multiple lines." Note the use of the word 'body' there and not 'conditional or body'. However, after looking this up I see that at least the Blink style guide seems to be relaxing this rule, so maybe that applies to Chromium as well. Anyway, the takeaway from this discussion for you should be: "Don't feel compelled to add braces around this one-line body just because the conditional is multi-line. https://codereview.chromium.org/23513049/diff/73001/chrome/browser/chrome_bro... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/23513049/diff/73001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_main_win.cc:232: content::BrowserThread::GetMessageLoopProxyForThread( I touched on this in my earlier comment. In short: When I was (in another changelist, see https://codereview.chromium.org/15969017/) making the Module Enumerator run automatically on XP there were concerns that it would interfere with startup. Carlos listed (in that changelist) a number of things that start at various intervals and said (paraphrasing) "judging by what else is starting up (and at which time) it looks like 45 seconds is a good slot to run this potentially lengthy operation at" (can take several seconds in worst cases). Since this changelist would (without the delay) reverse that decision (trigger the ModuleEnumerator to run sooner), we applied the same logic here. On 2013/09/24 20:16:16, sky wrote: > Why do we need to delay here? Assuming we do, document why and why 45 seconds. https://codereview.chromium.org/23513049/diff/81001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_unittest_win.cc (right): https://codereview.chromium.org/23513049/diff/81001/chrome/browser/install_mo... chrome/browser/install_module_verifier_unittest_win.cc:43: AddModule(list.get(), ModuleEnumerator::SHELL_EXTENSION, "chrome.dll"); If the aim here is a negative test (make sure that shell extensions and loaded modules are not present in the resulting list) it seems weird to use a dll name you explicitly check for. For example, what if you get back a chrome.dll of type SHELL_EXTENSION but not a chrome.dll of type LOADED_MODULE? Your test would pass, no? https://codereview.chromium.org/23513049/diff/81001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/81001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:44: class InstallModuleVerifier : private content::NotificationObserver { Private inheritance is against the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri... You can still put the implementation of Observe in the private clause, I believe.
Forgot: LGTM.
PTAL. https://codereview.chromium.org/23513049/diff/73001/chrome/browser/chrome_bro... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/23513049/diff/73001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_main_win.cc:232: content::BrowserThread::GetMessageLoopProxyForThread( On 2013/09/24 20:16:16, sky wrote: > Why do we need to delay here? Assuming we do, document why and why 45 seconds. See Finnur's comment in this CL, which references this prior comment from cpu: https://codereview.chromium.org/15969017/#msg6 I would love to have less arbitrary ways to do this kind of deferral but I believe this is the state of the art for now. https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_unittest_win.cc (right): https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... chrome/browser/install_module_verifier_unittest_win.cc:10: #include "base/bind.h" On 2013/09/24 20:16:16, sky wrote: > nit: newline between 9/10 Done. https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:9: #include "base/basictypes.h" On 2013/09/24 20:16:16, sky wrote: > nit: newline between 8/9 Done. https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:28: void OnModuleMatch(size_t module_id) { On 2013/09/24 18:52:25, Alexei Svitkine wrote: > Nit: I'd call this module_index for consistency with what's passed in. Done. https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:38: class InstallModuleVerifier : public content::NotificationObserver { On 2013/09/24 20:16:16, sky wrote: > Description, in fact all your private functions should have descriptions too. Done. https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:60: content::NotificationRegistrar notification_registrar_; On 2013/09/24 20:16:16, sky wrote: > private destructor to make it class this object owns its lifetime. Done. https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_win.h (right): https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.h:9: #include "base/callback_forward.h" On 2013/09/24 20:16:16, sky wrote: > nit: newline between 8/9 Done. https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.h:29: const base::Callback<void(size_t)>& delegate); On 2013/09/24 20:16:16, sky wrote: > Why the callback here? Is the plan that it'll be async soon? For testability. Internally the callback just does a histogram, but the tests can give a mock callback to verify the data that is being reported. https://codereview.chromium.org/23513049/diff/81001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_unittest_win.cc (right): https://codereview.chromium.org/23513049/diff/81001/chrome/browser/install_mo... chrome/browser/install_module_verifier_unittest_win.cc:43: AddModule(list.get(), ModuleEnumerator::SHELL_EXTENSION, "chrome.dll"); On 2013/09/25 12:54:22, Finnur wrote: > If the aim here is a negative test (make sure that shell extensions and loaded > modules are not present in the resulting list) it seems weird to use a dll name > you explicitly check for. For example, what if you get back a chrome.dll of type > SHELL_EXTENSION but not a chrome.dll of type LOADED_MODULE? Your test would > pass, no? You're right. It made sense a few patchsets ago, but not anymore. https://codereview.chromium.org/23513049/diff/81001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/81001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.cc:44: class InstallModuleVerifier : private content::NotificationObserver { On 2013/09/25 12:54:22, Finnur wrote: > Private inheritance is against the style guide: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri... > > You can still put the implementation of Observe in the private clause, I > believe. Done. Yes you can make Observe private but it is merely documentation. On the other hand, since the constructor is private there is no way to get an instance of this class so it's not a big deal.
sky: PTAL.
Please reupload as patchset didn't make it up correctly.
sky@: PTAL. New patchset, also adapted to a resource-based approach.
https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... File chrome/browser/install_module_verifier_win.h (right): https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_mo... chrome/browser/install_module_verifier_win.h:29: const base::Callback<void(size_t)>& delegate); On 2013/09/25 19:58:08, erikwright wrote: > On 2013/09/24 20:16:16, sky wrote: > > Why the callback here? Is the plan that it'll be async soon? > > For testability. Internally the callback just does a histogram, but the tests > can give a mock callback to verify the data that is being reported. Would a function pointer suffice then?
Take a look at the unit tests. They bind in a vector where the response is stored. So a function pointer would force that to have a single-arg signature and use a global. Yes, it's possible, but extra complexity and I don't see the gain. On Fri, Oct 4, 2013 at 2:20 PM, <sky@chromium.org> wrote: > > https://codereview.chromium.**org/23513049/diff/73001/** > chrome/browser/install_module_**verifier_win.h<https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_module_verifier_win.h> > File chrome/browser/install_module_**verifier_win.h (right): > > https://codereview.chromium.**org/23513049/diff/73001/** > chrome/browser/install_module_**verifier_win.h#newcode29<https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_module_verifier_win.h#newcode29> > chrome/browser/install_module_**verifier_win.h:29: const > base::Callback<void(size_t)>& delegate); > On 2013/09/25 19:58:08, erikwright wrote: > >> On 2013/09/24 20:16:16, sky wrote: >> > Why the callback here? Is the plan that it'll be async soon? >> > > For testability. Internally the callback just does a histogram, but >> > the tests > >> can give a mock callback to verify the data that is being reported. >> > > Would a function pointer suffice then? > > https://codereview.chromium.**org/23513049/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lg, one question, one nit https://codereview.chromium.org/23513049/diff/120001/chrome/browser/browser_r... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/23513049/diff/120001/chrome/browser/browser_r... chrome/browser/browser_resources.grd:394: <include name="IDR_ADDITIONAL_MODULES_LIST" file="internal\resources\browser\additional_modules_list.txt" type="BINDATA" /> does this build in a non google_chrome build? https://codereview.chromium.org/23513049/diff/120001/chrome/browser/install_m... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/120001/chrome/browser/install_m... chrome/browser/install_module_verifier_win.cc:172: AdditionalModules* additional_modules) { DCHECK(additional_modules);
On 2013/10/04 18:26:20, erikwright wrote: > Take a look at the unit tests. They bind in a vector where the response is > stored. So a function pointer would force that to have a single-arg > signature and use a global. Yes, it's possible, but extra complexity and I > don't see the gain. > > > > On Fri, Oct 4, 2013 at 2:20 PM, <mailto:sky@chromium.org> wrote: > > > > > https://codereview.chromium.**org/23513049/diff/73001/** > > > chrome/browser/install_module_**verifier_win.h<https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_module_verifier_win.h> > > File chrome/browser/install_module_**verifier_win.h (right): > > > > https://codereview.chromium.**org/23513049/diff/73001/** > > > chrome/browser/install_module_**verifier_win.h#newcode29<https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_module_verifier_win.h#newcode29> > > chrome/browser/install_module_**verifier_win.h:29: const > > base::Callback<void(size_t)>& delegate); > > On 2013/09/25 19:58:08, erikwright wrote: > > > >> On 2013/09/24 20:16:16, sky wrote: > >> > Why the callback here? Is the plan that it'll be async soon? > >> > > > > For testability. Internally the callback just does a histogram, but > >> > > the tests > > > >> can give a mock callback to verify the data that is being reported. > >> > > > > Would a function pointer suffice then? > > > > > https://codereview.chromium.**org/23513049/%3Chttps://codereview.chromium.org...> > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. If it's not async, why not have a return value then?
https://codereview.chromium.org/23513049/diff/120001/chrome/browser/install_m... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/120001/chrome/browser/install_m... chrome/browser/install_module_verifier_win.cc:87: InstallModuleVerifier() { nit: constructor/destructor before other methods in a section. https://codereview.chromium.org/23513049/diff/120001/chrome/browser/install_m... File chrome/browser/install_module_verifier_win.h (right): https://codereview.chromium.org/23513049/diff/120001/chrome/browser/install_m... chrome/browser/install_module_verifier_win.h:8: #include <utility> nit: sort
PTAL. https://codereview.chromium.org/23513049/diff/120001/chrome/browser/install_m... File chrome/browser/install_module_verifier_win.cc (right): https://codereview.chromium.org/23513049/diff/120001/chrome/browser/install_m... chrome/browser/install_module_verifier_win.cc:87: InstallModuleVerifier() { On 2013/10/04 19:21:54, sky wrote: > nit: constructor/destructor before other methods in a section. Done. https://codereview.chromium.org/23513049/diff/120001/chrome/browser/install_m... chrome/browser/install_module_verifier_win.cc:172: AdditionalModules* additional_modules) { On 2013/10/04 19:12:14, robertshield wrote: > DCHECK(additional_modules); Done. https://codereview.chromium.org/23513049/diff/120001/chrome/browser/install_m... File chrome/browser/install_module_verifier_win.h (right): https://codereview.chromium.org/23513049/diff/120001/chrome/browser/install_m... chrome/browser/install_module_verifier_win.h:8: #include <utility> On 2013/10/04 19:21:54, sky wrote: > nit: sort Done.
On 2013/10/04 19:16:53, sky wrote: > On 2013/10/04 18:26:20, erikwright wrote: > > Take a look at the unit tests. They bind in a vector where the response is > > stored. So a function pointer would force that to have a single-arg > > signature and use a global. Yes, it's possible, but extra complexity and I > > don't see the gain. > > > > > > > > On Fri, Oct 4, 2013 at 2:20 PM, <mailto:sky@chromium.org> wrote: > > > > > > > > https://codereview.chromium.**org/23513049/diff/73001/** > > > > > > chrome/browser/install_module_**verifier_win.h<https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_module_verifier_win.h> > > > File chrome/browser/install_module_**verifier_win.h (right): > > > > > > https://codereview.chromium.**org/23513049/diff/73001/** > > > > > > chrome/browser/install_module_**verifier_win.h#newcode29<https://codereview.chromium.org/23513049/diff/73001/chrome/browser/install_module_verifier_win.h#newcode29> > > > chrome/browser/install_module_**verifier_win.h:29: const > > > base::Callback<void(size_t)>& delegate); > > > On 2013/09/25 19:58:08, erikwright wrote: > > > > > >> On 2013/09/24 20:16:16, sky wrote: > > >> > Why the callback here? Is the plan that it'll be async soon? > > >> > > > > > > For testability. Internally the callback just does a histogram, but > > >> > > > the tests > > > > > >> can give a mock callback to verify the data that is being reported. > > >> > > > > > > Would a function pointer suffice then? > > > > > > > > > https://codereview.chromium.**org/23513049/%253Chttps://codereview.chromium.o...> > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > If it's not async, why not have a return value then? Sure. Adjusted accordingly.
LGTM
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/23513049/139001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/23513049/155001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_x64_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/23513049/155001
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/23513049/167001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/23513049/167001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/23513049/203001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/23513049/203001
Message was sent while issue was closed.
Change committed as 227542 |
