Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2256373002:
Adds request queue removal capability to OfflinePageBridge. (Closed)
|
Created:
4 years, 4 months ago by dewittj Modified:
4 years, 4 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@query-queue Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds request queue removal capability to OfflinePageBridge.
Also adds a callback for when the task has finished.
BUG=620421
Committed: https://crrev.com/1430d329f356914f91ebae7ec4542524ceccf173
Cr-Commit-Position: refs/heads/master@{#413989}
Patch Set 1 #Patch Set 2 : add test to bridge #Patch Set 3 : Rebase #Patch Set 4 : Add a comment. #
Total comments: 7
Patch Set 5 : address comments. #Patch Set 6 : Rename some callbacks. #
Total comments: 5
Patch Set 7 : Address comments, make remove return a result object rather than a request object. #Patch Set 8 : fix test #Patch Set 9 : Rebase. #Messages
Total messages: 50 (31 generated)
The CQ bit was checked by dewittj@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...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 dewittj@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 checked by dewittj@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...
Description was changed from ========== Adds request queue removal capability to OfflinePageBridge. Also adds a callback for when the task has finished. BUG= ========== to ========== Adds request queue removal capability to OfflinePageBridge. Also adds a callback for when the task has finished. BUG=620421 ==========
dewittj@chromium.org changed reviewers: + dimich@chromium.org, dougarnett@chromium.org
ptal, thanks!
Initially have a question about what terminology we should use. This CL is exposing a Clear api that does a Remove. Since we Delete pages, I wonder if we should also Delete requests. https://codereview.chromium.org/2256373002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2256373002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:257: public void clearRequestsFromQueue( Wonder why introduce "clear" terminology vs. remove (used in above javadoc and unit test name) or delete (used in this file for for pages). Is clear used in other similar apis?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2256373002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2256373002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:250: * The callback will be called with |null| in the case that the request coordinator is Consider if this wording may be better for api consumer: ... in the case that the request queue is not available. ? https://codereview.chromium.org/2256373002/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2256373002/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.cc:169: void RequestCoordinator::ExplicitlyRemoveRequestsCallback( arrgh, 3 callbacks to name and not be confused by. Maybe Pete and I can improve naming in a follow up.
ptal, thanks https://codereview.chromium.org/2256373002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2256373002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:250: * The callback will be called with |null| in the case that the request coordinator is On 2016/08/19 19:35:59, dougarnett wrote: > Consider if this wording may be better for api consumer: > ... in the case that the request queue is not available. ? Done. https://codereview.chromium.org/2256373002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:257: public void clearRequestsFromQueue( On 2016/08/19 18:26:31, dougarnett wrote: > Wonder why introduce "clear" terminology vs. remove (used in above javadoc and > unit test name) or delete (used in this file for for pages). Is clear used in > other similar apis? Done. https://codereview.chromium.org/2256373002/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2256373002/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.cc:169: void RequestCoordinator::ExplicitlyRemoveRequestsCallback( On 2016/08/19 19:35:59, dougarnett wrote: > arrgh, 3 callbacks to name and not be confused by. Maybe Pete and I can improve > naming in a follow up. Any suggestions for now?
lgtm https://codereview.chromium.org/2256373002/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2256373002/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.cc:169: void RequestCoordinator::ExplicitlyRemoveRequestsCallback( On 2016/08/19 20:21:17, dewittj wrote: > On 2016/08/19 19:35:59, dougarnett wrote: > > arrgh, 3 callbacks to name and not be confused by. Maybe Pete and I can > improve > > naming in a follow up. > Any suggestions for now? Wonder about: RemoveRequestsCallback => HandleRemovedRequests ExplicitlyRemoveRequestsCallback => HandleRemovedRequestsAndCallback RequestsRemovedCallback => RemoveRequestsCallback
The CQ bit was checked by dewittj@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 2016/08/19 20:32:10, dougarnett wrote: > lgtm > > https://codereview.chromium.org/2256373002/diff/60001/components/offline_page... > File components/offline_pages/background/request_coordinator.cc (right): > > https://codereview.chromium.org/2256373002/diff/60001/components/offline_page... > components/offline_pages/background/request_coordinator.cc:169: void > RequestCoordinator::ExplicitlyRemoveRequestsCallback( > On 2016/08/19 20:21:17, dewittj wrote: > > On 2016/08/19 19:35:59, dougarnett wrote: > > > arrgh, 3 callbacks to name and not be confused by. Maybe Pete and I can > > improve > > > naming in a follow up. > > Any suggestions for now? > > Wonder about: > > RemoveRequestsCallback => HandleRemovedRequests > ExplicitlyRemoveRequestsCallback => HandleRemovedRequestsAndCallback > RequestsRemovedCallback => RemoveRequestsCallback Done. Thanks!
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
drive by. https://codereview.chromium.org/2256373002/diff/100001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2256373002/diff/100001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:173: HandleRemovedRequests(results, requests); 3 things that I am not comfortable here: 1. results is never used (even in HandleRemovedRequests) 2. callback should be invoked first if available, followed by notifications 3. since you can probably pass a dummy callback in, you don't need 2 Handle... methods.
Thanks for the drive-by. https://codereview.chromium.org/2256373002/diff/100001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2256373002/diff/100001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:173: HandleRemovedRequests(results, requests); On 2016/08/19 21:51:19, fgorski wrote: > 3 things that I am not comfortable here: > 1. results is never used (even in HandleRemovedRequests) I agree this is concerning; I don't think it's a thing I should solve in this cl. > 2. callback should be invoked first if available, followed by notifications Can you explain that rationale? > 3. since you can probably pass a dummy callback in, you don't need 2 Handle... > methods. Since there are multiple HandleRemovedRequests calls, having a dummy callback would be a bit of noise, is having the second method that bad?
https://codereview.chromium.org/2256373002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2256373002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:258: List<SavePageRequest> requests, Callback<SavePageRequest[]> callback) { It'd be cleaner if the method took a list of request_ids rather than complete SavePageRequests. Mutators of collections taking ids rather than the whole objects are easier to use and the implementation throws away everything except ids anyway.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
ptal, thanks! https://codereview.chromium.org/2256373002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2256373002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:258: List<SavePageRequest> requests, Callback<SavePageRequest[]> callback) { On 2016/08/19 22:30:11, Dmitry Titov wrote: > It'd be cleaner if the method took a list of request_ids rather than complete > SavePageRequests. > > Mutators of collections taking ids rather than the whole objects are easier to > use and the implementation throws away everything except ids anyway. > Done. https://codereview.chromium.org/2256373002/diff/100001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2256373002/diff/100001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:173: HandleRemovedRequests(results, requests); On 2016/08/19 21:56:37, dewittj wrote: > On 2016/08/19 21:51:19, fgorski wrote: > > 3 things that I am not comfortable here: > > 1. results is never used (even in HandleRemovedRequests) > > I agree this is concerning; I don't think it's a thing I should solve in this > cl. I solved this for my API; However the request coordinator itself doesn't use the results from the remove operation. > > > 2. callback should be invoked first if available, followed by notifications I generally think your explanation makes sense, switched the order. > > Can you explain that rationale? > > > 3. since you can probably pass a dummy callback in, you don't need 2 Handle... > > methods. > > Since there are multiple HandleRemovedRequests calls, having a dummy callback > would be a bit of noise, is having the second method that bad?
The CQ bit was checked by dewittj@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: 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 dewittj@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
dimich: ping, thanks!
this needs to land ASAP so I can land dependent changes before branch, will land in the next few hours if I don't hear from anyone.
lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm
The CQ bit was checked by dewittj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougarnett@chromium.org, dimich@chromium.org Link to the patchset: https://codereview.chromium.org/2256373002/#ps160001 (title: "Rebase.")
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 ========== Adds request queue removal capability to OfflinePageBridge. Also adds a callback for when the task has finished. BUG=620421 ========== to ========== Adds request queue removal capability to OfflinePageBridge. Also adds a callback for when the task has finished. BUG=620421 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Adds request queue removal capability to OfflinePageBridge. Also adds a callback for when the task has finished. BUG=620421 ========== to ========== Adds request queue removal capability to OfflinePageBridge. Also adds a callback for when the task has finished. BUG=620421 Committed: https://crrev.com/1430d329f356914f91ebae7ec4542524ceccf173 Cr-Commit-Position: refs/heads/master@{#413989} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/1430d329f356914f91ebae7ec4542524ceccf173 Cr-Commit-Position: refs/heads/master@{#413989} |
