Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2286743004:
Sends switches to the Software Reporter to enable matching data collection. (Closed)
|
Created:
4 years, 3 months ago by ftirelo Modified:
4 years, 3 months ago Reviewers:
waffles, alito2, jochen (gone - plz use gerrit), csharp, robertshield, Jialiu Lin, Nathan Parker, alito CC:
chromium-reviews, grt+watch_chromium.org, Nathan Parker Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSends switches to the Software Reporter to enable matching data collection.
BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=634434
Committed: https://crrev.com/379385b7777ca2b90705226141acdfbb1335f6c4
Cr-Commit-Position: refs/heads/master@{#417278}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Syntax error #Patch Set 3 : First round of reviews #Patch Set 4 : More comments #Patch Set 5 : Missing comment #
Total comments: 4
Patch Set 6 : Second round of reviews #
Total comments: 2
Patch Set 7 : Remove useless log message #
Total comments: 12
Patch Set 8 : Addressing comments and browser tests #Patch Set 9 : Rebase #
Total comments: 18
Patch Set 10 : More reviews #
Total comments: 8
Patch Set 11 : More reviews #
Total comments: 2
Patch Set 12 : Always clean state on test setup #Messages
Total messages: 55 (17 generated)
ftirelo@chromium.org changed reviewers: + csharp@chromium.org, robertshield@google.com
ftirelo@chromium.org changed reviewers: + robertshield@chromium.org - robertshield@google.com
ftirelo@chromium.org changed reviewers: + alito@chromium.org
Just one question about the new local state pref. https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_client_info_win.cc (right): https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_client_info_win.cc:15: // static nit: remove comment? https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:602: local_state->SetInt64(prefs::kSwReporterLastTimeSentReport, Should this be set only if we actually did end up sending logs? Otherwise, as is, it is going to simply have (almost) the same value as the kSwReporterLastTimeTriggered pref.
Thoughts of varying usefulness: https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:374: if (profile->IsOffTheRecord()) Not sure this is correct as it will return false whenever a user pops an incognito window to the foreground. Ward does something a little more complicated which seems more correct, it scans all open profiles looking for one that has the right criteria: https://codesearch.chromium.org/chromium/src/chrome/browser/safe_browsing/inc... https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:670: // user status can change if this runs doesn't complete and we invoke the nit: s/runs/run/ https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:671: // reporter again from the same ReporterRunner object. Actually, I'm not sure I fully follow the last sentence here: is this dealing with the case where we run the reporter two times from a single instance of ReporterRunner and a user changes their extended sb opt in-between those two times? https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:697: // least | truncated comment? https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:703: return false; nit: if (!a || !b) return false; is a bit shorter https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:707: base::TimeDelta next_time_send_logs( nit: time_until_next_logs ?
drive-by https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_client_info_win.cc (right): https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_client_info_win.cc:28: default: remove this so that you get a compiler error in case version_info::Channel ever grows a new value https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:674: if (ShouldSendReporterLogs(*local_state)) { nit: omit braces https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:711: return next_time_send_logs.ToInternalValue() <= 0 || using the internal value for this comparison isn't so good. i think it's much better to do something like: base::Time time_of_next_send(last_time_sent_logs + base::TimeDelta::FromDays(kDaysBetweenReporterLogsSent)); base::Time now(base::Time::Now()); return time_of_next_send <= now || last_time_sent_logs > now; come to think of it, i would early-return the final part. i think it's more clear: const baes::Time now = base::Time::Now(); const base::Time last_time_sent_logs = base::Time::FromInternalValue( local_state.GetInt64(prefs::kSwReporterLastTimeSentReport)); // Send the logs if the last send was in the future. if (last_time_sent_logs > now) return true; // Otherwise, send them if the interval has passed. return (last_time_sent_logs + base::TimeDelta::FromDays(kDaysBetweenReporterLogsSent)) <= now; https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:721: invocation->command_line.AppendSwitchASCII( nit: avoid some string conversions by using AppendSwitchNative here and IntToString16 below
Thanks for all the comments. PTAnL. https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_client_info_win.cc (right): https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_client_info_win.cc:15: // static On 2016/08/26 23:18:08, alito wrote: > nit: remove comment? Done. https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_client_info_win.cc:28: default: On 2016/08/28 19:44:22, grt (UTC plus 2) wrote: > remove this so that you get a compiler error in case version_info::Channel ever > grows a new value If I simply remove this, I get this error: d:\workspace\chromium-data-from-reporter\src\chrome\browser\safe_browsing\srt_client_info_win.cc(32) : warning C4715: 'safe_browsing::ChannelAsInt': not all control paths return a value. The best solution IMO would be to: - have the following command in the default clause: NOTREACHED() << "Invalid channel: " << static_cast<int>(chrome::GetChannel()); - add a unit test to check for all possible values of version_info::Channel. Unfortunately, it's not possible to run an iterator over all values of an enum, unless it's designed to allow that (common trick: define special values BEGIN and END equal to the first and the last element in the sequence). One alternative to really cover this case is to have NOTREACHED in the default clause and add a unit test to call this function, so it would crash if the current build is defined in the new channel. The problem with that approach is that we will only notice an error when/if such build is tested, not when the new channel is introduced. I'll be happy to implement such a unit test if you think it is worth it, or any other suggestion you may have. https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:374: if (profile->IsOffTheRecord()) On 2016/08/27 04:04:29, robertshield_OOO_until_Sep_5 wrote: > Not sure this is correct as it will return false whenever a user pops an > incognito window to the foreground. > > Ward does something a little more complicated which seems more correct, it scans > all open profiles looking for one that has the right criteria: > > https://codesearch.chromium.org/chromium/src/chrome/browser/safe_browsing/inc... Thanks for the suggestion. It's something that I was not comfortable with, but didn't really know how to solve. We may want to also revisit this in other places. For example, in https://chromium.googlesource.com/experimental/chromium/src/+/refs/wip/bajone.... I will investigate more thoroughly what happens if the current window is in incognito mode and add a comment in the doc for the UI redesign. https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:602: local_state->SetInt64(prefs::kSwReporterLastTimeSentReport, On 2016/08/26 23:18:08, alito wrote: > Should this be set only if we actually did end up sending logs? Otherwise, as > is, it is going to simply have (almost) the same value as the > kSwReporterLastTimeTriggered pref. Good catch! https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:670: // user status can change if this runs doesn't complete and we invoke the On 2016/08/27 04:04:29, robertshield_OOO_until_Sep_5 wrote: > nit: s/runs/run/ > Done. https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:671: // reporter again from the same ReporterRunner object. On 2016/08/27 04:04:29, robertshield_OOO_until_Sep_5 wrote: > Actually, I'm not sure I fully follow the last sentence here: is this dealing > with the case where we run the reporter two times from a single instance of > ReporterRunner and a user changes their extended > sb opt in-between those two times? Yes. Changed wording to make the statement more precise. Please notice that the ReporterDone callback schedules this method (TryToRun) to run a few days after the current run. If I change the invocation for this object, I can potentially send switches to the reporter that are not valid anymore. https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:674: if (ShouldSendReporterLogs(*local_state)) { On 2016/08/28 19:44:22, grt (UTC plus 2) wrote: > nit: omit braces Done. https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:697: // least | On 2016/08/27 04:04:29, robertshield_OOO_until_Sep_5 wrote: > truncated comment? Done. https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:703: return false; On 2016/08/27 04:04:29, robertshield_OOO_until_Sep_5 wrote: > nit: > > if (!a || !b) > return false; > > is a bit shorter Done. https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:707: base::TimeDelta next_time_send_logs( On 2016/08/27 04:04:29, robertshield_OOO_until_Sep_5 wrote: > nit: time_until_next_logs ? Done. https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:711: return next_time_send_logs.ToInternalValue() <= 0 || On 2016/08/28 19:44:22, grt (UTC plus 2) wrote: > using the internal value for this comparison isn't so good. i think it's much > better to do something like: > > base::Time time_of_next_send(last_time_sent_logs + > base::TimeDelta::FromDays(kDaysBetweenReporterLogsSent)); > base::Time now(base::Time::Now()); > return time_of_next_send <= now || last_time_sent_logs > now; > > come to think of it, i would early-return the final part. i think it's more > clear: > > const baes::Time now = base::Time::Now(); > const base::Time last_time_sent_logs = base::Time::FromInternalValue( > local_state.GetInt64(prefs::kSwReporterLastTimeSentReport)); > // Send the logs if the last send was in the future. > if (last_time_sent_logs > now) > return true; > // Otherwise, send them if the interval has passed. > return (last_time_sent_logs + > base::TimeDelta::FromDays(kDaysBetweenReporterLogsSent)) <= now; That's a good idea. Rewritten using both suggestions (aux var for next_time and early return on future last time sent). https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:721: invocation->command_line.AppendSwitchASCII( On 2016/08/28 19:44:22, grt (UTC plus 2) wrote: > nit: avoid some string conversions by using AppendSwitchNative here and > IntToString16 below Done.
https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_client_info_win.cc (right): https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_client_info_win.cc:28: default: On 2016/08/29 00:10:36, ftirelo wrote: > On 2016/08/28 19:44:22, grt (UTC plus 2) wrote: > > remove this so that you get a compiler error in case version_info::Channel > ever > > grows a new value > > If I simply remove this, I get this error: > d:\workspace\chromium-data-from-reporter\src\chrome\browser\safe_browsing\srt_client_info_win.cc(32) > : warning C4715: 'safe_browsing::ChannelAsInt': not all control paths return a > value. > > The best solution IMO would be to: > - have the following command in the default clause: > NOTREACHED() << "Invalid channel: " > << static_cast<int>(chrome::GetChannel()); > - add a unit test to check for all possible values of version_info::Channel. > Unfortunately, it's not possible to run an iterator over all values of an enum, > unless it's designed to allow that (common trick: define special values BEGIN > and END equal to the first and the last element in the sequence). > > One alternative to really cover this case is to have NOTREACHED in the default > clause and add a unit test to call this function, so it would crash if the > current build is defined in the new channel. The problem with that approach is > that we will only notice an error when/if such build is tested, not when the new > channel is introduced. > > I'll be happy to implement such a unit test if you think it is worth it, or any > other suggestion you may have. Do this: switch (chrome::GetChannel() { case foo: return 0; case bar: return 1; ... (explicitly listing all items in the enum, but omitting default:) } NOTREACHED(); return 0; } this preserves the behavior you had before and clearly shows intent (DCHECK if GetChannel() returns something that you don't cover -- it can technically return any int value), and has the bonus of the compiler error in case version_info::Channel is ever expanded. there is no need for an extra unittest that checks for out-of-bound values. https://codereview.chromium.org/2286743004/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2286743004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:371: return std::any_of(browser_list->begin_last_active(), #include <algorithm> for this https://codereview.chromium.org/2286743004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:718: return time_until_next_logs <= now; fwiw: i find the code clear enough without the intermediate variable, but it's a personal thing so do whatever is consistent with surrounding code and feels right.
PTAL. https://codereview.chromium.org/2286743004/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2286743004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:371: return std::any_of(browser_list->begin_last_active(), On 2016/08/29 06:56:14, grt (UTC plus 2) wrote: > #include <algorithm> for this Done. https://codereview.chromium.org/2286743004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:718: return time_until_next_logs <= now; On 2016/08/29 06:56:14, grt (UTC plus 2) wrote: > fwiw: i find the code clear enough without the intermediate variable, but it's a > personal thing so do whatever is consistent with surrounding code and feels > right. Done.
looks good to me. cheers! https://codereview.chromium.org/2286743004/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_client_info_win.cc (right): https://codereview.chromium.org/2286743004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_client_info_win.cc:29: NOTREACHED() << "Invalid channel: " << static_cast<int>(chrome::GetChannel()); nit: i would omit the text. chances are this will never happen, so it just adds space in the build (albeit only debug builds). if it ever were to happen, the file and line # appear in the message so it won't be difficult to understand it better.
looks good to me. cheers!
On 2016/08/29 15:53:53, grt (UTC plus 2) wrote: > looks good to me. cheers! Thanks!
https://codereview.chromium.org/2286743004/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_client_info_win.cc (right): https://codereview.chromium.org/2286743004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_client_info_win.cc:29: NOTREACHED() << "Invalid channel: " << static_cast<int>(chrome::GetChannel()); On 2016/08/29 15:53:52, grt (UTC plus 2) wrote: > nit: i would omit the text. chances are this will never happen, so it just adds > space in the build (albeit only debug builds). if it ever were to happen, the > file and line # appear in the message so it won't be difficult to understand it > better. Done.
https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_client_info_win.cc (right): https://codereview.chromium.org/2286743004/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_client_info_win.cc:28: default: On 2016/08/29 06:56:14, grt (UTC plus 2) wrote: > On 2016/08/29 00:10:36, ftirelo wrote: > > On 2016/08/28 19:44:22, grt (UTC plus 2) wrote: > > > remove this so that you get a compiler error in case version_info::Channel > > ever > > > grows a new value > > > > If I simply remove this, I get this error: > > > d:\workspace\chromium-data-from-reporter\src\chrome\browser\safe_browsing\srt_client_info_win.cc(32) > > : warning C4715: 'safe_browsing::ChannelAsInt': not all control paths return a > > value. > > > > The best solution IMO would be to: > > - have the following command in the default clause: > > NOTREACHED() << "Invalid channel: " > > << static_cast<int>(chrome::GetChannel()); > > - add a unit test to check for all possible values of version_info::Channel. > > Unfortunately, it's not possible to run an iterator over all values of an > enum, > > unless it's designed to allow that (common trick: define special values BEGIN > > and END equal to the first and the last element in the sequence). > > > > One alternative to really cover this case is to have NOTREACHED in the default > > clause and add a unit test to call this function, so it would crash if the > > current build is defined in the new channel. The problem with that approach is > > that we will only notice an error when/if such build is tested, not when the > new > > channel is introduced. > > > > I'll be happy to implement such a unit test if you think it is worth it, or > any > > other suggestion you may have. > > Do this: > > switch (chrome::GetChannel() { > case foo: > return 0; > case bar: > return 1; > ... (explicitly listing all items in the enum, but omitting default:) > } > NOTREACHED(); > return 0; > } > > this preserves the behavior you had before and clearly shows intent (DCHECK if > GetChannel() returns something that you don't cover -- it can technically return > any int value), and has the bonus of the compiler error in case > version_info::Channel is ever expanded. there is no need for an extra unittest > that checks for out-of-bound values. Done.
lgtm
https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_client_info_win.h (right): https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_client_info_win.h:14: // line. The SRT binary expects to recieve Chrome's channel encoded as: nit: recieve -> receive https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:375: const Profile* profile = browser->profile(); What about converting this lambda to a helper function,BrowserIsSafeBrowsingExtended? (or something) (It seems just a little big for a lambda) https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:606: local_state->SetInt64(prefs::kSwReporterLastTimeSentReport, I wonder if this should be set right before the reporter is called. Setting it here could result in a user uploading logs but Chrome stopping before this value is set, so they'd run it again too quickly. Although if we set it before the reporter runs some times the reporter would stop before it uploads, so we'd miss some cases. But missing some cases seems like it might be better than getting too many. WDYT? https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:672: // Creating a new invocation to add switches for users who opted into Could you add some tests for this? https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:678: SwReporterInvocation actual_invocation = I think you want to create a copy constructor and use it here, since this will drop all the fields but the command line. https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:713: // Send the logs if the last send was in the future. nit: Could you explain why we care about this a bit?
Addressing comments and browser tests
https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_client_info_win.h (right): https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_client_info_win.h:14: // line. The SRT binary expects to recieve Chrome's channel encoded as: On 2016/08/30 13:39:08, csharp wrote: > nit: recieve -> receive Done. https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:375: const Profile* profile = browser->profile(); On 2016/08/30 13:39:08, csharp wrote: > What about converting this lambda to a helper > function,BrowserIsSafeBrowsingExtended? (or something) > > (It seems just a little big for a lambda) Done. https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:606: local_state->SetInt64(prefs::kSwReporterLastTimeSentReport, On 2016/08/30 13:39:08, csharp wrote: > I wonder if this should be set right before the reporter is called. Setting it > here could result in a user uploading logs but Chrome stopping before this value > is set, so they'd run it again too quickly. > > Although if we set it before the reporter runs some times the reporter would > stop before it uploads, so we'd miss some cases. But missing some cases seems > like it might be better than getting too many. WDYT? Done. https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:672: // Creating a new invocation to add switches for users who opted into On 2016/08/30 13:39:08, csharp wrote: > Could you add some tests for this? Done. https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:678: SwReporterInvocation actual_invocation = On 2016/08/30 13:39:08, csharp wrote: > I think you want to create a copy constructor and use it here, since this will > drop all the fields but the command line. Done. https://codereview.chromium.org/2286743004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:713: // Send the logs if the last send was in the future. On 2016/08/30 13:39:08, csharp wrote: > nit: Could you explain why we care about this a bit? Done.
lgtm https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:113: EXPECT_LT(now - base::TimeDelta::FromHours(1), last_time_sent_logs); This doesn't seem to quite line up with the function name. Could you change one to keep them in sync. This also makes me wonder if we should make now() mockable for this test, but I'm not sure it is worth opening that can of worms https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:196: // Expects |reporter_launch_parameters_| contain exactly the command line nit: contain -> to contain(?) https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:199: const base::CommandLine::SwitchMap& invocation_switches = What about using std::set_symmetric_difference https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:418: std::set<std::string>{kExtendedSafeBrowsingEnabledSwitch, Could you make this set a const? https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:453: RunReporter(); All of your tests have this same four middle lines, make a helper function? Or maybe you could just use TestReporterLaunchCycle https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:761: bool logging_enabled = ShouldSendReporterLogs(*local_state); Drop variable and just have the function call in the if below? https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:764: local_state->SetInt64(prefs::kSwReporterLastTimeSentReport, Could you add a brief comment explaining why we set it here instead of with the other local state values https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:841: : command_line(other.command_line) {} Please also copy suffix and is_experimental https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.h:49: const base::Feature kSwReporterExtendedSafeBrowsingFeature{ I think you want this to be extern too
PTAL https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:113: EXPECT_LT(now - base::TimeDelta::FromHours(1), last_time_sent_logs); On 2016/09/02 21:16:25, csharp wrote: > This doesn't seem to quite line up with the function name. Could you change one > to keep them in sync. > > This also makes me wonder if we should make now() mockable for this test, but > I'm not sure it is worth opening that can of worms > Done. https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:196: // Expects |reporter_launch_parameters_| contain exactly the command line On 2016/09/02 21:16:25, csharp wrote: > nit: contain -> to contain(?) Done. https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:199: const base::CommandLine::SwitchMap& invocation_switches = On 2016/09/02 21:16:25, csharp wrote: > What about using std::set_symmetric_difference std::set_symmetric_different relies on the order of the sets. Since we have no control on how CommandLine::SwitchesMap is implemented (today it's a map, but there is no guarantee it will not be changed to an unordered_map in the future), I'd rather not create that dependency now. Another difficulty in making the change, is that we are not really comparing collections of the same type (one is a map the other is a set). The function above will also require either a comparator function or an adapter for the map iterator (none is available in base/), and I'm not sure if the final outcome will be worth the added complexity. I simplified the code though, by checking first if both have the same size. https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:418: std::set<std::string>{kExtendedSafeBrowsingEnabledSwitch, On 2016/09/02 21:16:25, csharp wrote: > Could you make this set a const? Done. https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:453: RunReporter(); On 2016/09/02 21:16:25, csharp wrote: > All of your tests have this same four middle lines, make a helper function? > > Or maybe you could just use TestReporterLaunchCycle Done. https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:761: bool logging_enabled = ShouldSendReporterLogs(*local_state); On 2016/09/02 21:16:25, csharp wrote: > Drop variable and just have the function call in the if below? Done. https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:764: local_state->SetInt64(prefs::kSwReporterLastTimeSentReport, On 2016/09/02 21:16:25, csharp wrote: > Could you add a brief comment explaining why we set it here instead of with the > other local state values Done. https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:841: : command_line(other.command_line) {} On 2016/09/02 21:16:25, csharp wrote: > Please also copy suffix and is_experimental Done. https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2286743004/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.h:49: const base::Feature kSwReporterExtendedSafeBrowsingFeature{ On 2016/09/02 21:16:25, csharp wrote: > I think you want this to be extern too Done.
lg, one question re. preservation of test state https://codereview.chromium.org/2286743004/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2286743004/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:110: void ExpectLastReportSentInTheLastHour() { note that this might have some false positives in the cases of multiple successive runs. Is the value set in prefs in prefs::kSwReporterLastTimeSentReport guaranteed(ish) to be cleared between test runs? https://codereview.chromium.org/2286743004/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:117: // Checks if the last time set logs is set as no more than one hour ago, s/set/sent/ https://codereview.chromium.org/2286743004/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2286743004/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:448: bool BrowserIsSafeBrowsingExtended(const Browser* browser) { super-optional nit, only if you think it's a good idea: s/BrowserIsSafeBrowsingExtended/SafeBrowsingExtendedEnabledForBrowser/ https://codereview.chromium.org/2286743004/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:760: // can between this and the next run for this ReporterRunner object. For s/can/can change/
ftirelo@google.com changed reviewers: + ftirelo@google.com
PTAL. If everything is okay, I'll send to owners for approval. https://codereview.chromium.org/2286743004/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2286743004/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:110: void ExpectLastReportSentInTheLastHour() { On 2016/09/07 14:32:09, robertshield_OOO_until_Sep_5 wrote: > note that this might have some false positives in the cases of multiple > successive runs. Is the value set in prefs in > prefs::kSwReporterLastTimeSentReport guaranteed(ish) to be cleared between test > runs? Done. https://codereview.chromium.org/2286743004/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:117: // Checks if the last time set logs is set as no more than one hour ago, On 2016/09/07 14:32:09, robertshield_OOO_until_Sep_5 wrote: > s/set/sent/ Done. https://codereview.chromium.org/2286743004/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2286743004/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:448: bool BrowserIsSafeBrowsingExtended(const Browser* browser) { On 2016/09/07 14:32:09, robertshield_OOO_until_Sep_5 wrote: > super-optional nit, only if you think it's a good idea: > s/BrowserIsSafeBrowsingExtended/SafeBrowsingExtendedEnabledForBrowser/ Done. https://codereview.chromium.org/2286743004/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:760: // can between this and the next run for this ReporterRunner object. For On 2016/09/07 14:32:09, robertshield_OOO_until_Sep_5 wrote: > s/can/can change/ Done.
ftirelo@chromium.org changed reviewers: - ftirelo@google.com
alito@google.com changed reviewers: + alito@google.com
still lgtm.
lgtm https://codereview.chromium.org/2286743004/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2286743004/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:396: ClearLastTimeSentReport(); nit: Could you move this into the test harness Setup method?
https://codereview.chromium.org/2286743004/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2286743004/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:396: ClearLastTimeSentReport(); On 2016/09/07 19:21:37, robertshield_OOO_until_Sep_5 wrote: > nit: Could you move this into the test harness Setup method? Done.
ftirelo@chromium.org changed reviewers: + nparker@chromium.org, waffles@chromium.org
Adding waffles@ for OWNERS approval on components/component_updater and nparker@ for chrome/browser/safe_browsing.
The CQ bit was checked by ftirelo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ftirelo@chromium.org
components/component_updater lgtm
nparker@chromium.org changed reviewers: + jialiul@chromium.org
Jialiu -- Can you review? Thanks.
chrome/browser/safe_browsing/ lgtm
The CQ bit was checked by ftirelo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alito@chromium.org, csharp@chromium.org, robertshield@chromium.org, alito@google.com Link to the patchset: https://codereview.chromium.org/2286743004/#ps220001 (title: "Always clean state on test setup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ftirelo@chromium.org changed reviewers: + jochen@chromium.org
Adding jochen@ for OWNERS of chrome/browser/BUILD.gn.
lgtm
The CQ bit was checked by ftirelo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Sends switches to the Software Reporter to enable matching data collection. BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=634434 ========== to ========== Sends switches to the Software Reporter to enable matching data collection. BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=634434 Committed: https://crrev.com/379385b7777ca2b90705226141acdfbb1335f6c4 Cr-Commit-Position: refs/heads/master@{#417278} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/379385b7777ca2b90705226141acdfbb1335f6c4 Cr-Commit-Position: refs/heads/master@{#417278} |
