TabBase: Add ContentView/NativePage ownership

Refactor TabBase to give it ownership of ContentViews and NativePages.  This CL includes the following:
- Moves ContentView/NativePage object ownership to TabBase.
- TabBase now has the initialization/destruction methods associated with ContentViews and WebContents.
- Creates a TabObserver to pipe out changes to TabBase to listeners.
- Adds helper methods for creating delegates/observers on the WebContents/ContentView.

BUG=277135
NOTRY=true

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

[David Trainor- moved to gerrit, 2013-08-15 22:17:49]

Not ready yet.  Just an idea.

[Yaron, 2013-08-22 01:21:37]

Didn't make it through TabBase/TabAndroid yet

https://codereview.chromium.org/23020009/diff/4001/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/TabObserverAdapter.java
(right):

https://codereview.chromium.org/23020009/diff/4001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TabObserverAdapter.java:10:
public class TabObserverAdapter implements TabObserver {
This is just for convenience so observers don't have to implement all hooks?
That matches WebContentsObserver but I don't know how much it'd be needed. I
guess you expect us to have multiple specialized TabObservers and not one for
chrome on android and one for testshell, right? Actually, do you think we should
expose all of these? It seems like some could/should just be using
WebContentsObserver.

If it sticks, regarding naming: Adapter seems like a misnomer. How about
EmptyTabObserver or SimpleTabObserver. (I'd have recommend "Base" in their but
that's confusing with TabBase.

https://codereview.chromium.org/23020009/diff/4001/chrome/browser/android/tab...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/23020009/diff/4001/chrome/browser/android/tab...
chrome/browser/android/tab_android.cc:152: void
TabAndroid::SwapTabContents(content::WebContents* old_contents,
SInce you're modifying, rename SwapWebContents

[David Trainor- moved to gerrit, 2013-08-22 21:25:38]

https://codereview.chromium.org/23020009/diff/4001/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/TabObserverAdapter.java
(right):

https://codereview.chromium.org/23020009/diff/4001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TabObserverAdapter.java:10:
public class TabObserverAdapter implements TabObserver {
On 2013/08/22 01:21:38, Yaron wrote:
> This is just for convenience so observers don't have to implement all hooks?
> That matches WebContentsObserver but I don't know how much it'd be needed. I
> guess you expect us to have multiple specialized TabObservers and not one for
> chrome on android and one for testshell, right? Actually, do you think we
should
> expose all of these? It seems like some could/should just be using
> WebContentsObserver.
> 
> If it sticks, regarding naming: Adapter seems like a misnomer. How about
> EmptyTabObserver or SimpleTabObserver. (I'd have recommend "Base" in their but
> that's confusing with TabBase.

Renamed EmptyTabObserver.  We discussed using this to expose basic events for
tab and to get rid of notifications eventually.  Removed
initializingWebContents/destroyingWebContents as that exposes too much about the
internal structure of Tab.

https://codereview.chromium.org/23020009/diff/4001/chrome/browser/android/tab...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/23020009/diff/4001/chrome/browser/android/tab...
chrome/browser/android/tab_android.cc:152: void
TabAndroid::SwapTabContents(content::WebContents* old_contents,
On 2013/08/22 01:21:38, Yaron wrote:
> SInce you're modifying, rename SwapWebContents

Can't since this is an overridden method :'(.

[Ted C, 2013-08-22 22:33:53]

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:53: /** Gives
{@link ContentViewCore} a way to interact with the Android window. */
s/ContentViewCore/TabBase

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:61: * is shown
in a View that is not a ContentView.
this comment isn't exactly true is it.  When showing a native page, the content
view can still live on underneath it right?  This will typically only be null if
the page is frozen I think or during the creation process of a native view.

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:118: 
remove extra blank line

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:253: *
remove blank line

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:280: *
blank line

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:299: * @param
client The {@link ContentViewClient} to be bound to any current or new
indenting is off on the javadoc

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:319: public
NavigationHistory getDirectedNavigationHistory(boolean isForward, int itemLimit)
{
put this and the following up with the navigation methods about going forward
and back

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:342: public
void destroy() {
I would put this with the other destroy calls...same for initialize above, put
it with the init calls

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:467: private
void destroyNativePageInternal() {
move with other destroy methods

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:485: private
void destroyBase() {
move with friends

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/TabObserver.java
(right):

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabObserver.java:30: public
void onTabDestroyed(TabBase tab);
since we're in the TabObserver, do we need to specify Tab in any of these method
names?  I guess to keep with WebContentsObserver, it does in the Destroy call
but no where else.

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabObserver.java:36: public
void onTabContentChanged(TabBase tab);
I would call this onTabPageInfoChanged just to avoid confusion that this refers
to html content.

[David Trainor- moved to gerrit, 2013-08-23 18:30:02]

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:53: /** Gives
{@link ContentViewCore} a way to interact with the Android window. */
On 2013/08/22 22:33:53, Ted C wrote:
> s/ContentViewCore/TabBase

Done.

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:61: * is shown
in a View that is not a ContentView.
On 2013/08/22 22:33:53, Ted C wrote:
> this comment isn't exactly true is it.  When showing a native page, the
content
> view can still live on underneath it right?  This will typically only be null
if
> the page is frozen I think or during the creation process of a native view.

Ah yeah now it can still be not null.

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:118: 
On 2013/08/22 22:33:53, Ted C wrote:
> remove extra blank line

Done.

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:253: *
On 2013/08/22 22:33:53, Ted C wrote:
> remove blank line

Done.

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:280: *
On 2013/08/22 22:33:53, Ted C wrote:
> blank line

Done.

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:299: * @param
client The {@link ContentViewClient} to be bound to any current or new
On 2013/08/22 22:33:53, Ted C wrote:
> indenting is off on the javadoc

Done.

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:319: public
NavigationHistory getDirectedNavigationHistory(boolean isForward, int itemLimit)
{
On 2013/08/22 22:33:53, Ted C wrote:
> put this and the following up with the navigation methods about going forward
> and back

Done.

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:342: public
void destroy() {
On 2013/08/22 22:33:53, Ted C wrote:
> I would put this with the other destroy calls...same for initialize above, put
> it with the init calls

Done.

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:467: private
void destroyNativePageInternal() {
On 2013/08/22 22:33:53, Ted C wrote:
> move with other destroy methods

Done.

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:485: private
void destroyBase() {
On 2013/08/22 22:33:53, Ted C wrote:
> move with friends

Renamed to clearNativePtr because that's actually what it does.  I wanted it to
stay near setNativePtr.

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/TabObserver.java
(right):

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabObserver.java:30: public
void onTabDestroyed(TabBase tab);
On 2013/08/22 22:33:53, Ted C wrote:
> since we're in the TabObserver, do we need to specify Tab in any of these
method
> names?  I guess to keep with WebContentsObserver, it does in the Destroy call
> but no where else.

Done.

https://codereview.chromium.org/23020009/diff/13001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabObserver.java:36: public
void onTabContentChanged(TabBase tab);
On 2013/08/22 22:33:53, Ted C wrote:
> I would call this onTabPageInfoChanged just to avoid confusion that this
refers
> to html content.

Done.

[David Trainor- moved to gerrit, 2013-08-23 18:32:01]

akalin@: Can I get a review on synced_tab_delegate_android.cc?

[Yaron, 2013-08-23 19:09:18]

lgtm

https://codereview.chromium.org/23020009/diff/1013/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/EmptyTabObserver.java
(right):

https://codereview.chromium.org/23020009/diff/1013/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/EmptyTabObserver.java:8: *
An adapter to the {@link TabObserver} which has empty implementations of all
methods.
Nit: reword without adapter

https://codereview.chromium.org/23020009/diff/1013/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):

https://codereview.chromium.org/23020009/diff/1013/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:221: public
Context getContext() {
It looks like we dont' need this. It's only used in one place in the internal
tree and the application context is sufficient (and can be passed in)

https://codereview.chromium.org/23020009/diff/1013/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:350: public
void initialize() { }
Why include this?

https://codereview.chromium.org/23020009/diff/1013/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:469: /** This
is currently called when committing a pre-rendered page. */
weird to include a comment like this.

https://codereview.chromium.org/23020009/diff/1013/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:522:
sIdCounter.addAndGet(diff);
I liked the comment from downstream cause this looks liek a possible concurrency
issue.

https://codereview.chromium.org/23020009/diff/1013/chrome/browser/android/tab...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/23020009/diff/1013/chrome/browser/android/tab...
chrome/browser/android/tab_android.cc:39: using content::ContentViewCore;
Nit: drop using

https://codereview.chromium.org/23020009/diff/1013/chrome/browser/android/tab...
File chrome/browser/android/tab_android.h (right):

https://codereview.chromium.org/23020009/diff/1013/chrome/browser/android/tab...
chrome/browser/android/tab_android.h:114: // CoreTabHelperDelegate Methods
Nit, style is:

// CoreTabHelperDelegate:
virtual void..

[akalin, 2013-08-23 20:44:17]

synced_tab_delegate_android.cc lgtm

[David Trainor- moved to gerrit, 2013-08-23 21:39:34]

https://codereview.chromium.org/23020009/diff/1013/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/EmptyTabObserver.java
(right):

https://codereview.chromium.org/23020009/diff/1013/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/EmptyTabObserver.java:8: *
An adapter to the {@link TabObserver} which has empty implementations of all
methods.
On 2013/08/23 19:09:18, Yaron wrote:
> Nit: reword without adapter

Done.

https://codereview.chromium.org/23020009/diff/1013/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):

https://codereview.chromium.org/23020009/diff/1013/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:221: public
Context getContext() {
On 2013/08/23 19:09:18, Yaron wrote:
> It looks like we dont' need this. It's only used in one place in the internal
> tree and the application context is sufficient (and can be passed in)

Confused about this.  Will find you and we can discuss!

https://codereview.chromium.org/23020009/diff/1013/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:350: public
void initialize() { }
On 2013/08/23 19:09:18, Yaron wrote:
> Why include this?

I can get rid of it.  I don't think we have to initialize anything here at this
point.

https://codereview.chromium.org/23020009/diff/1013/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:469: /** This
is currently called when committing a pre-rendered page. */
On 2013/08/23 19:09:18, Yaron wrote:
> weird to include a comment like this.

Comment was there downstream.  I didn't want to randomly erase comments as I
moved things.  I can get rid of it though.

https://codereview.chromium.org/23020009/diff/1013/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:522:
sIdCounter.addAndGet(diff);
On 2013/08/23 19:09:18, Yaron wrote:
> I liked the comment from downstream cause this looks liek a possible
concurrency
> issue.

Oops!  Thanks!

https://codereview.chromium.org/23020009/diff/1013/chrome/browser/android/tab...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/23020009/diff/1013/chrome/browser/android/tab...
chrome/browser/android/tab_android.cc:39: using content::ContentViewCore;
On 2013/08/23 19:09:18, Yaron wrote:
> Nit: drop using

Got rid of both of these.

https://codereview.chromium.org/23020009/diff/1013/chrome/browser/android/tab...
File chrome/browser/android/tab_android.h (right):

https://codereview.chromium.org/23020009/diff/1013/chrome/browser/android/tab...
chrome/browser/android/tab_android.h:114: // CoreTabHelperDelegate Methods
On 2013/08/23 19:09:18, Yaron wrote:
> Nit, style is:
> 
> // CoreTabHelperDelegate:
> virtual void..

Moved it to the style I saw in web_contents_impl.h.  Does this work?

// CoreTabHelperDelegate ----------------------------------------------------
...

// Methods called from Java via JNI -----------------------------------------
...

[David Trainor- moved to gerrit, 2013-08-23 22:32:18]

Fixed getContext.

[commit-bot: I haz the power, 2013-08-27 19:06:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/23020009/50001

[commit-bot: I haz the power, 2013-08-28 17:14:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/23020009/51019

[commit-bot: I haz the power, 2013-08-28 17:42:43]

Change committed as 220051