New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 721964 link

Starred by 1 user
Status: Assigned
Owner:
drubery@chromium.org
Cc:
█ jialiul@chromium.org
vakh@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Refactor DownloadProtectionService, for better testing

Project Member Reported by nparker@chromium.org, May 13 2017 
download_protection_service.cc is monolithic and has class defined within it that we can't test easily. And it's hard to navigate since multiple classes within it have identical method names. Pull it apart into multiple files with classes in .h files, and add tests for functions within it.

It could go in chrome/browser/safe_browsing/download_protection/.

Comment 1 by vakh@chromium.org, May 19 2017

Labels: SafeBrowsing-Future-Projects OS-Linux OS-Mac OS-Windows
Owner: jialiul@chromium.org
Status: Assigned (was: Untriaged)
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 16 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5675f90259e71b69a2cfb1d9042a48b1b530be08

commit 5675f90259e71b69a2cfb1d9042a48b1b530be08
Author: Micah Morton <mortonm@google.com>
Date: Wed Aug 16 22:35:19 2017

Refactor DownloadProtectionService for modularity.

Pull the subclasses within DownloadProtectionService out into their own
files. Also, move common dependencies to download_protection_util.h.

Bug: 721964, 689520 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I73fb2479c6a168ad6e3be7c8f76b89e33c05d7ff
Reviewed-on: https://chromium-review.googlesource.com/583999
Commit-Queue: Micah Morton <mortonm@google.com>
Reviewed-by: Bill Budge <bbudge@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494982}
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/download/chrome_download_manager_delegate.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/download/chrome_download_manager_delegate.h
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/download/chrome_download_manager_delegate_unittest.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/download/download_browsertest.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/download/download_commands.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/download/download_danger_prompt.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/download/download_danger_prompt_browsertest.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/download/download_item_model.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/file_select_helper.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/BUILD.gn
[add] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/check_client_download_request.cc
[add] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/check_client_download_request.h
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/disk_image_type_sniffer_mac.cc
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/disk_image_type_sniffer_mac.h
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/disk_image_type_sniffer_mac_unittest.cc
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/download_feedback.cc
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/download_feedback.h
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/download_feedback_service.cc
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/download_feedback_service.h
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/download_feedback_service_unittest.cc
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/download_feedback_unittest.cc
[add] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/download_protection_service.cc
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/download_protection_service.h
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc
[add] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/download_protection_util.cc
[add] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/download_protection_util.h
[add] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/download_url_sb_client.cc
[add] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/download_url_sb_client.h
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/path_sanitizer.cc
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/path_sanitizer.h
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/path_sanitizer_unittest.cc
[add] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/ppapi_download_request.cc
[add] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/ppapi_download_request.h
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/sandboxed_dmg_analyzer_mac.cc
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/sandboxed_dmg_analyzer_mac.h
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/sandboxed_dmg_analyzer_mac_unittest.cc
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/sandboxed_zip_analyzer.cc
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/sandboxed_zip_analyzer.h
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/sandboxed_zip_analyzer_unittest.cc
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/two_phase_uploader.cc
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/two_phase_uploader.h
[rename] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/download_protection/two_phase_uploader_unittest.cc
[delete] https://crrev.com/8674c19bcc3997d76afba03341f882f3fd384107/chrome/browser/safe_browsing/download_protection_service.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/incident_reporting/blacklist_load_analyzer_win.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/incident_reporting/environment_data_collection_win.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/incident_reporting/environment_data_collection_win_unittest.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/local_database_manager.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/safe_browsing_service.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/services_delegate_impl.h
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/safe_browsing/test_safe_browsing_service.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/ui/cocoa/download/download_danger_prompt_impl.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/browser/ui/views/download/download_item_view.cc
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/test/BUILD.gn
[modify] https://crrev.com/5675f90259e71b69a2cfb1d9042a48b1b530be08/chrome/test/ppapi/ppapi_filechooser_browsertest.cc

Comment 3 by mortonm@google.com, Aug 18 2017

Status: Fixed (was: Assigned)

Comment 4 by jialiul@chromium.org, Aug 18 2017

Status: Assigned (was: Fixed)
Re-open this bug.
It is 80% done, but we still need to add more tests to c/b/download_protection/*

Comment 5 by jialiul@chromium.org, Aug 18 2017

Labels: SafeBrowsing-Triaged

Comment 6 by est...@chromium.org, Nov 10 2017

Labels: Hotlist-EnamelAndFriendsFixIt

Comment 7 by jialiul@chromium.org, Nov 28 2017

Labels: -Pri-3 Pri-2

Comment 8 by est...@chromium.org, Feb 18 2018

Labels: -Hotlist-EnamelAndFriendsFixIt

Comment 9 by nparker@chromium.org, Sep 24

Cc: jialiul@chromium.org
Owner: drubery@chromium.org
--> drubery who's already looking carefully at this code.

Comment 10 by nparker@chromium.org, Oct 26

Labels: -SafeBrowsing-Future-Projects
►

Sign in to add a comment