Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2631933002:
Adding status info to DownloadUIItem and piping it through. (Closed)
|
Created:
3 years, 11 months ago by Dmitry Titov Modified:
3 years, 10 months 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. |
DescriptionAdding 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 #Messages
Total messages: 67 (41 generated)
The CQ bit was checked by dimich@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...
dimich@chromium.org changed reviewers: + dewittj@chromium.org, mscales@chromium.org
This is WIP, still needs tests. Justin: could you look and see if it works for what you need for NTP bridge. Dan: Is this enough to hook up Offline Pages to progress indicatiors on Download Home? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
petewil@chromium.org changed reviewers: + petewil@chromium.org
Drive by: a few nits, looks fine otherwise. I see you adding a way to keep the RC as a member variable, but I don't see it getting used, I presume that is coming in a later patch. https://codereview.chromium.org/2631933002/diff/1/chrome/android/java/src/org... 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... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:56: public long downloadPreogress() { nit- Preogress -> Progress (and again further down in the file) https://codereview.chromium.org/2631933002/diff/1/components/offline_pages/co... File components/offline_pages/core/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2631933002/diff/1/components/offline_pages/co... components/offline_pages/core/downloads/download_ui_adapter.h:145: // Always valid, a service. Should we note that this pointer is unowned?
The CQ bit was checked by dimich@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
first comments, more will come https://codereview.chromium.org/2631933002/diff/1/components/offline_pages/co... File components/offline_pages/core/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2631933002/diff/1/components/offline_pages/co... components/offline_pages/core/downloads/download_ui_adapter.cc:103: void DownloadUIAdapter::OnCompleted(const SavePageRequest& request, OK to do in future, but now that we have classes that Observe both the model and the request coordinator we might want to have more descriptive method names. https://codereview.chromium.org/2631933002/diff/1/components/offline_pages/co... File components/offline_pages/core/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2631933002/diff/1/components/offline_pages/co... components/offline_pages/core/downloads/download_ui_adapter.h:39: virtual bool IsVisibleInUI(const ClientId& client_id) = 0; Eventually I will need a way to ask the DownloadUIAdapter to reevaluate certain client IDs - in particular during startup when tabs are being added that have legitimate recent tabs. So probably long-term an enum of "VISIBLE" "DISABLED" "INVISIBLE" would be better than a bool. WE can do this in a future patch. https://codereview.chromium.org/2631933002/diff/1/components/offline_pages/co... components/offline_pages/core/downloads/download_ui_adapter.h:139: void addItemHelper(std::unique_ptr<ItemInfo> item_info); nit: capital A, also next line.
idea looks good! Waiting for tests :) https://codereview.chromium.org/2631933002/diff/20001/chrome/android/java/src... 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... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:56: public long downloadPreogress() { s/Preogress/Progress/ also in next function https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... File components/offline_pages/core/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:49: // DownloadUIAdapter::OnTemporaryHidenStatusChanged(). s/Temporary/Temporarily/ s/Hiden/Hidden https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:50: virtual bool IsTemporaryHiddenInUI(const ClientId& client_id) = 0; s/Temporary/Temporarily/ https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:114: void OnTemporaryHidenStatusChanged(const ClientId& client_id, bool hidden); s/Hiden/Hidden https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... File components/offline_pages/core/downloads/download_ui_item.cc (right): https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.cc:20: download_progress(DownloadProgress(1,1)), current is 1 byte? https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.cc:29: download_progress(DownloadProgress(1,0)), current > max? https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... File components/offline_pages/core/downloads/download_ui_item.h (right): https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.h:48: int64_t current; s/current/current_bytes/ (or whatever unit it represents) https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.h:59: // instance across all DownloadUIItem providers. I'm not sure across all providers is a requirement. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.h:60: std::string guid; nit: can we rename guid?
I think I was added to this review by mistake, using the alias wibblymat. 99.9% sure you meant someone else! On Wed, 18 Jan 2017, 19:24 , <dewittj@chromium.org> wrote: > idea looks good! Waiting for tests :) > > > > https://codereview.chromium.org/2631933002/diff/20001/chrome/android/java/src... > 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... > > > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:56: > public long downloadPreogress() { > s/Preogress/Progress/ also in next function > > > https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... > File components/offline_pages/core/downloads/download_ui_adapter.h > (right): > > > https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... > components/offline_pages/core/downloads/download_ui_adapter.h:49: // > DownloadUIAdapter::OnTemporaryHidenStatusChanged(). > s/Temporary/Temporarily/ > s/Hiden/Hidden > > > https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... > components/offline_pages/core/downloads/download_ui_adapter.h:50: > virtual bool IsTemporaryHiddenInUI(const ClientId& client_id) = 0; > s/Temporary/Temporarily/ > > > https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... > components/offline_pages/core/downloads/download_ui_adapter.h:114: void > OnTemporaryHidenStatusChanged(const ClientId& client_id, bool hidden); > s/Hiden/Hidden > > > https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... > File components/offline_pages/core/downloads/download_ui_item.cc > (right): > > > https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... > components/offline_pages/core/downloads/download_ui_item.cc:20: > download_progress(DownloadProgress(1,1)), > current is 1 byte? > > > https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... > components/offline_pages/core/downloads/download_ui_item.cc:29: > download_progress(DownloadProgress(1,0)), > current > max? > > > https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... > File components/offline_pages/core/downloads/download_ui_item.h (right): > > > https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... > components/offline_pages/core/downloads/download_ui_item.h:48: int64_t > current; > s/current/current_bytes/ (or whatever unit it represents) > > > https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... > components/offline_pages/core/downloads/download_ui_item.h:59: // > instance across all DownloadUIItem providers. > I'm not sure across all providers is a requirement. > > > https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... > components/offline_pages/core/downloads/download_ui_item.h:60: > std::string guid; > nit: can we rename guid? > > https://codereview.chromium.org/2631933002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
dimich@chromium.org changed reviewers: + dfalcantara@chromium.org - mscales@chromium.org
+Dan to look from DownloadHome integration side.
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
drive by https://codereview.chromium.org/2631933002/diff/20001/chrome/android/java/src... 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... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:9: /** Download State, in sync with DownloadUIItem::DownloadState */ There is a way to enforce that using compiler. For that, please remove these and replace with" import org.chromium.components.offlinepages.downloads.DownloadState; After you apply the comment in https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... File components/offline_pages/core/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.cc:108: RequestNotifier::BackgroundSavePageResult status) { nit: git cl format https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.cc:302: nit: remove empty line. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... File components/offline_pages/core/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:76: explicit DownloadUIAdapter(OfflinePageModel* model, nit: remove explicit https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:81: // This adapter is potentially shared by UI elements, each of which adds nit: git cl format. something is off with spaces. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:165: void addItemHelper(std::unique_ptr<ItemInfo> item_info); AddItemHelper https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:166: void deleteItemHelper(const std::string& guid); DeleteItemHelper https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... File components/offline_pages/core/downloads/download_ui_item.h (right): https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.h:28: enum DownloadState { Please, put this line above: // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.components.offlinepages.downloads And add entry in omponents/offline_pages/core/BUILD.gn in offline_page_model_enums_java section. Which will generate an enum and things will be kept in sync for you. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.h:35: // The abstract notion of download progress. The |current| is always less less or "less or equal" ?
Looks alright so far; guessing the part that fires off the updates from the offline backend comes later? https://codereview.chromium.org/2631933002/diff/20001/chrome/android/java/src... 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... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:9: /** Download State, in sync with DownloadUIItem::DownloadState */ On 2017/01/20 17:03:01, fgorski wrote: > There is a way to enforce that using compiler. For that, please remove these and > replace with" > import org.chromium.components.offlinepages.downloads.DownloadState; > > After you apply the comment in > https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... Alternatively, can you just use org.chromium.content_public.browser.DownloadState? Looks like you're trying to match those states.
The CQ bit was checked by dimich@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: 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 dimich@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...
Updated, added tests. PTAL. https://codereview.chromium.org/2631933002/diff/20001/chrome/android/java/src... 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... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:9: /** Download State, in sync with DownloadUIItem::DownloadState */ On 2017/01/20 18:40:01, dfalcantara (load balance plz) wrote: > On 2017/01/20 17:03:01, fgorski wrote: > > There is a way to enforce that using compiler. For that, please remove these > and > > replace with" > > import org.chromium.components.offlinepages.downloads.DownloadState; > > > > After you apply the comment in > > > https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... > > Alternatively, can you just use > org.chromium.content_public.browser.DownloadState? Looks like you're trying to > match those states. Can not include content/public/browser stuff in all places I need (components), bad dependency. So going with Filip's advice. https://codereview.chromium.org/2631933002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:56: public long downloadPreogress() { On 2017/01/18 19:24:07, dewittj wrote: > s/Preogress/Progress/ also in next function Done. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... File components/offline_pages/core/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.cc:108: RequestNotifier::BackgroundSavePageResult status) { On 2017/01/20 17:03:01, fgorski wrote: > nit: git cl format Done. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.cc:302: On 2017/01/20 17:03:01, fgorski wrote: > nit: remove empty line. Done. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... File components/offline_pages/core/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:49: // DownloadUIAdapter::OnTemporaryHidenStatusChanged(). On 2017/01/18 19:24:07, dewittj wrote: > s/Temporary/Temporarily/ > s/Hiden/Hidden Done. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:50: virtual bool IsTemporaryHiddenInUI(const ClientId& client_id) = 0; On 2017/01/18 19:24:07, dewittj wrote: > s/Temporary/Temporarily/ Done. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:76: explicit DownloadUIAdapter(OfflinePageModel* model, On 2017/01/20 17:03:01, fgorski wrote: > nit: remove explicit Done. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:81: // This adapter is potentially shared by UI elements, each of which adds On 2017/01/20 17:03:01, fgorski wrote: > nit: git cl format. something is off with spaces. Done. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:114: void OnTemporaryHidenStatusChanged(const ClientId& client_id, bool hidden); On 2017/01/18 19:24:07, dewittj wrote: > s/Hiden/Hidden Done. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:165: void addItemHelper(std::unique_ptr<ItemInfo> item_info); On 2017/01/20 17:03:02, fgorski wrote: > AddItemHelper Done. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:166: void deleteItemHelper(const std::string& guid); On 2017/01/20 17:03:02, fgorski wrote: > DeleteItemHelper Done. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... File components/offline_pages/core/downloads/download_ui_item.cc (right): https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.cc:20: download_progress(DownloadProgress(1,1)), On 2017/01/18 19:24:07, dewittj wrote: > current is 1 byte? Done. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.cc:29: download_progress(DownloadProgress(1,0)), On 2017/01/18 19:24:07, dewittj wrote: > current > max? Done. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... File components/offline_pages/core/downloads/download_ui_item.h (right): https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.h:28: enum DownloadState { On 2017/01/20 17:03:02, fgorski wrote: > Please, put this line above: > // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.components.offlinepages.downloads > > And add entry in omponents/offline_pages/core/BUILD.gn in > offline_page_model_enums_java section. > > Which will generate an enum and things will be kept in sync for you. Nice advice, thanks! https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.h:35: // The abstract notion of download progress. The |current| is always less On 2017/01/20 17:03:02, fgorski wrote: > less or "less or equal" ? Removed the whole concept of current/max progress, leaved the bytes_downloaded since this is the most likely candidate for implementation now, and also the kind supported by DownlaodHome and mocks... https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.h:48: int64_t current; On 2017/01/18 19:24:07, dewittj wrote: > s/current/current_bytes/ (or whatever unit it represents) Done. Replaced with just byte counter. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.h:59: // instance across all DownloadUIItem providers. On 2017/01/18 19:24:07, dewittj wrote: > I'm not sure across all providers is a requirement. Right, it is not. I believe we always keep a bit "is this item an offline page" separately for now so we never send the ids of pages to Download Backend and vice versa. https://codereview.chromium.org/2631933002/diff/20001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.h:60: std::string guid; On 2017/01/18 19:24:07, dewittj wrote: > nit: can we rename guid? Looks like a separate CL... That CL can also start putting an offline_id there instead of guid and we also can stop generating guids for client_id for Downloads and async_load namespaces... This also will make this adapter work for Recent tabs.
more. Still PTAL. https://codereview.chromium.org/2631933002/diff/1/chrome/android/java/src/org... 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... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:56: public long downloadPreogress() { On 2017/01/17 18:27:34, Pete Williamson wrote: > nit- Preogress -> Progress (and again further down in the file) Done. https://codereview.chromium.org/2631933002/diff/1/components/offline_pages/co... File components/offline_pages/core/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2631933002/diff/1/components/offline_pages/co... components/offline_pages/core/downloads/download_ui_adapter.cc:103: void DownloadUIAdapter::OnCompleted(const SavePageRequest& request, On 2017/01/18 18:49:48, dewittj wrote: > OK to do in future, but now that we have classes that Observe both the model and > the request coordinator we might want to have more descriptive method names. Acknowledged. https://codereview.chromium.org/2631933002/diff/1/components/offline_pages/co... File components/offline_pages/core/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2631933002/diff/1/components/offline_pages/co... components/offline_pages/core/downloads/download_ui_adapter.h:39: virtual bool IsVisibleInUI(const ClientId& client_id) = 0; On 2017/01/18 18:49:48, dewittj wrote: > Eventually I will need a way to ask the DownloadUIAdapter to reevaluate certain > client IDs - in particular during startup when tabs are being added that have > legitimate recent tabs. So probably long-term an enum of "VISIBLE" "DISABLED" > "INVISIBLE" would be better than a bool. WE can do this in a future patch. Done. https://codereview.chromium.org/2631933002/diff/1/components/offline_pages/co... components/offline_pages/core/downloads/download_ui_adapter.h:139: void addItemHelper(std::unique_ptr<ItemInfo> item_info); On 2017/01/18 18:49:48, dewittj wrote: > nit: capital A, also next line. Done. https://codereview.chromium.org/2631933002/diff/1/components/offline_pages/co... components/offline_pages/core/downloads/download_ui_adapter.h:145: // Always valid, a service. On 2017/01/17 18:27:34, Pete Williamson wrote: > Should we note that this pointer is unowned? It is a raw pointer, so unowned... The comment says it's a service (valid always).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
lgtm, thanks! https://codereview.chromium.org/2631933002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2631933002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:476: OfflinePageModelFactory::GetForBrowserContext(browser_context); do we need to |nullptr|-check here https://codereview.chromium.org/2631933002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:483: RequestCoordinatorFactory::GetForBrowserContext(browser_context); also do we need to nullptr-check here https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... File components/offline_pages/core/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:46: // temporarily. This is relatively special case, for example for Last_N this is *a* relatively.... https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... File components/offline_pages/core/downloads/download_ui_adapter_unittest.cc (right): https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter_unittest.cc:143: class DownloadUIAdapterTest : public testing::Test, Any chance you could document what is set up in SetUp()? There are some tests that begin with PumpLoop() so something interesting is going on :)
some more nits: * Could use a better CL description. * Can we rename |guid| to something more general?
lgtm with nits. https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... File components/offline_pages/core/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:49: // DownloadUIAdapter::OnTemporarilyHiddenStatusChanged(). nit: should this comment match the method below? void TemporaryHiddenStatusChanged(const ClientId& client_id); https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... File components/offline_pages/core/downloads/download_ui_item.h (right): https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.h:35: // Unique id. It is filled with the offline_id, which should be ok nit: ID
https://codereview.chromium.org/2631933002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2631933002/diff/60001/chrome/browser/android/... 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
also need to fixup junit/.../OfflinePageDownloadBridgeTest.java
https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... File components/offline_pages/core/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:77: RequestCoordinator* coordinator, Recent tabs doesn't need |coordinator| can we make that optional?
The CQ bit was checked by dimich@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...
https://codereview.chromium.org/2631933002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2631933002/diff/60001/chrome/browser/android/... 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, this doesn't seem to match DownloadUIItem Done. https://codereview.chromium.org/2631933002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:476: OfflinePageModelFactory::GetForBrowserContext(browser_context); On 2017/01/30 20:59:34, dewittj wrote: > do we need to |nullptr|-check here Not sure what would be a great action here if there is no model at this point. It doesn't introduce new crashes since this is exactly moved from DownloadUIAdapter::FromOfflinePageModel, so the code path is equivalent. On the other hand, I don't think we need to support (and have code) for creating bridges for incognito... and define how they should behave. Crash is fine in this case. Added DCHECK. https://codereview.chromium.org/2631933002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:483: RequestCoordinatorFactory::GetForBrowserContext(browser_context); On 2017/01/30 20:59:34, dewittj wrote: > also do we need to nullptr-check here Ditto as above. https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... File components/offline_pages/core/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:46: // temporarily. This is relatively special case, for example for Last_N On 2017/01/30 20:59:34, dewittj wrote: > this is *a* relatively.... Done. https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:49: // DownloadUIAdapter::OnTemporarilyHiddenStatusChanged(). On 2017/01/30 21:39:50, fgorski wrote: > nit: should this comment match the method below? > > void TemporaryHiddenStatusChanged(const ClientId& client_id); Done. https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter.h:77: RequestCoordinator* coordinator, On 2017/02/07 22:29:32, dewittj wrote: > Recent tabs doesn't need |coordinator| can we make that optional? It's minor dependency, in terms of resources. And makes thinking about DUIA easier, less checks etc... https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... File components/offline_pages/core/downloads/download_ui_adapter_unittest.cc (right): https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... components/offline_pages/core/downloads/download_ui_adapter_unittest.cc:143: class DownloadUIAdapterTest : public testing::Test, On 2017/01/30 20:59:34, dewittj wrote: > Any chance you could document what is set up in SetUp()? There are some tests > that begin with PumpLoop() so something interesting is going on :) Done. https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... File components/offline_pages/core/downloads/download_ui_item.h (right): https://codereview.chromium.org/2631933002/diff/60001/components/offline_page... components/offline_pages/core/downloads/download_ui_item.h:35: // Unique id. It is filled with the offline_id, which should be ok On 2017/01/30 21:39:51, fgorski wrote: > nit: ID Done. Also reverted the rest of the comment since it is not yet using offlien_id there.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by dimich@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by dimich@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...
On 2017/01/20 18:40:02, dfalcantara (load balance plz) wrote: > Looks alright so far; guessing the part that fires off the updates from the > offline backend comes later? > Hi Dan, could you take another look? This patch makes Adapter to fires state changes, and OfflinePageDownloadBridge::Observer will get onItemUpdated(OfflinePageDownloadItem item) with updated downloadState. The subsequent patch will cause the same onItemUpdated with downloadProgressBytes updated as well, on periodic basis during DownloadState.IN_PROGRESS.
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 dimich@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2631933002/#ps120001 (title: "fix chrome_test_java")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm https://codereview.chromium.org/2631933002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java (right): https://codereview.chromium.org/2631933002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:48: /** @return current download progress while the item is downloaded. If it doesn't fit on one line, /** sits on a line by itself
https://codereview.chromium.org/2631933002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java (right): https://codereview.chromium.org/2631933002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:48: /** @return current download progress while the item is downloaded. On 2017/02/09 23:56:57, dfalcantara (load balance plz) wrote: > If it doesn't fit on one line, /** sits on a line by itself Done.
The CQ bit was checked by dimich@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org, dfalcantara@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2631933002/#ps140001 (title: "fix comment")
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_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dewittj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486746207845540,
"parent_rev": "61760068105446f2812daa2c6fe8408ff2431f10", "commit_rev":
"9d507add10786ba9b0e6e4291ffe75940d4a9846"}
Message was sent while issue was closed.
Description was changed from ========== Adding status info to DownloadUIItem and piping it through. BUG=681269 ========== to ========== 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/+/9d507add10786ba9b0e6e4291ffe... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/9d507add10786ba9b0e6e4291ffe... |
