Add logs for WebAPK server failures

Add logs for WebAPK server failures

This will make it easier to determine whether a WebAPK cannot be installed due
to a WebAPK server error.

BUG=690152
Review-Url: https://codereview.chromium.org/2687163002
Cr-Commit-Position: refs/heads/master@{#450072}
Committed: https://chromium.googlesource.com/chromium/src/+/6d3fe481d1e1510b36c74ae18611da5646d0cff6

[pkotwicz, 2017-02-09 21:21:55]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2017-02-09 21:22:06]

Xi and Glenn can you please take a look?

[pkotwicz, 2017-02-09 21:22:29]

Description was changed from

==========
Add logs for WebAPK server failures

This will make it easier to determine whether a WebAPK cannot be installed due
to a WebAPK server error.

BUG=690152
==========

to

==========
Add logs for WebAPK server failures

This will make it easier to determine whether a WebAPK cannot be installed due
to a WebAPK server error.

BUG=690152
==========

[pkotwicz, 2017-02-09 21:22:30]

pkotwicz@chromium.org changed reviewers:
+ hanxi@chromium.org, hartmanng@chromium.org

[pkotwicz, 2017-02-09 21:23:56]

Xi and Glenn can you please take a look?

https://codereview.chromium.org/2687163002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2687163002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:410: response_string.c_str());
The response string won't look too pretty right now because it is a proto even
for errors

[Xi Han, 2017-02-09 22:02:14]

https://codereview.chromium.org/2687163002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2687163002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:410: response_string.c_str());
On 2017/02/09 21:23:56, pkotwicz wrote:
> The response string won't look too pretty right now because it is a proto even
> for errors

I don't think we should put the entire response_string in the log, only
|response_code| should be enough.

[pkotwicz, 2017-02-09 22:54:22]

Xi, can you please take another look?

https://codereview.chromium.org/2687163002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2687163002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:410: response_string.c_str());
On 2017/02/09 22:02:14, Xi Han wrote:
> On 2017/02/09 21:23:56, pkotwicz wrote:
> > The response string won't look too pretty right now because it is a proto
even
> > for errors
> 
> I don't think we should put the entire response_string in the log, only
> |response_code| should be enough.

Done.

[Xi Han, 2017-02-10 14:45:45]

LGTM!

[hartmanng, 2017-02-10 14:58:46]

lgtm

[pkotwicz, 2017-02-10 15:35:53]

pkotwicz@chromium.org changed reviewers:
+ dominickn@chromium.org

[pkotwicz, 2017-02-10 15:35:54]

Dominick, can you please take a look?

[dominickn, 2017-02-13 00:32:14]

lgtm

[pkotwicz, 2017-02-13 18:10:43]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2017-02-13 18:11:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 18:47:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 18:47:55]

Failed to apply patch for chrome/browser/android/webapk/webapk_installer.cc:
While running git apply --index -p1;
  error: patch failed: chrome/browser/android/webapk/webapk_installer.cc:411
  error: chrome/browser/android/webapk/webapk_installer.cc: patch does not apply

Patch:       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
685f190a2b2cb9b72d83385192539017b5eeae31..7030c3130188bfd204bb08dae9b01c63a2b8cc54
100644
--- a/chrome/browser/android/webapk/webapk_installer.cc
+++ b/chrome/browser/android/webapk/webapk_installer.cc
@@ -402,6 +402,8 @@ void WebApkInstaller::OnURLFetchComplete(const
net::URLFetcher* source) {
 
   if (!source->GetStatus().is_success() ||
       source->GetResponseCode() != net::HTTP_OK) {
+    LOG(WARNING) << base::StringPrintf(
+        "WebAPK server returned response code %d.", source->GetResponseCode());
     OnFailure();
     return;
   }
@@ -411,12 +413,14 @@ void WebApkInstaller::OnURLFetchComplete(const
net::URLFetcher* source) {
 
   std::unique_ptr<webapk::WebApkResponse> response(new webapk::WebApkResponse);
   if (!response->ParseFromString(response_string)) {
+    LOG(WARNING) << "WebAPK server did not return proto.";
     OnFailure();
     return;
   }
 
   GURL signed_download_url(response->signed_download_url());
   if (!signed_download_url.is_valid() || response->package_name().empty()) {
+    LOG(WARNING) << "WebAPK server returned incomplete proto.";
     OnFailure();
     return;
   }

[pkotwicz, 2017-02-13 19:49:31]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-02-13 19:49:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, hanxi@chromium.org, hartmanng@chromium.org
Link to the patchset: https://codereview.chromium.org/2687163002/#ps60001
(title: "Merge branch 'master' into better_logs")

[commit-bot: I haz the power, 2017-02-13 19:50:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 20:45:06]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487015371711590,
"parent_rev": "81b2ccb859e32e2553cb74e19aef8350bccfbf21", "commit_rev":
"6d3fe481d1e1510b36c74ae18611da5646d0cff6"}

[commit-bot: I haz the power, 2017-02-13 20:46:08]

Description was changed from

==========
Add logs for WebAPK server failures

This will make it easier to determine whether a WebAPK cannot be installed due
to a WebAPK server error.

BUG=690152
==========

to

==========
Add logs for WebAPK server failures

This will make it easier to determine whether a WebAPK cannot be installed due
to a WebAPK server error.

BUG=690152

Review-Url: https://codereview.chromium.org/2687163002
Cr-Commit-Position: refs/heads/master@{#450072}
Committed:
https://chromium.googlesource.com/chromium/src/+/6d3fe481d1e1510b36c74ae18611...
==========

[commit-bot: I haz the power, 2017-02-13 20:46:09]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/6d3fe481d1e1510b36c74ae18611...