Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2637153002:
Submit a sample of notification images to Safe Browsing (Closed)
|
Created:
3 years, 11 months ago by harkness Modified:
3 years, 11 months ago CC:
chromium-reviews, Peter Beverloo, grt+watch_chromium.org, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org, asvitkine+watch_chromium.org, Jialiu Lin Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSubmit a sample of notification images to Safe Browsing
These will be scanned for social engineering behavior.
Only uploads if all of the following are true:
- User has opted in to SBER_LEVEL_SCOUT.
- Origin is not on CSD phishing whitelist.
- Device has sent < 5 reports in last 24 hours (see persistence TODO).
- Device is part of the enabled experiment group.
The notification image bitmap is downscaled to <= 512x512, encoded as a
PNG, then sent to the CSD server as a NotificationImageReportRequest
protobuf from chrome/common/safe_browsing/csd.proto.
BUG=678443
Review-Url: https://codereview.chromium.org/2637153002
Cr-Commit-Position: refs/heads/master@{#445365}
Committed: https://chromium.googlesource.com/chromium/src/+/f4f2180f841790ca788134df1cc8bf75b1d6ac58
Patch Set 1 #
Total comments: 7
Patch Set 2 : Updated resizing code and added mime_type to the proto. #Patch Set 3 : Updated/confirmed URL for reporting #
Total comments: 15
Patch Set 4 : Refactored scout check #
Total comments: 14
Patch Set 5 : Changed the Finch to be a params based trial. Also addressed nits. #
Total comments: 12
Patch Set 6 : updated test to check for not logging when report chance is 0. #
Total comments: 4
Patch Set 7 : nits #Messages
Total messages: 30 (11 generated)
harkness@chromium.org changed reviewers: + johnme@chromium.org, nparker@chromium.org, peter@chromium.org
nparker@chromium.org: Please review changes in chrome/*/safe_browsing/ peter@chromium.org: Please review changes in chrome/browser/notifications/ This is an updated version of 2624193004. John is out on vacation, and he asked me to add in a Finch experiment. This is my first Finch experiment, but it seems to follow the Finch 101. I can't find a way to manually link this to the earlier review. If there is one, please let me know and I can link it so the old comments aren't lost.
I didn't get a chance to do a full pass at the threading changes from the earlier CL -- I will get to that tomorrow. +jialiul for another pair of eyes on things going into the PingManager https://codereview.chromium.org/2637153002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/notification_image_reporter.cc (right): https://codereview.chromium.org/2637153002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/notification_image_reporter.cc:163: double scale = std::min(MAX_SIZE / image.width(), MAX_SIZE / image.height()); Check for div by zero https://codereview.chromium.org/2637153002/diff/1/chrome/common/safe_browsing... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/2637153002/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/csd.proto:842: optional bytes png_data = 1; awoz@ has requested that this be "data" and that we add back a mime type. If we don't have an mime type, we can just set it to "image/png" since that's what we're filling in. This way the proto can support other types in the future. See https://critique.corp.google.com/#review/143594840/depot/google3/quality/trus...
Thanks! https://codereview.chromium.org/2637153002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/notification_image_reporter.cc (right): https://codereview.chromium.org/2637153002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/notification_image_reporter.cc:69: trial->AppendGroup("enabled", 20); The plan is to land this with a zero percent chance, then turn it to 20% using Finch once the Safe Browsing CSD server is ready to receive reports. https://codereview.chromium.org/2637153002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/notification_image_reporter.cc:163: double scale = std::min(MAX_SIZE / image.width(), MAX_SIZE / image.height()); Technically this is fine, as infinity is greater than zero so the image won't be downscaled. But yeah, it might be clearer to wrap this block in an `if (w > max || h > max)` condition, and then you can remove the `scale >= 1.0 ? image // already small enough` line.
Updated as per John and Nathan's comments. https://codereview.chromium.org/2637153002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/notification_image_reporter.cc (right): https://codereview.chromium.org/2637153002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/notification_image_reporter.cc:69: trial->AppendGroup("enabled", 20); On 2017/01/19 08:58:53, johnme wrote: > The plan is to land this with a zero percent chance, then turn it to 20% using > Finch once the Safe Browsing CSD server is ready to receive reports. Updated to 0 for the default. https://codereview.chromium.org/2637153002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/notification_image_reporter.cc:163: double scale = std::min(MAX_SIZE / image.width(), MAX_SIZE / image.height()); On 2017/01/19 08:58:53, johnme wrote: > Technically this is fine, as infinity is greater than zero so the image won't be > downscaled. But yeah, it might be clearer to wrap this block in an `if (w > max > || h > max)` condition, and then you can remove the `scale >= 1.0 ? image // > already small enough` line. I modified the code as John suggested, but also threw in a h>0 and w>0 check just for sanity. https://codereview.chromium.org/2637153002/diff/1/chrome/common/safe_browsing... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/2637153002/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/csd.proto:842: optional bytes png_data = 1; On 2017/01/19 01:22:09, Nathan Parker wrote: > awoz@ has requested that this be "data" and that we add back a mime type. If we > don't have an mime type, we can just set it to "image/png" since that's what > we're filling in. > > This way the proto can support other types in the future. > > See > https://critique.corp.google.com/#review/143594840/depot/google3/quality/trus... I added in mime_type as a field, and it's hard coded to "image/png" in the notification image reporter.
harkness@chromium.org changed reviewers: + isherman@chromium.org
isherman@ could you please take a look at UMA changes?
jialiul@chromium.org changed reviewers: + jialiul@chromium.org
https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/notification_image_reporter.cc (right): https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:95: if (!IsReportingEnabled()) { Finch checking does not need to be in the IO thread, right? I'll suggest move the finch checking and scout checking logic to SafeBrowsingPingManager::ReportNotificationImage. such that, you don't need to worry about underreporting. https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:137: switch (GetExtendedReportingLevel(*profile->GetPrefs())) { nit: since you only care about scout, it is more concise to use if statement here. if(GetExtendedReportingLevel(*profile->GetPrefs()) == SBER_LEVEL_SCOUT) { ...send report... } or you can do if (IsExtendedReportingEnabled(prefs) && IsScout(prefs)) { ... } Also, maybe move this logic to SafeBrowsingPingManager::ReportNotificationImage. https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:161: DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); Is this still on UI thread? Maybe add DCHECK_CURRENTLY_ON(BrowserThread::UI); https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:177: std::vector<unsigned char> png_bytes; how about DCHECK(gfx::PNGCodec::EncodeBGRASkBitmap(downscaled_image, false, &png_bytes)); https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:182: notification_image_reporter_->ReportNotificationImageOnIO( Do finch and Scout checks here, if(finch && scout) notification_image_reporter_->ReportNotificationImageOnIO(..) Then you'll solve the underreport problem.
https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/notification_image_reporter.cc (right): https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:95: if (!IsReportingEnabled()) { On 2017/01/19 18:54:47, Jialiu Lin wrote: > Finch checking does not need to be in the IO thread, right? > I'll suggest move the finch checking and scout checking logic to > SafeBrowsingPingManager::ReportNotificationImage. such that, you don't need to > worry about underreporting. Unfortunately, IsReportingEnabled is virtual, and ReportNotificationImageOnUI is static, so I can't move it there. https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:137: switch (GetExtendedReportingLevel(*profile->GetPrefs())) { On 2017/01/19 18:54:47, Jialiu Lin wrote: > nit: since you only care about scout, it is more concise to use if statement > here. > > if(GetExtendedReportingLevel(*profile->GetPrefs()) == SBER_LEVEL_SCOUT) { > ...send report... > } > > or you can do > if (IsExtendedReportingEnabled(prefs) && IsScout(prefs)) { > ... > } > > Also, maybe move this logic to SafeBrowsingPingManager::ReportNotificationImage. I refactored the switch into an if which is much clearer. However, because the scout check uses the preferences service, it needs to be run on the UI thread, so I left the code here instead of moving it into the PingManager which runs on the IO thread. https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:161: DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); On 2017/01/19 18:54:47, Jialiu Lin wrote: > Is this still on UI thread? Maybe add > DCHECK_CURRENTLY_ON(BrowserThread::UI); No, the blocking pool threads are distinct from the UI thread, so this cannot run on the UI thread. https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:177: std::vector<unsigned char> png_bytes; On 2017/01/19 18:54:46, Jialiu Lin wrote: > how about > DCHECK(gfx::PNGCodec::EncodeBGRASkBitmap(downscaled_image, false, &png_bytes)); That would break things if DCHECKs weren't enabled, since the entire line of code would be compiled out. https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:182: notification_image_reporter_->ReportNotificationImageOnIO( On 2017/01/19 18:54:47, Jialiu Lin wrote: > Do finch and Scout checks here, > if(finch && scout) > notification_image_reporter_->ReportNotificationImageOnIO(..) > > Then you'll solve the underreport problem. As mentioned in the other comment, this needs to happen on the UI thread, so it can't happen here.
Metrics LGTM https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/notification_image_reporter.cc (right): https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:43: void LogReportResult(const GURL& url, int net_error) { nit: Looks like the |url| is not used, so it could probably be safely omitted.
lgtm, Thanks for all the explanations. https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/notification_image_reporter.cc (right): https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:95: if (!IsReportingEnabled()) { On 2017/01/19 20:09:09, harkness wrote: > On 2017/01/19 18:54:47, Jialiu Lin wrote: > > Finch checking does not need to be in the IO thread, right? > > I'll suggest move the finch checking and scout checking logic to > > SafeBrowsingPingManager::ReportNotificationImage. such that, you don't need to > > worry about underreporting. > > Unfortunately, IsReportingEnabled is virtual, and ReportNotificationImageOnUI is > static, so I can't move it there. Acknowledged. https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:137: switch (GetExtendedReportingLevel(*profile->GetPrefs())) { On 2017/01/19 20:09:08, harkness wrote: > On 2017/01/19 18:54:47, Jialiu Lin wrote: > > nit: since you only care about scout, it is more concise to use if statement > > here. > > > > if(GetExtendedReportingLevel(*profile->GetPrefs()) == SBER_LEVEL_SCOUT) { > > ...send report... > > } > > > > or you can do > > if (IsExtendedReportingEnabled(prefs) && IsScout(prefs)) { > > ... > > } > > > > Also, maybe move this logic to > SafeBrowsingPingManager::ReportNotificationImage. > > I refactored the switch into an if which is much clearer. However, because the > scout check uses the preferences service, it needs to be run on the UI thread, > so I left the code here instead of moving it into the PingManager which runs on > the IO thread. Acknowledged. https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:161: DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); On 2017/01/19 20:09:09, harkness wrote: > On 2017/01/19 18:54:47, Jialiu Lin wrote: > > Is this still on UI thread? Maybe add > > DCHECK_CURRENTLY_ON(BrowserThread::UI); > > No, the blocking pool threads are distinct from the UI thread, so this cannot > run on the UI thread. Acknowledged. https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:177: std::vector<unsigned char> png_bytes; On 2017/01/19 20:09:09, harkness wrote: > On 2017/01/19 18:54:46, Jialiu Lin wrote: > > how about > > DCHECK(gfx::PNGCodec::EncodeBGRASkBitmap(downscaled_image, false, > &png_bytes)); > > That would break things if DCHECKs weren't enabled, since the entire line of > code would be compiled out. Acknowledged. https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2637153002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:182: notification_image_reporter_->ReportNotificationImageOnIO( On 2017/01/19 20:09:09, harkness wrote: > On 2017/01/19 18:54:47, Jialiu Lin wrote: > > Do finch and Scout checks here, > > if(finch && scout) > > notification_image_reporter_->ReportNotificationImageOnIO(..) > > > > Then you'll solve the underreport problem. > > As mentioned in the other comment, this needs to happen on the UI thread, so it > can't happen here. Acknowledged.
Thanks for picking this up Jen! Notifications lgtm
The CQ bit was checked by harkness@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...
https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/notification_image_reporter.cc (right): https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:40: const char kImageReporting[] = "NotificationImageReporting"; nit: Add "FieldTrial" (or "Feature" if you go with my suggestion below) to this variable name https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:43: void LogReportResult(const GURL& url, int net_error) { On 2017/01/19 22:53:40, Ilya Sherman wrote: > nit: Looks like the |url| is not used, so it could probably be safely omitted. Looks like that's part of the ReportSender interface, and is likely used elsewhere. You could add a comment so it doesn't look like a mistake. https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:123: return (base::FieldTrialList::FindFullName(kImageReporting) == "enabled"); This is a bit inflexible since you can never change the group name, nor have multiple enabled groups doing different things. A more flexible way (and easier to test since it can be used with a simple flag) is with a "Features." e.g.: https://cs.chromium.org/chromium/src/components/safe_browsing_db/v4_feature_l... Then you add something like this to the finch config: "enabled_features": ["NotificationImageReporting"] And then you can test with --enable_features=NotificationImageReporting. https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/notification_image_reporter.h (right): https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.h:69: // by invoking DownscaleNotificationImageOnBlockingPool. Add a comment on why this is static and takes a weak ptr (because it has to do more thread hops after this, and needs the ptr to get invalidated if it gets destroyed, I assume).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/notification_image_reporter.cc (right): https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:123: return (base::FieldTrialList::FindFullName(kImageReporting) == "enabled"); On 2017/01/20 00:25:43, Nathan Parker wrote: > This is a bit inflexible since you can never change the group name, nor have > multiple enabled groups doing different things. > > A more flexible way (and easier to test since it can be used with a simple flag) > is with a "Features." e.g.: > > https://cs.chromium.org/chromium/src/components/safe_browsing_db/v4_feature_l... > > Then you add something like this to the finch config: > "enabled_features": ["NotificationImageReporting"] > > And then you can test with --enable_features=NotificationImageReporting. Actually, this isn't sufficient to select a random sample of images from one particular user -- it'll let you pick particular users to enable. So I'd suggest using "params" on the field trial. You can put in a string value of, say, a ascii float (e.g. "0.2"). Then you'd convert that here from string->float and call random to see if this image should be selected. If the param isn't present, you can default the fraction to 0.0, so it'll be off be default. And you won't need the call to base::FieldTrialList::FactoryGetFieldTrial() -- all that config will go in the Google3 finch config. https://codereview.chromium.org/2637153002/diff/60001/chrome/common/safe_brow... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/2637153002/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/csd.proto:844: // Encoding scheme for the bitmap. If not specified, assumed to be image/png. Let's remove the "if not specified" part since the backend isn't making that assumption, and we should just populate this. https://codereview.chromium.org/2637153002/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/csd.proto:852: optional Dimensions dimensions = 3; Add comment // Dimensions of the image stored in |data|.
https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/notification_image_reporter.cc (right): https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:40: const char kImageReporting[] = "NotificationImageReporting"; On 2017/01/20 00:25:43, Nathan Parker wrote: > nit: Add "FieldTrial" (or "Feature" if you go with my suggestion below) to this > variable name Done. https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:43: void LogReportResult(const GURL& url, int net_error) { On 2017/01/20 00:25:43, Nathan Parker wrote: > On 2017/01/19 22:53:40, Ilya Sherman wrote: > > nit: Looks like the |url| is not used, so it could probably be safely omitted. > > Looks like that's part of the ReportSender interface, and is likely used > elsewhere. You could add a comment so it doesn't look like a mistake. Done. https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:123: return (base::FieldTrialList::FindFullName(kImageReporting) == "enabled"); On 2017/01/20 01:37:28, Nathan Parker wrote: > On 2017/01/20 00:25:43, Nathan Parker wrote: > > This is a bit inflexible since you can never change the group name, nor have > > multiple enabled groups doing different things. > > > > A more flexible way (and easier to test since it can be used with a simple > flag) > > is with a "Features." e.g.: > > > > > https://cs.chromium.org/chromium/src/components/safe_browsing_db/v4_feature_l... > > > > Then you add something like this to the finch config: > > "enabled_features": ["NotificationImageReporting"] > > > > And then you can test with --enable_features=NotificationImageReporting. > > > Actually, this isn't sufficient to select a random sample of images from one > particular user -- it'll let you pick particular users to enable. > > So I'd suggest using "params" on the field trial. You can put in a string value > of, say, a ascii float (e.g. "0.2"). Then you'd convert that here from > string->float and call random to see if this image should be selected. > > If the param isn't present, you can default the fraction to 0.0, so it'll be off > be default. And you won't need the call to > base::FieldTrialList::FactoryGetFieldTrial() -- all that config will go in the > Google3 finch config. Done. https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/notification_image_reporter.h (right): https://codereview.chromium.org/2637153002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.h:69: // by invoking DownscaleNotificationImageOnBlockingPool. On 2017/01/20 00:25:43, Nathan Parker wrote: > Add a comment on why this is static and takes a weak ptr (because it has to do > more thread hops after this, and needs the ptr to get invalidated if it gets > destroyed, I assume). Done. https://codereview.chromium.org/2637153002/diff/60001/chrome/common/safe_brow... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/2637153002/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/csd.proto:844: // Encoding scheme for the bitmap. If not specified, assumed to be image/png. On 2017/01/20 01:37:29, Nathan Parker wrote: > Let's remove the "if not specified" part since the backend isn't making that > assumption, and we should just populate this. Done. https://codereview.chromium.org/2637153002/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/csd.proto:852: optional Dimensions dimensions = 3; On 2017/01/20 01:37:28, Nathan Parker wrote: > Add comment > // Dimensions of the image stored in |data|. Done.
LGTM! Just one test request. Thanks https://codereview.chromium.org/2637153002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/notification_image_reporter.cc (right): https://codereview.chromium.org/2637153002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:123: double report_chance = variations::GetVariationParamByFeatureAsDouble( Nice! https://codereview.chromium.org/2637153002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/notification_image_reporter_unittest.cc (right): https://codereview.chromium.org/2637153002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter_unittest.cc:49: double GetReportChance() const override { return 1.0; } Can you add a test that verifies it doesn't report if GetReportChance is 0? https://codereview.chromium.org/2637153002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2637153002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:54427: + The net error code for reports sent by NotificationImageReporter. add: Logged for failed reports.
https://codereview.chromium.org/2637153002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/notification_image_reporter.cc (right): https://codereview.chromium.org/2637153002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.cc:123: double report_chance = variations::GetVariationParamByFeatureAsDouble( On 2017/01/20 21:18:49, Nathan Parker wrote: > Nice! Acknowledged. https://codereview.chromium.org/2637153002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/notification_image_reporter_unittest.cc (right): https://codereview.chromium.org/2637153002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter_unittest.cc:49: double GetReportChance() const override { return 1.0; } On 2017/01/20 21:18:49, Nathan Parker wrote: > Can you add a test that verifies it doesn't report if GetReportChance is 0? Done. https://codereview.chromium.org/2637153002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2637153002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:54427: + The net error code for reports sent by NotificationImageReporter. On 2017/01/20 21:18:49, Nathan Parker wrote: > add: Logged for failed reports. This is actually logged for both successful and failed reports. Successful ones log with Net::OK.
lgtm - thanks a lot for taking this patch over! https://codereview.chromium.org/2637153002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/notification_image_reporter.h (right): https://codereview.chromium.org/2637153002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.h:70: // callable from any thread and will invoke a member function on the IO thread Nit: s/callable from any thread/called on the UI thread/ https://codereview.chromium.org/2637153002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.h:79: // then invokes SendReportOnIO. This should be callable from any thread and Nit: s/callable from any thread/called on a blocking pool thread/ https://codereview.chromium.org/2637153002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2637153002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:54427: + The net error code for reports sent by NotificationImageReporter. On 2017/01/23 11:30:10, harkness wrote: > On 2017/01/20 21:18:49, Nathan Parker wrote: > > add: Logged for failed reports. > > This is actually logged for both successful and failed reports. Successful ones > log with Net::OK. Nit: Perhaps add "or net:OK if successful" to clarify this? https://codereview.chromium.org/2637153002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/notification_image_reporter_unittest.cc (right): https://codereview.chromium.org/2637153002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/notification_image_reporter_unittest.cc:66: double reporting_chance_ = 1; Micro-nit: 1.0 https://codereview.chromium.org/2637153002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/notification_image_reporter_unittest.cc:261: notification_image_reporter_->SetReportingChance(0); Micro-nit: 0.0 (https://google.github.io/styleguide/cppguide.html#0_and_nullptr/NULL)
https://codereview.chromium.org/2637153002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/notification_image_reporter.h (right): https://codereview.chromium.org/2637153002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.h:70: // callable from any thread and will invoke a member function on the IO thread On 2017/01/23 11:34:34, johnme wrote: > Nit: s/callable from any thread/called on the UI thread/ Done. https://codereview.chromium.org/2637153002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/notification_image_reporter.h:79: // then invokes SendReportOnIO. This should be callable from any thread and On 2017/01/23 11:34:34, johnme wrote: > Nit: s/callable from any thread/called on a blocking pool thread/ Done. https://codereview.chromium.org/2637153002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2637153002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:54427: + The net error code for reports sent by NotificationImageReporter. On 2017/01/23 11:34:34, johnme wrote: > On 2017/01/23 11:30:10, harkness wrote: > > On 2017/01/20 21:18:49, Nathan Parker wrote: > > > add: Logged for failed reports. > > > > This is actually logged for both successful and failed reports. Successful > ones > > log with Net::OK. > > Nit: Perhaps add "or net:OK if successful" to clarify this? Done. https://codereview.chromium.org/2637153002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/notification_image_reporter_unittest.cc (right): https://codereview.chromium.org/2637153002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/notification_image_reporter_unittest.cc:66: double reporting_chance_ = 1; On 2017/01/23 11:34:34, johnme wrote: > Micro-nit: 1.0 Done. https://codereview.chromium.org/2637153002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/notification_image_reporter_unittest.cc:261: notification_image_reporter_->SetReportingChance(0); On 2017/01/23 11:34:34, johnme wrote: > Micro-nit: 0.0 > (https://google.github.io/styleguide/cppguide.html#0_and_nullptr/NULL) Done.
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, isherman@chromium.org, jialiul@chromium.org, nparker@chromium.org, johnme@chromium.org Link to the patchset: https://codereview.chromium.org/2637153002/#ps120001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485171966987450,
"parent_rev": "4d512b02d6169ea3e9949297b356204fdaa30a40", "commit_rev":
"f4f2180f841790ca788134df1cc8bf75b1d6ac58"}
Message was sent while issue was closed.
Description was changed from ========== Submit a sample of notification images to Safe Browsing These will be scanned for social engineering behavior. Only uploads if all of the following are true: - User has opted in to SBER_LEVEL_SCOUT. - Origin is not on CSD phishing whitelist. - Device has sent < 5 reports in last 24 hours (see persistence TODO). - Device is part of the enabled experiment group. The notification image bitmap is downscaled to <= 512x512, encoded as a PNG, then sent to the CSD server as a NotificationImageReportRequest protobuf from chrome/common/safe_browsing/csd.proto. BUG=678443 ========== to ========== Submit a sample of notification images to Safe Browsing These will be scanned for social engineering behavior. Only uploads if all of the following are true: - User has opted in to SBER_LEVEL_SCOUT. - Origin is not on CSD phishing whitelist. - Device has sent < 5 reports in last 24 hours (see persistence TODO). - Device is part of the enabled experiment group. The notification image bitmap is downscaled to <= 512x512, encoded as a PNG, then sent to the CSD server as a NotificationImageReportRequest protobuf from chrome/common/safe_browsing/csd.proto. BUG=678443 Review-Url: https://codereview.chromium.org/2637153002 Cr-Commit-Position: refs/heads/master@{#445365} Committed: https://chromium.googlesource.com/chromium/src/+/f4f2180f841790ca788134df1cc8... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/f4f2180f841790ca788134df1cc8... |
