Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
266193002:
Extend PR_ParseTimeString() to accept some ISO 8601 formats to fix timezone parsing in SyslogParser. (Closed)
|
Created:
6 years, 7 months ago by Thiemo Nagel Modified:
6 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, Shin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionExtend PR_ParseTimeString() to accept some ISO 8601 formats to fix timezone parsing in SyslogParser.
Previously, SyslogParser::ParseTime() would parse time, reformat it in a way that is understood by NSPR and then hand it off to PR_ParseTimeString() where it is parsed again. This double-parsing is unnecessarily convoluted and slow. Extending PR_ParseTimeString() simplifies and speeds up SyslogParser::ParseTime() which is time-critical in case of large log files.
BUG=370509
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270815
Patch Set 1 : #
Total comments: 7
Patch Set 2 : Rebase. #Patch Set 3 : Address kalman's comments. #Patch Set 4 : Rebase. #Patch Set 5 : New approach: Extend PR_ParseTimeString() to handle some ISO 8601 formats. #
Total comments: 20
Patch Set 6 : Address wtc's comments. #
Total comments: 15
Patch Set 7 : Rebase. #Patch Set 8 : Address wtc's comments. #Patch Set 9 : Fix microsecond parsing for Windows and for timezoneless (local time) inputs. #
Total comments: 4
Patch Set 10 : Address Mark's comments. #
Messages
Total messages: 41 (0 generated)
Hi Benjamin, could you please take a look at this CL? I saw the test fail locally and then fixed the code and improved test coverage by changing sample input to use two different timezones. -- I don't fully understand why this wasn't caught earlier, though. Best, Thiemo
thanks! I have no idea how this ever worked, but I've never seen this code, the author and both reviewers have left. +Xiaowen who owns this stuff these days? https://codereview.chromium.org/266193002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/log_private/syslog_parser.cc (right): https://codereview.chromium.org/266193002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/log_private/syslog_parser.cc:75: std::string& output) { if these are refs they need to be const. but given you call GetNext() the tokenizer can't be const. and given you write to output, it can't be const either. so these need to be pointers. https://codereview.chromium.org/266193002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/log_private/syslog_parser.cc:100: DCHECK(tokens[15] == ":"); DCHECKs here seem wrong especially since we can return an Error from here. maybe there should be a NOTREACHED but better still would be to return a SyslogParser::Error if any of these are wrong, and then write a test which exercises that. since writing a test is likely to be really tricky (you'd need to generate real system logs, ugh) NOTREACHED is fine. https://codereview.chromium.org/266193002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/log_private/syslog_parser.cc:105: tokens[13] + tokens[14] + tokens[16]; // +/- tz offset maybe have a look at sscanf[_s] here? surely there is an easier way than this.
On 2014/05/05 19:36:23, kalman wrote: > +Xiaowen who owns this stuff these days? +benchan maybe
It'd be helpful to briefly describe the issue in the commit, or better, file a bug so that the QA team could help verify the fix as well. Thanks.
Hi Benjamin, thank you for your comments! I've tried to address them, but there's a follow-up questions to your 3rd comment. Could you please take another look? Following Ben's advice, I've filed https://crbug.com/370509 to track the issue. Best, Thiemo https://codereview.chromium.org/266193002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/log_private/syslog_parser.cc (right): https://codereview.chromium.org/266193002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/log_private/syslog_parser.cc:75: std::string& output) { I wasn't aware of that convention. Thanks a lot! https://codereview.chromium.org/266193002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/log_private/syslog_parser.cc:100: DCHECK(tokens[15] == ":"); OK. Done. https://codereview.chromium.org/266193002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/log_private/syslog_parser.cc:105: tokens[13] + tokens[14] + tokens[16]; // +/- tz offset On 2014/05/05 19:36:24, kalman wrote: > maybe have a look at sscanf[_s] here? surely there is an easier way than this. Could you please explain what specifically you'd suggest to change? I see nothing wrong with the original author's approach of re-shuffling the time string so that it can be parsed by base::Time::FromString().
https://codereview.chromium.org/266193002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/log_private/syslog_parser.cc (right): https://codereview.chromium.org/266193002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/log_private/syslog_parser.cc:105: tokens[13] + tokens[14] + tokens[16]; // +/- tz offset On 2014/05/06 18:15:49, Thiemo Nagel wrote: > On 2014/05/05 19:36:24, kalman wrote: > > maybe have a look at sscanf[_s] here? surely there is an easier way than this. > > Could you please explain what specifically you'd suggest to change? I see > nothing wrong with the original author's approach of re-shuffling the time > string so that it can be parsed by base::Time::FromString(). The code is very hard to read. De-formatting it using sscanf would be much nicer, like: char month, day, year, hour, min, sec; sscanf(tokens, "%c/%c/%c/ etc", &month, &day, &year, &hour, &min, &sec); plus whatever the required buffer safety etc mechanisms are.
Hi Ryan and Benjamin! Ryan: What do you think about adding (partial) ISO 8601 support to PR_ParseTimeString() ? Could you please take a look at this CL? Benjamin: Would you be happy with this approach? Thank you for your feedback! Thiemo
I don't think we can just change third_party code like that.
(Much as it would be nice to leave syslog date parsing out of the extensions code.)
Ditto kalman. Not LGTM.
On 2014/05/07 15:27:13, Ryan Sleevi wrote: > Ditto kalman. Not LGTM. Oh well. What do I do then? Is there any chance to get that change upstreamed and would that be desirable?
Wtc maintains upstream, he would know.
On 2014/05/07 15:31:19, Thiemo Nagel wrote: > Is there any chance to get that change upstreamed > and would that be desirable? I treat base/third_party/nspr/prtime.cc as a permanent fork of the NSPR file prtime.c. So it is not necessary to upstream your change. We should certainly submit your patch to NSPR upstream though.
On 2014/05/07 19:05:49, wtc wrote: > I treat base/third_party/nspr/prtime.cc as a permanent fork of the NSPR file > prtime.c. So it is not necessary to upstream your change. > > We should certainly submit your patch to NSPR upstream though. Thank you for your response! I'm perfectly happy to contribute to upstream, too. How do I proceed now? Could you please be so kind to review this CL? Or would you prefer me to submit to upstream first?
Review comments on patch set 5: Thiemo: I have some suggested changes for your changes to prtime.cc. Please take a look and let me know if they make sense. Thanks. https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... File base/third_party/nspr/prtime.cc (right): https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:65: * Unit tests are in base/time/pr_time_unittest.cc. Maybe this entire comment block should be moved to base/third_party/nspr/README.chromium? https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:852: if ('5' <= *end && *end <= '9') tmp_usec++; /* round to nearest */ I suggest we always truncate. The reason is that rounding up may produce surprising results. For example: 2014-05-08T23:59:59.9999995Z would be rounded up to 2014-05-09T00:00:00.000000Z https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:857: while (ndigits++ < 6) tmp_usec *= 10; Nit: put tmp_usec *= 10; on a separate line. https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:861: if (*rest == 'Z') { Should we only allow 'Z' if 'T' was used as a delimiter? https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:863: end++; It seems that we should skip the AM/PM processing code if we have seen 'Z'. https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:955: !(*s == 'T' && '0' <= s[1] && s[1] <= '9')) /* allow ISO 8601 T delimiter */ I think we should skip over 'T' right here, so that we don't need to check for 'T' in the while loops on lines 1069 and 1078 below. https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:958: /* Ok, we parsed three 1-2 digit numbers, with / or - This comment needs to be updated because "three 1-2 digit numbers" is no longer true. https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:960: (DD/MM/YY or MM/DD/YY or YY/MM/DD.) The last item should be YY[YY]/MM/DD or [YY]YY/MM/DD. See the next comment. https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:963: if (n1 > 31 || n1 == 0) /* must be YY[YY]/MM/DD */ Nit: [YY]YY/MM/DD seems more accurate. Not very sure about this though :-) https://codereview.chromium.org/266193002/diff/100001/base/time/pr_time_unitt... File base/time/pr_time_unittest.cc (right): https://codereview.chromium.org/266193002/diff/100001/base/time/pr_time_unitt... base/time/pr_time_unittest.cc:20: PRTime comparison_time_cest = 1373275692440380LL; Nit: use either NSPR's PR_INT64(1373275692440380) or the INT64_C macro from <stdint.h> INT64_C(1373275692440380)
Hi wtc, I've updated the CL with the improvements that you have suggested, except for the checking of 'T' in while loops, which I think cannot be easily removed. Could you please take another look? Thank you and best regards, Thiemo https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... File base/third_party/nspr/prtime.cc (right): https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:65: * Unit tests are in base/time/pr_time_unittest.cc. On 2014/05/08 21:04:19, wtc wrote: > > Maybe this entire comment block should be moved to > base/third_party/nspr/README.chromium? I don't have an opinion about that... https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:852: if ('5' <= *end && *end <= '9') tmp_usec++; /* round to nearest */ On 2014/05/08 21:04:19, wtc wrote: > I suggest we always truncate. The reason is that rounding up may produce > surprising results. For example: > > 2014-05-08T23:59:59.9999995Z > > would be rounded up to > > 2014-05-09T00:00:00.000000Z I wouldn't consider that surprising... ;-) Anyways, I'm truncating now. https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:857: while (ndigits++ < 6) tmp_usec *= 10; On 2014/05/08 21:04:19, wtc wrote: > > Nit: put > tmp_usec *= 10; > on a separate line. Done. https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:861: if (*rest == 'Z') { On 2014/05/08 21:04:19, wtc wrote: > Should we only allow 'Z' if 'T' was used as a delimiter? I wouldn't do that since according to Wikipedia "it is permitted to omit the 'T' character by mutual agreement." https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:863: end++; On 2014/05/08 21:04:19, wtc wrote: > > It seems that we should skip the AM/PM processing code if we have seen 'Z'. Done. https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:955: !(*s == 'T' && '0' <= s[1] && s[1] <= '9')) /* allow ISO 8601 T delimiter */ On 2014/05/08 21:04:19, wtc wrote: > I think we should skip over 'T' right here, so that we don't need to check for > 'T' in the while loops on lines 1069 and 1078 below. We could skip over the 'T' here, but we cannot eliminate the logic from the "skip to the end of this token" while() loop (line 1069) as it is required to prevent the loop from eating the rest of the token. I've updated the CL, trying to make it more legible. If you still think skipping over 'T' here is better, I can do that, but I can't think of an easy way of removing the logic from line 1069 (except by introducing a new variable like dont_skip_this_token which I don't consider particularly elegant, either). https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:958: /* Ok, we parsed three 1-2 digit numbers, with / or - On 2014/05/08 21:04:19, wtc wrote: > > This comment needs to be updated because "three 1-2 digit numbers" is no longer > true. Done. https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:960: (DD/MM/YY or MM/DD/YY or YY/MM/DD.) On 2014/05/08 21:04:19, wtc wrote: > > The last item should be YY[YY]/MM/DD or [YY]YY/MM/DD. See the next comment. Done. https://codereview.chromium.org/266193002/diff/100001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:963: if (n1 > 31 || n1 == 0) /* must be YY[YY]/MM/DD */ Absolutely! https://codereview.chromium.org/266193002/diff/100001/base/time/pr_time_unitt... File base/time/pr_time_unittest.cc (right): https://codereview.chromium.org/266193002/diff/100001/base/time/pr_time_unitt... base/time/pr_time_unittest.cc:20: PRTime comparison_time_cest = 1373275692440380LL; On 2014/05/08 21:04:19, wtc wrote: > > Nit: use either NSPR's > > PR_INT64(1373275692440380) > > or the INT64_C macro from <stdint.h> > > INT64_C(1373275692440380) Done. https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... File base/third_party/nspr/prtime.cc (left): https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:1033: while (*rest && *rest is redundant here.
Patch set 6 LGTM. I have some questions and suggested changes for coding style nits. You may ignore some of the suggested changes. The parsing code in this function is very complicated. I did my best to review it, but I believe you understand the relevant code better than I do. So I trust your decisions on how to make the code more readable. https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... File base/third_party/nspr/prtime.cc (right): https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:860: zone = TT_GMT; Should we do end++; here? Otherwise 'Z' may be inspected again. https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:861: } Nit: Lines 843-861 probably should follow the braces style of this file. if (*rest == '.') { rest++; end++; ... } if (*end == 'Z') { zone = TT_GMT; } else if (tmp_hour <= 12) https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:865: and possibly sec, so it worked as a unit. */ By "it worked as a unit", did you mean we have fully parsed the time and are ready for the next token? https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:914: break; This break statement means you want to disallow a three-digit year. I am fine with that (line 942 also has a break statement). Just wanted to confirm this. https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:916: } Nit: it seems that this new if block (lines 910-916) should be inside the previous if statement, i.e., we only allow digits 3 and 4 if there is digit 2: n1 = (*s++ - '0'); /* first 1, 2 or 4 digits */ if (*s >= '0' && *s <= '9') { n1 = n1*10 + (*s++ - '0'); if (*s >= '0' && *s <= '9') /* option al digits 3 and 4 */ { n1 = n1*10 + (*s++ - '0'); if (*s < '0' || *s > '9') break; n1 = n1*10 + (*s++ - '0'); } } Your current code is fine because it matches the code on lines 934-946. Note that that code seems to allow a five-digit year. I forgot why. https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:1068: !(*rest == 'T' && rest[1] >= '0' && rest[1] <= '9') /* T precedes time in ISO 8601 */ Nit: maybe this comment should be moved to the comment block at line 1060. https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:1092: Nit: delete this blank line.
Hi Mark, I've extended PR_ParseTimeString() functionality and added test coverage in pr_time_unittest.cc for it. Could you please take a look at pr_time_unittest.cc? (It seems a bit weird that the code is owned by wtc but the corresponding unit tests aren't.) Hi wtc, this change accommodates all your comments. However, in the mean time trybots discovered an issue with microsecond parsing on Windows which I believe is fixed now, and which led me to another issue with microsecond parsing of timezone-less strings, which I believe is fixed now, too. In the process, I've removed #define LL_I2L(l, i) ((l) = (PRInt64)(i)) #define LL_MUL(r, a, b) ((r) = (a) * (b)) the utility of which didn't occur to me. Mainly due to this change, I'm asking you to take another look. Thank you both! Thiemo https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... File base/third_party/nspr/prtime.cc (right): https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:860: zone = TT_GMT; On 2014/05/12 19:47:15, wtc wrote: > Should we do end++ here? Otherwise 'Z' may be inspected again. It's not strictly necessary because the break will skip through to the while loop in line 1062 which skips the rest of the token. Nevertheless I've added it (and replaced end by rest) in the hope of making the code more readable. https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:861: } > Nit: Lines 843-861 probably should follow the braces style of this file. Thanks, I fully agree. Done. https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:865: and possibly sec, so it worked as a unit. */ On 2014/05/12 19:47:15, wtc wrote: > By "it worked as a unit", did you mean we have fully parsed the time and are > ready for the next token? That's how I understand the original author. (I've only moved the pre-existing comment a bit.) I'm updating the comment now to make it clearer. https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:914: break; On 2014/05/12 19:47:15, wtc wrote: > This break statement means you want to disallow a three-digit year. Yes, that's the intent. https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:916: } On 2014/05/12 19:47:15, wtc wrote: > Nit: it seems that this new if block (lines 910-916) should be inside the > previous if statement. Yes, good point, that's more readable. > Note that that code seems to allow a five-digit year. I forgot why. I certainly would be interested to know. Anyways, ISO 8601 does specify only four digits, that's why I'm not supporting a fifth one. https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:1068: !(*rest == 'T' && rest[1] >= '0' && rest[1] <= '9') /* T precedes time in ISO 8601 */ On 2014/05/12 19:47:15, wtc wrote: > Nit: maybe this comment should be moved to the comment block at line 1060. Done. At that occasion, I've updated the pre-existing comment which was out of sync with the code. https://codereview.chromium.org/266193002/diff/120001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:1092: On 2014/05/12 19:47:15, wtc wrote: > > Nit: delete this blank line. Done.
https://codereview.chromium.org/266193002/diff/180001/base/time/pr_time_unitt... File base/time/pr_time_unittest.cc (right): https://codereview.chromium.org/266193002/diff/180001/base/time/pr_time_unitt... base/time/pr_time_unittest.cc:59: }; Alignment nit: this closing } should not line up with the member initializers. https://codereview.chromium.org/266193002/diff/180001/base/time/pr_time_unitt... base/time/pr_time_unittest.cc:243: EXPECT_EQ(0, parsed_time); I don’t think it’s important or reasonable to test for any particular value of parsed_time if PR_ParseTimeString failed. I would remove this EXPECT. Same on lines 251 and 259.
Hi Mark, many thanks you for comments! I've uploaded a new version that takes these into account. Best, Thiemo https://codereview.chromium.org/266193002/diff/180001/base/time/pr_time_unitt... File base/time/pr_time_unittest.cc (right): https://codereview.chromium.org/266193002/diff/180001/base/time/pr_time_unitt... base/time/pr_time_unittest.cc:59: }; On 2014/05/13 14:22:50, Mark Mentovai wrote: > Alignment nit: this closing } should not line up with the member initializers. Thank you! Done. https://codereview.chromium.org/266193002/diff/180001/base/time/pr_time_unitt... base/time/pr_time_unittest.cc:243: EXPECT_EQ(0, parsed_time); On 2014/05/13 14:22:50, Mark Mentovai wrote: > I don’t think it’s important or reasonable to test for any particular value of > parsed_time if PR_ParseTimeString failed. I would remove this EXPECT. Same on > lines 251 and 259. That's fair. Done.
LGTM in base/time.
Patch set 10 LGTM. Nice work!
On 2014/05/15 04:10:40, wtc wrote: > Patch set 10 LGTM. Nice work! Thank you! :)
The CQ bit was checked by tnagel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tnagel@chromium.org/266193002/200001
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: rsleevi@chromium.org. Please make sure to get positive review before another CQ attempt.
On 2014/05/15 05:56:42, Thiemo Nagel wrote: > On 2014/05/15 04:10:40, wtc wrote: > > Patch set 10 LGTM. Nice work! > lgtm on syslog_parser. Thanks for the fix!
Clearing disapproval - LGTM
The CQ bit was unchecked by tnagel@chromium.org
The CQ bit was checked by tnagel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tnagel@chromium.org/266193002/200001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
Hi Kalman, sorry, I had overlooked that I need your approval, too. Could you please take a look? Thank you! Thiemo On 2014/05/15 10:57:04, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
lgtm, nice cleanup.
On 2014/05/15 14:57:06, kalman wrote: > lgtm, nice cleanup. Thank you!
The CQ bit was checked by tnagel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tnagel@chromium.org/266193002/200001
Message was sent while issue was closed.
Change committed as 270815 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
