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

Issue 16018005: Use DownloadItem::GetState() in chrome/ (Closed)

Created:
7 years, 6 months ago by cmarcelo
Modified:
7 years, 6 months ago
CC:
chromium-reviews, Randy Smith (Not in Mondays), nkostylev+watch_chromium.org, benjhayden+dwatch_chromium.org, tfarina, kkania, sail+watch_chromium.org, robertshield, dcheng, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Use DownloadItem::GetState() in chrome/ 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 the remaining cases inside chrome/. Changing content/ and DownloadItem itself 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=206076

Patch Set 1 #

Total comments: 5

Patch Set 2 : Update after asanka comments #

Patch Set 3 : Remove assumption in download_item_view #

Patch Set 4 : Rebased version #

Patch Set 5 : Stopping complete_animation_ when download is cancelled #

Unified diffs Side-by-side diffs Delta from patch set Stats (+109 lines, -98 lines) Patch
M chrome/browser/automation/testing_automation_provider.cc View 1 2 3 2 chunks +13 lines, -18 lines 0 comments Download
M chrome/browser/chromeos/drive/download_handler.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/cocoa/download/download_item_cell_unittest.mm View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/ui/cocoa/download/download_item_controller.mm View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/cocoa/download/download_shelf_controller.mm View 2 chunks +6 lines, -4 lines 0 comments Download
M chrome/browser/ui/cocoa/download/download_shelf_controller_unittest.mm View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/cocoa/download/download_util_mac.mm View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/gtk/custom_drag.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/gtk/download/download_item_gtk.cc View 1 2 3 3 chunks +5 lines, -4 lines 0 comments Download
M chrome/browser/ui/gtk/download/download_shelf_gtk.cc View 2 chunks +5 lines, -4 lines 0 comments Download
M chrome/browser/ui/views/download/download_item_view.cc View 1 2 3 4 3 chunks +8 lines, -5 lines 0 comments Download
M chrome/browser/ui/views/download/download_shelf_view.cc View 1 2 3 2 chunks +5 lines, -4 lines 0 comments Download
M chrome/browser/ui/webui/downloads_dom_handler.cc View 1 2 3 1 chunk +61 lines, -52 lines 0 comments Download

Messages

Total messages: 23 (0 generated)
cmarcelo
Could you take a look at this? phajdan.jr: c/b/automation, main reviewer sky: c/b/ui hashimoto: c/b/chromeos/drive
7 years, 6 months ago (2013-05-27 21:15:18 UTC) #1
hashimoto
chrome/browser/chromeos/drive/download_handler.cc lgtm
7 years, 6 months ago (2013-05-28 00:29:06 UTC) #2
sky
LGTM
7 years, 6 months ago (2013-05-28 15:19:25 UTC) #3
Paweł Hajdan Jr.
Randy, at this point you're more familiar with the plans for the downloads: could you ...
7 years, 6 months ago (2013-05-28 16:55:58 UTC) #4
Randy Smith (Not in Mondays)
So I'm personally in favor of this change, but I believe it should be reviewed ...
7 years, 6 months ago (2013-05-28 17:00:39 UTC) #5
cmarcelo
On 2013/05/28 16:55:58, Paweł Hajdan Jr. wrote: > If you know it's problematic, I think ...
7 years, 6 months ago (2013-05-28 17:40:00 UTC) #6
asanka
https://codereview.chromium.org/16018005/diff/1/chrome/browser/automation/testing_automation_provider.cc File chrome/browser/automation/testing_automation_provider.cc (right): https://codereview.chromium.org/16018005/diff/1/chrome/browser/automation/testing_automation_provider.cc#newcode2624 chrome/browser/automation/testing_automation_provider.cc:2624: download_state != DownloadItem::IN_PROGRESS) { This check is necessary for ...
7 years, 6 months ago (2013-05-28 18:43:14 UTC) #7
cmarcelo
Updated. asanka, I opted to get rid of the second checks and early returning. Could ...
7 years, 6 months ago (2013-05-28 19:50:53 UTC) #8
asanka
LGTM
7 years, 6 months ago (2013-05-28 20:42:40 UTC) #9
cmarcelo
phajdan.jr, does asanka's review covers yours?
7 years, 6 months ago (2013-05-28 22:16:39 UTC) #10
Paweł Hajdan Jr.
On 2013/05/28 22:16:39, cmarcelo wrote: > phajdan.jr, does asanka's review covers yours? Yes, removing myself ...
7 years, 6 months ago (2013-05-28 22:57:37 UTC) #11
cmarcelo
win_rel bot suggests that the assumption about completing_animation_ may be incorrect in the case where ...
7 years, 6 months ago (2013-05-29 10:18:05 UTC) #12
cmarcelo
asanka, could you take a look? Based on bot results, our assumption that complete_animation_ would ...
7 years, 6 months ago (2013-06-06 13:17:22 UTC) #13
asanka
On 2013/06/06 13:17:22, cmarcelo wrote: > asanka, could you take a look? > > Based ...
7 years, 6 months ago (2013-06-11 20:38:59 UTC) #14
cmarcelo
Thanks for looking. On 2013/06/11 20:38:59, asanka wrote: > I think the correct behavior is ...
7 years, 6 months ago (2013-06-11 20:46:12 UTC) #15
asanka
On 2013/06/11 20:46:12, cmarcelo wrote: > Thanks for looking. > > On 2013/06/11 20:38:59, asanka ...
7 years, 6 months ago (2013-06-11 21:03:57 UTC) #16
asanka
LGTM
7 years, 6 months ago (2013-06-12 17:39:06 UTC) #17
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/16018005/50001
7 years, 6 months ago (2013-06-12 19:49:19 UTC) #18
commit-bot: I haz the power
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=124833
7 years, 6 months ago (2013-06-12 22:41:53 UTC) #19
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/16018005/50001
7 years, 6 months ago (2013-06-13 03:59:44 UTC) #20
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 6 months ago (2013-06-13 06:37:45 UTC) #21
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/16018005/50001
7 years, 6 months ago (2013-06-13 13:33:42 UTC) #22
commit-bot: I haz the power
7 years, 6 months ago (2013-06-13 15:57:12 UTC) #23
Message was sent while issue was closed.
Change committed as 206076

Powered by Google App Engine
This is Rietveld 408576698