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 9b27c223b841c8693739c3f5e186db61e293a6bb..1084ff1c6be913bbc450b45d406d0c59d99b12e5 100644
--- a/chrome/browser/android/webapk/webapk_installer.cc
+++ b/chrome/browser/android/webapk/webapk_installer.cc
@@ -24,7 +24,7 @@
 #include "content/public/common/manifest_util.h"
 #include "jni/WebApkInstaller_jni.h"
 #include "net/http/http_status_code.h"
-#include "net/url_request/url_fetcher.h"
+#include "third_party/protobuf/src/google/protobuf/message_lite.h"
 #include "third_party/smhasher/src/MurmurHash2.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "url/gurl.h"
@@ -33,7 +33,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.
+const char kDefaultWebApkServerUrlResponseType[] = "?alt=proto";
 
 // The MIME type of the POST data sent to the server.
 const char kProtoMimeType[] = "application/x-protobuf";
@@ -81,6 +84,7 @@ WebApkInstaller::WebApkInstaller(const ShortcutInfo& shortcut_info,
                                  const SkBitmap& shortcut_icon)
     : shortcut_info_(shortcut_info),
       shortcut_icon_(shortcut_icon),
+      taskType_(UNDEFINED),

[pkotwicz, 2016/08/08 18:59:16]

Nit: Camel case for variables in C++ code

[Xi Han, 2016/08/08 21:25:05]

Updated.

       io_weak_ptr_factory_(this) {
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   server_url_ =
@@ -95,13 +99,16 @@ void WebApkInstaller::InstallAsync(content::BrowserContext* browser_context,
                                    const FinishCallback& finish_callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   finish_callback_ = finish_callback;
+  taskType_ = 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(
@@ -110,16 +117,46 @@ void WebApkInstaller::InstallAsyncWithURLRequestContextGetter(
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   request_context_getter_ = request_context_getter;
   finish_callback_ = finish_callback;
+  taskType_ = INSTALL;
 
   SendCreateWebApkRequest();
 }
 
+void WebApkInstaller::UpdateAsync(content::BrowserContext* browser_context,
+                                  const FinishCallback& finish_callback,
+                                  const std::string& webapk_package,
+                                  int version) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  finish_callback_ = finish_callback;
+  webapk_package_ = webapk_package;
+  version_ = version;
+  taskType_ = 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) {
-  return Java_WebApkInstaller_installAsyncFromNative(env, java_file_path.obj(),
-                                                     java_package_name.obj());
+  if (taskType_ == INSTALL) {
+    taskType_ = UNDEFINED;

[pkotwicz, 2016/08/08 18:59:15]

Nit: Don't bother resetting |task_type_|. If you want you can check whether
|task_type_| is UNDEFINED in InstallAsync() & UpdateAsync().

[Xi Han, 2016/08/08 21:25:05]

Removed.

+    return Java_WebApkInstaller_installAsyncFromNative(
+        env, java_file_path.obj(), java_package_name.obj());
+  } else if (taskType_ == UPDATE) {
+    taskType_ = UNDEFINED;
+    return Java_WebApkInstaller_updateAsyncFromNative(

[pkotwicz, 2016/08/08 18:59:16]

Thank you for adding the Java calls for updating!

[Xi Han, 2016/08/08 21:25:05]

Thanks for catching this! I missed the difference before the meeting with Play.

+        env, java_file_path.obj(), java_package_name.obj());
+  } else {
+    return false;
+  }
 }
 
 void WebApkInstaller::OnURLFetchComplete(const net::URLFetcher* source) {
@@ -135,8 +172,8 @@ void WebApkInstaller::OnURLFetchComplete(const net::URLFetcher* source) {
   std::string response_string;
   source->GetResponseAsString(&response_string);
 
-  std::unique_ptr<webapk::CreateWebApkResponse> response(
-      new webapk::CreateWebApkResponse);
+  std::unique_ptr<webapk::WebApkResponse> response(
+      new webapk::WebApkResponse);
   if (!response->ParseFromString(response_string)) {
     OnFailure();
     return;
@@ -157,28 +194,55 @@ 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::CreateWebApkRequest> request =
-      BuildCreateWebApkRequest();
 
+  std::unique_ptr<webapk::CreateWebApkRequest> request
+      = std::unique_ptr<webapk::CreateWebApkRequest>(
+          new webapk::CreateWebApkRequest);
+
+  PopulateWebApkProto(request->mutable_webapk());
+  // The WebAPK server URL to download and install a WebAPK is:
+  // |server_url_| + |kDeaultWebApkServerUrlResponseType|.
+  GURL server_url(server_url_.spec() + kDefaultWebApkServerUrlResponseType);
+  SendRequest(std::move(request), net::URLFetcher::POST, server_url);
+}
+
+void WebApkInstaller::SendUpdateWebApkRequest() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+

[pkotwicz, 2016/08/08 18:59:15]

The server expects a webapk::WebApk proto not a webapk::UpdateWebApkRequest
proto (You reviewed https://codereview.chromium.org/2223443002/ 😎)

[Xi Han, 2016/08/08 21:25:05]

I thought there was technical issue that we need to send webapk::WebApk proto
for now, but not sure whether to abandon both webapk::UpdateWebApkRequest and
webapk::CreateWebApkRequest entirely. Ok, if that is the case, removed both.

+  std::unique_ptr<webapk::UpdateWebApkRequest> request
+      = std::unique_ptr<webapk::UpdateWebApkRequest>(
+          new webapk::UpdateWebApkRequest);
+
+  webapk::WebApk* webapk = request->mutable_webapk();
+  PopulateWebApkProto(webapk);
+  webapk->set_package_name(webapk_package_);
+  webapk->set_version(std::to_string(version_));
+  // The WebAPK server URL to download and update a WebAPK is:
+  // |server_url_| + |WebAPK package name| + "/"
+  // + |kDeaultWebApkServerUrlResponseType|
+  GURL server_url(server_url_.spec() + webapk_package_ + "/"
+                  + kDefaultWebApkServerUrlResponseType);

[pkotwicz, 2016/08/08 18:59:15]

Glenn can you comment? I thought the server URL would be the same for the
"create" and "update" requests.

[hartmanng, 2016/08/08 19:09:51]

Xi is correct - the server currently expects "<host>/v1alpha.webApk/<name of apk
to update>/?alt=proto" (see the Update entry in go/webapk-api).

[hartmanng, 2016/08/08 19:10:49]

Sorry, the period above was a typo, I really meant:

"<host>/v1alpha/webApk/<name of apk
to update>/?alt=proto"

[Xi Han, 2016/08/08 21:25:05]

Thank you Glenn for clarifying the url.

+  SendRequest(std::move(request), net::URLFetcher::PUT, server_url);

[pkotwicz, 2016/08/08 18:59:15]

I am unsure whether we want to use PUT here. Are PUT requests cached?
https://stormpath.com/blog/put-or-post suggests that they might. We do not want
update requests to be cached because even if |webapk| is the same as a previous
request, the ShellAPK version may have changed

[hartmanng, 2016/08/08 19:09:50]

Again, this is what the server is expecting currently. I don't know much about
PUT and caching, but you bring up a good point. +scott to comment on his
reasoning for using PUT in the original API - should we be changing this one to
POST as well?

+}
+
+void WebApkInstaller::SendRequest(
+    std::unique_ptr<::google::protobuf::MessageLite> request_proto,
+    net::URLFetcher::RequestType request_type,
+    const GURL& server_url) {
   timer_.Start(FROM_HERE,
                base::TimeDelta::FromMilliseconds(kWebApkDownloadUrlTimeoutMs),
                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;
-  request->SerializeToString(&serialized_request);
+  request_proto->SerializeToString(&serialized_request);
   url_fetcher_->SetUploadData(kProtoMimeType, serialized_request);
   url_fetcher_->Start();
 }
@@ -206,8 +270,8 @@ void WebApkInstaller::OnGotWebApkDownloadUrl(const std::string& download_url,
 }
 
 void WebApkInstaller::OnWebApkDownloaded(const base::FilePath& file_path,
-                                       const std::string& package_name,
-                                       FileDownloader::Result result) {
+                                         const std::string& package_name,
+                                         FileDownloader::Result result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
   timer_.Stop();
@@ -230,12 +294,7 @@ void WebApkInstaller::OnWebApkDownloaded(const base::FilePath& file_path,
     OnFailure();
 }
 
-std::unique_ptr<webapk::CreateWebApkRequest>
-WebApkInstaller::BuildCreateWebApkRequest() {
-  std::unique_ptr<webapk::CreateWebApkRequest> request(
-      new webapk::CreateWebApkRequest);
-
-  webapk::WebApk* webapk = request->mutable_webapk();
+void WebApkInstaller::PopulateWebApkProto(webapk::WebApk* webapk) {
   webapk->set_manifest_url(shortcut_info_.manifest_url.spec());
   webapk->set_requester_application_package(
       base::android::BuildInfo::GetInstance()->package_name());
@@ -265,8 +324,6 @@ WebApkInstaller::BuildCreateWebApkRequest() {
   std::vector<unsigned char> png_bytes;
   gfx::PNGCodec::EncodeBGRASkBitmap(shortcut_icon_, false, &png_bytes);
   image->set_image_data(&png_bytes.front(), png_bytes.size());
-
-  return request;
 }
 
 void WebApkInstaller::OnTimeout() {