Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(857)

Issue 15880002: Use DownloadItem::GetState() in c/b/{download,extensions} (Closed)

Created:
7 years, 7 months ago by cmarcelo
Modified:
7 years, 7 months ago
Reviewers:
benjhayden, cmarcelo
CC:
chromium-reviews, benjhayden+dwatch_chromium.org, asanka, chromium-apps-reviews_chromium.org, Aaron Boodman
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Use DownloadItem::GetState() in c/b/{download,extensions} Stop using DownloadItem::Is*() functions that have an equivalent state exposed via GetState(). Rationale is that there shouldn't be multiple ways of getting the same state information. To make review easier, this patch covers chrome/browser/download and chrome/browser/extensions. The remaining cases and changing the DownloadItem to remove the Is*() functions will come later. BUG=241141 TEST=browser_tests and unit_tests both with *Download*:*SavePage* filters Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202229

Patch Set 1 #

Patch Set 2 : Fix download_ui_controller_unittest.cc #

Unified diffs Side-by-side diffs Delta from patch set Stats (+84 lines, -100 lines) Patch
M chrome/browser/download/chrome_download_manager_delegate.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/download/chrome_download_manager_delegate_unittest.cc View 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/download/download_browsertest.cc View 4 chunks +7 lines, -7 lines 0 comments Download
M chrome/browser/download/download_danger_prompt.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/download/download_danger_prompt_browsertest.cc View 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/browser/download/download_item_model.cc View 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/download/download_item_model_unittest.cc View 3 chunks +0 lines, -12 lines 0 comments Download
M chrome/browser/download/download_shelf.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/download/download_shelf_context_menu.cc View 1 3 chunks +5 lines, -4 lines 0 comments Download
M chrome/browser/download/download_shelf_unittest.cc View 3 chunks +6 lines, -6 lines 0 comments Download
M chrome/browser/download/download_status_updater.cc View 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/download/download_status_updater_unittest.cc View 2 chunks +6 lines, -4 lines 0 comments Download
M chrome/browser/download/download_target_determiner.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/download/download_target_determiner_unittest.cc View 2 chunks +2 lines, -4 lines 0 comments Download
M chrome/browser/download/download_ui_controller.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/download/download_ui_controller_unittest.cc View 1 3 chunks +6 lines, -15 lines 0 comments Download
M chrome/browser/download/download_util.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/download/save_page_browsertest.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/api/downloads/downloads_api.cc View 1 3 chunks +5 lines, -3 lines 0 comments Download
M chrome/browser/extensions/api/downloads/downloads_api_unittest.cc View 24 chunks +27 lines, -27 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
cmarcelo
7 years, 7 months ago (2013-05-23 14:02:23 UTC) #1
benjhayden
LGTM Thank you very much for your help!
7 years, 7 months ago (2013-05-23 17:43:21 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caio.de.oliveira.filho@intel.com/15880002/1
7 years, 7 months ago (2013-05-23 18:04:38 UTC) #3
cmarcelo
Fixing the problem with expectations identified by the try bots. Will upload a new patch ...
7 years, 7 months ago (2013-05-23 19:06:16 UTC) #4
cmarcelo
benjhayden, could you check the updated unittest? (Note that changes in the other two files ...
7 years, 7 months ago (2013-05-23 19:25:18 UTC) #5
benjhayden
lgtm
7 years, 7 months ago (2013-05-23 19:29:23 UTC) #6
cmarcelo
Given the greens I'll try CQ again.
7 years, 7 months ago (2013-05-24 14:50:32 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caio.de.oliveira.filho@intel.com/15880002/24001
7 years, 7 months ago (2013-05-24 14:50:46 UTC) #8
commit-bot: I haz the power
7 years, 7 months ago (2013-05-24 23:41:17 UTC) #9
Message was sent while issue was closed.
Change committed as 202229

Powered by Google App Engine
This is Rietveld 408576698