Create a skeleton AppBannerManager class

Create a skeleton AppBannerManager class

* Adds an AppBannerManager that will be used to manage a single
  AppBannerView.
* Each AppBannerManager is owned by a particular TabBase and is
  alerted to changes its TabBase by a TabObserver.
  The TabObserver's main use is to update the AppBannerManager
  when the WebContents has changed.

Note that this class is a skeleton: most of the real functionality
(like creating a AppBannerView) has been parted off.

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

[gone, 2014-02-11 09:26:54]

Hey guys, this is a skeleton for the real class.  Figured I'd check it in and
then add the rest of the bits later so the things I'm adding through other CLs
would have a more obvious hook.

[Ted C, 2014-02-11 17:48:45]

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

https://codereview.chromium.org/155503004/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:49:
return new EmptyTabObserver() {
Hmm...I wonder if we are missing the callback for when we unfreeze tabs.  I
don't see that any of these will get called in that case, might want to check
that case.

https://codereview.chromium.org/155503004/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:73:
if (mContentView != contentView) {
this looks like it will all fit on one line

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

https://codereview.chromium.org/155503004/diff/60001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:24: :
MetaTagObserver("google-play-id"),
should this be a static?

https://codereview.chromium.org/155503004/diff/60001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:28:
weak_java_banner_view_manager_.reset();
won't this happen in the destructor of the java weak ptr?

https://codereview.chromium.org/155503004/diff/60001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:31: void
AppBannerManager::Destroy(JNIEnv* env, jobject obj) { delete this; }
This looks odd to me.  I would probably put the delete on another line.

https://codereview.chromium.org/155503004/diff/60001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:38: if (web_contents() !=
content_view_core->GetWebContents())
looks like web_contents_observer already does this check for you.

Also, looking at this, I would split up the ContentView tracking in your java
code from the web_contents updating bit.  Instead of passing a
native_content_view_core.  Pass TabBase.getWebContents();

Then use:
WebContents::FromJavaWebContents(jobject jweb_contents); To get the native
pointer.

https://codereview.chromium.org/155503004/diff/60001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:51: web_contents(),
expected_url, tag_content)) {
This also looks odd to me.  Maybe:
!AppBannerSettingsHelper::IsAllowed(web_contents(),
                                    expected_url,
                                    tag_content)

Also, how could web_contents() not be set?  That seems like it would be a fine
DCHECK.  What would it be observing if you didn't have a web_contents?

https://codereview.chromium.org/155503004/diff/60001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:55: request_url_ =
GURL(expected_url);
Do you actually need to store these in native?  Seems like you might only need
to pump them out to java.

https://codereview.chromium.org/155503004/diff/60001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:76: bool
RegisterAppBannerManager(JNIEnv* env) { return RegisterNativesImpl(env); }
same comment about the delete above.  I don't see this form in .cc usually.

[gone, 2014-02-11 19:04:14]

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

https://chromiumcodereview.appspot.com/155503004/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:49:
return new EmptyTabObserver() {
On 2014/02/11 17:48:45, Ted C wrote:
> Hmm...I wonder if we are missing the callback for when we unfreeze tabs.  I
> don't see that any of these will get called in that case, might want to check
> that case.

Added the observer callback you mentioned.

https://chromiumcodereview.appspot.com/155503004/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:73:
if (mContentView != contentView) {
On 2014/02/11 17:48:45, Ted C wrote:
> this looks like it will all fit on one line

Done.

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

https://chromiumcodereview.appspot.com/155503004/diff/60001/chrome/browser/an...
chrome/browser/android/banners/app_banner_manager.cc:24: :
MetaTagObserver("google-play-id"),
On 2014/02/11 17:48:45, Ted C wrote:
> should this be a static?

Done.

https://chromiumcodereview.appspot.com/155503004/diff/60001/chrome/browser/an...
chrome/browser/android/banners/app_banner_manager.cc:28:
weak_java_banner_view_manager_.reset();
On 2014/02/11 17:48:45, Ted C wrote:
> won't this happen in the destructor of the java weak ptr?

Done.

https://chromiumcodereview.appspot.com/155503004/diff/60001/chrome/browser/an...
chrome/browser/android/banners/app_banner_manager.cc:31: void
AppBannerManager::Destroy(JNIEnv* env, jobject obj) { delete this; }
On 2014/02/11 17:48:45, Ted C wrote:
> This looks odd to me.  I would probably put the delete on another line.

Done.

https://chromiumcodereview.appspot.com/155503004/diff/60001/chrome/browser/an...
chrome/browser/android/banners/app_banner_manager.cc:38: if (web_contents() !=
content_view_core->GetWebContents())
On 2014/02/11 17:48:45, Ted C wrote:
> looks like web_contents_observer already does this check for you.
> 
> Also, looking at this, I would split up the ContentView tracking in your java
> code from the web_contents updating bit.  Instead of passing a
> native_content_view_core.  Pass TabBase.getWebContents();
> 
> Then use:
> WebContents::FromJavaWebContents(jobject jweb_contents); To get the native
> pointer.

Done.

https://chromiumcodereview.appspot.com/155503004/diff/60001/chrome/browser/an...
chrome/browser/android/banners/app_banner_manager.cc:51: web_contents(),
expected_url, tag_content)) {
On 2014/02/11 17:48:45, Ted C wrote:
> This also looks odd to me.  Maybe:
> !AppBannerSettingsHelper::IsAllowed(web_contents(),
>                                     expected_url,
>                                     tag_content)
> 
> Also, how could web_contents() not be set?  That seems like it would be a fine
> DCHECK.  What would it be observing if you didn't have a web_contents?

Formatter sure is fond of doing weird things.

https://chromiumcodereview.appspot.com/155503004/diff/60001/chrome/browser/an...
chrome/browser/android/banners/app_banner_manager.cc:55: request_url_ =
GURL(expected_url);
On 2014/02/11 17:48:45, Ted C wrote:
> Do you actually need to store these in native?  Seems like you might only need
> to pump them out to java.

They're needed for the rest of the async pipeline, since tasks get fired from
both native and Java land.  I'm taking them out of this CL since they're not
used yet.

https://chromiumcodereview.appspot.com/155503004/diff/60001/chrome/browser/an...
chrome/browser/android/banners/app_banner_manager.cc:76: bool
RegisterAppBannerManager(JNIEnv* env) { return RegisterNativesImpl(env); }
On 2014/02/11 17:48:45, Ted C wrote:
> same comment about the delete above.  I don't see this form in .cc usually.

Yay formatter!

[David Trainor- moved to gerrit, 2014-02-11 20:21:39]

https://chromiumcodereview.appspot.com/155503004/diff/120001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):

https://chromiumcodereview.appspot.com/155503004/diff/120001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:680:
mAppBannerManager = new AppBannerManager(this);
Unregister this in destroyContentView?  Maybe we need another onContentChanged
:/.

[gone, 2014-02-12 19:08:30]

https://chromiumcodereview.appspot.com/155503004/diff/120001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):

https://chromiumcodereview.appspot.com/155503004/diff/120001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:680:
mAppBannerManager = new AppBannerManager(this);
On 2014/02/11 20:21:40, David Trainor wrote:
> Unregister this in destroyContentView?  Maybe we need another onContentChanged
> :/.

Added it, haven't seen any ill side effects (yet).

[Ted C, 2014-02-13 03:57:50]

https://codereview.chromium.org/155503004/diff/220001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):

https://codereview.chromium.org/155503004/diff/220001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:785: for
(TabObserver observer : mObservers) observer.onContentChanged(this);
hmm...this gives me the scares.  At this point, native will be gone and the tab
should be dead, which is an odd time to try and start interacting with it.  I
think we should only keep the onDestroyed above and require people to listen to
both.

https://codereview.chromium.org/155503004/diff/220001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager.h (right):

https://codereview.chromium.org/155503004/diff/220001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager.h:50: JavaObjectWeakGlobalRef
weak_java_banner_view_manager_;
I would put a blank line after this

[gone, 2014-02-13 05:01:16]

https://codereview.chromium.org/155503004/diff/220001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):

https://codereview.chromium.org/155503004/diff/220001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:785: for
(TabObserver observer : mObservers) observer.onContentChanged(this);
On 2014/02/13 03:57:50, Ted C wrote:
> hmm...this gives me the scares.  At this point, native will be gone and the
tab
> should be dead, which is an odd time to try and start interacting with it.  I
> think we should only keep the onDestroyed above and require people to listen
to
> both.

Took it back out.  Bringing popcorn for the fisticuffs tomorrow.

https://codereview.chromium.org/155503004/diff/220001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager.h (right):

https://codereview.chromium.org/155503004/diff/220001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager.h:50: JavaObjectWeakGlobalRef
weak_java_banner_view_manager_;
On 2014/02/13 03:57:50, Ted C wrote:
> I would put a blank line after this

Done.

[Ted C, 2014-02-13 18:03:14]

lgtm

[David Trainor- moved to gerrit, 2014-02-13 18:05:23]

lgtm

[gone, 2014-02-13 18:06:02]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2014-02-13 18:06:39]

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

[commit-bot: I haz the power, 2014-02-13 19:55:26]

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

[commit-bot: I haz the power, 2014-02-13 21:47:09]

Change committed as 251136