Allow installing apps via the AppBannerInfoBar

Allow installing apps via the AppBannerInfoBar

* Button cycles between "Install", "Installing", and "Open",
  depending on the installation status of the app being promoted.

* Clicking on the icon or the title of the AppBannerInfoBar should
  send the user to see details about the app that is being promoted.

* Open up access to LinkClicked() from ConfirmInfoBar.
  Piggyback on LinkClicked() to determine that the user wants to
  see the link related to the app.

* Add an ID to the icon of the infobar so it can be retrieved and
  have an OnClickListener attached to it.

* Update the status of the button when the user hops in and out
  of the Chrome application.

BUG=453170
Committed: https://crrev.com/3791e101d07edca21342c643f3b28342cb54e524
Cr-Commit-Position: refs/heads/master@{#315182}

[gone, 2015-02-06 03:50:33]

dfalcantara@chromium.org changed reviewers:
+ newt@chromium.org, tedchoc@chromium.org

[gone, 2015-02-06 03:50:33]

Follow-up CL to 896243004, which adds in all the functionality that the old
AppBannerView used to have.

[gone, 2015-02-06 03:51:43]

+Ben as FYI

[gone, 2015-02-06 20:13:29]

Rebased, PTAL.

[newt (away), 2015-02-06 22:46:27]

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:38:
public class AppBannerManager extends EmptyTabObserver implements
AppDetailsDelegate.Observer,
This class implements so many different interfaces, it would be helpful to have
each interface implementation commented, e.g

    // IntentCallback implementation

Or you could use anonymous classes to reduce the number of interfaces that
AppBannerManager implements.

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:79:
ApplicationStatus.registerApplicationStateListener(this);
Why not listen for window visibility changed events? You don't really care if
other Activities are visible in Chrome; you only care if the infobar is visible.

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:98:
mInstallTask.cancel();
Should we cancel the installation if the user exits chrome while it's still in
progress? That would surprise me as a user. (Or am I misunderstanding what
mInstallTask.cancel() does?)

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:156:
context.startActivity(launchIntent);
I seem to recall an advantage to using an Activity context when launching new
activities. Should we be using an activity context here?

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:162:
appData.installIntent(), this, R.string.low_memory_error);
Why "low memory"?

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:173:
public void onIntentCompleted(WindowAndroid window, int resultCode,
This will be called as a result of both showing app details and installing the
app. Why are both cases handled in the same way?

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBar.java
(right):

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBar.java:95:
updateButton();
why call updateButton() here?

https://codereview.chromium.org/871103004/diff/20001/chrome/browser/android/b...
File chrome/browser/android/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/871103004/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:366:
ScopedJavaLocalRef<jobject> jobj = weak_java_banner_view_manager_.get(env);
what if jobj is null? Same issue below.

https://codereview.chromium.org/871103004/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:367:
Java_AppBannerManager_monitorInstall(env,
Why not call monitorInstall() from Java?

https://codereview.chromium.org/871103004/diff/20001/chrome/browser/ui/androi...
File chrome/browser/ui/android/infobars/app_banner_infobar.h (right):

https://codereview.chromium.org/871103004/diff/20001/chrome/browser/ui/androi...
chrome/browser/ui/android/infobars/app_banner_infobar.h:32: void
OnInstallStateChanged(int new_state);
comment this method (and don't say: "Called when the install state has changed"
:) )

[gone, 2015-02-06 22:55:01]

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:79:
ApplicationStatus.registerApplicationStateListener(this);
On 2015/02/06 22:46:27, newt wrote:
> Why not listen for window visibility changed events? You don't really care if
> other Activities are visible in Chrome; you only care if the infobar is
visible.

InfoBarView isn't a legit View; I'd need to add it to InfoBarLayout.  Alright
with that?

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:98:
mInstallTask.cancel();
On 2015/02/06 22:46:27, newt wrote:
> Should we cancel the installation if the user exits chrome while it's still in
> progress? That would surprise me as a user. (Or am I misunderstanding what
> mInstallTask.cancel() does?)

This just watches the install progress; the Play Store is the one that's
actually installing things.

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:156:
context.startActivity(launchIntent);
On 2015/02/06 22:46:27, newt wrote:
> I seem to recall an advantage to using an Activity context when launching new
> activities. Should we be using an activity context here?

Doesn't seem to matter here; apps launch just fine.

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:162:
appData.installIntent(), this, R.string.low_memory_error);
On 2015/02/06 22:46:27, newt wrote:
> Why "low memory"? 

Carry over from the AppBannerView.  Most of the other calling interfaces also
(strangely) use this string for lack of a better one.

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:173:
public void onIntentCompleted(WindowAndroid window, int resultCode,
On 2015/02/06 22:46:27, newt wrote:
> This will be called as a result of both showing app details and installing the
> app. Why are both cases handled in the same way?

This is only called as a result of the Play Store Intent returning after showing
app details.

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBar.java
(right):

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBar.java:95:
updateButton();
On 2015/02/06 22:46:27, newt wrote:
> why call updateButton() here?

Needed to re-disable the "installing" button after re-enabling clicking on the
infobar close button.

https://codereview.chromium.org/871103004/diff/20001/chrome/browser/android/b...
File chrome/browser/android/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/871103004/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:367:
Java_AppBannerManager_monitorInstall(env,
On 2015/02/06 22:46:27, newt wrote:
> Why not call monitorInstall() from Java?

AppData isn't available from there; it doesn't store it.

[gone, 2015-02-06 23:05:45]

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:173:
public void onIntentCompleted(WindowAndroid window, int resultCode,
On 2015/02/06 22:55:00, dfalcantara wrote:
> On 2015/02/06 22:46:27, newt wrote:
> > This will be called as a result of both showing app details and installing
the
> > app. Why are both cases handled in the same way?
> 
> This is only called as a result of the Play Store Intent returning after
showing
> app details.

Ah, got it.  It's handled the same way because the Java class doesn't really
know anything about the package or Intent being fired.  It's just acting on
behalf of the native code, which houses the logic for what to do after an Intent
returns.

[gone, 2015-02-06 23:08:05]

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:173:
public void onIntentCompleted(WindowAndroid window, int resultCode,
On 2015/02/06 23:05:45, dfalcantara wrote:
> On 2015/02/06 22:55:00, dfalcantara wrote:
> > On 2015/02/06 22:46:27, newt wrote:
> > > This will be called as a result of both showing app details and installing
> the
> > > app. Why are both cases handled in the same way?
> > 
> > This is only called as a result of the Play Store Intent returning after
> showing
> > app details.
> 
> Ah, got it.  It's handled the same way because the Java class doesn't really
> know anything about the package or Intent being fired.  It's just acting on
> behalf of the native code, which houses the logic for what to do after an
Intent
> returns.

Redacting that again.  Showing the app details should just hide the infobar
(which it does).  Shouldn't need to have a callback in that case...

[newt (away), 2015-02-06 23:15:47]

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:79:
ApplicationStatus.registerApplicationStateListener(this);
On 2015/02/06 22:55:00, dfalcantara wrote:
> On 2015/02/06 22:46:27, newt wrote:
> > Why not listen for window visibility changed events? You don't really care
if
> > other Activities are visible in Chrome; you only care if the infobar is
> visible.
> 
> InfoBarView isn't a legit View; I'd need to add it to InfoBarLayout.  Alright
> with that?

We could add it to one of the custom views in the AppBannerInfobar. Let's wait
until the UI is in place.

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:98:
mInstallTask.cancel();
On 2015/02/06 22:55:00, dfalcantara wrote:
> On 2015/02/06 22:46:27, newt wrote:
> > Should we cancel the installation if the user exits chrome while it's still
in
> > progress? That would surprise me as a user. (Or am I misunderstanding what
> > mInstallTask.cancel() does?)
> 
> This just watches the install progress; the Play Store is the one that's
> actually installing things.

Gotcha. Sounds good then.

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:162:
appData.installIntent(), this, R.string.low_memory_error);
On 2015/02/06 22:55:00, dfalcantara wrote:
> On 2015/02/06 22:46:27, newt wrote:
> > Why "low memory"? 
> 
> Carry over from the AppBannerView.  Most of the other calling interfaces also
> (strangely) use this string for lack of a better one.

We might as well say "Too many camels" here. This is a meaningless and
misleading error message.

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:173:
public void onIntentCompleted(WindowAndroid window, int resultCode,
On 2015/02/06 23:08:05, dfalcantara wrote:
> On 2015/02/06 23:05:45, dfalcantara wrote:
> > On 2015/02/06 22:55:00, dfalcantara wrote:
> > > On 2015/02/06 22:46:27, newt wrote:
> > > > This will be called as a result of both showing app details and
installing
> > the
> > > > app. Why are both cases handled in the same way?
> > > 
> > > This is only called as a result of the Play Store Intent returning after
> > showing
> > > app details.
> > 
> > Ah, got it.  It's handled the same way because the Java class doesn't really
> > know anything about the package or Intent being fired.  It's just acting on
> > behalf of the native code, which houses the logic for what to do after an
> Intent
> > returns.
> 
> Redacting that again.  Showing the app details should just hide the infobar
> (which it does).  Shouldn't need to have a callback in that case...

I'm not crazy after all :)

[gone, 2015-02-06 23:32:08]

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:38:
public class AppBannerManager extends EmptyTabObserver implements
AppDetailsDelegate.Observer,
On 2015/02/06 22:46:27, newt wrote:
> This class implements so many different interfaces, it would be helpful to
have
> each interface implementation commented, e.g
> 
>     // IntentCallback implementation
> 
> Or you could use anonymous classes to reduce the number of interfaces that
> AppBannerManager implements.

Done.

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:162:
appData.installIntent(), this, R.string.low_memory_error);
On 2015/02/06 23:15:47, newt wrote:
> On 2015/02/06 22:55:00, dfalcantara wrote:
> > On 2015/02/06 22:46:27, newt wrote:
> > > Why "low memory"? 
> > 
> > Carry over from the AppBannerView.  Most of the other calling interfaces
also
> > (strangely) use this string for lack of a better one.
> 
> We might as well say "Too many camels" here. This is a meaningless and
> misleading error message.

Debatably... the error message indicates that Chrome was killed before the user
could make it back.  What message would you suggest?

https://codereview.chromium.org/871103004/diff/20001/chrome/browser/android/b...
File chrome/browser/android/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/871103004/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:366:
ScopedJavaLocalRef<jobject> jobj = weak_java_banner_view_manager_.get(env);
On 2015/02/06 22:46:27, newt wrote:
> what if jobj is null? Same issue below.

Null checking now.

https://codereview.chromium.org/871103004/diff/20001/chrome/browser/ui/androi...
File chrome/browser/ui/android/infobars/app_banner_infobar.h (right):

https://codereview.chromium.org/871103004/diff/20001/chrome/browser/ui/androi...
chrome/browser/ui/android/infobars/app_banner_infobar.h:32: void
OnInstallStateChanged(int new_state);
On 2015/02/06 22:46:27, newt wrote:
> comment this method (and don't say: "Called when the install state has
changed"
> :) )

Added a "may have" instead of a has.

[gone, 2015-02-07 00:20:42]

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://codereview.chromium.org/871103004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:162:
appData.installIntent(), this, R.string.low_memory_error);
On 2015/02/06 23:32:08, dfalcantara wrote:
> On 2015/02/06 23:15:47, newt wrote:
> > On 2015/02/06 22:55:00, dfalcantara wrote:
> > > On 2015/02/06 22:46:27, newt wrote:
> > > > Why "low memory"? 
> > > 
> > > Carry over from the AppBannerView.  Most of the other calling interfaces
> also
> > > (strangely) use this string for lack of a better one.
> > 
> > We might as well say "Too many camels" here. This is a meaningless and
> > misleading error message.
> 
> Debatably... the error message indicates that Chrome was killed before the
user
> could make it back.  What message would you suggest?

Working with Dave to get rid of the error message requirement.  We don't
actually care if Chrome dies since the app will already be installing (or not)
by the time it gets back.  If Chrome dies, the infobar's gone anyway.  Are you
alright with landing while I land that other patch?

[newt (away), 2015-02-07 01:26:28]

lgtm

https://codereview.chromium.org/871103004/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://codereview.chromium.org/871103004/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:196:
nativeOnIntentCompleted(mNativePointer, resultCode == Activity.RESULT_OK);
Can we rename nativeOnIntentCompleted() to nativeOnInstallStarted(), or is that
not an accurate name? The current name is rather vague.

[newt (away), 2015-02-07 02:15:10]

https://codereview.chromium.org/871103004/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java
(right):

https://codereview.chromium.org/871103004/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:177:
mIconView.setId(R.id.infobar_icon);
This isn't needed. I've added a method getIcon() to InfoBarLayout, which should
be used instead.

[gone, 2015-02-07 02:36:54]

New patchsets have been uploaded after l-g-t-m from newt@chromium.org

[gone, 2015-02-07 02:58:00]

https://codereview.chromium.org/871103004/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://codereview.chromium.org/871103004/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:196:
nativeOnIntentCompleted(mNativePointer, resultCode == Activity.RESULT_OK);
On 2015/02/07 01:26:28, newt wrote:
> Can we rename nativeOnIntentCompleted() to nativeOnInstallStarted(), or is
that
> not an accurate name? The current name is rather vague. 

Changed it to OnInstallIntentReturned.  Also fixed a bug that starts monitoring
for an install that might never have been started.

https://codereview.chromium.org/871103004/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java
(right):

https://codereview.chromium.org/871103004/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:177:
mIconView.setId(R.id.infobar_icon);
On 2015/02/07 02:15:09, newt wrote:
> This isn't needed. I've added a method getIcon() to InfoBarLayout, which
should
> be used instead.

Done.

[gone, 2015-02-07 03:18:57]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2015-02-07 03:19:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/871103004/140001

[commit-bot: I haz the power, 2015-02-07 04:05:18]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-07 04:05:46]

Patchset 8 (id:??) landed as
https://crrev.com/3791e101d07edca21342c643f3b28342cb54e524
Cr-Commit-Position: refs/heads/master@{#315182}