Follow up "Trigger app banner when add to homescreen is pressed and WebAPKs are enabled."

Follow up "Trigger app banner when add to homescreen is pressed and WebAPKs are enabled."

This is a follow up CL to address the remaining comments on CL
(https://codereview.chromium.org/2290603005/).

BUG=627250
Committed: https://crrev.com/fba4f93bd21ac63a49812ccf57c6ac0045337799
Cr-Commit-Position: refs/heads/master@{#417934}

[Xi Han, 2016-09-09 18:52:25]

Description was changed from

==========
Follow up "Trigger app banner when add to homescreen is pressed and WebAPKs are
enabled."

This is a follow up CL to address the remaining comments on CL
(https://codereview.chromium.org/2290603005/).

BUG=627250
==========

to

==========
Follow up "Trigger app banner when add to homescreen is pressed and WebAPKs are
enabled."

This is a follow up CL to address the remaining comments on CL
(https://codereview.chromium.org/2290603005/).

BUG=627250
==========

[Xi Han, 2016-09-09 18:52:25]

hanxi@chromium.org changed reviewers:
+ pkasting@chromium.org

[Xi Han, 2016-09-09 18:53:45]

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

[Xi Han, 2016-09-09 18:54:18]

Hi pkasting@: could you please take a look? Thanks!

[Peter Kasting, 2016-09-09 21:50:45]

LGTM

https://codereview.chromium.org/2324263002/diff/1/chrome/browser/android/bann...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2324263002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:43: return
!info.get() || info->url.is_empty();
Nit: .get() probably not necessary

https://codereview.chromium.org/2324263002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:370:
DVLOG(1) << "The WebAPK installation failed.";
Nit: Move this to the top of the block and replace the then-redundant comment?

https://codereview.chromium.org/2324263002/diff/1/chrome/browser/android/bann...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2324263002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_manager_android.cc:249:
referrer.append("&").append(kPlayInlineReferrer);
Nit: Slightly simpler:

  // Attach the chrome_inline referrer value.
  std::string referrer = ExtractQueryValueForName(url, "referrer");
  if (!referrer.empty())
    referrer += "&";
  const char kPlayInlineReferrer[] = "playinline=chrome_inline";
  referrer += kPlayInlineReferrer;

[dominickn, 2016-09-12 03:50:32]

lgtm % pkasting's nits.

[Xi Han, 2016-09-12 13:49:51]

https://codereview.chromium.org/2324263002/diff/1/chrome/browser/android/bann...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2324263002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:43: return
!info.get() || info->url.is_empty();
On 2016/09/09 21:50:45, Peter Kasting wrote:
> Nit: .get() probably not necessary

Done.

https://codereview.chromium.org/2324263002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:370:
DVLOG(1) << "The WebAPK installation failed.";
On 2016/09/09 21:50:45, Peter Kasting wrote:
> Nit: Move this to the top of the block and replace the then-redundant comment?

Done.

https://codereview.chromium.org/2324263002/diff/1/chrome/browser/android/bann...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2324263002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_manager_android.cc:249:
referrer.append("&").append(kPlayInlineReferrer);
On 2016/09/09 21:50:45, Peter Kasting wrote:
> Nit: Slightly simpler:
> 
>   // Attach the chrome_inline referrer value.
>   std::string referrer = ExtractQueryValueForName(url, "referrer");
>   if (!referrer.empty())
>     referrer += "&";
>   const char kPlayInlineReferrer[] = "playinline=chrome_inline";
>   referrer += kPlayInlineReferrer;

Done.

[Xi Han, 2016-09-12 13:49:56]

The CQ bit was checked by hanxi@chromium.org

[Xi Han, 2016-09-12 13:49:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2324263002/#ps20001
(title: "Nits.")

[commit-bot: I haz the power, 2016-09-12 13:50:07]

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, 2016-09-12 14:31:32]

Description was changed from

==========
Follow up "Trigger app banner when add to homescreen is pressed and WebAPKs are
enabled."

This is a follow up CL to address the remaining comments on CL
(https://codereview.chromium.org/2290603005/).

BUG=627250
==========

to

==========
Follow up "Trigger app banner when add to homescreen is pressed and WebAPKs are
enabled."

This is a follow up CL to address the remaining comments on CL
(https://codereview.chromium.org/2290603005/).

BUG=627250
==========

[commit-bot: I haz the power, 2016-09-12 14:31:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-12 14:33:22]

Description was changed from

==========
Follow up "Trigger app banner when add to homescreen is pressed and WebAPKs are
enabled."

This is a follow up CL to address the remaining comments on CL
(https://codereview.chromium.org/2290603005/).

BUG=627250
==========

to

==========
Follow up "Trigger app banner when add to homescreen is pressed and WebAPKs are
enabled."

This is a follow up CL to address the remaining comments on CL
(https://codereview.chromium.org/2290603005/).

BUG=627250

Committed: https://crrev.com/fba4f93bd21ac63a49812ccf57c6ac0045337799
Cr-Commit-Position: refs/heads/master@{#417934}
==========

[commit-bot: I haz the power, 2016-09-12 14:33:23]

Patchset 2 (id:??) landed as
https://crrev.com/fba4f93bd21ac63a49812ccf57c6ac0045337799
Cr-Commit-Position: refs/heads/master@{#417934}