Remove unused open WebAPK logic in AppBannerInfoBarDelegateAndroid.

Remove unused "open WebAPK" logic in AppBannerInfoBarDelegateAndroid.

https://codereview.chromium.org/2707993003/ changes the menu item of
"Add to home screen" to "Open [WebAPK]" if the WebAPK is already
installed, making "open WebAPK" logic in
AppBannerInfoBarDelegateAndroid obselete.

BUG=696133
Review-Url: https://codereview.chromium.org/2751423002
Cr-Commit-Position: refs/heads/master@{#459070}
Committed: https://chromium.googlesource.com/chromium/src/+/5e8963652e6ed98c45ddbbe23a72db8b5b52d25b

[F, 2017-03-16 14:51:32]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 14:51:49]

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

[F, 2017-03-16 14:52:23]

Description was changed from

==========
Remove unused open WebAPK logic in AppBannerInfoBarDelegateAndroid.

BUG=
==========

to

==========
Remove unused open WebAPK logic in AppBannerInfoBarDelegateAndroid.

BUG=696133
==========

[commit-bot: I haz the power, 2017-03-16 15:32:48]

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

[commit-bot: I haz the power, 2017-03-16 15:32:49]

Dry run: This issue passed the CQ dry run.

[F, 2017-03-16 15:38:10]

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

[F, 2017-03-16 15:38:11]

Hi Dominick, PTAL. Thanks!

[dominickn, 2017-03-16 23:42:04]

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

https://codereview.chromium.org/2751423002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:58:
std::string webapk_package_name = "";
This variable isn't used anymore, remove.

https://codereview.chromium.org/2751423002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:59: const
GURL& url = shortcut_info->url;
Remove this variable and just pass shortcut_info->url into the
base::MakeUnique<AppBannerInfoBarAndroid> call lower down.

https://codereview.chromium.org/2751423002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:419:
webapk::TrackUserAction(webapk::USER_ACTION_OPEN_DISMISS);
OPEN_DISMISS and OPEN are no longer triggered by anything right? Can you
annotate that this is the case in
chrome/browser/android/webapk/webapk_metrics.h? Don't remove them from the enum
since that will mess up the histograms backend.

[F, 2017-03-17 15:26:38]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[F, 2017-03-17 15:26:53]

Thanks Dominick, PTAL!

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

https://codereview.chromium.org/2751423002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:58:
std::string webapk_package_name = "";
On 2017/03/16 23:42:04, dominickn wrote:
> This variable isn't used anymore, remove.

Done.

https://codereview.chromium.org/2751423002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:59: const
GURL& url = shortcut_info->url;
On 2017/03/16 23:42:04, dominickn wrote:
> Remove this variable and just pass shortcut_info->url into the
> base::MakeUnique<AppBannerInfoBarAndroid> call lower down.

Done.

https://codereview.chromium.org/2751423002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:419:
webapk::TrackUserAction(webapk::USER_ACTION_OPEN_DISMISS);
On 2017/03/16 23:42:04, dominickn wrote:
> OPEN_DISMISS and OPEN are no longer triggered by anything right? Can you
> annotate that this is the case in
> chrome/browser/android/webapk/webapk_metrics.h? Don't remove them from the
enum
> since that will mess up the histograms backend.

Done.

[commit-bot: I haz the power, 2017-03-17 15:27:04]

Dry run: 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-03-17 16:19:07]

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

[commit-bot: I haz the power, 2017-03-17 16:19:08]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pkotwicz, 2017-03-17 16:51:34]

Description was changed from

==========
Remove unused open WebAPK logic in AppBannerInfoBarDelegateAndroid.

BUG=696133
==========

to

==========
Remove unused "open WebAPK" logic in AppBannerInfoBarDelegateAndroid.

BUG=696133
==========

[pkotwicz, 2017-03-17 16:51:48]

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

[pkotwicz, 2017-03-17 16:51:49]

Drive by: Can you please clarify the CL description and mention the CL which
makes the code no longer necessary

[F, 2017-03-17 17:19:07]

Description was changed from

==========
Remove unused "open WebAPK" logic in AppBannerInfoBarDelegateAndroid.

BUG=696133
==========

to

==========
Remove unused "open WebAPK" logic in AppBannerInfoBarDelegateAndroid.

https://codereview.chromium.org/2707993003/ changes the menu item of
"Add to home screen" to "Open [WebAPK]" if the WebAPK is already
installed, making "open WebAPK" logic in
AppBannerInfoBarDelegateAndroid obselete.

BUG=696133
==========

[F, 2017-03-17 18:14:24]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 18:15:02]

Dry run: 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-03-17 19:41:07]

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

[commit-bot: I haz the power, 2017-03-17 19:41:08]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dominickn, 2017-03-19 22:56:56]

https://codereview.chromium.org/2751423002/diff/20001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2751423002/diff/20001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:64:
std::move(infobar_delegate), shortcut_info->url, is_webapk);
Oops, I gave you bad advice here. |shortcut_info| was std::move'd into the
AppBannerInfoBarDelegateAndroid on line 58, so accessing shortcut_info->url
segmentation faults and fails the tests.

That being said, I think |url| should never have been a reference type before
because the thing it's referring to is on an object that is being std::move'd
out from under it. Can you bring the |url| variable back except make it a value,
not a reference?

[F, 2017-03-22 14:23:14]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 14:23:32]

Dry run: 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-03-22 14:59:45]

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

[commit-bot: I haz the power, 2017-03-22 14:59:46]

Dry run: This issue passed the CQ dry run.

[F, 2017-03-22 18:49:23]

Thanks Dominick & Peter, PTAL!

https://codereview.chromium.org/2751423002/diff/20001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2751423002/diff/20001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:64:
std::move(infobar_delegate), shortcut_info->url, is_webapk);
On 2017/03/19 22:56:56, dominickn wrote:
> Oops, I gave you bad advice here. |shortcut_info| was std::move'd into the
> AppBannerInfoBarDelegateAndroid on line 58, so accessing shortcut_info->url
> segmentation faults and fails the tests.
> 
> That being said, I think |url| should never have been a reference type before
> because the thing it's referring to is on an object that is being std::move'd
> out from under it. Can you bring the |url| variable back except make it a
value,
> not a reference?

Thanks for finding the cause! Done :)

[dominickn, 2017-03-22 23:20:33]

lgtm, thanks. Sorry for the bad advice again!

[F, 2017-03-23 13:59:54]

The CQ bit was checked by zpeng@chromium.org

[commit-bot: I haz the power, 2017-03-23 14:00:16]

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-03-23 14:04:41]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490277594186860,
"parent_rev": "08f7da01c04697fb873396bdf84b4ee34175fddd", "commit_rev":
"5e8963652e6ed98c45ddbbe23a72db8b5b52d25b"}

[commit-bot: I haz the power, 2017-03-23 14:05:21]

Description was changed from

==========
Remove unused "open WebAPK" logic in AppBannerInfoBarDelegateAndroid.

https://codereview.chromium.org/2707993003/ changes the menu item of
"Add to home screen" to "Open [WebAPK]" if the WebAPK is already
installed, making "open WebAPK" logic in
AppBannerInfoBarDelegateAndroid obselete.

BUG=696133
==========

to

==========
Remove unused "open WebAPK" logic in AppBannerInfoBarDelegateAndroid.

https://codereview.chromium.org/2707993003/ changes the menu item of
"Add to home screen" to "Open [WebAPK]" if the WebAPK is already
installed, making "open WebAPK" logic in
AppBannerInfoBarDelegateAndroid obselete.

BUG=696133

Review-Url: https://codereview.chromium.org/2751423002
Cr-Commit-Position: refs/heads/master@{#459070}
Committed:
https://chromium.googlesource.com/chromium/src/+/5e8963652e6ed98c45ddbbe23a72...
==========

[commit-bot: I haz the power, 2017-03-23 14:05:22]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5e8963652e6ed98c45ddbbe23a72...