Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2521353005:
[OfflinePages] Call NotifyInterrupted for pending requests (Closed)
|
Created:
4 years ago by dougarnett Modified:
4 years ago CC:
Dmitry Titov, 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[OfflinePages] Call NotifyInterrupted for pending requests
BUG=668231
Committed: https://crrev.com/59651690714d5c1b475d60a796ca11db423038cd
Cr-Commit-Position: refs/heads/master@{#435154}
Patch Set 1 #Patch Set 2 : Fix test #
Total comments: 4
Patch Set 3 : Fixed OnAdded to handle request_state same as OnChanged #
Total comments: 2
Patch Set 4 : Another option for OnAdded #
Total comments: 2
Patch Set 5 : Update per Filip's comment #Patch Set 6 : Fix indeterminate progress bar for Downloading... #
Total comments: 10
Patch Set 7 : Addressed fgorski feedback #
Total comments: 4
Patch Set 8 : Update for petewil feedback #Messages
Total messages: 49 (29 generated)
The CQ bit was checked by dougarnett@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...
dougarnett@chromium.org changed reviewers: + dimich@chromium.org, fgorski@chromium.org, qinmin@chromium.org
This gets "Download pending..." showing for AVAILABLE but not actively offlining requests.
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 dougarnett@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...
petewil@chromium.org changed reviewers: + petewil@chromium.org
Drive-by comment https://codereview.chromium.org/2521353005/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2521353005/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:434: NotifyChanged(request); Did you test this in the running app as opposed to just unit tests? I'm concerned that the code may not expect a NotifyChange when a request first gets added in addition to a NotifyAdded, and want to make sure it doesn't cause any problems.
https://codereview.chromium.org/2521353005/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2521353005/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:434: NotifyChanged(request); On 2016/11/28 18:12:03, Pete Williamson wrote: > Did you test this in the running app as opposed to just unit tests? I'm > concerned that the code may not expect a NotifyChange when a request first gets > added in addition to a NotifyAdded, and want to make sure it doesn't cause any > problems. Yeah, actually added this due to behavior seen in app testing.
lgtm
Description was changed from ========== [OfflinePages] Call NotifyInterrupted for pending requests BUG=668231 ========== to ========== [OfflinePages] Call NotifyInterrupted for pending requests BUG=668231 ==========
dougarnett@chromium.org changed reviewers: - dimich@chromium.org
https://codereview.chromium.org/2521353005/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2521353005/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:434: NotifyChanged(request); Why both added and changed? I don't understand it.
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
https://codereview.chromium.org/2521353005/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2521353005/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:434: NotifyChanged(request); On 2016/11/28 20:53:49, fgorski wrote: > Why both added and changed? I don't understand it. Dropped this and fixed request state handling in OnAdded
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/2521353005/diff/40001/components/offline_page... File components/offline_pages/downloads/download_notifying_observer.cc (right): https://codereview.chromium.org/2521353005/diff/40001/components/offline_page... components/offline_pages/downloads/download_notifying_observer.cc:48: OnChanged(request); Hmm, actually this didn't work. Seems that we need to do a Progress first at our level or handle Interrupted differently in notifier.
https://codereview.chromium.org/2521353005/diff/40001/components/offline_page... File components/offline_pages/downloads/download_notifying_observer.cc (right): https://codereview.chromium.org/2521353005/diff/40001/components/offline_page... components/offline_pages/downloads/download_notifying_observer.cc:48: OnChanged(request); On 2016/11/28 21:33:26, dougarnett wrote: > Hmm, actually this didn't work. Seems that we need to do a Progress first at our > level or handle Interrupted differently in notifier. Here is one approach at this level to affect the creation of the notification and updating it to Pending if appropriate. Min, do you have a suggestion for better handling at SystemDownloadNotifier level?
Suggestion on how to update the solution, but I think it is getting close. https://codereview.chromium.org/2521353005/diff/60001/components/offline_page... File components/offline_pages/downloads/download_notifying_observer.cc (right): https://codereview.chromium.org/2521353005/diff/60001/components/offline_page... components/offline_pages/downloads/download_notifying_observer.cc:58: if (request.request_state() == SavePageRequest::RequestState::AVAILABLE) Here is the response to your previous comment that I didn't post soon enough: That would make sense. I think what you want is to extract the code from onChange (instead of calling it) and then call it from here if (request.request_state() != SavePageRequest::RequestState::OFFLINING) As you say, that ensures creation and then immediately puts a proper state update on the notification.
The CQ bit was checked by dougarnett@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/2521353005/diff/60001/components/offline_page... File components/offline_pages/downloads/download_notifying_observer.cc (right): https://codereview.chromium.org/2521353005/diff/60001/components/offline_page... components/offline_pages/downloads/download_notifying_observer.cc:58: if (request.request_state() == SavePageRequest::RequestState::AVAILABLE) On 2016/11/28 22:30:24, fgorski wrote: > Here is the response to your previous comment that I didn't post soon enough: > > > That would make sense. I think what you want is to extract the code from > onChange (instead of calling it) and then call it from here if > (request.request_state() != SavePageRequest::RequestState::OFFLINING) > > As you say, that ensures creation and then immediately puts a proper state > update on the notification. Done. Btw, chatted with Min and he does suggest going with this two call approach for now.
The CQ bit was checked by dougarnett@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: This issue passed the CQ dry run.
lgtm with comments. (nits, comments, test missing) Overall I think the change is really solid. thanks for working on it. /Filip https://codereview.chromium.org/2521353005/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageNotificationBridge.java (right): https://codereview.chromium.org/2521353005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageNotificationBridge.java:76: boolean isOfflining) { do we still need this parameter? https://codereview.chromium.org/2521353005/diff/100001/components/offline_pag... File components/offline_pages/downloads/download_notifying_observer.cc (right): https://codereview.chromium.org/2521353005/diff/100001/components/offline_pag... components/offline_pages/downloads/download_notifying_observer.cc:51: // There is no Added call for the notifier so we perform a Progress call nit: how about: Calling Progress ensures notification is created in lieu of specific Add/Create call. https://codereview.chromium.org/2521353005/diff/100001/components/offline_pag... components/offline_pages/downloads/download_notifying_observer.cc:56: // TODO(dougarnett): Handle request state in notifier impl so this call Why? If we do so, it will require double call for cases when we are offlining in an active tab. I think the solution you are providing here is well documented and works. No further work should be necessary, unless I don't understand something. I think you can remove the TODO. https://codereview.chromium.org/2521353005/diff/100001/components/offline_pag... components/offline_pages/downloads/download_notifying_observer.cc:88: void DownloadNotifyingObserver::NotifyRequestStateChange( Thanks for extracting this one. I think the overall flow is very reasonable and clean now. https://codereview.chromium.org/2521353005/diff/100001/components/offline_pag... File components/offline_pages/downloads/download_notifying_observer_unittest.cc (right): https://codereview.chromium.org/2521353005/diff/100001/components/offline_pag... components/offline_pages/downloads/download_notifying_observer_unittest.cc:143: TEST_F(DownloadNotifyingObserverTest, OnAddedAsOffling) { Technically nothing prevents us from adding as paused. Can you please add a test for it? And if you think it should be impossible, add a test and code that prevents it. I think we should at least document the capability (with test) even if we never use it.
The CQ bit was checked by dougarnett@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/2521353005/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageNotificationBridge.java (right): https://codereview.chromium.org/2521353005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageNotificationBridge.java:76: boolean isOfflining) { On 2016/11/29 17:50:59, fgorski wrote: > do we still need this parameter? No, I will remove. Potentially later we would add a percentage if/when we decide to report some value for it. https://codereview.chromium.org/2521353005/diff/100001/components/offline_pag... File components/offline_pages/downloads/download_notifying_observer.cc (right): https://codereview.chromium.org/2521353005/diff/100001/components/offline_pag... components/offline_pages/downloads/download_notifying_observer.cc:51: // There is no Added call for the notifier so we perform a Progress call On 2016/11/29 17:50:59, fgorski wrote: > nit: how about: > Calling Progress ensures notification is created in lieu of specific Add/Create > call. Done. https://codereview.chromium.org/2521353005/diff/100001/components/offline_pag... components/offline_pages/downloads/download_notifying_observer.cc:56: // TODO(dougarnett): Handle request state in notifier impl so this call On 2016/11/29 17:50:59, fgorski wrote: > Why? If we do so, it will require double call for cases when we are offlining in > an active tab. I think the solution you are providing here is well documented > and works. No further work should be necessary, unless I don't understand > something. I think you can remove the TODO. Removed TODO. The concern here was if we experience a UI flicker, we might not want a double call at this level (for either case). https://codereview.chromium.org/2521353005/diff/100001/components/offline_pag... components/offline_pages/downloads/download_notifying_observer.cc:88: void DownloadNotifyingObserver::NotifyRequestStateChange( On 2016/11/29 17:50:59, fgorski wrote: > Thanks for extracting this one. I think the overall flow is very reasonable and > clean now. Acknowledged. https://codereview.chromium.org/2521353005/diff/100001/components/offline_pag... File components/offline_pages/downloads/download_notifying_observer_unittest.cc (right): https://codereview.chromium.org/2521353005/diff/100001/components/offline_pag... components/offline_pages/downloads/download_notifying_observer_unittest.cc:143: TEST_F(DownloadNotifyingObserverTest, OnAddedAsOffling) { On 2016/11/29 17:50:59, fgorski wrote: > Technically nothing prevents us from adding as paused. Can you please add a test > for it? > > And if you think it should be impossible, add a test and code that prevents it. > I think we should at least document the capability (with test) even if we never > use it. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Mostly there, a few questions. https://codereview.chromium.org/2521353005/diff/120001/components/offline_pag... File components/offline_pages/downloads/download_notifying_observer.cc (right): https://codereview.chromium.org/2521353005/diff/120001/components/offline_pag... components/offline_pages/downloads/download_notifying_observer.cc:90: else if (request.request_state() == SavePageRequest::RequestState::AVAILABLE) Why does available imply interrupted? Does this only get called with AVAILABLE in the interrupted case? If so, we should document that requirement somewhere in a comment. https://codereview.chromium.org/2521353005/diff/120001/components/offline_pag... File components/offline_pages/downloads/download_notifying_observer_unittest.cc (right): https://codereview.chromium.org/2521353005/diff/120001/components/offline_pag... components/offline_pages/downloads/download_notifying_observer_unittest.cc:140: EXPECT_EQ(kTestCreationTime, notifier()->download_item()->start_time); It may be overkill to verify all three of these on every test. As long as it gets verified once, checking only the GUID or URL on the others should be enough.
https://codereview.chromium.org/2521353005/diff/120001/components/offline_pag... File components/offline_pages/downloads/download_notifying_observer.cc (right): https://codereview.chromium.org/2521353005/diff/120001/components/offline_pag... components/offline_pages/downloads/download_notifying_observer.cc:90: else if (request.request_state() == SavePageRequest::RequestState::AVAILABLE) On 2016/11/29 23:26:16, Pete Williamson wrote: > Why does available imply interrupted? Does this only get called with AVAILABLE > in the interrupted case? If so, we should document that requirement somewhere > in a comment. Done. Added method comment plus interface comments. https://codereview.chromium.org/2521353005/diff/120001/components/offline_pag... File components/offline_pages/downloads/download_notifying_observer_unittest.cc (right): https://codereview.chromium.org/2521353005/diff/120001/components/offline_pag... components/offline_pages/downloads/download_notifying_observer_unittest.cc:140: EXPECT_EQ(kTestCreationTime, notifier()->download_item()->start_time); On 2016/11/29 23:26:16, Pete Williamson wrote: > It may be overkill to verify all three of these on every test. As long as it > gets verified once, checking only the GUID or URL on the others should be > enough. Done
lgtm
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2521353005/#ps140001 (title: "Update for petewil feedback")
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: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by dougarnett@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": 1480477541607260,
"parent_rev": "184b633123f8f9512b38a7f083e7859b904f76ce", "commit_rev":
"36fe8b9857ca2f4cab6a8abe9b46b144ec70ee17"}
Message was sent while issue was closed.
Description was changed from ========== [OfflinePages] Call NotifyInterrupted for pending requests BUG=668231 ========== to ========== [OfflinePages] Call NotifyInterrupted for pending requests BUG=668231 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [OfflinePages] Call NotifyInterrupted for pending requests BUG=668231 ========== to ========== [OfflinePages] Call NotifyInterrupted for pending requests BUG=668231 Committed: https://crrev.com/59651690714d5c1b475d60a796ca11db423038cd Cr-Commit-Position: refs/heads/master@{#435154} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/59651690714d5c1b475d60a796ca11db423038cd Cr-Commit-Position: refs/heads/master@{#435154} |
