Chromium Code Reviews
Help | Chromium Project | Sign in
(9)

Issue 2631933002: Adding status info to DownloadUIItem and piping it through. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by Dmitry Titov
Modified:
1 month, 2 weeks ago
CC:
chromium-reviews, dewittj+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, agrieve+watch_chromium.org, dimich+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Adding status info to DownloadUIItem and piping it through. BUG=681269 Review-Url: https://codereview.chromium.org/2631933002 Cr-Commit-Position: refs/heads/master@{#449655} Committed: https://chromium.googlesource.com/chromium/src/+/9d507add10786ba9b0e6e4291ffe75940d4a9846

Patch Set 1 #

Total comments: 10

Patch Set 2 : feedback, test compile #

Total comments: 37

Patch Set 3 : completed adapter and added tests #

Patch Set 4 : fix dependency for tests #

Total comments: 16

Patch Set 5 : latest comments reflected #

Patch Set 6 : fix junit test #

Patch Set 7 : fix chrome_test_java #

Total comments: 2

Patch Set 8 : fix comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+552 lines, -110 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java View 1 2 1 chunk +9 lines, -6 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java View 1 2 3 4 5 6 7 2 chunks +22 lines, -2 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/download/ui/StubbedProvider.java View 1 2 3 4 5 6 1 chunk +9 lines, -4 lines 0 comments Download
M chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridgeTest.java View 1 2 3 4 5 6 chunks +16 lines, -8 lines 0 comments Download
M chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc View 1 2 3 4 4 chunks +49 lines, -5 lines 0 comments Download
M components/offline_pages/core/BUILD.gn View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M components/offline_pages/core/downloads/BUILD.gn View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M components/offline_pages/core/downloads/download_ui_adapter.h View 1 2 3 4 5 chunks +67 lines, -11 lines 0 comments Download
M components/offline_pages/core/downloads/download_ui_adapter.cc View 1 2 7 chunks +156 lines, -46 lines 0 comments Download
M components/offline_pages/core/downloads/download_ui_adapter_unittest.cc View 1 2 3 4 10 chunks +191 lines, -25 lines 0 comments Download
M components/offline_pages/core/downloads/download_ui_item.h View 1 2 3 4 1 chunk +11 lines, -1 line 0 comments Download
M components/offline_pages/core/downloads/download_ui_item.cc View 1 2 2 chunks +20 lines, -2 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 67 (41 generated)
Dmitry Titov
This is WIP, still needs tests. Justin: could you look and see if it works ...
2 months, 1 week ago (2017-01-14 02:24:35 UTC) #4
Pete Williamson
Drive by: a few nits, looks fine otherwise. I see you adding a way to ...
2 months, 1 week ago (2017-01-17 18:27:34 UTC) #8
dewittj
first comments, more will come https://codereview.chromium.org/2631933002/diff/1/components/offline_pages/core/downloads/download_ui_adapter.cc File components/offline_pages/core/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2631933002/diff/1/components/offline_pages/core/downloads/download_ui_adapter.cc#newcode103 components/offline_pages/core/downloads/download_ui_adapter.cc:103: void DownloadUIAdapter::OnCompleted(const SavePageRequest& request, ...
2 months, 1 week ago (2017-01-18 18:49:48 UTC) #13
dewittj
idea looks good! Waiting for tests :) https://codereview.chromium.org/2631933002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java (right): https://codereview.chromium.org/2631933002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java#newcode56 chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:56: public long ...
2 months, 1 week ago (2017-01-18 19:24:07 UTC) #14
wibblymat
I think I was added to this review by mistake, using the alias wibblymat. 99.9% ...
2 months ago (2017-01-20 01:36:15 UTC) #15
Dmitry Titov
+Dan to look from DownloadHome integration side.
2 months ago (2017-01-20 06:38:37 UTC) #17
fgorski
drive by https://codereview.chromium.org/2631933002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java (right): https://codereview.chromium.org/2631933002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java#newcode9 chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:9: /** Download State, in sync with DownloadUIItem::DownloadState ...
2 months ago (2017-01-20 17:03:02 UTC) #19
dfalcantara (load balance plz)
Looks alright so far; guessing the part that fires off the updates from the offline ...
2 months ago (2017-01-20 18:40:02 UTC) #20
Dmitry Titov
Updated, added tests. PTAL. https://codereview.chromium.org/2631933002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java (right): https://codereview.chromium.org/2631933002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java#newcode9 chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:9: /** Download State, in sync ...
1 month, 4 weeks ago (2017-01-27 04:26:25 UTC) #27
Dmitry Titov
more. Still PTAL. https://codereview.chromium.org/2631933002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java (right): https://codereview.chromium.org/2631933002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java#newcode56 chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:56: public long downloadPreogress() { On 2017/01/17 ...
1 month, 4 weeks ago (2017-01-27 04:30:23 UTC) #28
dewittj
lgtm, thanks! https://codereview.chromium.org/2631933002/diff/60001/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2631933002/diff/60001/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc#newcode476 chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:476: OfflinePageModelFactory::GetForBrowserContext(browser_context); do we need to |nullptr|-check here ...
1 month, 3 weeks ago (2017-01-30 20:59:34 UTC) #31
dewittj
some more nits: * Could use a better CL description. * Can we rename |guid| ...
1 month, 3 weeks ago (2017-01-30 21:33:32 UTC) #32
fgorski
lgtm with nits. https://codereview.chromium.org/2631933002/diff/60001/components/offline_pages/core/downloads/download_ui_adapter.h File components/offline_pages/core/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2631933002/diff/60001/components/offline_pages/core/downloads/download_ui_adapter.h#newcode49 components/offline_pages/core/downloads/download_ui_adapter.h:49: // DownloadUIAdapter::OnTemporarilyHiddenStatusChanged(). nit: should this comment ...
1 month, 3 weeks ago (2017-01-30 21:39:51 UTC) #33
dewittj
https://codereview.chromium.org/2631933002/diff/60001/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2631933002/diff/60001/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc#newcode220 chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:220: item->download_progress.current, item->download_progress.max, hm, this doesn't seem to match DownloadUIItem
1 month, 3 weeks ago (2017-02-03 22:05:41 UTC) #34
dewittj
also need to fixup junit/.../OfflinePageDownloadBridgeTest.java
1 month, 3 weeks ago (2017-02-03 22:17:12 UTC) #35
dewittj
https://codereview.chromium.org/2631933002/diff/60001/components/offline_pages/core/downloads/download_ui_adapter.h File components/offline_pages/core/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2631933002/diff/60001/components/offline_pages/core/downloads/download_ui_adapter.h#newcode77 components/offline_pages/core/downloads/download_ui_adapter.h:77: RequestCoordinator* coordinator, Recent tabs doesn't need |coordinator| can we ...
1 month, 2 weeks ago (2017-02-07 22:29:33 UTC) #36
Dmitry Titov
https://codereview.chromium.org/2631933002/diff/60001/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2631933002/diff/60001/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc#newcode220 chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:220: item->download_progress.current, item->download_progress.max, On 2017/02/03 22:05:40, dewittj wrote: > hm, ...
1 month, 2 weeks ago (2017-02-08 00:45:08 UTC) #39
Dmitry Titov
On 2017/01/20 18:40:02, dfalcantara (load balance plz) wrote: > Looks alright so far; guessing the ...
1 month, 2 weeks ago (2017-02-08 03:05:37 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2631933002/120001
1 month, 2 weeks ago (2017-02-09 18:40:46 UTC) #53
commit-bot: I haz the power
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_presubmit/builds/360770)
1 month, 2 weeks ago (2017-02-09 18:49:54 UTC) #55
dfalcantara (load balance plz)
lgtm https://codereview.chromium.org/2631933002/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java (right): https://codereview.chromium.org/2631933002/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java#newcode48 chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:48: /** @return current download progress while the item ...
1 month, 2 weeks ago (2017-02-09 23:56:57 UTC) #56
Dmitry Titov
https://codereview.chromium.org/2631933002/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java (right): https://codereview.chromium.org/2631933002/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java#newcode48 chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:48: /** @return current download progress while the item is ...
1 month, 2 weeks ago (2017-02-10 02:04:39 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2631933002/140001
1 month, 2 weeks ago (2017-02-10 02:05:12 UTC) #60
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/308474)
1 month, 2 weeks ago (2017-02-10 08:06:15 UTC) #62
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2631933002/140001
1 month, 2 weeks ago (2017-02-10 17:04:19 UTC) #64
commit-bot: I haz the power
1 month, 2 weeks ago (2017-02-10 18:03:50 UTC) #67
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/9d507add10786ba9b0e6e4291ffe...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld d1a128a62