Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22640018:
Set up Finch trial for malware download warnings (Closed)
|
Created:
7 years, 4 months ago by felt Modified:
7 years, 4 months ago CC:
chromium-reviews, benjhayden+dwatch_chromium.org, asanka, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSet up Finch trial for malware download warnings
This adds a set of experimental strings to the malware download warning for a finch experiment.
BUG=267335
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216839
Patch Set 1 #
Total comments: 12
Patch Set 2 : Updated for reviewer comments #Patch Set 3 : Linter fix -- missing newline at EOF #
Total comments: 2
Patch Set 4 : case bug #Patch Set 5 : Minor change to the strings #
Total comments: 16
Patch Set 6 : Reviewer comments #Patch Set 7 : string16 to base string 16 #
Total comments: 20
Patch Set 8 : More style #Patch Set 9 : More fixes #Patch Set 10 : Accidental indent #
Messages
Total messages: 22 (0 generated)
Hi Asanka & Dan, Can you please review this today? I'm sorry for the short notice: Avni has asked me to try to land this today for M30, and I didn't get the needed string approvals until yesterday. If you don't have time today, pls let me know and I'll try to find another reviewer. Asanka - looking for feedback on whole CL. Dan - need your approval for chrome/browser/ui/webui/downloads_dom_handler.cc [and you might want to take a look at chrome/browser/resources/downloads/downloads.js too] Best, Adrienne
You should get someone to review the Finch trial implementation since I'm not familiar with it. https://codereview.chromium.org/22640018/diff/1/chrome/browser/download/downl... File chrome/browser/download/download_util.cc (right): https://codereview.chromium.org/22640018/diff/1/chrome/browser/download/downl... chrome/browser/download/download_util.cc:276: if (elided_filename == string16()) { Nit: elided_filename.empty() https://codereview.chromium.org/22640018/diff/1/chrome/browser/download/downl... File chrome/browser/download/download_util.h (right): https://codereview.chromium.org/22640018/diff/1/chrome/browser/download/downl... chrome/browser/download/download_util.h:123: const char kFinchTrialName[] = "MalwareDownloadWarning"; Declare the strings here, and move the definitions to download_util.cc.
https://codereview.chromium.org/22640018/diff/1/chrome/browser/resources/down... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/22640018/diff/1/chrome/browser/resources/down... chrome/browser/resources/downloads/downloads.js:426: this.finch_string_ = download.finch_string; this.finchString_ https://codereview.chromium.org/22640018/diff/1/chrome/browser/resources/down... chrome/browser/resources/downloads/downloads.js:456: if (this.finch_string_ && this.finch_string_ != '') { empty strings are falsey, don't need the second != '' check https://codereview.chromium.org/22640018/diff/1/chrome/browser/ui/webui/downl... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): https://codereview.chromium.org/22640018/diff/1/chrome/browser/ui/webui/downl... chrome/browser/ui/webui/downloads_dom_handler.cc:171: std::string trialCondition = trial_condition (cpp_vars_like_this, jsVarsLikeThis) https://codereview.chromium.org/22640018/diff/1/chrome/browser/ui/webui/downl... chrome/browser/ui/webui/downloads_dom_handler.cc:194: if ((trialCondition != "") && if (!trial_condition.empty()) { std::string finch_string; content::DangerType danger_type = download_item->GetDangerType(); if (danger_type == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL || danger_type == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT || danger_type == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_HOST) { finch_string = download_util::AssembleMalwareFinchString( trial_condition, file_name); } file_value->SetString("finch_string", finch_string); }
Thanks, uploaded a new patchset. https://codereview.chromium.org/22640018/diff/1/chrome/browser/download/downl... File chrome/browser/download/download_util.cc (right): https://codereview.chromium.org/22640018/diff/1/chrome/browser/download/downl... chrome/browser/download/download_util.cc:276: if (elided_filename == string16()) { On 2013/08/09 18:50:17, asanka wrote: > Nit: elided_filename.empty() Done. https://codereview.chromium.org/22640018/diff/1/chrome/browser/download/downl... File chrome/browser/download/download_util.h (right): https://codereview.chromium.org/22640018/diff/1/chrome/browser/download/downl... chrome/browser/download/download_util.h:123: const char kFinchTrialName[] = "MalwareDownloadWarning"; On 2013/08/09 18:50:17, asanka wrote: > Declare the strings here, and move the definitions to download_util.cc. Done. https://codereview.chromium.org/22640018/diff/1/chrome/browser/resources/down... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/22640018/diff/1/chrome/browser/resources/down... chrome/browser/resources/downloads/downloads.js:426: this.finch_string_ = download.finch_string; On 2013/08/09 18:54:05, Dan Beam wrote: > this.finchString_ Done. https://codereview.chromium.org/22640018/diff/1/chrome/browser/resources/down... chrome/browser/resources/downloads/downloads.js:456: if (this.finch_string_ && this.finch_string_ != '') { On 2013/08/09 18:54:05, Dan Beam wrote: > empty strings are falsey, don't need the second != '' check Done. https://codereview.chromium.org/22640018/diff/1/chrome/browser/ui/webui/downl... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): https://codereview.chromium.org/22640018/diff/1/chrome/browser/ui/webui/downl... chrome/browser/ui/webui/downloads_dom_handler.cc:171: std::string trialCondition = On 2013/08/09 18:54:05, Dan Beam wrote: > trial_condition (cpp_vars_like_this, jsVarsLikeThis) Done here & elsewhere https://codereview.chromium.org/22640018/diff/1/chrome/browser/ui/webui/downl... chrome/browser/ui/webui/downloads_dom_handler.cc:194: if ((trialCondition != "") && On 2013/08/09 18:54:05, Dan Beam wrote: > if (!trial_condition.empty()) { > std::string finch_string; > content::DangerType danger_type = download_item->GetDangerType(); > if (danger_type == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL || > danger_type == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT || > danger_type == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_HOST) { > finch_string = download_util::AssembleMalwareFinchString( > trial_condition, > file_name); > } > file_value->SetString("finch_string", finch_string); > } Done, thanks
Hi Rob, Could you please review my use of base::FieldTrialList::FindFullName in: chrome/browser/download/download_danger_prompt.cc chrome/browser/download/download_item_model.cc chrome/browser/ui/webui/downloads_dom_handler.cc I've been asked to land this today so that it can certainly get into M30, so I'd appreciate it if you could look this afternoon. Just a few lines of code. Thanks, Adrienne
https://codereview.chromium.org/22640018/diff/2017/chrome/browser/resources/d... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/22640018/diff/2017/chrome/browser/resources/d... chrome/browser/resources/downloads/downloads.js:458: this.dangerDesc_.textContent = this.finch_string_; this.finchString_
https://codereview.chromium.org/22640018/diff/2017/chrome/browser/resources/d... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/22640018/diff/2017/chrome/browser/resources/d... chrome/browser/resources/downloads/downloads.js:458: this.dangerDesc_.textContent = this.finch_string_; On 2013/08/09 19:35:15, asanka wrote: > this.finchString_ Done.
LGTM. Did you try it out with --force-fieldtrials? You can use the links at http://testsafebrowsing.appspot.com/chrome to generate warnings.
On 2013/08/09 20:22:17, asanka wrote: > LGTM. > > Did you try it out with --force-fieldtrials? You can use the links at > http://testsafebrowsing.appspot.com/chrome to generate warnings. I'm in the middle of getting a Windows build set up to test the conditions, I can't trigger all of the code paths on Mac/Linux.
https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... File chrome/browser/download/download_util.cc (right): https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... chrome/browser/download/download_util.cc:286: string16 AssembleMalwareFinchString(const std::string& trial_condition, base::string16 everywhere https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... chrome/browser/download/download_util.cc:290: if (elided_filename.empty()) { nit: no curlies when if () and {} are 1-liners https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... chrome/browser/download/download_util.cc:294: } nit: \n https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... chrome/browser/download/download_util.cc:351: } nit: \n https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... File chrome/browser/download/download_util_unittest.cc (right): https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... chrome/browser/download/download_util_unittest.cc:19: }; nit: I think you can replace L13-19 with typedef testing::Test DownloadUtilTest; https://codereview.chromium.org/22640018/diff/40001/chrome/browser/ui/webui/d... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): https://codereview.chromium.org/22640018/diff/40001/chrome/browser/ui/webui/d... chrome/browser/ui/webui/downloads_dom_handler.cc:172: base::FieldTrialList::FindFullName(download_util::kFinchTrialName); ^ move this https://codereview.chromium.org/22640018/diff/40001/chrome/browser/ui/webui/d... chrome/browser/ui/webui/downloads_dom_handler.cc:193: file_value->SetString("danger_type", danger_type_value); to here (as low as possible, there's no reason to call this until right here)
https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... File chrome/browser/download/download_util_unittest.cc (right): https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... chrome/browser/download/download_util_unittest.cc:19: }; On 2013/08/09 21:25:02, Dan Beam wrote: > nit: I think you can replace L13-19 with > > typedef testing::Test DownloadUtilTest; Yeah. Actually you don't need a fixture for this. Just use TEST(DownloadUtilTest, ...) below instead of TEST_F(...)
https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... File chrome/browser/download/download_util.cc (right): https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... chrome/browser/download/download_util.cc:286: string16 AssembleMalwareFinchString(const std::string& trial_condition, On 2013/08/09 21:25:02, Dan Beam wrote: > base::string16 everywhere Done. https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... chrome/browser/download/download_util.cc:290: if (elided_filename.empty()) { On 2013/08/09 21:25:02, Dan Beam wrote: > nit: no curlies when if () and {} are 1-liners Is that in the style guide? I can't find it and it seems different reviewers have different opinions about this. (Done, though.) https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... chrome/browser/download/download_util.cc:294: } On 2013/08/09 21:25:02, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... chrome/browser/download/download_util.cc:351: } On 2013/08/09 21:25:02, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... File chrome/browser/download/download_util_unittest.cc (right): https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... chrome/browser/download/download_util_unittest.cc:19: }; On 2013/08/09 21:40:32, asanka wrote: > On 2013/08/09 21:25:02, Dan Beam wrote: > > nit: I think you can replace L13-19 with > > > > typedef testing::Test DownloadUtilTest; > > Yeah. Actually you don't need a fixture for this. Just use > TEST(DownloadUtilTest, ...) below instead of TEST_F(...) Done. https://codereview.chromium.org/22640018/diff/40001/chrome/browser/ui/webui/d... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): https://codereview.chromium.org/22640018/diff/40001/chrome/browser/ui/webui/d... chrome/browser/ui/webui/downloads_dom_handler.cc:172: base::FieldTrialList::FindFullName(download_util::kFinchTrialName); On 2013/08/09 21:25:02, Dan Beam wrote: > ^ move this Done. https://codereview.chromium.org/22640018/diff/40001/chrome/browser/ui/webui/d... chrome/browser/ui/webui/downloads_dom_handler.cc:193: file_value->SetString("danger_type", danger_type_value); On 2013/08/09 21:25:02, Dan Beam wrote: > to here (as low as possible, there's no reason to call this until right here) Done.
lgtm w/nits (please address first) https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... File chrome/browser/download/download_util.cc (right): https://codereview.chromium.org/22640018/diff/40001/chrome/browser/download/d... chrome/browser/download/download_util.cc:290: if (elided_filename.empty()) { On 2013/08/09 22:38:03, felt wrote: > On 2013/08/09 21:25:02, Dan Beam wrote: > > nit: no curlies when if () and {} are 1-liners > > Is that in the style guide? I can't find it and it seems different reviewers > have different opinions about this. > > (Done, though.) "In general, curly braces are not required for single-line statements, but they are allowed if you like them; conditional or loop statements with complex conditions or statements may be more readable with curly braces." http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... In chrome we pretty much always omit *conditional* (if/else) curlies when possible. Some apply this rule to for/while etc., which is less consistent in chrome and more disputed. https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_danger_prompt.cc (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_danger_prompt.cc:116: case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_HOST: nit: lower |trial_condition| to here https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_item_model.cc (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_item_model.cc:311: case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL: arguably, you'd put |trial_condition| here and below as well... here's the rule in the style guide, btw - http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_... https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_util.cc (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_util.cc:306: NULL); nit: shaves code, reduces duplication if (trial_condition == kCondition1Control) return ASCIIToUTF16("This file appears malicious."); base::string16 message; if (trial_condition == kCondition2Control) message = ASCIIToUTF16("$1 appears malicious."); else if (...) ... return ReplaceStringPlaceholders(message, filename, NULL); https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_util.h (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_util.h:139: const string16& elided_filename); nit: indent off, base::string16 https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_util_unittest.cc (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_util_unittest.cc:20: } nit: remove blank line (IMO)
https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_danger_prompt.cc (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_danger_prompt.cc:116: case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_HOST: On 2013/08/09 23:24:51, Dan Beam wrote: > nit: lower |trial_condition| to here I don't think I can do this because of compiler errors (see next comment too) https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_item_model.cc (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_item_model.cc:311: case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL: On 2013/08/09 23:24:51, Dan Beam wrote: > arguably, you'd put |trial_condition| here and below as well... > > here's the rule in the style guide, btw - > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_... I'd like to do this, but it causes compiler errors like this: ../../chrome/browser/download/download_danger_prompt.cc:127:19: error: jump to case label [-fpermissive] ../../chrome/browser/download/download_danger_prompt.cc:115:19: error: crosses initialization of ‘std::string trial_condition’ because it's not inside a scope smaller than the switch (as it is in the webui version). https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_util.cc (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_util.cc:306: NULL); On 2013/08/09 23:24:51, Dan Beam wrote: > nit: shaves code, reduces duplication > > if (trial_condition == kCondition1Control) > return ASCIIToUTF16("This file appears malicious."); > > base::string16 message; > if (trial_condition == kCondition2Control) > message = ASCIIToUTF16("$1 appears malicious."); > else if (...) > ... > > return ReplaceStringPlaceholders(message, filename, NULL); > Done. https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_util.h (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_util.h:139: const string16& elided_filename); On 2013/08/09 23:24:51, Dan Beam wrote: > nit: indent off, base::string16 Done. https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_util_unittest.cc (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_util_unittest.cc:20: } On 2013/08/09 23:24:51, Dan Beam wrote: > nit: remove blank line (IMO) the linter complains without it
Asanka, I don't think rkaplow is going to look at it today, and I'm not sure what there is for him to review. Dan and you reviewed the code quite closely already IMO and I know the Finch code works -- I've tested it & it's the same way I've pulled Finch trial conditions in the past.
https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_item_model.cc (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_item_model.cc:311: case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL: On 2013/08/09 23:48:34, felt wrote: > On 2013/08/09 23:24:51, Dan Beam wrote: > > arguably, you'd put |trial_condition| here and below as well... > > > > here's the rule in the style guide, btw - > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_... > > I'd like to do this, but it causes compiler errors like this: > > ../../chrome/browser/download/download_danger_prompt.cc:127:19: error: jump to > case label [-fpermissive] > ../../chrome/browser/download/download_danger_prompt.cc:115:19: error: crosses > initialization of ‘std::string trial_condition’ > > because it's not inside a scope smaller than the switch (as it is in the webui > version). case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL: { // <-- add curlies }
https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_util_unittest.cc (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_util_unittest.cc:20: } On 2013/08/09 23:48:34, felt wrote: > On 2013/08/09 23:24:51, Dan Beam wrote: > > nit: remove blank line (IMO) > > the linter complains without it if you're talking about "1 error" next to your patchset, nobody listens to that.
last nits, swearz https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_danger_prompt.cc (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_danger_prompt.cc:121: } else { and here https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_item_model.cc (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_item_model.cc:315: return download_util::AssembleMalwareFinchString(trial_condition, nit: also don't use else after an if + return, i.e. if (trial_condition.empty()) return l10n_util::GetStringUTF16(IDS_PROMPT_MALICIOUS_DOWNLOAD_URL); return download_util::AssembleMalwareFinchString(trial_condition, elided_filename); https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_item_model.cc:331: } else { and here
Thanks Dan! You are a good reviewer. https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_danger_prompt.cc (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_danger_prompt.cc:121: } else { On 2013/08/10 00:13:56, Dan Beam wrote: > and here Done. https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_item_model.cc (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_item_model.cc:311: case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL: On 2013/08/10 00:08:49, Dan Beam wrote: > On 2013/08/09 23:48:34, felt wrote: > > On 2013/08/09 23:24:51, Dan Beam wrote: > > > arguably, you'd put |trial_condition| here and below as well... > > > > > > here's the rule in the style guide, btw - > > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_... > > > > I'd like to do this, but it causes compiler errors like this: > > > > ../../chrome/browser/download/download_danger_prompt.cc:127:19: error: jump to > > case label [-fpermissive] > > ../../chrome/browser/download/download_danger_prompt.cc:115:19: error: > crosses > > initialization of ‘std::string trial_condition’ > > > > because it's not inside a scope smaller than the switch (as it is in the webui > > version). > > case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL: { // <-- add curlies > } Done. https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_item_model.cc:315: return download_util::AssembleMalwareFinchString(trial_condition, On 2013/08/10 00:13:56, Dan Beam wrote: > nit: also don't use else after an if + return, i.e. > > if (trial_condition.empty()) > return l10n_util::GetStringUTF16(IDS_PROMPT_MALICIOUS_DOWNLOAD_URL); > > return download_util::AssembleMalwareFinchString(trial_condition, > elided_filename); Done https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_item_model.cc:331: } else { On 2013/08/10 00:13:56, Dan Beam wrote: > and here Done. https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... File chrome/browser/download/download_util_unittest.cc (right): https://codereview.chromium.org/22640018/diff/49001/chrome/browser/download/d... chrome/browser/download/download_util_unittest.cc:20: } On 2013/08/10 00:10:14, Dan Beam wrote: > On 2013/08/09 23:48:34, felt wrote: > > On 2013/08/09 23:24:51, Dan Beam wrote: > > > nit: remove blank line (IMO) > > > > the linter complains without it > > if you're talking about "1 error" next to your patchset, nobody listens to that. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/22640018/27003
Message was sent while issue was closed.
Change committed as 216839
Message was sent while issue was closed.
On 2013/08/10 13:39:24, I haz the power (commit-bot) wrote: > Change committed as 216839 Hi - I was away on vacation last week and didn't set an OOO on my chromium account. Glad to see it got reviewed. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
