Initial CL for talking to the WebAPK server to generate WebAPK

chrome/browser/android/webapk/webapk_builder.cc

Index: chrome/browser/android/webapk/webapk_builder.cc
diff --git a/chrome/browser/android/webapk/webapk_builder.cc b/chrome/browser/android/webapk/webapk_builder.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d6be1c26e87aaea4d80e59cf5b0ed92a95bc0bf5
--- /dev/null
+++ b/chrome/browser/android/webapk/webapk_builder.cc
@@ -0,0 +1,267 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.

[Robert Sesek, 2016/07/25 23:01:33]

What about unittests for this file, to cover the various timeout/error
scenarios?

[pkotwicz, 2016/07/26 17:54:46]

I am confused as to what you want unittested.

Do you want a test testing that the callback is called in timeout/error
scenarios?

Something like this?
std::unique_ptr<WebApkBuilder> webapk_builder(new WebApkBuilder(...));
webapk_builder->BuildAsync(...);
// For the sake of the test do not do any of the intermediate processing
webapk_builder->OnWebApkDownloaded(...); // Call OnWebApkDownloaded with invalid
parameters
EXPECT_TRUE(CallbackCalled()); // Check that the callback passed in BuildAsync()
has been called

[Robert Sesek, 2016/07/26 21:15:50]

This class should have tests was the larger point I was making.
Right. And what happens if the download is partial and times out? Is the
download file left around?

[pkotwicz, 2016/07/27 02:50:22]

The bigger issue is: "After a WebAPK is installed, when will the downloaded APK
be deleted?" I have added a TODO to address this issue. You are right that once
I implement deleting the downloaded file that I should unit test the logic.

I am not sure what I should unit test out of the current implementation (I
honestly do not know). This file used to have unit tests but the unit tested
code was moved to manifest_util.cc

[Robert Sesek, 2016/07/27 15:37:09]

This class can finish its operation in four unique ways:
- Success
- Download failure
- Request timeout
- Download timeout

There should be a test for each one of these. Right now the test may just be
validating the bool in the callback. When adding new classes, I find it's always
good to add the basic test support as well, since that decreases friction to
adding new tests in the future.

[pkotwicz, 2016/07/28 18:29:51]

I have added integration tests in webapk_installer_unittest.cc The tests check
that the WebApkInstaller installation process finishes.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/android/webapk/webapk_builder.h"
+
+#include "base/android/build_info.h"
+#include "base/android/jni_android.h"
+#include "base/android/jni_string.h"
+#include "base/android/path_utils.h"
+#include "base/bind.h"
+#include "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/md5.h"
+#include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/android/webapk/webapk.pb.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_switches.h"
+#include "components/version_info/version_info.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/common/manifest_util.h"
+#include "jni/WebApkBuilder_jni.h"
+#include "net/http/http_status_code.h"
+#include "net/url_request/url_fetcher.h"
+#include "ui/gfx/codec/png_codec.h"
+#include "url/gurl.h"
+
+namespace {
+
+// The default WebAPK server URL.
+const char kDefaultWebApkServerUrl[] = "https://webapk.googleapis.com/v1alpha/webApks?alt=proto";

[Robert Sesek, 2016/07/25 23:01:33]

nit: 80 cols, break after =

[pkotwicz, 2016/07/26 17:54:46]

Done.

+
+// The MIME type of the POST data sent to the server.
+const char kProtoMimeType[] = "application/x-protobuf";
+
+// The number of milliseconds to wait for the WebAPK download URL from the
+// WebAPK server.
+const int kWebApkDownloadUrlTimeoutMs = 4000;
+
+// The number of milliseconds to wait for the WebAPK download to complete.
+const int kDownloadTimeoutMs = 20000;
+
+// Returns the scope from |info| if it is specified. Otherwise, returns the
+// default scope.
+GURL GetScope(const ShortcutInfo& info) {
+  return (info.scope.is_valid()) ? info.scope : info.url.GetOrigin();
+}
+
+// Computes a MD5 hash of |bitmap|'s PNG encoded bytes.
+std::string ComputeBitmapHash(const SkBitmap& bitmap) {
+  std::vector<unsigned char> png_bytes;
+  gfx::PNGCodec::EncodeBGRASkBitmap(bitmap, false, &png_bytes);
+  base::MD5Digest digest;

[Robert Sesek, 2016/07/25 23:01:33]

Why MD5?

[pkotwicz, 2016/07/26 17:54:46]

I chose MD5 because we use the hash in many places in Chrome. Is there a
different hash that we should be using?

The server will compare the hash of the app icon sent by the client to the hash
of the app icon that the server has fetched to determine whether the images are
the same. If the hashes are the same, the WebAPK server can send a WebAPK that
it has previously generated back to the client (CertifiedWebAPK). If the hashes
are different, the WebAPK server needs to send back a WebAPK which has been
personalized to the one user & device (OneOffWebAPK). Here is an (old) design
docs for OneOffWebAPKs
https://docs.google.com/document/d/1PMwF8r0oruUcyW8zkfHAzyKxGKrBk6fGkFW88T3zN...

[Robert Sesek, 2016/07/26 22:33:26]

MD5 is compromised and there are several attacks against it, so I don't think
any new code should be using it.
The server is making a trust decision based off this hash, which indicates that
it should be cryptographically secure. MD5 is broken (as noted above), and SHA1
is at risk. I'd recommend SHA256.

[Robert Sesek, 2016/07/26 22:37:30]

More background: https://goto.google.com/rtmye

[pkotwicz, 2016/07/27 02:50:22]

Sorry, my answer was incomplete

The WebAPK server compares:
- The icon URL (I missed this part in my previous answer)
- The bitmap hash
against the icon URL and bitmap hash that the WebAPK server retrieved for a
given Web Manifest while crawling the web

The goal of the hash is to enable the server to detect whether the page
generates a different app icon for each user agent. For instance www.funny.com
could dynamically generate a different looking icon at www.funny.com/appicon.png
for English speaking users and for French speaking users based on the User Agent

If the WebAPK server incorrectly thinks that two bitmaps are identical, the user
will get the WebAPK that the server generated while it was crawling the web (not
another's users WebAPK)

I don't mind using SHA256. However, I am still confused why a cryptographically
secure hash is needed

[Robert Sesek, 2016/07/27 15:37:09]

With MD5 in this code, you're currently using a cryptographic hash function,
just a bad and broken one. If you're going to use a cryptographic hash function,
it needs to be SHA256. If you can tolerate collisions in these hash values, then
you can use a faster, smaller, non-cryptographic hash function.

[pkotwicz, 2016/07/28 18:29:51]

Switched to Murmur2 instead

+  base::MD5Sum(&png_bytes.front(), png_bytes.size(), &digest);
+  return base::MD5DigestToBase16(digest);
+}
+
+// Converts a color from the format specified in content::Manifest to a CSS
+// string.
+std::string ColorToString(int64_t color) {

[Robert Sesek, 2016/07/25 23:01:33]

I'm surprised there isn't a function already to do this in the codebase.

[pkotwicz, 2016/07/26 17:54:46]

I don't see anything in ui/gfx/color_utils.h or SkColor.h

The int64_t color format using by content::Manifest is pretty special. It is
different than SkColor.

+  if (color == content::Manifest::kInvalidOrMissingColor)
+    return "";
+
+  SkColor sk_color = reinterpret_cast<uint32_t&>(color);

[Robert Sesek, 2016/07/25 23:01:33]

Why reinterpret this as a reference?

[pkotwicz, 2016/07/26 17:54:46]

I believe this the only way of using reinterpret cast to convert signed 64 bit
to unsigned 32 bit.
This is reversing the logic in ManifestParser::ParseColor() and matches the
logic in ExtractColor() in manifest_parser_unittest.cc

Is there a better way of doing the conversion?

[Robert Sesek, 2016/07/26 21:15:51]

You can just mask the lower 32 bits if you want a 32-bit quantity.

[pkotwicz, 2016/07/27 02:50:22]

How would I convert from a signed 32 bit integer to an unsigned 32 bit integer
though?

I would rather not change the implementation of ManifestParser::ParseColor()
because the parsed value is cached in persistent storage.
ManifestParser::ParseColor() currently uses reinterpret_cast.

Apparently reinterpret_cast<> and static_cast<> from signed to unsigned produces
the same result if the cpu architecture uses 2's complement to represent
negative numbers.

[Robert Sesek, 2016/07/27 15:37:09]

I see. Why put this in a signed int32 at all then? Java can read this as a long
and only access the lower 32 bits, and then the comment about negative alphas in
manifest_parser.cc would no longer apply.

[pkotwicz, 2016/07/28 18:29:51]

I am unsure what you are suggesting. Are you suggesting changing the
implementation of ManifestParser::ParseColor() line 149 in particular?

[Robert Sesek, 2016/07/29 17:38:05]

I'm trying to understand why a signed int32 was used when it can lose alpha
precision, when an int64/java long could have been used instead.

[pkotwicz, 2016/07/29 17:51:39]

I am not sure why this decision was made. I suspect that part of the reason is
compatibility with the Android int color format specified in
https://developer.android.com/reference/android/graphics/Color.html

+  int r = SkColorGetR(sk_color);
+  int g = SkColorGetG(sk_color);
+  int b = SkColorGetB(sk_color);
+  double a = SkColorGetA(sk_color) / 255.0;
+  return base::StringPrintf("rgba(%d,%d,%d,%.2f)", r, g, b, a);
+}
+
+}  // anonymous namespace
+
+WebApkBuilder::WebApkBuilder(content::BrowserContext* browser_context,
+                             const ShortcutInfo& shortcut_info,
+                             const SkBitmap& shortcut_icon)
+    : browser_context_(browser_context),
+      shortcut_info_(shortcut_info),
+      shortcut_icon_(shortcut_icon),
+      io_weak_ptr_factory_(this) {
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  server_url_ =
+      GURL(command_line->HasSwitch(switches::kWebApkServerUrl)
+               ? command_line->GetSwitchValueASCII(switches::kWebApkServerUrl)
+               : kDefaultWebApkServerUrl);
+}
+
+WebApkBuilder::~WebApkBuilder() {}
+
+// static
+bool WebApkBuilder::Register(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+void WebApkBuilder::BuildAsync(const FinishCallback& finish_callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  finish_callback_ = finish_callback;
+  // base::Unretained() is safe because WebApkBuilder owns itself and does not
+  // start the timeout timer till after
+  // InitializeRequestContextGetterOnUIThread() is called.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&WebApkBuilder::InitializeRequestContextGetterOnUIThread,
+                 base::Unretained(this)));
+}
+
+void WebApkBuilder::OnURLFetchComplete(const net::URLFetcher* source) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  timer_.Stop();
+
+  if (!source->GetStatus().is_success() ||
+      source->GetResponseCode() != net::HTTP_OK) {
+    OnFailure();
+    return;
+  }
+
+  std::string response_string;
+  source->GetResponseAsString(&response_string);
+
+  std::unique_ptr<webapk::CreateWebApkResponse> response(
+      new webapk::CreateWebApkResponse);
+  if (!response->ParseFromString(response_string)) {
+    OnFailure();
+    return;
+  }
+
+  if (response->signed_download_url().empty() ||
+      response->webapk_package_name().empty()) {
+    OnFailure();
+    return;
+  }
+  OnGotWebApkDownloadUrl(response->signed_download_url(),
+                         response->webapk_package_name());
+}
+
+void WebApkBuilder::InitializeRequestContextGetterOnUIThread() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  // Must be called on UI thread.
+  request_context_getter_ =
+      Profile::FromBrowserContext(browser_context_)->GetRequestContext();
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&WebApkBuilder::SendCreateWebApkRequest,
+                 io_weak_ptr_factory_.GetWeakPtr()));
+}
+
+void WebApkBuilder::SendCreateWebApkRequest() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  std::unique_ptr<webapk::CreateWebApkRequest> request =
+      BuildCreateWebApkRequest();
+
+  timer_.Start(
+      FROM_HERE, base::TimeDelta::FromMilliseconds(kWebApkDownloadUrlTimeoutMs),
+      base::Bind(&WebApkBuilder::OnTimeout, io_weak_ptr_factory_.GetWeakPtr()));
+
+  url_fetcher_ =
+      net::URLFetcher::Create(server_url_, net::URLFetcher::POST, this);
+  url_fetcher_->SetRequestContext(request_context_getter_);
+  std::string serialized_request;
+  request->SerializeToString(&serialized_request);
+  url_fetcher_->SetUploadData(kProtoMimeType, serialized_request);
+  url_fetcher_->Start();
+}
+
+void WebApkBuilder::OnGotWebApkDownloadUrl(const std::string& download_url,
+                                           const std::string& package_name) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  base::FilePath output_dir;
+  base::android::GetCacheDirectory(&output_dir);
+  // TODO(pkotwicz): Download WebAPKs into WebAPK-specific subdirectory
+  // directory.
+
+  timer_.Start(
+      FROM_HERE, base::TimeDelta::FromMilliseconds(kDownloadTimeoutMs),
+      base::Bind(&WebApkBuilder::OnTimeout, io_weak_ptr_factory_.GetWeakPtr()));
+
+  base::FilePath output_path = output_dir.AppendASCII(package_name);
+  downloader_.reset(new FileDownloader(
+      GURL(download_url), output_path, true, request_context_getter_,
+      base::Bind(&WebApkBuilder::OnWebApkDownloaded,
+                 io_weak_ptr_factory_.GetWeakPtr(), output_path,
+                 package_name)));
+}
+
+void WebApkBuilder::OnWebApkDownloaded(const base::FilePath& file_path,
+                                       const std::string& package_name,
+                                       FileDownloader::Result result) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  timer_.Stop();
+
+  if (result != FileDownloader::DOWNLOADED) {
+    OnFailure();
+    return;
+  }
+
+  JNIEnv* env = base::android::AttachCurrentThread();
+  base::android::ScopedJavaLocalRef<jstring> java_file_path =
+      base::android::ConvertUTF8ToJavaString(env, file_path.value());
+  base::android::ScopedJavaLocalRef<jstring> java_package_name =
+      base::android::ConvertUTF8ToJavaString(env, package_name);
+  bool success = Java_WebApkBuilder_installAsync(env, java_file_path.obj(),
+                                                 java_package_name.obj());
+  if (success)
+    OnSuccess();
+  else
+    OnFailure();
+}
+
+std::unique_ptr<webapk::CreateWebApkRequest>
+WebApkBuilder::BuildCreateWebApkRequest() {
+  std::unique_ptr<webapk::CreateWebApkRequest> request(
+      new webapk::CreateWebApkRequest);
+
+  webapk::WebApk* webapk = request->mutable_webapk();
+  webapk->set_manifest_url(shortcut_info_.manifest_url.spec());
+  webapk->set_requester_application_package(
+      base::android::BuildInfo::GetInstance()->package_name());
+  webapk->set_requester_application_version(version_info::GetVersionNumber());
+
+  webapk::WebAppManifest* web_app_manifest = webapk->mutable_manifest();
+  web_app_manifest->set_name(base::UTF16ToUTF8(shortcut_info_.name));
+  web_app_manifest->set_short_name(
+      base::UTF16ToUTF8(shortcut_info_.short_name));
+  web_app_manifest->set_start_url(shortcut_info_.url.spec());
+  web_app_manifest->set_orientation(
+      content::WebScreenOrientationLockTypeToString(
+          shortcut_info_.orientation));
+  web_app_manifest->set_display_mode(
+      content::WebDisplayModeToString(shortcut_info_.display));
+  web_app_manifest->set_background_color(
+      ColorToString(shortcut_info_.background_color));
+  web_app_manifest->set_theme_color(ColorToString(shortcut_info_.theme_color));
+
+  std::string* scope = web_app_manifest->add_scopes();
+  scope->assign(GetScope(shortcut_info_).spec());
+  webapk::Image* image = web_app_manifest->add_icons();
+  image->set_src(shortcut_info_.icon_url.spec());
+  // TODO(pkotwicz): Get MD5 hash of untransformed icon's bytes (with no
+  // encoding/decoding).
+  image->set_hash(ComputeBitmapHash(shortcut_icon_));
+  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 WebApkBuilder::OnTimeout() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  OnFailure();
+}
+
+void WebApkBuilder::OnSuccess() {
+  finish_callback_.Run(true);
+  delete this;
+}
+
+void WebApkBuilder::OnFailure() {
+  finish_callback_.Run(false);
+  delete this;
+}