Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2277123002:
[Offline pages] Hooking up cancel/pause/resume buttons for offline page related notifications (Closed)
|
Created:
4 years, 4 months ago by fgorski Modified:
4 years, 3 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline pages] Hooking up cancel/pause/resume buttons for offline page related notifications
* Adding cancel/pause/resume implementations on the bridge
and wiring them to the request coordinator
* Updating request coordinator to not report completion
more than once for success and failure of offliner.
BUG=630817
R=dimich@chromium.org,qinmin@chromium.org
Committed: https://crrev.com/1a708e3e93a3a287edb614234f4f39f9e3d2a735
Cr-Commit-Position: refs/heads/master@{#414556}
Patch Set 1 #Patch Set 2 : Fixing tests #
Total comments: 6
Patch Set 3 : Addressing comments from dewittj #
Total comments: 9
Patch Set 4 : Addressing feedback from petewil #Patch Set 5 : Rebasing #
Messages
Total messages: 40 (23 generated)
Description was changed from ========== [Offline pages] Hooking up cancel/pause/resume buttons for offline page related notifications * Adding cancel/pause/resume implementations on the bridge and wiring them to the request coordinator BUG=630817 R=dimich@chromium.org,qinmin@chromium.org ========== to ========== [Offline pages] Hooking up cancel/pause/resume buttons for offline page related notifications * Adding cancel/pause/resume implementations on the bridge and wiring them to the request coordinator * Updating request coordinator to not report completion more than once for success and failure of offliner. BUG=630817 R=dimich@chromium.org,qinmin@chromium.org ==========
fgorski@chromium.org changed reviewers: + petewil@chromium.org
The CQ bit was checked by fgorski@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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by fgorski@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...
dewittj@chromium.org changed reviewers: + dewittj@chromium.org
https://codereview.chromium.org/2277123002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2277123002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:74: const RequestContinuationCallback& callback, Using a callback here is fancy but maybe less readable than just calling FitlerRequestsByGuid directly inside the continuation functions. https://codereview.chromium.org/2277123002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:79: (request.client_id().name_space == kDownloadNamespace || Any chance you can make a function NamespaceIsUsedForDownloads (or similar) in the client_namespace_constants file so that anyone adding a namespace knows whether to consider this case? https://codereview.chromium.org/2277123002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:91: void CancelRequestsContinuation(RequestCoordinator* coordinator, As we discussed offline, I'd recommend passing a BrowserContext* here and using RequestCoordinatorFactory just as a way to ensure that there aren't any ReqeustCoordinator lifetime issues.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fgorski@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...
PTAL https://codereview.chromium.org/2277123002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2277123002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:74: const RequestContinuationCallback& callback, On 2016/08/25 17:26:57, dewittj wrote: > Using a callback here is fancy but maybe less readable than just calling > FitlerRequestsByGuid directly inside the continuation functions. Done. https://codereview.chromium.org/2277123002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:79: (request.client_id().name_space == kDownloadNamespace || On 2016/08/25 17:26:57, dewittj wrote: > Any chance you can make a function NamespaceIsUsedForDownloads (or similar) in > the client_namespace_constants file so that anyone adding a namespace knows > whether to consider this case? Good idea, but I'd rather have it in separate patch. We might have more of cases like that around the code. crbug.com/641053 https://codereview.chromium.org/2277123002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:91: void CancelRequestsContinuation(RequestCoordinator* coordinator, On 2016/08/25 17:26:57, dewittj wrote: > As we discussed offline, I'd recommend passing a BrowserContext* here and using > RequestCoordinatorFactory just as a way to ensure that there aren't any > ReqeustCoordinator lifetime issues. Done.
lgtm https://codereview.chromium.org/2277123002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2277123002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:77: request.client_id().name_space == kAsyncNamespace)) { Like DewittJ suggested elsewhere, should we have a function that checks a list of namespaces so we can add a new namespace in just one place? https://codereview.chromium.org/2277123002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:101: const std::string& guid, nit - indentation off https://codereview.chromium.org/2277123002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:242: if (request_coordinator) { Should we log a warning if RC is not found? (Here and below)
lgtm
https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java (right): https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:142: if (hasUserGesture) { Does offline page supports auto resumption after browser is killed?
The CQ bit was checked by fgorski@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...
Addressed comments. https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java (right): https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:142: if (hasUserGesture) { On 2016/08/25 18:40:38, qinmin wrote: > Does offline page supports auto resumption after browser is killed? The requests are going to be resumed based on the background scheduler. It is irrelevant otherwise. We can update that presumption if this is not what you think should happen. Unless you are saying that browser would issue a pause to all requests. For us it makes no sense to do that, so perhaps we should add user gesture flag to the pause as well? https://codereview.chromium.org/2277123002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2277123002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:77: request.client_id().name_space == kAsyncNamespace)) { On 2016/08/25 18:30:41, Pete Williamson wrote: > Like DewittJ suggested elsewhere, should we have a function that checks a list > of namespaces so we can add a new namespace in just one place? Correct. crbug.com/641053 https://codereview.chromium.org/2277123002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:101: const std::string& guid, On 2016/08/25 18:30:41, Pete Williamson wrote: > nit - indentation off Done. https://codereview.chromium.org/2277123002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:242: if (request_coordinator) { On 2016/08/25 18:30:41, Pete Williamson wrote: > Should we log a warning if RC is not found? (Here and below) Done.
https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java (right): https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:142: if (hasUserGesture) { On 2016/08/25 18:51:34, fgorski wrote: > On 2016/08/25 18:40:38, qinmin wrote: > > Does offline page supports auto resumption after browser is killed? > > The requests are going to be resumed based on the background scheduler. It is > irrelevant otherwise. > We can update that presumption if this is not what you think should happen. > > Unless you are saying that browser would issue a pause to all requests. > For us it makes no sense to do that, so perhaps we should add user gesture flag > to the pause as well? Regular download already does auto resumption. 1. When network is disconnected and comes back, chrome auto resumes the download if the network type allows. 2. When chrome is killed, download will automatically resume by gcmNetworkManager. 3. When browser is killed and user restarts browser, download will also automatically resume. I don't think offline page needs separate logics on that.
On 2016/08/25 19:02:01, qinmin wrote: > https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java > (right): > > https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:142: > if (hasUserGesture) { > On 2016/08/25 18:51:34, fgorski wrote: > > On 2016/08/25 18:40:38, qinmin wrote: > > > Does offline page supports auto resumption after browser is killed? > > > > The requests are going to be resumed based on the background scheduler. It is > > irrelevant otherwise. > > We can update that presumption if this is not what you think should happen. > > > > Unless you are saying that browser would issue a pause to all requests. > > For us it makes no sense to do that, so perhaps we should add user gesture > flag > > to the pause as well? > > Regular download already does auto resumption. > 1. When network is disconnected and comes back, chrome auto resumes the download > if the network type allows. > 2. When chrome is killed, download will automatically resume by > gcmNetworkManager. > 3. When browser is killed and user restarts browser, download will also > automatically resume. > I don't think offline page needs separate logics on that. Offline page does not actively download until GCM network scheduler tells it that conditions are right. Pausing it means that request coordinator will not be able to start it. It will require user's interaction to have it resume. Does it make sense?
On 2016/08/25 19:09:26, fgorski wrote: > On 2016/08/25 19:02:01, qinmin wrote: > > > https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java > > (right): > > > > > https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:142: > > if (hasUserGesture) { > > On 2016/08/25 18:51:34, fgorski wrote: > > > On 2016/08/25 18:40:38, qinmin wrote: > > > > Does offline page supports auto resumption after browser is killed? > > > > > > The requests are going to be resumed based on the background scheduler. It > is > > > irrelevant otherwise. > > > We can update that presumption if this is not what you think should happen. > > > > > > Unless you are saying that browser would issue a pause to all requests. > > > For us it makes no sense to do that, so perhaps we should add user gesture > > flag > > > to the pause as well? > > > > Regular download already does auto resumption. > > 1. When network is disconnected and comes back, chrome auto resumes the > download > > if the network type allows. > > 2. When chrome is killed, download will automatically resume by > > gcmNetworkManager. > > 3. When browser is killed and user restarts browser, download will also > > automatically resume. > > I don't think offline page needs separate logics on that. > > Offline page does not actively download until GCM network scheduler tells it > that conditions are right. > Pausing it means that request coordinator will not be able to start it. > It will require user's interaction to have it resume. > > Does it make sense? Pausing is different from interruption. For regular download, if a download is paused manually, it will no longer resume. So resume() will never get called, it is the same case here. Automatic resumption will only happen when the download is interrupted, either due to network interruption or browser kill. Resumption will be scheduled by GCmNetworkManager, or on network reconnection/browser restart. I don't think offline page download is anything different from regular download, so we shouldn't care whether user gesture is around or not.
The CQ bit was unchecked by fgorski@chromium.org
On 2016/08/25 19:14:28, qinmin wrote: > On 2016/08/25 19:09:26, fgorski wrote: > > On 2016/08/25 19:02:01, qinmin wrote: > > > > > > https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:142: > > > if (hasUserGesture) { > > > On 2016/08/25 18:51:34, fgorski wrote: > > > > On 2016/08/25 18:40:38, qinmin wrote: > > > > > Does offline page supports auto resumption after browser is killed? > > > > > > > > The requests are going to be resumed based on the background scheduler. It > > is > > > > irrelevant otherwise. > > > > We can update that presumption if this is not what you think should > happen. > > > > > > > > Unless you are saying that browser would issue a pause to all requests. > > > > For us it makes no sense to do that, so perhaps we should add user gesture > > > flag > > > > to the pause as well? > > > > > > Regular download already does auto resumption. > > > 1. When network is disconnected and comes back, chrome auto resumes the > > download > > > if the network type allows. > > > 2. When chrome is killed, download will automatically resume by > > > gcmNetworkManager. > > > 3. When browser is killed and user restarts browser, download will also > > > automatically resume. > > > I don't think offline page needs separate logics on that. > > > > Offline page does not actively download until GCM network scheduler tells it > > that conditions are right. > > Pausing it means that request coordinator will not be able to start it. > > It will require user's interaction to have it resume. > > > > Does it make sense? > > Pausing is different from interruption. > For regular download, if a download is paused manually, it will no longer > resume. So resume() will never get called, it is the same case here. Correct some words here, if a download is paused manually, it will only resume when user manually click resume > Automatic resumption will only happen when the download is interrupted, either > due to network interruption or browser kill. Resumption will be scheduled by > GCmNetworkManager, or on network reconnection/browser restart. > I don't think offline page download is anything different from regular download, > so we shouldn't care whether user gesture is around or not.
The CQ bit was checked by fgorski@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2277123002/#ps60001 (title: "Addressing feedback from petewil")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/25 19:16:27, qinmin wrote: > On 2016/08/25 19:14:28, qinmin wrote: > > On 2016/08/25 19:09:26, fgorski wrote: > > > On 2016/08/25 19:02:01, qinmin wrote: > > > > > > > > > > https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... > > > > File > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:142: > > > > if (hasUserGesture) { > > > > On 2016/08/25 18:51:34, fgorski wrote: > > > > > On 2016/08/25 18:40:38, qinmin wrote: > > > > > > Does offline page supports auto resumption after browser is killed? > > > > > > > > > > The requests are going to be resumed based on the background scheduler. > It > > > is > > > > > irrelevant otherwise. > > > > > We can update that presumption if this is not what you think should > > happen. > > > > > > > > > > Unless you are saying that browser would issue a pause to all requests. > > > > > For us it makes no sense to do that, so perhaps we should add user > gesture > > > > flag > > > > > to the pause as well? > > > > > > > > Regular download already does auto resumption. > > > > 1. When network is disconnected and comes back, chrome auto resumes the > > > download > > > > if the network type allows. > > > > 2. When chrome is killed, download will automatically resume by > > > > gcmNetworkManager. > > > > 3. When browser is killed and user restarts browser, download will also > > > > automatically resume. > > > > I don't think offline page needs separate logics on that. > > > > > > Offline page does not actively download until GCM network scheduler tells it > > > that conditions are right. > > > Pausing it means that request coordinator will not be able to start it. > > > It will require user's interaction to have it resume. > > > > > > Does it make sense? > > > > Pausing is different from interruption. > > For regular download, if a download is paused manually, it will no longer > > resume. So resume() will never get called, it is the same case here. > > Correct some words here, if a download is paused manually, it will only resume > when user manually click resume > > > Automatic resumption will only happen when the download is interrupted, either > > due to network interruption or browser kill. Resumption will be scheduled by > > GCmNetworkManager, or on network reconnection/browser restart. > > I don't think offline page download is anything different from regular > download, > > so we shouldn't care whether user gesture is around or not. OK. I'd like to land this as is. And then do some testing. If needed I'll have a follow up patch. OK? I simply don't want us to resume offline page downloads that were paused by the user.
On 2016/08/25 19:18:27, fgorski wrote: > On 2016/08/25 19:16:27, qinmin wrote: > > On 2016/08/25 19:14:28, qinmin wrote: > > > On 2016/08/25 19:09:26, fgorski wrote: > > > > On 2016/08/25 19:02:01, qinmin wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... > > > > > File > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2277123002/diff/40001/chrome/android/java/src... > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:142: > > > > > if (hasUserGesture) { > > > > > On 2016/08/25 18:51:34, fgorski wrote: > > > > > > On 2016/08/25 18:40:38, qinmin wrote: > > > > > > > Does offline page supports auto resumption after browser is killed? > > > > > > > > > > > > The requests are going to be resumed based on the background > scheduler. > > It > > > > is > > > > > > irrelevant otherwise. > > > > > > We can update that presumption if this is not what you think should > > > happen. > > > > > > > > > > > > Unless you are saying that browser would issue a pause to all > requests. > > > > > > For us it makes no sense to do that, so perhaps we should add user > > gesture > > > > > flag > > > > > > to the pause as well? > > > > > > > > > > Regular download already does auto resumption. > > > > > 1. When network is disconnected and comes back, chrome auto resumes the > > > > download > > > > > if the network type allows. > > > > > 2. When chrome is killed, download will automatically resume by > > > > > gcmNetworkManager. > > > > > 3. When browser is killed and user restarts browser, download will also > > > > > automatically resume. > > > > > I don't think offline page needs separate logics on that. > > > > > > > > Offline page does not actively download until GCM network scheduler tells > it > > > > that conditions are right. > > > > Pausing it means that request coordinator will not be able to start it. > > > > It will require user's interaction to have it resume. > > > > > > > > Does it make sense? > > > > > > Pausing is different from interruption. > > > For regular download, if a download is paused manually, it will no longer > > > resume. So resume() will never get called, it is the same case here. > > > > Correct some words here, if a download is paused manually, it will only resume > > when user manually click resume > > > > > Automatic resumption will only happen when the download is interrupted, > either > > > due to network interruption or browser kill. Resumption will be scheduled by > > > GCmNetworkManager, or on network reconnection/browser restart. > > > I don't think offline page download is anything different from regular > > download, > > > so we shouldn't care whether user gesture is around or not. > > OK. I'd like to land this as is. And then do some testing. If needed I'll have a > follow up patch. OK? > > I simply don't want us to resume offline page downloads that were paused by the > user. Ok, you can go ahead and submit this. But ignoring the user gesture param shouldn't have any impact since resume() will never get called on manually paused download. For interrupted downloads, we should consolidate the auto resumption logic for both regular downloads and offline page downloads. Currently regular downloads already have the working logic implemented.
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:
While running git apply --index -3 -p1;
error: patch failed:
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:7
Falling back to three-way merge...
Applied patch to
'chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc'
with conflicts.
U
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
Patch:
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
Index:
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
diff --git
a/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
b/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
index
ce4eaa611c9d91bd407b31222c7ae182d1b31c72..d62d1338955b6a5e79f1257c262f5b5617cd33c3
100644
---
a/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
+++
b/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
@@ -7,17 +7,22 @@
#include <vector>
#include "base/android/jni_string.h"
+#include "base/bind.h"
#include "base/guid.h"
+#include "base/logging.h"
#include "base/memory/ptr_util.h"
#include
"chrome/browser/android/offline_pages/downloads/offline_page_notification_bridge.h"
#include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
#include "chrome/browser/android/offline_pages/offline_page_model_factory.h"
+#include "chrome/browser/android/offline_pages/request_coordinator_factory.h"
#include "chrome/browser/android/tab_android.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_android.h"
+#include "components/offline_pages/background/request_coordinator.h"
#include "components/offline_pages/client_namespace_constants.h"
#include "components/offline_pages/downloads/download_ui_item.h"
#include "components/offline_pages/offline_page_model.h"
+#include "content/public/browser/browser_context.h"
#include "content/public/browser/web_contents.h"
#include "jni/OfflinePageDownloadBridge_jni.h"
#include "net/base/filename_util.h"
@@ -60,14 +65,71 @@ ScopedJavaLocalRef<jobject> ToJavaOfflinePageDownloadItem(
ConvertUTF8ToJavaString(env, item.target_path.value()),
item.start_time.ToJavaTime(), item.total_bytes);
}
+
+std::vector<int64_t> FilterRequestsByGuid(
+ const std::vector<SavePageRequest>& requests,
+ const std::string& guid) {
+ std::vector<int64_t> request_ids;
+ for (const SavePageRequest& request : requests) {
+ if (request.client_id().id == guid &&
+ (request.client_id().name_space == kDownloadNamespace ||
+ request.client_id().name_space == kAsyncNamespace)) {
+ request_ids.push_back(request.request_id());
+ }
+ }
+ return request_ids;
+}
+
+void CancelRequestCallback(const RequestQueue::UpdateMultipleRequestResults&) {
+ // Results ignored here, as UI uses observer to update itself.
+}
+
+void CancelRequestsContinuation(content::BrowserContext* browser_context,
+ const std::string& guid,
+ const std::vector<SavePageRequest>& requests) {
+ RequestCoordinator* coordinator =
+ RequestCoordinatorFactory::GetForBrowserContext(browser_context);
+ if (coordinator) {
+ std::vector<int64_t> request_ids = FilterRequestsByGuid(requests, guid);
+ coordinator->RemoveRequests(request_ids,
+ base::Bind(&CancelRequestCallback));
+ } else {
+ LOG(WARNING) << "CancelRequestsContinuation has no valid coordinator.";
+ }
+}
+
+void PauseRequestsContinuation(content::BrowserContext* browser_context,
+ const std::string& guid,
+ const std::vector<SavePageRequest>& requests) {
+ RequestCoordinator* coordinator =
+ RequestCoordinatorFactory::GetForBrowserContext(browser_context);
+ if (coordinator)
+ coordinator->PauseRequests(FilterRequestsByGuid(requests, guid));
+ else
+ LOG(WARNING) << "PauseRequestsContinuation has no valid coordinator.";
+}
+
+void ResumeRequestsContinuation(content::BrowserContext* browser_context,
+ const std::string& guid,
+ const std::vector<SavePageRequest>& requests) {
+ RequestCoordinator* coordinator =
+ RequestCoordinatorFactory::GetForBrowserContext(browser_context);
+ if (coordinator)
+ coordinator->ResumeRequests(FilterRequestsByGuid(requests, guid));
+ else
+ LOG(WARNING) << "ResumeRequestsContinuation has no valid coordinator.";
+}
+
} // namespace
OfflinePageDownloadBridge::OfflinePageDownloadBridge(
JNIEnv* env,
const JavaParamRef<jobject>& obj,
- DownloadUIAdapter* download_ui_adapter)
+ DownloadUIAdapter* download_ui_adapter,
+ content::BrowserContext* browser_context)
: weak_java_ref_(env, obj),
- download_ui_adapter_(download_ui_adapter) {
+ download_ui_adapter_(download_ui_adapter),
+ browser_context_(browser_context) {
DCHECK(download_ui_adapter_);
download_ui_adapter_->AddObserver(this);
}
@@ -150,7 +212,7 @@ void OfflinePageDownloadBridge::StartDownload(
offline_pages::OfflinePageModel* offline_page_model =
OfflinePageModelFactory::GetForBrowserContext(
- tab->GetProfile()->GetOriginalProfile());
+ tab->GetProfile()->GetOriginalProfile());
if (!offline_page_model)
return;
@@ -174,6 +236,54 @@ void OfflinePageDownloadBridge::StartDownload(
base::Bind(&OfflinePageDownloadBridge::SavePageCallback, item));
}
+void OfflinePageDownloadBridge::CancelDownload(
+ JNIEnv* env,
+ const JavaParamRef<jobject>& obj,
+ const JavaParamRef<jstring>& j_guid) {
+ std::string guid = ConvertJavaStringToUTF8(env, j_guid);
+ RequestCoordinator* coordinator =
+ RequestCoordinatorFactory::GetForBrowserContext(browser_context_);
+
+ if (coordinator) {
+ coordinator->GetAllRequests(
+ base::Bind(&CancelRequestsContinuation, browser_context_, guid));
+ } else {
+ LOG(WARNING) << "CancelDownload has no valid coordinator.";
+ }
+}
+
+void OfflinePageDownloadBridge::PauseDownload(
+ JNIEnv* env,
+ const JavaParamRef<jobject>& obj,
+ const JavaParamRef<jstring>& j_guid) {
+ std::string guid = ConvertJavaStringToUTF8(env, j_guid);
+ RequestCoordinator* coordinator =
+ RequestCoordinatorFactory::GetForBrowserContext(browser_context_);
+
+ if (coordinator) {
+ coordinator->GetAllRequests(
+ base::Bind(&PauseRequestsContinuation, browser_context_, guid));
+ } else {
+ LOG(WARNING) << "PauseDownload has no valid coordinator.";
+ }
+}
+
+void OfflinePageDownloadBridge::ResumeDownload(
+ JNIEnv* env,
+ const JavaParamRef<jobject>& obj,
+ const JavaParamRef<jstring>& j_guid) {
+ std::string guid = ConvertJavaStringToUTF8(env, j_guid);
+ RequestCoordinator* coordinator =
+ RequestCoordinatorFactory::GetForBrowserContext(browser_context_);
+
+ if (coordinator) {
+ coordinator->GetAllRequests(
+ base::Bind(&ResumeRequestsContinuation, browser_context_, guid));
+ } else {
+ LOG(WARNING) << "ResumeDownload has no valid coordinator.";
+ }
+}
+
void OfflinePageDownloadBridge::ItemsLoaded() {
JNIEnv* env = AttachCurrentThread();
ScopedJavaLocalRef<jobject> obj = weak_java_ref_.get(env);
@@ -212,15 +322,17 @@ void OfflinePageDownloadBridge::ItemUpdated(const
DownloadUIItem& item) {
static jlong Init(JNIEnv* env,
const JavaParamRef<jobject>& obj,
const JavaParamRef<jobject>& j_profile) {
- Profile* profile = ProfileAndroid::FromProfileAndroid(j_profile);
+ content::BrowserContext* browser_context =
+ ProfileAndroid::FromProfileAndroid(j_profile);
+
OfflinePageModel* offline_page_model =
- OfflinePageModelFactory::GetForBrowserContext(profile);
+ OfflinePageModelFactory::GetForBrowserContext(browser_context);
DownloadUIAdapter* adapter =
DownloadUIAdapter::FromOfflinePageModel(offline_page_model);
return reinterpret_cast<jlong>(
- new OfflinePageDownloadBridge(env, obj, adapter));
+ new OfflinePageDownloadBridge(env, obj, adapter, browser_context));
}
} // namespace android
The CQ bit was checked by fgorski@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2277123002/#ps80001 (title: "Rebasing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Hooking up cancel/pause/resume buttons for offline page related notifications * Adding cancel/pause/resume implementations on the bridge and wiring them to the request coordinator * Updating request coordinator to not report completion more than once for success and failure of offliner. BUG=630817 R=dimich@chromium.org,qinmin@chromium.org ========== to ========== [Offline pages] Hooking up cancel/pause/resume buttons for offline page related notifications * Adding cancel/pause/resume implementations on the bridge and wiring them to the request coordinator * Updating request coordinator to not report completion more than once for success and failure of offliner. BUG=630817 R=dimich@chromium.org,qinmin@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Hooking up cancel/pause/resume buttons for offline page related notifications * Adding cancel/pause/resume implementations on the bridge and wiring them to the request coordinator * Updating request coordinator to not report completion more than once for success and failure of offliner. BUG=630817 R=dimich@chromium.org,qinmin@chromium.org ========== to ========== [Offline pages] Hooking up cancel/pause/resume buttons for offline page related notifications * Adding cancel/pause/resume implementations on the bridge and wiring them to the request coordinator * Updating request coordinator to not report completion more than once for success and failure of offliner. BUG=630817 R=dimich@chromium.org,qinmin@chromium.org Committed: https://crrev.com/1a708e3e93a3a287edb614234f4f39f9e3d2a735 Cr-Commit-Position: refs/heads/master@{#414556} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1a708e3e93a3a287edb614234f4f39f9e3d2a735 Cr-Commit-Position: refs/heads/master@{#414556} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
