[OfflinePages] Call NotifyInterrupted for pending requests

components/offline_pages/downloads/download_notifying_observer.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/offline_pages/downloads/download_notifying_observer.h"
 
 #include "components/offline_pages/background/request_coordinator.h"
 #include "components/offline_pages/background/save_page_request.h"
 #include "components/offline_pages/client_policy_controller.h"
 #include "components/offline_pages/downloads/download_ui_adapter.h"
@@ skipping 29 matching lines @@
       std::move(notifier), request_coordinator->GetPolicyController());
   request_coordinator->AddObserver(observer);
   // |request_coordinator| takes ownership of observer here.
   request_coordinator->SetUserData(&kUserDataKey, observer);
 }
 
 void DownloadNotifyingObserver::OnAdded(const SavePageRequest& request) {
   DCHECK(notifier_.get());
   if (!IsVisibleInUI(request.client_id()))
     return;
+
+  // There is no Added call for the notifier so we perform a Progress call

[fgorski, 2016/11/29 17:50:59]

nit: how about:
Calling Progress ensures notification is created in lieu of specific Add/Create
call.

[dougarnett, 2016/11/29 20:43:30]

Done.

+  // to cause the notification to be created.
   notifier_->NotifyDownloadProgress(DownloadUIItem(request));
+
+  // Now we need to update the notification if it is not active/offlining.
+  // TODO(dougarnett): Handle request state in notifier impl so this call

[fgorski, 2016/11/29 17:50:59]

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.

[dougarnett, 2016/11/29 20:43:30]

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).

+  // is not needed.
+  if (request.request_state() != SavePageRequest::RequestState::OFFLINING)
+    NotifyRequestStateChange(request);
 }
 
 void DownloadNotifyingObserver::OnChanged(const SavePageRequest& request) {
   DCHECK(notifier_.get());
   if (!IsVisibleInUI(request.client_id()))
     return;
-  if (request.request_state() == SavePageRequest::RequestState::PAUSED)
-    notifier_->NotifyDownloadPaused(DownloadUIItem(request));
-  else
-    notifier_->NotifyDownloadProgress(DownloadUIItem(request));
+  NotifyRequestStateChange(request);
 }
 
 void DownloadNotifyingObserver::OnCompleted(
     const SavePageRequest& request,
     RequestCoordinator::BackgroundSavePageResult status) {
   DCHECK(notifier_.get());
   if (!IsVisibleInUI(request.client_id()))
     return;
   if (status == RequestCoordinator::BackgroundSavePageResult::SUCCESS)
     notifier_->NotifyDownloadSuccessful(DownloadUIItem(request));
   else if (status == RequestCoordinator::BackgroundSavePageResult::REMOVED)
     notifier_->NotifyDownloadCanceled(DownloadUIItem(request));
   else
     notifier_->NotifyDownloadFailed(DownloadUIItem(request));
 }
 
 bool DownloadNotifyingObserver::IsVisibleInUI(const ClientId& page) {
   return policy_controller_->IsSupportedByDownload(page.name_space) &&
          base::IsValidGUID(page.id);
 }
 
+void DownloadNotifyingObserver::NotifyRequestStateChange(

[fgorski, 2016/11/29 17:50:59]

Thanks for extracting this one. I think the overall flow is very reasonable and
clean now.

[dougarnett, 2016/11/29 20:43:30]

Acknowledged.

+    const SavePageRequest& request) {
+  if (request.request_state() == SavePageRequest::RequestState::PAUSED)
+    notifier_->NotifyDownloadPaused(DownloadUIItem(request));
+  else if (request.request_state() == SavePageRequest::RequestState::AVAILABLE)
+    notifier_->NotifyDownloadInterrupted(DownloadUIItem(request));
+  else
+    notifier_->NotifyDownloadProgress(DownloadUIItem(request));
+}
+
 }  // namespace offline_pages