Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2439533002:
Download Feedback Service should upload all eligible downloads (Closed)
|
Created:
4 years, 2 months ago by Jialiu Lin Modified:
4 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, dbeam+watch-downloads_chromium.org, jam, Nathan Parker, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL fixes a 3-year-old regression which makes download
feedback service only upload UNCOMMON downloads when user
clicks "DISCARD". The correct behavior should be uploading
all eligible downloads no matter user clicks KEEP or DISCARD
on download shelf, where eligible means:
1) user is not incognito
2) user opted in for safe browsing extended reporting.
3) DownloadItemModel::ShouldAllowDownloadFeedback returns true
This CL also changes the logic of StealDangerousDownload(),
since if user chose to keep the download we cannot delete it
after feedback is sent.
In order to make download file immediately available to user
without making them wait for upload complete, we make a
temporary copy of the download file in DownloadItemImpl::
StealDangerousDownload(..) function, and upload this copy via
DownloadFeedbackService::BeginFeedbackOrDeleteFile. This temp
copy will be deleted after feedback is sent
(~DownloadFeedbackImpl()).
BUG=655799
Committed: https://crrev.com/2fe67a026c44af1daf09616c2b7bda2db5abee12
Cr-Commit-Position: refs/heads/master@{#432528}
Patch Set 1 #Patch Set 2 : rebase-update #Patch Set 3 : rebase update #Patch Set 4 : nits #Patch Set 5 : nit function renaming #
Total comments: 19
Patch Set 6 : Keeping download after feedback sent #Patch Set 7 : fix content_unittests #Patch Set 8 : validate download after download feedback uploaded #Patch Set 9 : nit #Patch Set 10 : haut download completion util uploader is done. #Patch Set 11 : make a temp copy of download file for download feedback with KEEP #Patch Set 12 : fix comments and nits #
Total comments: 20
Patch Set 13 : address comments from asanka@ and pkasting@ #Patch Set 14 : make try bots happy #
Total comments: 10
Patch Set 15 : address/acknowledge comments from asanka@ #
Total comments: 2
Patch Set 16 : consolidate UMA invocation #Patch Set 17 : make windows bots happy #Messages
Total messages: 112 (76 generated)
The CQ bit was checked by jialiul@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jialiul@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: This issue passed the CQ dry run.
The CQ bit was checked by jialiul@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...
Description was changed from ========== draft BUG= ========== to ========== This CL fixes a 3-year-old regression which makes download feedback service only upload UNCOMMON downloads when user clicks "DISCARD". The correct behavior should be uploading all eligible downloads with all dangerous type no matter user keeps or discards the download. Note eligible means user is not incognito && opted in for safe browsing extended reporting. BUG=655799 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== This CL fixes a 3-year-old regression which makes download feedback service only upload UNCOMMON downloads when user clicks "DISCARD". The correct behavior should be uploading all eligible downloads with all dangerous type no matter user keeps or discards the download. Note eligible means user is not incognito && opted in for safe browsing extended reporting. BUG=655799 ========== to ========== This CL fixes a 3-year-old regression which makes download feedback service only upload UNCOMMON downloads when user clicks "DISCARD". The correct behavior should be uploading all eligible downloads no matter user clicks KEEP or DISCARD on download shelf, where eligible means: 1) user is not incognito 2) user opted in for safe browsing extended reporting. 3) DownloadItemModel::ShouldAllowDownloadFeedback returns true BUG=655799 ==========
jialiul@chromium.org changed reviewers: + asanka@chromium.org, pkasting@chromium.org
Hi asanka@ and pkasting@, Surprisingly, this is a 3 years old regression (without anyone complaining for a long time....), but per Safe Browsing Bineval team's recent request, we are trying to get this in M56. Note, this fix does not cover Mac OS, since Bineval binary analysis pipeline is not ready for dmg or other Mac executable types yet. Thanks!
The CQ bit was checked by jialiul@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: This issue passed the CQ dry run.
You added two reviewers. Please say what you want each to review? Or are you trying to get both of us to explicitly look at everything?
Ah, sorry. asanka@, could you take a look at the overall flow (and content/browser/download/download_item_impl.cc for owner review)? pkasting@, could you look at files under c/b/ui/views/*? Thanks!
My only real concern is the question about whether a KEEP download will still be deleted when successfully submitted. https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:269: if (!safe_browsing::ExtendedReportingPrefExists(*prefs)) { Nit: Remove ! and reverse conditional arms, so the "else" doesn't mentally read like a double-negative. https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:597: if (ShouldAllowDownloadFeedback()) { Nit: No {} https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:789: if ((!enabled || !SubmitDownloadToFeedbackService()) && Doesn't this result in deleting the download when submission succeeds, even if the command was KEEP? Is that a problem? https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:790: download_command_id == DownloadCommands::DISCARD) Nit: This might be clearer if written like: if (feedback_enabled && SubmitDownloadToFeedbackService()) return; // WARNING: |this| is now deleted. if (download_command_id == DownloadCommands::DISCARD) download()->Remove(); https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.h (right): https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.h:90: bool ShouldAllowDownloadFeedback(); Nit: I suggest a comment that states the purpose of the code instead of its function, and a function name more parallel to the others you use: // Returns whether the current download should be sent to the feedback service // (if the user approves). bool ShouldSubmitDownloadToFeedbackService(); However, note that both callers of this method just want to gate calls to MaybeSubmitDownloadToFeedbackService() on it. So consider just rolling it into that function, and updating the comment I suggest below to "...if the user has approved and the download is suitable for submission,..." https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.h:96: void MaybeSubmitDownloadToFeedbackService( It's very confusing that you have both MaybeSubmitDownloadToFeedbackService() and PossiblySubmitDownloadToFeedbackService(). Below I suggest a name change for the latter. I would also change the comments on here to soemthing like the following: // Sends the current download to the feedback service if the user has // approved, then performs the specified |download_command|. // // User approval comes in the form of a positive response to the // DownloadFeedbackDialogView, which this function will first show if it has // not been shown before. https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.h:163: void PossiblySubmitDownloadToFeedbackService( Nit: Call this function something more precise. I suggest SubmitDownloadWhenFeedbackServiceEnabled(). https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.h:165: bool enabled); Nit: I would rename this |feedback_enabled| and put it before |command_id|. https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_shelf_context_menu_view.cc (right): https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_shelf_context_menu_view.cc:63: if (download_item_view_->ShouldAllowDownloadFeedback()) Nit: Can combine this if with the outer one
The CQ bit was checked by jialiul@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...
Unfortunately the StealDangerousDownload logic and the upload logic aren't written to deal with KEEP :-( Once StealDangerousDownload is called, the download is removed from the system and ownership of the file is transferred to the caller, which is the DownloadFeedbackService. The latter deletes the file after completion. We'll need to rework this logic and assumptions made thereof before we can upload reports accepted dangerous downloads. https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:789: if ((!enabled || !SubmitDownloadToFeedbackService()) && On 2016/10/24 at 21:39:07, Peter Kasting wrote: > Doesn't this result in deleting the download when submission succeeds, even if the command was KEEP? Is that a problem? Yes. The StealDangerousDownload logic is meant to acquire a dangerous download after the user has chosen to discard it. Even if we didn't delete the file, we can't run the upload logic after accepting a dangerous download. :-( We expose the file to external consumers at that point. The upload logic may interfere with applications that are trying to access the download file and prevent the user from opening the file successfully.
Ah. Sounds like this can't land as-is then.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Thanks asanka@ and pkasting@. I'm now halfway changing the uploading logic (This CL is growing...). After I resolve all the related unit_/browser_tests, I'll ping you for another round of review. https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:269: if (!safe_browsing::ExtendedReportingPrefExists(*prefs)) { On 2016/10/24 21:39:07, Peter Kasting wrote: > Nit: Remove ! and reverse conditional arms, so the "else" doesn't mentally read > like a double-negative. Done. https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:597: if (ShouldAllowDownloadFeedback()) { On 2016/10/24 21:39:07, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:789: if ((!enabled || !SubmitDownloadToFeedbackService()) && On 2016/10/24 21:39:07, Peter Kasting wrote: > Doesn't this result in deleting the download when submission succeeds, even if > the command was KEEP? Is that a problem? oops, Thanks for catching this. Seems changes need to be made to more files. https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:790: download_command_id == DownloadCommands::DISCARD) On 2016/10/24 21:39:07, Peter Kasting wrote: > Nit: This might be clearer if written like: > > if (feedback_enabled && SubmitDownloadToFeedbackService()) > return; // WARNING: |this| is now deleted. > > if (download_command_id == DownloadCommands::DISCARD) > download()->Remove(); Done. https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.h (right): https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.h:90: bool ShouldAllowDownloadFeedback(); On 2016/10/24 21:39:07, Peter Kasting wrote: > Nit: I suggest a comment that states the purpose of the code instead of its > function, and a function name more parallel to the others you use: > > // Returns whether the current download should be sent to the feedback service > // (if the user approves). > bool ShouldSubmitDownloadToFeedbackService(); > > However, note that both callers of this method just want to gate calls to > MaybeSubmitDownloadToFeedbackService() on it. So consider just rolling it into > that function, and updating the comment I suggest below to "...if the user has > approved and the download is suitable for submission,..." Function merged into MaybeSubmitDownloadToFeedbackService(). https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.h:96: void MaybeSubmitDownloadToFeedbackService( On 2016/10/24 21:39:07, Peter Kasting wrote: > It's very confusing that you have both MaybeSubmitDownloadToFeedbackService() > and PossiblySubmitDownloadToFeedbackService(). > > Below I suggest a name change for the latter. I would also change the comments > on here to soemthing like the following: > > // Sends the current download to the feedback service if the user has > // approved, then performs the specified |download_command|. > // > // User approval comes in the form of a positive response to the > // DownloadFeedbackDialogView, which this function will first show if it has > // not been shown before. Good suggestion! I have renamed the second function accordingly. https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.h:163: void PossiblySubmitDownloadToFeedbackService( On 2016/10/24 21:39:07, Peter Kasting wrote: > Nit: Call this function something more precise. I suggest > SubmitDownloadWhenFeedbackServiceEnabled(). Done. https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.h:165: bool enabled); On 2016/10/24 21:39:07, Peter Kasting wrote: > Nit: I would rename this |feedback_enabled| and put it before |command_id|. Renamed |enabled| to |feedback_enabled|, though I have to keep |feedback_enabled| at the end of the variable list, since it will take in user decision callback values from DownloadFeedbackDialogView. https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_shelf_context_menu_view.cc (right): https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_shelf_context_menu_view.cc:63: if (download_item_view_->ShouldAllowDownloadFeedback()) On 2016/10/24 21:39:07, Peter Kasting wrote: > Nit: Can combine this if with the outer one Done.
The CQ bit was checked by jialiul@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by jialiul@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jialiul@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: This issue passed the CQ dry run.
On 2016/10/25 at 21:07:10, jialiul wrote: > Thanks asanka@ and pkasting@. > I'm now halfway changing the uploading logic (This CL is growing...). After I resolve all the related unit_/browser_tests, I'll ping you for another round of review. Cool. I haven't looked at the upload logic yet. If you aren't already, could you consider making a temporary copy of the downloaded file (or a size capped prefix of the file), and upload using the copy? This way we can play well with applications that open the downloaded file immediately after the download completes. Also, this allows Chrome to signal download completion sooner rather than wait for the upload to complete. > https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/download/download_item_view.cc (right): > > https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/download/download_item_view.cc:269: if (!safe_browsing::ExtendedReportingPrefExists(*prefs)) { > On 2016/10/24 21:39:07, Peter Kasting wrote: > > Nit: Remove ! and reverse conditional arms, so the "else" doesn't mentally read > > like a double-negative. > Done. > > https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/download/download_item_view.cc:597: if (ShouldAllowDownloadFeedback()) { > On 2016/10/24 21:39:07, Peter Kasting wrote: > > Nit: No {} > > Done. > > https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/download/download_item_view.cc:789: if ((!enabled || !SubmitDownloadToFeedbackService()) && > On 2016/10/24 21:39:07, Peter Kasting wrote: > > Doesn't this result in deleting the download when submission succeeds, even if > > the command was KEEP? Is that a problem? > > oops, Thanks for catching this. Seems changes need to be made to more files. > > https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/download/download_item_view.cc:790: download_command_id == DownloadCommands::DISCARD) > On 2016/10/24 21:39:07, Peter Kasting wrote: > > Nit: This might be clearer if written like: > > > > if (feedback_enabled && SubmitDownloadToFeedbackService()) > > return; // WARNING: |this| is now deleted. > > > > if (download_command_id == DownloadCommands::DISCARD) > > download()->Remove(); > > Done. > > https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/download/download_item_view.h (right): > > https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/download/download_item_view.h:90: bool ShouldAllowDownloadFeedback(); > On 2016/10/24 21:39:07, Peter Kasting wrote: > > Nit: I suggest a comment that states the purpose of the code instead of its > > function, and a function name more parallel to the others you use: > > > > // Returns whether the current download should be sent to the feedback service > > // (if the user approves). > > bool ShouldSubmitDownloadToFeedbackService(); > > > > However, note that both callers of this method just want to gate calls to > > MaybeSubmitDownloadToFeedbackService() on it. So consider just rolling it into > > that function, and updating the comment I suggest below to "...if the user has > > approved and the download is suitable for submission,..." > Function merged into MaybeSubmitDownloadToFeedbackService(). > > https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/download/download_item_view.h:96: void MaybeSubmitDownloadToFeedbackService( > On 2016/10/24 21:39:07, Peter Kasting wrote: > > It's very confusing that you have both MaybeSubmitDownloadToFeedbackService() > > and PossiblySubmitDownloadToFeedbackService(). > > > > Below I suggest a name change for the latter. I would also change the comments > > on here to soemthing like the following: > > > > // Sends the current download to the feedback service if the user has > > // approved, then performs the specified |download_command|. > > // > > // User approval comes in the form of a positive response to the > > // DownloadFeedbackDialogView, which this function will first show if it has > > // not been shown before. > > Good suggestion! I have renamed the second function accordingly. > > https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/download/download_item_view.h:163: void PossiblySubmitDownloadToFeedbackService( > On 2016/10/24 21:39:07, Peter Kasting wrote: > > Nit: Call this function something more precise. I suggest > > SubmitDownloadWhenFeedbackServiceEnabled(). > > Done. > > https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/download/download_item_view.h:165: bool enabled); > On 2016/10/24 21:39:07, Peter Kasting wrote: > > Nit: I would rename this |feedback_enabled| and put it before |command_id|. > > Renamed |enabled| to |feedback_enabled|, though I have to keep |feedback_enabled| at the end of the variable list, since it will take in user decision callback values from DownloadFeedbackDialogView. > > https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/download/download_shelf_context_menu_view.cc (right): > > https://codereview.chromium.org/2439533002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/download/download_shelf_context_menu_view.cc:63: if (download_item_view_->ShouldAllowDownloadFeedback()) > On 2016/10/24 21:39:07, Peter Kasting wrote: > > Nit: Can combine this if with the outer one > > Done.
Thanks, asanka@. My current implementation is to delay the download completion until upload is done by adding adding another condition in IsDownloadReadyForCompletion(), then call MaybeCompleteDownload() when feedback is done. I'll try using the temporary file approach to see which way is better.
The CQ bit was checked by jialiul@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...
Description was changed from ========== This CL fixes a 3-year-old regression which makes download feedback service only upload UNCOMMON downloads when user clicks "DISCARD". The correct behavior should be uploading all eligible downloads no matter user clicks KEEP or DISCARD on download shelf, where eligible means: 1) user is not incognito 2) user opted in for safe browsing extended reporting. 3) DownloadItemModel::ShouldAllowDownloadFeedback returns true BUG=655799 ========== to ========== This CL fixes a 3-year-old regression which makes download feedback service only upload UNCOMMON downloads when user clicks "DISCARD". The correct behavior should be uploading all eligible downloads no matter user clicks KEEP or DISCARD on download shelf, where eligible means: 1) user is not incognito 2) user opted in for safe browsing extended reporting. 3) DownloadItemModel::ShouldAllowDownloadFeedback returns true This change also change the logic of StealDangerousDownload(..), since if user chose to keep the download we cannot delete it after feedback is sent. In order to make download file immediately available to user without making them wait for upload complete, we make a temporary copy of the download file in DownloadItemImpl::StealDangerousDownload(..) function, and upload this copy via DownloadFeedbackService::BeginFeedbackOrDeleteFile. This temp copy will be deleted after feedback is sent (~DownloadFeedbackImpl()). BUG=655799 ==========
Description was changed from ========== This CL fixes a 3-year-old regression which makes download feedback service only upload UNCOMMON downloads when user clicks "DISCARD". The correct behavior should be uploading all eligible downloads no matter user clicks KEEP or DISCARD on download shelf, where eligible means: 1) user is not incognito 2) user opted in for safe browsing extended reporting. 3) DownloadItemModel::ShouldAllowDownloadFeedback returns true This change also change the logic of StealDangerousDownload(..), since if user chose to keep the download we cannot delete it after feedback is sent. In order to make download file immediately available to user without making them wait for upload complete, we make a temporary copy of the download file in DownloadItemImpl::StealDangerousDownload(..) function, and upload this copy via DownloadFeedbackService::BeginFeedbackOrDeleteFile. This temp copy will be deleted after feedback is sent (~DownloadFeedbackImpl()). BUG=655799 ========== to ========== This CL fixes a 3-year-old regression which makes download feedback service only upload UNCOMMON downloads when user clicks "DISCARD". The correct behavior should be uploading all eligible downloads no matter user clicks KEEP or DISCARD on download shelf, where eligible means: 1) user is not incognito 2) user opted in for safe browsing extended reporting. 3) DownloadItemModel::ShouldAllowDownloadFeedback returns true This CL also changes the logic of StealDangerousDownload(), since if user chose to keep the download we cannot delete it after feedback is sent. In order to make download file immediately available to user without making them wait for upload complete, we make a temporary copy of the download file in DownloadItemImpl:: StealDangerousDownload(..) function, and upload this copy via DownloadFeedbackService::BeginFeedbackOrDeleteFile. This temp copy will be deleted after feedback is sent (~DownloadFeedbackImpl()). BUG=655799 ==========
Hi asanka@, Could you take a look at this latest version? In this version, I changed the logic of StealDangerousDownload() to make a temporary copy of the download file (if user chose to keep uncommon download) and immediately validate the uncommon download. This copy will be uploaded via DownloadFeedbackService::BeginFeedbackOrDeleteFile, and will be deleted after feedback is sent in (DownloadFeedbackImpl::~DownloadFeedbackImpl()). Manually tested, seems no crash. Test bots seems happy too. Let me know if anything doesn't add up.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/10/27 at 23:27:37, jialiul wrote: > Hi asanka@, > Could you take a look at this latest version? > In this version, I changed the logic of StealDangerousDownload() to make a > temporary copy of the download file (if user chose to keep uncommon download) and immediately validate the uncommon download. This copy will be uploaded via > DownloadFeedbackService::BeginFeedbackOrDeleteFile, and will > be deleted after feedback is sent in > (DownloadFeedbackImpl::~DownloadFeedbackImpl()). > > Manually tested, seems no crash. Test bots seems happy too. > Let me know if anything doesn't add up. Sorry about the delay. I'll have a look at this later today.
Overall LG. What's the largest file size that's supported by the feedback service? If the download is larger than that size, what should we do? Try to do a partial upload or skip the upload? https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/downloa... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/downloa... chrome/browser/download/download_browsertest.cc:3411: download->Cancel(true); Should also test that the target file exists. You can accept the download and wait for it to complete. At that point, the download file should still exist. https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_shelf_context_menu_view.cc (right): https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_shelf_context_menu_view.cc:62: DownloadCommands::KEEP) { Can you add a DCHECK here to make sure command_id is never DISCARD? https://codereview.chromium.org/2439533002/diff/210001/content/browser/downlo... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2439533002/diff/210001/content/browser/downlo... content/browser/download/download_item_impl.cc:317: DCHECK(IsDangerous()); Can you also DCHECK AllDataSaved() ? It appears that the logic below will only have the desired effect if AllDataSaved() is true. I.e. the file is complete. https://codereview.chromium.org/2439533002/diff/210001/content/browser/downlo... content/browser/download/download_item_impl.cc:332: } else if (download_file_) { If there's no download_file_, then you can callback.Run(current_path_). Otherwise that branch will cause the callback to never be run. All branches must invoke |callback| at some point. Otherwise the download will stall.
Do you still want me to review anything in this latest version?
https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_feedback_service.cc (right): https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_feedback_service.cc:172: if (path.empty()) We should probably measure this. Not a big deal if you don't, but if the failure rate is high, then we'd need to revisit our strategy. https://codereview.chromium.org/2439533002/diff/210001/content/browser/downlo... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2439533002/diff/210001/content/browser/downlo... content/browser/download/download_item_impl.cc:104: return base::FilePath(); Can this branch delete the file at |temp_file_path| if |temp_file_path| contains a valid path? base::CopyFile may leave a partial file around if it fails part-way. Leaving those files around can be harmful in some cases. E.g. if CopyFile failed because the temporary file location ran out of space lots of things will go wrong quickly unless we free up the space again. Also FYI the download file and the temp file may end up on different storage devices and may be subject to different size restrictions.
On 2016/11/01 21:01:09, Peter Kasting wrote: > Do you still want me to review anything in this latest version? Yes :-) Thanks, pkasting@!
On 2016/11/01 20:58:58, asanka wrote: > Overall LG. > > What's the largest file size that's supported by the feedback service? If the > download is larger than that size, what should we do? Try to do a partial upload > or skip the upload? Feedback service only tries to upload file if it is smaller than 50M. Download larger than this size will be skipped. This is gated in ShouldAllowDownloadFeedback(). I'll address all the other comments tomorrow. > > https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/downloa... > File chrome/browser/download/download_browsertest.cc (right): > > https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/downloa... > chrome/browser/download/download_browsertest.cc:3411: download->Cancel(true); > Should also test that the target file exists. You can accept the download and > wait for it to complete. At that point, the download file should still exist. > > https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... > File chrome/browser/ui/views/download/download_shelf_context_menu_view.cc > (right): > > https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... > chrome/browser/ui/views/download/download_shelf_context_menu_view.cc:62: > DownloadCommands::KEEP) { > Can you add a DCHECK here to make sure command_id is never DISCARD? > > https://codereview.chromium.org/2439533002/diff/210001/content/browser/downlo... > File content/browser/download/download_item_impl.cc (right): > > https://codereview.chromium.org/2439533002/diff/210001/content/browser/downlo... > content/browser/download/download_item_impl.cc:317: DCHECK(IsDangerous()); > Can you also DCHECK AllDataSaved() ? > > It appears that the logic below will only have the desired effect if > AllDataSaved() is true. I.e. the file is complete. > > https://codereview.chromium.org/2439533002/diff/210001/content/browser/downlo... > content/browser/download/download_item_impl.cc:332: } else if (download_file_) { > If there's no download_file_, then you can callback.Run(current_path_). > Otherwise that branch will cause the callback to never be run. All branches must > invoke |callback| at some point. Otherwise the download will stall.
LGTM, but I have concerns, which might be better addressed in another CL. https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:595: // WARNING: all end states after this point delete |this|. I think this lies; when MaybeSubmit...() calls DownloadFeedbackDialogView::Show(), that doesn't delete |this|, does it? In either case I'd move this to be at the end of the function like the comment in SubmitDownloadWhenFeedbackServiceEnabled(), but be careful that the wording is accurate. https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.h (right): https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.h:88: // Submit download to download feedback service if the user has approved and Nit: Verbs in function comments should be descriptive ("Submits") rather than imperative ("Submit"); see http://google.github.io/styleguide/cppguide.html#Function_Comments . (Several places) https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.h:90: // If user hasn't seen SBER opt-in text before, show SBER opt-in dialog first. Does this function delete |this|? If so it should probably say so. Note that I don't think this does this in all cases, see comments in .cc file. https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.h:153: // |download_command|, and |this| will have been deleted. This comment doesn't seem to accurately describe what the code does. It looks like the code attempts to submit the download for feedback, and if that fails, it applies the download command. I think the code and comments here are all more confusing than they need to be because of the way that trying to submit the download for feedback "takes over" the download command. If submitting the download for feedback is a purely async process that makes a temporary copy of the downloaded file (only when it's small enough for submission) and always deletes that temporary copy when it's done, then basically feedback submission becomes decoupled from handling the download completion at all. Is that change feasible to make? Because it would avoid a lot of hassling about precise comments and architecture for achieving the current design.
The CQ bit was checked by jialiul@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 jialiul@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...
jialiul@chromium.org changed reviewers: + noyau@chromium.org
Thanks for your comments, asanka@ and pkasting@! Let me know if you have further comments. Meanwhile, add other reviewers for owner review: noyau@ for chrome/browser/ntp_snippets/* holte@ for uma metric nick@ for content/public/* Thanks! https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/downloa... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/downloa... chrome/browser/download/download_browsertest.cc:3411: download->Cancel(true); On 2016/11/01 20:58:58, asanka wrote: > Should also test that the target file exists. You can accept the download and > wait for it to complete. At that point, the download file should still exist. Done. https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_feedback_service.cc (right): https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_feedback_service.cc:172: if (path.empty()) On 2016/11/01 21:09:23, asanka wrote: > We should probably measure this. Not a big deal if you don't, but if the failure > rate is high, then we'd need to revisit our strategy. Done. https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:595: // WARNING: all end states after this point delete |this|. On 2016/11/01 23:48:32, Peter Kasting wrote: > I think this lies; when MaybeSubmit...() calls > DownloadFeedbackDialogView::Show(), that doesn't delete |this|, does it? > > In either case I'd move this to be at the end of the function like the comment > in SubmitDownloadWhenFeedbackServiceEnabled(), but be careful that the wording > is accurate. Done. https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.h (right): https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.h:88: // Submit download to download feedback service if the user has approved and On 2016/11/01 23:48:32, Peter Kasting wrote: > Nit: Verbs in function comments should be descriptive ("Submits") rather than > imperative ("Submit"); see > http://google.github.io/styleguide/cppguide.html#Function_Comments . (Several > places) Done. https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.h:90: // If user hasn't seen SBER opt-in text before, show SBER opt-in dialog first. On 2016/11/01 23:48:32, Peter Kasting wrote: > Does this function delete |this|? If so it should probably say so. Note that I > don't think this does this in all cases, see comments in .cc file. Agree, I don't think it deletes |this|. Will fix comment in .cc file. https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.h:153: // |download_command|, and |this| will have been deleted. On 2016/11/01 23:48:32, Peter Kasting wrote: > This comment doesn't seem to accurately describe what the code does. It looks > like the code attempts to submit the download for feedback, and if that fails, > it applies the download command. > > I think the code and comments here are all more confusing than they need to be > because of the way that trying to submit the download for feedback "takes over" > the download command. If submitting the download for feedback is a purely async > process that makes a temporary copy of the downloaded file (only when it's small > enough for submission) and always deletes that temporary copy when it's done, > then basically feedback submission becomes decoupled from handling the download > completion at all. > > Is that change feasible to make? Because it would avoid a lot of hassling about > precise comments and architecture for achieving the current design. I agree. If the "make a temp copy" strategy works out, it is much cleaner to use it for both DISCARD and KEEP. However, at this point, I am leaning towards only applying this strategy on "KEEP" (7% of all download feedback cases). Since a number of things can go wrong with the temp copy, e.g. temporary file location runs out of space, download file and the temp file may end up on different storage devices and may be subject to different size restrictions, etc. I added a UMA metrics to track the failing rate in this CL too (per asanka@'s request). So if this strategy looks fine in M56, I'll do a refactor to use temp copy for both KEEP and DISCARD. https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_shelf_context_menu_view.cc (right): https://codereview.chromium.org/2439533002/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_shelf_context_menu_view.cc:62: DownloadCommands::KEEP) { On 2016/11/01 20:58:58, asanka wrote: > Can you add a DCHECK here to make sure command_id is never DISCARD? Done. https://codereview.chromium.org/2439533002/diff/210001/content/browser/downlo... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2439533002/diff/210001/content/browser/downlo... content/browser/download/download_item_impl.cc:104: return base::FilePath(); On 2016/11/01 21:09:23, asanka wrote: > Can this branch delete the file at |temp_file_path| if |temp_file_path| contains > a valid path? > > base::CopyFile may leave a partial file around if it fails part-way. Leaving > those files around can be harmful in some cases. E.g. if CopyFile failed because > the temporary file location ran out of space lots of things will go wrong > quickly unless we free up the space again. > > Also FYI the download file and the temp file may end up on different storage > devices and may be subject to different size restrictions. Done. https://codereview.chromium.org/2439533002/diff/210001/content/browser/downlo... content/browser/download/download_item_impl.cc:317: DCHECK(IsDangerous()); On 2016/11/01 20:58:58, asanka wrote: > Can you also DCHECK AllDataSaved() ? > > It appears that the logic below will only have the desired effect if > AllDataSaved() is true. I.e. the file is complete. Done. https://codereview.chromium.org/2439533002/diff/210001/content/browser/downlo... content/browser/download/download_item_impl.cc:332: } else if (download_file_) { On 2016/11/01 20:58:58, asanka wrote: > If there's no download_file_, then you can callback.Run(current_path_). > Otherwise that branch will cause the callback to never be run. All branches must > invoke |callback| at some point. Otherwise the download will stall. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by jialiul@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...
Patchset #14 (id:230002) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
LGTM. You can treat the comments as nits or stuff for followups as appropriate. Thanks! https://codereview.chromium.org/2439533002/diff/250001/chrome/browser/downloa... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/2439533002/diff/250001/chrome/browser/downloa... chrome/browser/download/download_browsertest.cc:3425: ASSERT_FALSE(updated_downloads.empty()); Should assert that there's exactly one download. Otherwise the property assertions that follow won't be reliable. https://codereview.chromium.org/2439533002/diff/250001/chrome/browser/downloa... chrome/browser/download/download_browsertest.cc:3428: } I'm okay with the tests as is. But ideally we'd test against a throttled upload server and verify that we can safely delete the file at download->GetFullPath() without the upload process getting interrupted. :-) I don't know if the reliability of the upload mechanism is tested elsewhere, but its resilience against the download target file getting removed is a new requirement. Perhaps we should add such tests as followups? I'm worried that we can regress on this aspect without any tests to catch it. https://codereview.chromium.org/2439533002/diff/250001/content/browser/downlo... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2439533002/diff/250001/content/browser/downlo... content/browser/download/download_item_impl.cc:103: } } else { clang-format should've fixed this for you. https://codereview.chromium.org/2439533002/diff/250001/content/browser/downlo... content/browser/download/download_item_impl.cc:106: if (base::PathExists(temp_file_path)) { DeleteFile is a no-op if the file doesn't exist and we don't check the return code. The only thing we need to check prior to calling DeleteFile() is whether the |temp_file_path| is a directory because DeleteFile() works on both files and directories. So what you'd want is something like: if (!base::DirectoryExists(temp_file_path)) base::DeleteFile(temp_file_path); Incidentally, this logic already exists in DeleteDownloadedFile(). So you can give it a more generic name and call it instead. I'm okay if you do the check here as well, since it's just two lines. https://codereview.chromium.org/2439533002/diff/250001/content/public/browser... File content/public/browser/download_item.h (right): https://codereview.chromium.org/2439533002/diff/250001/content/public/browser... content/public/browser/download_item.h:123: // cleanup. Nit: I suggest changing 'X will be <verb>d' to '<verb>s X' and '<verb> X' to also be '<verb>s X'.
The CQ bit was checked by jialiul@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...
jialiul@chromium.org changed reviewers: - noyau@chromium.org
Thanks for your comments asanka@. And sorry for my late reply. I was OOO last week. I agree that we probably need more tests to check the reliability of the new uploading mechanism. I will add more tests together with other refactoring in a later CL (issue 665100). I may pick your brain on some details later. Thanks! https://codereview.chromium.org/2439533002/diff/250001/chrome/browser/downloa... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/2439533002/diff/250001/chrome/browser/downloa... chrome/browser/download/download_browsertest.cc:3425: ASSERT_FALSE(updated_downloads.empty()); On 2016/11/07 16:01:55, asanka wrote: > Should assert that there's exactly one download. Otherwise the property > assertions that follow won't be reliable. Done. https://codereview.chromium.org/2439533002/diff/250001/chrome/browser/downloa... chrome/browser/download/download_browsertest.cc:3428: } On 2016/11/07 16:01:54, asanka wrote: > I'm okay with the tests as is. But ideally we'd test against a throttled upload > server and verify that we can safely delete the file at download->GetFullPath() > without the upload process getting interrupted. :-) > > I don't know if the reliability of the upload mechanism is tested elsewhere, but > its resilience against the download target file getting removed is a new > requirement. Perhaps we should add such tests as followups? I'm worried that we > can regress on this aspect without any tests to catch it. acknowledged. I agree. We need more tests to verify the reliability. Since I'm trying to get this change in M56 (branch on Nov 17), I'll probably defer this to a later CL. And also as pkasting@ suggested, if this temp file solution works out fine, we can also use this mechanism for discarded downloads, thus unify the logic of handling file uploading. I created crbug.com/665100 as a reminder of these two follow-ups. https://codereview.chromium.org/2439533002/diff/250001/content/browser/downlo... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2439533002/diff/250001/content/browser/downlo... content/browser/download/download_item_impl.cc:103: } On 2016/11/07 16:01:55, asanka wrote: > } else { > > clang-format should've fixed this for you. Done. https://codereview.chromium.org/2439533002/diff/250001/content/browser/downlo... content/browser/download/download_item_impl.cc:106: if (base::PathExists(temp_file_path)) { On 2016/11/07 16:01:55, asanka wrote: > DeleteFile is a no-op if the file doesn't exist and we don't check the return > code. The only thing we need to check prior to calling DeleteFile() is whether > the |temp_file_path| is a directory because DeleteFile() works on both files and > directories. > > So what you'd want is something like: > > if (!base::DirectoryExists(temp_file_path)) > base::DeleteFile(temp_file_path); > > Incidentally, this logic already exists in DeleteDownloadedFile(). So you can > give it a more generic name and call it instead. I'm okay if you do the check > here as well, since it's just two lines. Done. https://codereview.chromium.org/2439533002/diff/250001/content/public/browser... File content/public/browser/download_item.h (right): https://codereview.chromium.org/2439533002/diff/250001/content/public/browser... content/public/browser/download_item.h:123: // cleanup. On 2016/11/07 16:01:55, asanka wrote: > Nit: I suggest changing 'X will be <verb>d' to '<verb>s X' and '<verb> X' to > also be '<verb>s X'. Done.
jialiul@chromium.org changed reviewers: + holte@chromium.org, nick@semenkovich.com, treib@chromium.org
Add other reviewers: treib for chrome/browser/ntp_snippets/* holte@ for uma metric nick@ for content/public/* Please feel free to re-assign if you're not available to review. I am targeting M56.
jialiul@chromium.org changed reviewers: - nick@semenkovich.com
jialiul@chromium.org changed reviewers: + nick@chromium.org
nick@chromium.org: Please review changes in content/public/*
content/public lgtm
https://codereview.chromium.org/2439533002/diff/270001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_feedback_service.cc (right): https://codereview.chromium.org/2439533002/diff/270001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_feedback_service.cc:173: UMA_HISTOGRAM_BOOLEAN("SBDownloadFeedback.EmptyFilePathFailure", true); It would be better to consolidate these into single macro invocation, since each creates a static variable.
Thanks, holte@! UMA macro invocation consolidated. https://codereview.chromium.org/2439533002/diff/270001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_feedback_service.cc (right): https://codereview.chromium.org/2439533002/diff/270001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_feedback_service.cc:173: UMA_HISTOGRAM_BOOLEAN("SBDownloadFeedback.EmptyFilePathFailure", true); On 2016/11/14 21:34:34, Steven Holte wrote: > It would be better to consolidate these into single macro invocation, since each > creates a static variable. Done.
On 2016/11/14 21:55:07, Jialiu Lin wrote: > Thanks, holte@! UMA macro invocation consolidated. > > https://codereview.chromium.org/2439533002/diff/270001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/download_feedback_service.cc (right): > > https://codereview.chromium.org/2439533002/diff/270001/chrome/browser/safe_br... > chrome/browser/safe_browsing/download_feedback_service.cc:173: > UMA_HISTOGRAM_BOOLEAN("SBDownloadFeedback.EmptyFilePathFailure", true); > On 2016/11/14 21:34:34, Steven Holte wrote: > > It would be better to consolidate these into single macro invocation, since > each > > creates a static variable. > Done. lgtm
c/b/ntp_snippets/ lgtm
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, asanka@chromium.org, nick@chromium.org Link to the patchset: https://codereview.chromium.org/2439533002/#ps290001 (title: "consolidate UMA invocation")
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jialiul@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 jialiul@chromium.org to run a CQ dry run
Patchset #17 (id:310001) has been deleted
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 jialiul@chromium.org
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, treib@chromium.org, nick@chromium.org, pkasting@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2439533002/#ps330001 (title: "make windows bots happy")
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by jialiul@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by jialiul@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.
Description was changed from ========== This CL fixes a 3-year-old regression which makes download feedback service only upload UNCOMMON downloads when user clicks "DISCARD". The correct behavior should be uploading all eligible downloads no matter user clicks KEEP or DISCARD on download shelf, where eligible means: 1) user is not incognito 2) user opted in for safe browsing extended reporting. 3) DownloadItemModel::ShouldAllowDownloadFeedback returns true This CL also changes the logic of StealDangerousDownload(), since if user chose to keep the download we cannot delete it after feedback is sent. In order to make download file immediately available to user without making them wait for upload complete, we make a temporary copy of the download file in DownloadItemImpl:: StealDangerousDownload(..) function, and upload this copy via DownloadFeedbackService::BeginFeedbackOrDeleteFile. This temp copy will be deleted after feedback is sent (~DownloadFeedbackImpl()). BUG=655799 ========== to ========== This CL fixes a 3-year-old regression which makes download feedback service only upload UNCOMMON downloads when user clicks "DISCARD". The correct behavior should be uploading all eligible downloads no matter user clicks KEEP or DISCARD on download shelf, where eligible means: 1) user is not incognito 2) user opted in for safe browsing extended reporting. 3) DownloadItemModel::ShouldAllowDownloadFeedback returns true This CL also changes the logic of StealDangerousDownload(), since if user chose to keep the download we cannot delete it after feedback is sent. In order to make download file immediately available to user without making them wait for upload complete, we make a temporary copy of the download file in DownloadItemImpl:: StealDangerousDownload(..) function, and upload this copy via DownloadFeedbackService::BeginFeedbackOrDeleteFile. This temp copy will be deleted after feedback is sent (~DownloadFeedbackImpl()). BUG=655799 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:330001)
Message was sent while issue was closed.
Description was changed from ========== This CL fixes a 3-year-old regression which makes download feedback service only upload UNCOMMON downloads when user clicks "DISCARD". The correct behavior should be uploading all eligible downloads no matter user clicks KEEP or DISCARD on download shelf, where eligible means: 1) user is not incognito 2) user opted in for safe browsing extended reporting. 3) DownloadItemModel::ShouldAllowDownloadFeedback returns true This CL also changes the logic of StealDangerousDownload(), since if user chose to keep the download we cannot delete it after feedback is sent. In order to make download file immediately available to user without making them wait for upload complete, we make a temporary copy of the download file in DownloadItemImpl:: StealDangerousDownload(..) function, and upload this copy via DownloadFeedbackService::BeginFeedbackOrDeleteFile. This temp copy will be deleted after feedback is sent (~DownloadFeedbackImpl()). BUG=655799 ========== to ========== This CL fixes a 3-year-old regression which makes download feedback service only upload UNCOMMON downloads when user clicks "DISCARD". The correct behavior should be uploading all eligible downloads no matter user clicks KEEP or DISCARD on download shelf, where eligible means: 1) user is not incognito 2) user opted in for safe browsing extended reporting. 3) DownloadItemModel::ShouldAllowDownloadFeedback returns true This CL also changes the logic of StealDangerousDownload(), since if user chose to keep the download we cannot delete it after feedback is sent. In order to make download file immediately available to user without making them wait for upload complete, we make a temporary copy of the download file in DownloadItemImpl:: StealDangerousDownload(..) function, and upload this copy via DownloadFeedbackService::BeginFeedbackOrDeleteFile. This temp copy will be deleted after feedback is sent (~DownloadFeedbackImpl()). BUG=655799 Committed: https://crrev.com/2fe67a026c44af1daf09616c2b7bda2db5abee12 Cr-Commit-Position: refs/heads/master@{#432528} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/2fe67a026c44af1daf09616c2b7bda2db5abee12 Cr-Commit-Position: refs/heads/master@{#432528} |
