[Offline Pages] Link the internals page with the offline model and request

chrome/browser/ui/webui/offline_internals_ui.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 "chrome/browser/ui/webui/offline_internals_ui.h"
 
-#include <string>
+#include <stdint.h>
+#include <stdlib.h>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/values.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/profiles/profile.h"
 #include "chrome/common/url_constants.h"
+#include "components/offline_pages/background/request_coordinator.h"
+#include "components/offline_pages/background/save_page_request.h"
+#include "components/offline_pages/offline_page_model.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/browser/web_ui_controller.h"
 #include "content/public/browser/web_ui_data_source.h"
 #include "content/public/browser/web_ui_message_handler.h"
 #include "grit/browser_resources.h"
 
 namespace {

[dpapad, 2016/06/09 23:36:30]

Nit: Should this be in offline_pages namespace? Perhaps this would eliminate the
need to repeat offline_pages:: multiple times in this file.

 
 // Class acting as a controller of the chrome://offline-internals WebUI.
 class OfflineInternalsUIMessageHandler : public content::WebUIMessageHandler {
  public:
   OfflineInternalsUIMessageHandler();
   ~OfflineInternalsUIMessageHandler() override;
 
   // WebUIMessageHandler implementation.
   void RegisterMessages() override;
 
  private:
   // Deletes all the pages in the store.
   void HandleDeleteAllPages(const base::ListValue* args);
 
   // Delete selected list of page ids from the store.
   void HandleDeleteSelectedPages(const base::ListValue* args);
 
-  // Load all information.
-  void HandleGetOfflineInternalsInfo(const base::ListValue* args);
+  // Load Request Queue info.
+  void HandleGetRequestQueue(const base::ListValue* args);

[Dan Beam, 2016/06/09 23:35:02]

typically these are all just named
HandleExactNameOfChromeSendOrCrSendWithPromise and also document arguments (or
that there are no arguments)

[chili, 2016/06/10 01:04:52]

Done.

+
+  // Load Stored pages info.
+  void HandleGetStoredPages(const base::ListValue* args);
+
+  // Callback for async GetAllPages calls.
+  void HandleStoredPagesCallback(const base::Value* callback_id,
+      const offline_pages::MultipleOfflinePageItemResult& pages);
+
+  // Callback for async GetRequests calls.
+  void HandleRequestQueueCallback(
+      const base::Value* callback_id,
+      offline_pages::RequestQueue::GetRequestsResult result,
+      const std::vector<offline_pages::SavePageRequest>& requests);
+
+  // Callback for DeletePage/ClearAll calls.
+  void HandleDeletedPagesCallback(const base::Value* callback_id,
+                                  const offline_pages::DeletePageResult result);
+

[Dan Beam, 2016/06/09 23:35:02]

doc comment

[chili, 2016/06/10 01:04:52]

Done.

+  std::string GetStringFromDeletePageResult(
+      offline_pages::DeletePageResult value);
+

[Dan Beam, 2016/06/09 23:35:01]

doc comment

[chili, 2016/06/10 01:04:52]

Done.

+  std::string GetStringFromSavePageStatus(
+      offline_pages::SavePageRequest::Status status);
+
+  // Offline page model to call methods on.
+  offline_pages::OfflinePageModel* offline_page_model_;

[Dan Beam, 2016/06/09 23:35:02]

where is this initialized originally?

[Dan Beam, 2016/06/09 23:35:02]

nit: \n

[chili, 2016/06/10 01:04:52]

initialized in RegisterMessages, because that's when we can guarantee that
web_ui() is set

[chili, 2016/06/10 01:04:52]

Done.

+  // Request coordinator for background offline actions.
+  offline_pages::RequestCoordinator* request_coordinator_;
 
   // Factory for creating references in callbacks.
   base::WeakPtrFactory<OfflineInternalsUIMessageHandler> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(OfflineInternalsUIMessageHandler);
 };
 
 OfflineInternalsUIMessageHandler::OfflineInternalsUIMessageHandler()
     : weak_ptr_factory_(this) {}
 
 OfflineInternalsUIMessageHandler::~OfflineInternalsUIMessageHandler() {}
 
+std::string OfflineInternalsUIMessageHandler::GetStringFromDeletePageResult(
+    offline_pages::DeletePageResult value) {

[Dan Beam, 2016/06/09 23:35:01]

nit: I think this implementation would be more future-proof and typesafe:


std::string OfflineInternalsUIMessageHandler::DeletePageResultToString(
    offline_pages::DeletePageResult result) {
  switch (result) {
    case SomeResult:
      return "Some string";
    ...
  }
  NOTREACHED();
  return "Unknown";
}

[chili, 2016/06/10 01:04:52]

Done.

+  const std::string kDeletePageResultToString[] = {
+    "Not ready", "Pending", "Started", "Failed", "Expired"};
+  int int_value = static_cast<int>(value);
+  if (int_value >= 5)
+    return "Unknown";
+  else

[Dan Beam, 2016/06/09 23:35:02]

no else after return

[chili, 2016/06/10 01:04:52]

Done.

+    return kDeletePageResultToString[int_value];
+}
+
+std::string OfflineInternalsUIMessageHandler::GetStringFromSavePageStatus(
+    offline_pages::SavePageRequest::Status status) {
+  const std::string kRequestStatusToString[] = {

[dpapad, 2016/06/09 23:36:30]

This constant array is identical with line 91. Should it be moved outside the
function and shared between the two functions?

[chili, 2016/06/10 01:04:52]

when i copied/pasted due to a previous comment, copied wrong thing :( Fixed now!

+      "Not ready", "Pending", "Started", "Failed", "Expired"};
+  int int_value = static_cast<int>(status);
+  if (int_value >= 5)
+    return "Unknown";
+  else

[Dan Beam, 2016/06/09 23:35:02]

no else after return

[chili, 2016/06/10 01:04:52]

Done.

+    return kRequestStatusToString[int_value];
+}
+
 void OfflineInternalsUIMessageHandler::HandleDeleteAllPages(
     const base::ListValue* args) {
   const base::Value* callback_id;

[Dan Beam, 2016/06/09 23:35:02]

what's the ownership of |callback_id|?  is it safe to pass into base::Bind()
like this?

[chili, 2016/06/10 01:04:52]

I'm a bit rusty on C++ here.  this won't compile when I passed in *callback_id
and made the parameter type const base::Value&... 

I believe *callback_id is allocated on the heap.  When I looked at all the value
creation stuff, it seems like they were created by calling new <valueType>

[dpapad, 2016/06/10 01:25:09]

You can follow this example
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/about_h...
to get the callback_id as an std::string, instead of a pointer and pass it
around by value instead, to avoid leaking memory.

   args->Get(0, &callback_id);

[dpapad, 2016/06/09 23:36:30]

CHECK(args->Get(0, &callback_id));

[chili, 2016/06/10 01:04:52]

Done.

-  CallJavascriptFunction("cr.webUIResponse",
-      *callback_id,
-      base::FundamentalValue(true),
-      base::StringValue("success"));
+
+  // Pass back success because ClearAll doesn't return a status.
+  offline_page_model_->ClearAll(
+      base::Bind(&OfflineInternalsUIMessageHandler::HandleDeletedPagesCallback,
+                 weak_ptr_factory_.GetWeakPtr(), callback_id,
+                 offline_pages::DeletePageResult::SUCCESS));
 }
 
 void OfflineInternalsUIMessageHandler::HandleDeleteSelectedPages(
     const base::ListValue* args) {
   const base::Value* callback_id;
   args->Get(0, &callback_id);
-  CallJavascriptFunction("cr.webUIResponse",
-      *callback_id,
-      base::FundamentalValue(true),
-      base::StringValue("success"));
+
+  std::vector<int64_t> offline_ids;
+  const base::ListValue* offline_ids_from_arg;
+  args->GetList(1, &offline_ids_from_arg);
+
+  for (size_t i = 0; i < offline_ids_from_arg->GetSize(); i++) {
+    std::string value;
+    offline_ids_from_arg->GetString(i, &value);
+    offline_ids.push_back(atoll(value.c_str()));

[Dan Beam, 2016/06/09 23:35:01]

is this different/better than base::StringToInt64()?

https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.h...

[chili, 2016/06/10 01:04:52]

ooh, didn't know about StringToInt64. Using that now.

I don't think it's different or better.  It's just part of the default library

+  }
+
+  offline_page_model_->DeletePagesByOfflineId(
+      offline_ids,
+      base::Bind(&OfflineInternalsUIMessageHandler::HandleDeletedPagesCallback,
+                 weak_ptr_factory_.GetWeakPtr(), callback_id));
 }
 
-void OfflineInternalsUIMessageHandler::HandleGetOfflineInternalsInfo(
+void OfflineInternalsUIMessageHandler::HandleDeletedPagesCallback(
+    const base::Value* callback_id,
+    offline_pages::DeletePageResult result) {
+

[dpapad, 2016/06/09 23:36:30]

Nit: No need for \n here.

[chili, 2016/06/10 01:04:52]

Done.

+  ResolveJavascriptCallback(
+      *callback_id,
+      base::StringValue(GetStringFromDeletePageResult(result)));
+}
+
+void OfflineInternalsUIMessageHandler::HandleStoredPagesCallback(
+    const base::Value* callback_id,
+    const offline_pages::MultipleOfflinePageItemResult& pages) {
+  base::ListValue results;
+
+  for (const auto& page : pages) {
+    base::DictionaryValue* js_page_object = new base::DictionaryValue();
+    results.Append(js_page_object);
+    js_page_object->SetString("onlineUrl", page.url.spec());
+    js_page_object->SetString("namespace", page.client_id.name_space);
+    js_page_object->SetDouble("size", page.file_size);
+    js_page_object->SetString("id", std::to_string(page.offline_id));
+    js_page_object->SetString("filePath", page.file_path.value());
+    js_page_object->SetDouble("creationTime", page.creation_time.ToJsTime());
+    js_page_object->SetDouble("lastAccessTime",
+                              page.last_access_time.ToJsTime());
+    js_page_object->SetInteger("accessCount", page.access_count);
+  }
+  ResolveJavascriptCallback(*callback_id, results);
+}
+
+void OfflineInternalsUIMessageHandler::HandleRequestQueueCallback(
+    const base::Value* callback_id,
+    offline_pages::RequestQueue::GetRequestsResult result,
+    const std::vector<offline_pages::SavePageRequest>& requests) {
+  base::ListValue js_requests;
+  if (result == offline_pages::RequestQueue::GetRequestsResult::kSuccess) {
+    for (const auto& request : requests) {
+      base::DictionaryValue* js_request_object = new base::DictionaryValue();
+      js_requests.Append(js_request_object);
+      js_request_object->SetString("onlineUrl", request.url().spec());
+      js_request_object->SetDouble("creationTime",
+                                   request.creation_time().ToJsTime());
+      js_request_object->SetString(
+          "status",
+          GetStringFromSavePageStatus(request.GetStatus(base::Time::Now())));
+      js_request_object->SetString("namespace", request.client_id().name_space);
+      js_request_object->SetDouble("lastAttempt",
+                                   request.last_attempt_time().ToJsTime());
+      js_request_object->SetString("id", std::to_string(request.request_id()));
+    }
+  }
+  ResolveJavascriptCallback(*callback_id, js_requests);
+}
+
+void OfflineInternalsUIMessageHandler::HandleGetRequestQueue(
     const base::ListValue* args) {
   AllowJavascript();
   const base::Value* callback_id;
   args->Get(0, &callback_id);
-  base::DictionaryValue results;
-  results.Set("AllPages", new base::ListValue());
-  results.Set("Queue", new base::ListValue());
+  base::ListValue results;
 
-  CallJavascriptFunction("cr.webUIResponse",
-      *callback_id,
-      base::FundamentalValue(true),
-      results);
+  if (request_coordinator_ != nullptr)

[Dan Beam, 2016/06/09 23:35:01]

nit: curlies around 2+ line ifs

[Dan Beam, 2016/06/09 23:35:02]

nit: if (request_coordinator_) seems equivalent but shorter

[dpapad, 2016/06/09 23:36:30]

The Promise that JS holds is never fulfilled (resolved or rejected) if this
condition is false. Can we handle this case such that no Promise is left
indefinitely un-fulfiled.

[chili, 2016/06/10 01:04:52]

Done.

[chili, 2016/06/10 01:04:52]

Done.

[chili, 2016/06/10 01:04:52]

Done.

+    request_coordinator_->queue()->GetRequests(

[dpapad, 2016/06/09 23:36:30]

Nit: Perhaps more readable to use curly braces for this if block, since it spans
multiple lines.

[chili, 2016/06/10 01:04:52]

Done.

+        base::Bind(
+            &OfflineInternalsUIMessageHandler::HandleRequestQueueCallback,
+            weak_ptr_factory_.GetWeakPtr(), callback_id));
+}
+
+void OfflineInternalsUIMessageHandler::HandleGetStoredPages(
+    const base::ListValue* args) {
+  AllowJavascript();
+  const base::Value* callback_id;
+  args->Get(0, &callback_id);
+  base::ListValue results;
+
+  if (offline_page_model_ != nullptr)
+    offline_page_model_->GetAllPages(
+        base::Bind(&OfflineInternalsUIMessageHandler::HandleStoredPagesCallback,
+                   weak_ptr_factory_.GetWeakPtr(), callback_id));

[dpapad, 2016/06/09 23:36:30]

Same here, what happens to the Promise?

[chili, 2016/06/10 01:04:52]

Done.

 }
 
 void OfflineInternalsUIMessageHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
       "deleteAllPages",
       base::Bind(&OfflineInternalsUIMessageHandler::HandleDeleteAllPages,
                  weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback(
       "deleteSelectedPages",
       base::Bind(&OfflineInternalsUIMessageHandler::HandleDeleteSelectedPages,
                  weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback(
-      "getOfflineInternalsInfo",
-      base::Bind(
-          &OfflineInternalsUIMessageHandler::HandleGetOfflineInternalsInfo,
-          weak_ptr_factory_.GetWeakPtr()));
+      "getRequestQueueInfo",

[Dan Beam, 2016/06/09 23:35:02]

nit: could this be just "getRequestQueue"?

[chili, 2016/06/10 01:04:52]

Renamed the method, so keeping as is

+      base::Bind(&OfflineInternalsUIMessageHandler::HandleGetRequestQueue,
+                 weak_ptr_factory_.GetWeakPtr()));
+  web_ui()->RegisterMessageCallback(
+      "getStoredPagesInfo",

[Dan Beam, 2016/06/09 23:35:02]

could this just be "getStoredPages"?

[chili, 2016/06/10 01:04:52]

Acknowledged.

+      base::Bind(&OfflineInternalsUIMessageHandler::HandleGetStoredPages,
+                 weak_ptr_factory_.GetWeakPtr()));
+
+  // Get the offline page model associated with this web ui.
+  Profile* profile = Profile::FromWebUI(web_ui());
+  offline_page_model_ =
+      offline_pages::OfflinePageModelFactory::GetForBrowserContext(profile);
+  request_coordinator_ =
+      offline_pages::RequestCoordinatorFactory::GetForBrowserContext(profile);
 }
 
 }  // namespace
 
 OfflineInternalsUI::OfflineInternalsUI(content::WebUI* web_ui)
     : content::WebUIController(web_ui) {
   // chrome://offline-internals source.
   content::WebUIDataSource* html_source =
       content::WebUIDataSource::Create(chrome::kChromeUIOfflineInternalsHost);
 
   // Required resources.
   html_source->SetJsonPath("strings.js");
   html_source->AddResourcePath("offline_internals.css",
                                IDR_OFFLINE_INTERNALS_CSS);
   html_source->AddResourcePath("offline_internals.js",
                                IDR_OFFLINE_INTERNALS_JS);
   html_source->SetDefaultResource(IDR_OFFLINE_INTERNALS_HTML);
 
   content::WebUIDataSource::Add(Profile::FromWebUI(web_ui), html_source);
 
   web_ui->AddMessageHandler(new OfflineInternalsUIMessageHandler());
 }
 
 OfflineInternalsUI::~OfflineInternalsUI() {}