|
|
Created:
3 years, 10 months ago by Nathan Parker Modified:
3 years, 10 months ago CC:
asanka, asvitkine+watch_chromium.org, chromium-reviews, grt+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow Safe Browsing backend to select downloads to upload.
This gives the server more nuanced control so it can mark a file as
verdict UNKNOWN but still request it get uploaded, to improve the
verdict for later.
BUG=687685
Review-Url: https://codereview.chromium.org/2696973002
Cr-Commit-Position: refs/heads/master@{#452181}
Committed: https://chromium.googlesource.com/chromium/src/+/9a5e4eca3101ae44e53ae30064b5efba3e2bfdc4
Patch Set 1 #Patch Set 2 : Add unittest for DownloadProtectionService #
Total comments: 4
Patch Set 3 : Fix a test, and address jialiul's comments #Patch Set 4 : Switch logic to make the upload_requested bit authoritative for not-SAFE #Patch Set 5 : Fix histogram desc #Patch Set 6 : rebase #Patch Set 7 : Fix up comments #
Total comments: 4
Patch Set 8 : Switch histogram to use enum, per isherman #Messages
Total messages: 57 (36 generated)
The CQ bit was checked by nparker@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...
nparker@chromium.org changed reviewers: + jialiul@google.com
jialiul@chromium.org changed reviewers: + jialiul@chromium.org
lgtm https://codereview.chromium.org/2696973002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_feedback_service.h (right): https://codereview.chromium.org/2696973002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_feedback_service.h:47: // upload by the server with |upload_requested| if it's needed for better nit: Do we need to mention "Unknown" verdict in the comment? This code does not check whether the verdict is "unknown" or not. https://codereview.chromium.org/2696973002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2696973002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:58350: + requested for upload by the server but for which the verdict would otherwise this summary is a little bit hard to parse...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by nparker@chromium.org to run a CQ dry run
Thanks! PTAL. https://codereview.chromium.org/2696973002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_feedback_service.h (right): https://codereview.chromium.org/2696973002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_feedback_service.h:47: // upload by the server with |upload_requested| if it's needed for better On 2017/02/15 00:45:27, Jialiu Lin wrote: > nit: Do we need to mention "Unknown" verdict in the comment? This code does not > check whether the verdict is "unknown" or not. good point. The plan is that the backend will only use this w/ unknown verdicts, but I'm not enforcing it in the code. Fixed the comment. https://codereview.chromium.org/2696973002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2696973002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:58350: + requested for upload by the server but for which the verdict would otherwise On 2017/02/15 00:45:27, Jialiu Lin wrote: > this summary is a little bit hard to parse... Done.
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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nparker@chromium.org to run a CQ dry run
The CQ bit was checked by nparker@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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by nparker@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 checked by nparker@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...
nparker@chromium.org changed reviewers: - jialiul@google.com
I switched the logic to match what auk@ described. PTAL.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nparker@chromium.org
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...)
nparker@chromium.org changed reviewers: + asanka@chromium.org, holte@google.com
asaka: Please review chrome/browser/download/download_browsertest.cc holte: Please review histograms.xml Thanks!
lgtm
nparker@chromium.org changed reviewers: + isherman@chromium.org - holte@google.com
-holte (vacation), +isherman for histograms.xml. (Thanks!)
https://codereview.chromium.org/2696973002/diff/90009/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2696973002/diff/90009/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:58447: +<histogram name="SBDownloadFeedback.UploadRequestedByServer" units="boolean"> Please use an enum, with labels specific to this histogram. (Only histograms.xml changes needed -- C++ code is fine as-is.) https://codereview.chromium.org/2696973002/diff/90009/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:58450: + Records how many non-SAFE files were picked for upload by the server. I'd write this as: "For each non-SAFE file, records whether that files was picked for upload by the server."
Thanks! isherman -- PTAL. https://codereview.chromium.org/2696973002/diff/90009/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2696973002/diff/90009/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:58447: +<histogram name="SBDownloadFeedback.UploadRequestedByServer" units="boolean"> On 2017/02/18 00:31:55, Ilya Sherman wrote: > Please use an enum, with labels specific to this histogram. (Only > histograms.xml changes needed -- C++ code is fine as-is.) Done. https://codereview.chromium.org/2696973002/diff/90009/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:58450: + Records how many non-SAFE files were picked for upload by the server. On 2017/02/18 00:31:55, Ilya Sherman wrote: > I'd write this as: "For each non-SAFE file, records whether that files was > picked for upload by the server." Done.
Metrics LGTM, thanks.
The CQ bit was checked by nparker@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, jialiul@chromium.org Link to the patchset: https://codereview.chromium.org/2696973002/#ps130001 (title: "Switch histogram to use enum, per isherman")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by nparker@chromium.org
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by nparker@chromium.org
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_TIMED_OUT, no build URL)
The CQ bit was checked by nparker@chromium.org
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": 130001, "attempt_start_ts": 1487788719149550, "parent_rev": "77ec80cbdc1be45775bffd27605bfd9549f8a9a6", "commit_rev": "9a5e4eca3101ae44e53ae30064b5efba3e2bfdc4"}
Message was sent while issue was closed.
Description was changed from ========== Allow Safe Browsing backend to select downloads to upload. This gives the server more nuanced control so it can mark a file as verdict UNKNOWN but still request it get uploaded, to improve the verdict for later. BUG=687685 ========== to ========== Allow Safe Browsing backend to select downloads to upload. This gives the server more nuanced control so it can mark a file as verdict UNKNOWN but still request it get uploaded, to improve the verdict for later. BUG=687685 Review-Url: https://codereview.chromium.org/2696973002 Cr-Commit-Position: refs/heads/master@{#452181} Committed: https://chromium.googlesource.com/chromium/src/+/9a5e4eca3101ae44e53ae30064b5... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001) as https://chromium.googlesource.com/chromium/src/+/9a5e4eca3101ae44e53ae30064b5... |