Let AppBannerManager really create and manage banners

Update AppBannerManager so that it can create and dismiss Banners

* AppBannerManager is given the ability to create banners.  See 
  the C++ header for details on how the asynchronous pipeline works.

* DetailsDelegate class is added to provide information to the 
  AppBannerManager about packages being advertised.

* AppBannerManager dismisses banners after navigations.

* AppBannerManager fires intents provided by the DetailsDelegate
  to launch the Play store and installer.

* AppBannerView is notified when the install state changes so it
  can update its buttons.

Design doc (has image showing how the pipeline works, text is out of date):
http://goo.gl/a77OXx

Depends on: 
https://chromiumcodereview.appspot.com/141853007/
https://chromiumcodereview.appspot.com/156013005/

NOTRY=true
BUG=341556
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252557

[gone, 2014-02-14 19:59:16]

With your powers combined, hopefully you can catch any wackiness with my
implementation of the asynchronicity.

[gone, 2014-02-15 00:13:57]

FWIW, I made an image which hopefully clarifies the pipeline a bit:
http://goo.gl/a77OXx

[Yaron, 2014-02-19 22:41:35]

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

https://codereview.chromium.org/156343002/diff/90001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:141:
if (!isBannerForCurrentPage(url) || sDetailsDelegate == null) return null;
Hmm. I think these url-related calls should be on the UI thread. I think instead
you should have |dismissCurrentBanner| cancel this async task. You can do a
final check in onPostExecute whether to throw away the result you just computed
if it's no longer valid. Oh you already do that.. so ya, I think just drop it
from here and add the |cancel| call.

https://codereview.chromium.org/156343002/diff/90001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:143:
data.siteUrl = mContentView.getUrl();
Same point here. If you need this on bg thread, grab it ahead of time or in
onPostExecute.

https://codereview.chromium.org/156343002/diff/90001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/DetailsDelegate.java
(right):

https://codereview.chromium.org/156343002/diff/90001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/DetailsDelegate.java:10:
public interface DetailsDelegate {
Nit: rename AppDetailsDelegate

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

https://codereview.chromium.org/156343002/diff/90001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:64:
ScopedJavaLocalRef<jobject> jobj = weak_java_banner_view_manager_.get(env);
I think this can legitimately be null as the message comes from the renderer but
the Tab (only strong ref to java object) can go away.

https://codereview.chromium.org/156343002/diff/90001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:80: VLOG(1) << "Failed to
retrieve image: " << url;
DVLOG

https://codereview.chromium.org/156343002/diff/90001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:98:
ScopedJavaLocalRef<jobject> jobj = weak_java_banner_view_manager_.get(env);
null-check

https://codereview.chromium.org/156343002/diff/90001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:117: return;
Is this early return sufficient? Shouldn't you call the java |resetState| ?

https://codereview.chromium.org/156343002/diff/90001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:123: fetcher_.reset(new
chrome::BitmapFetcher(GURL(image_url), this));
Do you need to check whether the image_url is valid before fetching it? Is there
any sanitization we need to do?

[gone, 2014-02-20 22:07:23]

https://chromiumcodereview.appspot.com/156343002/diff/90001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://chromiumcodereview.appspot.com/156343002/diff/90001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:141:
if (!isBannerForCurrentPage(url) || sDetailsDelegate == null) return null;
On 2014/02/19 22:41:36, Yaron wrote:
> Hmm. I think these url-related calls should be on the UI thread. I think
instead
> you should have |dismissCurrentBanner| cancel this async task. You can do a
> final check in onPostExecute whether to throw away the result you just
computed
> if it's no longer valid. Oh you already do that.. so ya, I think just drop it
> from here and add the |cancel| call.

Done.

https://chromiumcodereview.appspot.com/156343002/diff/90001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:143:
data.siteUrl = mContentView.getUrl();
On 2014/02/19 22:41:36, Yaron wrote:
> Same point here. If you need this on bg thread, grab it ahead of time or in
> onPostExecute.

Done.

https://chromiumcodereview.appspot.com/156343002/diff/90001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/DetailsDelegate.java
(right):

https://chromiumcodereview.appspot.com/156343002/diff/90001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/DetailsDelegate.java:10:
public interface DetailsDelegate {
On 2014/02/19 22:41:36, Yaron wrote:
> Nit: rename AppDetailsDelegate

Done.

https://chromiumcodereview.appspot.com/156343002/diff/90001/chrome/browser/an...
File chrome/browser/android/banners/app_banner_manager.cc (right):

https://chromiumcodereview.appspot.com/156343002/diff/90001/chrome/browser/an...
chrome/browser/android/banners/app_banner_manager.cc:64:
ScopedJavaLocalRef<jobject> jobj = weak_java_banner_view_manager_.get(env);
On 2014/02/19 22:41:36, Yaron wrote:
> I think this can legitimately be null as the message comes from the renderer
but
> the Tab (only strong ref to java object) can go away.

Entirely possible... added null checks everywhere.

https://chromiumcodereview.appspot.com/156343002/diff/90001/chrome/browser/an...
chrome/browser/android/banners/app_banner_manager.cc:80: VLOG(1) << "Failed to
retrieve image: " << url;
On 2014/02/19 22:41:36, Yaron wrote:
> DVLOG

Done.

https://chromiumcodereview.appspot.com/156343002/diff/90001/chrome/browser/an...
chrome/browser/android/banners/app_banner_manager.cc:98:
ScopedJavaLocalRef<jobject> jobj = weak_java_banner_view_manager_.get(env);
On 2014/02/19 22:41:36, Yaron wrote:
> null-check

Done.

https://chromiumcodereview.appspot.com/156343002/diff/90001/chrome/browser/an...
chrome/browser/android/banners/app_banner_manager.cc:117: return;
On 2014/02/19 22:41:36, Yaron wrote:
> Is this early return sufficient? Shouldn't you call the java |resetState| ?

Cleaned up the conditions so that this side doesn't check the URLs: those checks
are done on the Java side where it can reset the state as necessary.

https://chromiumcodereview.appspot.com/156343002/diff/90001/chrome/browser/an...
chrome/browser/android/banners/app_banner_manager.cc:123: fetcher_.reset(new
chrome::BitmapFetcher(GURL(image_url), this));
On 2014/02/19 22:41:36, Yaron wrote:
> Do you need to check whether the image_url is valid before fetching it? Is
there
> any sanitization we need to do?

No objection here, but any idea how we could go about validating the URL?

[Yaron, 2014-02-21 02:50:51]

lgtm

https://chromiumcodereview.appspot.com/156343002/diff/140001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://chromiumcodereview.appspot.com/156343002/diff/140001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:156:
return;
Unneeded

https://chromiumcodereview.appspot.com/156343002/diff/140001/chrome/browser/a...
File chrome/browser/android/banners/app_banner_manager.cc (right):

https://chromiumcodereview.appspot.com/156343002/diff/140001/chrome/browser/a...
chrome/browser/android/banners/app_banner_manager.cc:76: return;
I assume you have to free the bitmap or it'll leak.

[Yaron, 2014-02-21 02:50:54]

lgtm

lgtm

[Yaron, 2014-02-21 02:51:28]

https://chromiumcodereview.appspot.com/156343002/diff/140001/chrome/browser/a...
File chrome/browser/android/banners/app_banner_manager.cc (right):

https://chromiumcodereview.appspot.com/156343002/diff/140001/chrome/browser/a...
chrome/browser/android/banners/app_banner_manager.cc:76: return;
On 2014/02/21 02:50:51, Yaron wrote:
> I assume you have to free the bitmap or it'll leak.

Ignore this. The caller seems to own it and clean it up.

[gone, 2014-02-21 08:13:47]

https://chromiumcodereview.appspot.com/156343002/diff/140001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://chromiumcodereview.appspot.com/156343002/diff/140001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:156:
return;
On 2014/02/21 02:50:51, Yaron wrote:
> Unneeded

Done.

[gone, 2014-02-21 08:14:11]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2014-02-21 08:14:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/156343002/320001

[commit-bot: I haz the power, 2014-02-21 09:53:40]

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

[commit-bot: I haz the power, 2014-02-21 09:53:40]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[gone, 2014-02-21 15:06:15]

On 2014/02/21 09:53:40, I haz the power (commit-bot) wrote:
> Retried try job too often on win_rel for step(s) browser_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Submitting NOTRY=true to get around flakes.

[gone, 2014-02-21 15:06:29]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2014-02-21 15:06:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/156343002/320001

[commit-bot: I haz the power, 2014-02-21 15:07:43]

Change committed as 252557