Add calls to the server to request WebAPK updates.

chrome/browser/android/webapk/webapk_installer.cc

Index: chrome/browser/android/webapk/webapk_installer.cc
diff --git a/chrome/browser/android/webapk/webapk_installer.cc b/chrome/browser/android/webapk/webapk_installer.cc
index 79a12d031ab657f61e60c8adc079cfd02ddfdb24..c7d7266a880358b5956f7361ea04132f35fa1282 100644
--- a/chrome/browser/android/webapk/webapk_installer.cc
+++ b/chrome/browser/android/webapk/webapk_installer.cc
@@ -24,7 +24,6 @@
 #include "content/public/browser/browser_thread.h"
 #include "jni/WebApkInstaller_jni.h"
 #include "net/http/http_status_code.h"
-#include "net/url_request/url_fetcher.h"
 #include "third_party/smhasher/src/MurmurHash2.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "url/gurl.h"
@@ -33,7 +32,10 @@ namespace {
 
 // The default WebAPK server URL.
 const char kDefaultWebApkServerUrl[] =
-    "https://webapk.googleapis.com/v1alpha/webApks?alt=proto";
+    "https://webapk.googleapis.com/v1alpha/webApks/";
+
+// The response format type expected from WebAPK server.

[pkotwicz, 2016/08/09 14:35:42]

Nit: "from WebAPK server" -> "from the WebAPK server"

[Xi Han, 2016/08/11 19:01:15]

Done.

+const char kDefaultWebApkServerUrlResponseType[] = "?alt=proto";
 
 // The MIME type of the POST data sent to the server.
 const char kProtoMimeType[] = "application/x-protobuf";
@@ -88,6 +90,7 @@ WebApkInstaller::WebApkInstaller(const ShortcutInfo& shortcut_info,
       shortcut_icon_(shortcut_icon),
       webapk_download_url_timeout_ms_(kWebApkDownloadUrlTimeoutMs),
       download_timeout_ms_(kDownloadTimeoutMs),
+      task_type_(UNDEFINED),
       io_weak_ptr_factory_(this) {
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   server_url_ =
@@ -102,13 +105,16 @@ void WebApkInstaller::InstallAsync(content::BrowserContext* browser_context,
                                    const FinishCallback& finish_callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   finish_callback_ = finish_callback;
+  task_type_ = INSTALL;
   // base::Unretained() is safe because WebApkInstaller owns itself and does not
   // start the timeout timer till after
   // InitializeRequestContextGetterOnUIThread() is called.
-  content::BrowserThread::PostTask(
+  content::BrowserThread::PostTaskAndReply(
       content::BrowserThread::UI, FROM_HERE,
       base::Bind(&WebApkInstaller::InitializeRequestContextGetterOnUIThread,
-                 base::Unretained(this), browser_context));
+                 base::Unretained(this), browser_context),
+      base::Bind(&WebApkInstaller::SendCreateWebApkRequest,
+                 io_weak_ptr_factory_.GetWeakPtr()));
 }
 
 void WebApkInstaller::InstallAsyncWithURLRequestContextGetter(
@@ -117,6 +123,7 @@ void WebApkInstaller::InstallAsyncWithURLRequestContextGetter(
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   request_context_getter_ = request_context_getter;
   finish_callback_ = finish_callback;
+  task_type_ = INSTALL;
 
   SendCreateWebApkRequest();
 }
@@ -126,12 +133,39 @@ void WebApkInstaller::SetTimeoutMs(int timeout_ms) {
   download_timeout_ms_ = timeout_ms;
 }
 
+void WebApkInstaller::UpdateAsync(content::BrowserContext* browser_context,
+                                  const FinishCallback& finish_callback,
+                                  const std::string& webapk_package,
+                                  int version) {

[pkotwicz, 2016/08/09 14:35:42]

Nit: |version| -> |webapk_version|

[Xi Han, 2016/08/11 19:01:15]

Done.

+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  finish_callback_ = finish_callback;
+  webapk_package_ = webapk_package;
+  webapk_version_ = version;
+  task_type_ = UPDATE;
+  // base::Unretained() is safe because WebApkInstaller owns itself and does not
+  // start the timeout timer till after
+  // InitializeRequestContextGetterOnUIThread() is called.
+  content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&WebApkInstaller::InitializeRequestContextGetterOnUIThread,
+          base::Unretained(this), browser_context),
+      base::Bind(&WebApkInstaller::SendUpdateWebApkRequest,
+                 io_weak_ptr_factory_.GetWeakPtr()));
+}
+
 bool WebApkInstaller::StartDownloadedWebApkInstall(
     JNIEnv* env,
     const base::android::ScopedJavaLocalRef<jstring>& java_file_path,
     const base::android::ScopedJavaLocalRef<jstring>& java_package_name) {

[pkotwicz, 2016/08/09 14:35:42]

Can you move this logic to OnWebApkDownloaded()? This function exists so that
WebApkInstallerTest can stub it out. Since it is stubbed out in tests the
function should be short.

I am suggesting renaming this function to:
WebApkInstaller::StartInstallingDownloadedWebApk()
and adding a new function:
WebApkInstaller::StartUpdateUsingDownloadedWebApk()

[Xi Han, 2016/08/11 19:01:14]

Done.

-  return Java_WebApkInstaller_installAsyncFromNative(env, java_file_path.obj(),
-                                                     java_package_name.obj());
+  if (task_type_ == INSTALL) {
+    return Java_WebApkInstaller_installAsyncFromNative(
+        env, java_file_path.obj(), java_package_name.obj());
+  } else if (task_type_ == UPDATE) {
+    return Java_WebApkInstaller_updateAsyncFromNative(
+        env, java_file_path.obj(), java_package_name.obj());
+  } else {
+    return false;
+  }
 }
 
 void WebApkInstaller::OnURLFetchComplete(const net::URLFetcher* source) {
@@ -168,27 +202,46 @@ void WebApkInstaller::InitializeRequestContextGetterOnUIThread(
   // Profile::GetRequestContext() must be called on UI thread.
   request_context_getter_ =
       Profile::FromBrowserContext(browser_context)->GetRequestContext();
-
-  content::BrowserThread::PostTask(
-      content::BrowserThread::IO, FROM_HERE,
-      base::Bind(&WebApkInstaller::SendCreateWebApkRequest,
-                 io_weak_ptr_factory_.GetWeakPtr()));
 }
 
 void WebApkInstaller::SendCreateWebApkRequest() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-  std::unique_ptr<webapk::WebApk> webapk_proto = BuildWebApkProto();
 
+  std::unique_ptr<webapk::WebApk> webapk = BuildWebApkProto();

[pkotwicz, 2016/08/09 14:35:42]

Nit: Remove the comment. It just reiterates what is on  line 213

[Xi Han, 2016/08/11 19:01:15]

Done.

+  // The WebAPK server URL to download and install a WebAPK is:
+  // |server_url_| + |kDeaultWebApkServerUrlResponseType|.
+  GURL server_url(server_url_.spec() + kDefaultWebApkServerUrlResponseType);
+  SendRequest(std::move(webapk), net::URLFetcher::POST, server_url);
+}
+
+void WebApkInstaller::SendUpdateWebApkRequest() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  std::unique_ptr<webapk::WebApk> webapk = BuildWebApkProto();
+  webapk->set_package_name(webapk_package_);
+  webapk->set_version(std::to_string(webapk_version_));
+  // The WebAPK server URL to download and update a WebAPK is:
+  // |server_url_| + |WebAPK package name| + "/"
+  // + |kDeaultWebApkServerUrlResponseType|

[pkotwicz, 2016/08/09 14:35:42]

Nit: The comment is not useful. It just reiterates what is on line 226-227.

Is the WebAPK version code supposed to be part of the server URL? The comment in
webapk_service.proto suggests that it is

If you want you can add a new function to clarify things:
GURL GetServerUrlForUpdate(const std::string& webapk_package_name) {}
Computing |server_url| in SendUpdateWebApkRequest() is fine by me though

Aside: It would be nice if we could change the server and simplify the server
URL. (Maybe log a bug?)

[Xi Han, 2016/08/11 19:01:15]

I think it is part of the package name in the comments.
I would prefer it inline since it is very simple.
Filed a bug 636552.

+  GURL server_url(server_url_.spec() + webapk_package_ + "/"
+                  + kDefaultWebApkServerUrlResponseType);
+  SendRequest(std::move(webapk), net::URLFetcher::PUT, server_url);
+}
+
+void WebApkInstaller::SendRequest(
+    std::unique_ptr<webapk::WebApk> request_proto,
+    net::URLFetcher::RequestType request_type,
+    const GURL& server_url) {
   timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(
                               webapk_download_url_timeout_ms_),
                base::Bind(&WebApkInstaller::OnTimeout,
                           io_weak_ptr_factory_.GetWeakPtr()));
 
   url_fetcher_ =
-      net::URLFetcher::Create(server_url_, net::URLFetcher::POST, this);
+      net::URLFetcher::Create(server_url, request_type, this);
   url_fetcher_->SetRequestContext(request_context_getter_);
   std::string serialized_request;
-  webapk_proto->SerializeToString(&serialized_request);
+  request_proto->SerializeToString(&serialized_request);
   url_fetcher_->SetUploadData(kProtoMimeType, serialized_request);
   url_fetcher_->Start();
 }