Break apart the TabModelBase's JNI calls into its own class

An upcoming TabModel will diverge in key ways from the current TabModelBase.  Break apart the JNI calls from TabModelBase so that the new TabModelJniBridge class is basically a way to call out to the Java-side TabModel.

BUG=415747
Committed: https://crrev.com/4ecd374e81d8097d8107ec12267926c1281cee9d
Cr-Commit-Position: refs/heads/master@{#299223}

[gone, 2014-09-19 21:41:23]

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

[David Trainor- moved to gerrit, 2014-09-19 22:20:33]

https://chromiumcodereview.appspot.com/583373002/diff/20001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java
(right):

https://chromiumcodereview.appspot.com/583373002/diff/20001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:38:
protected final ObserverList<TabModelObserver> mObservers;
javadoc.  Same for all other protected/public stuff you added/changed

https://chromiumcodereview.appspot.com/583373002/diff/20001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:69:
mOrderController = orderController;
Should we be null checking mOrderController/mModelDelegate if we have a
constructor that doesn't set them?  Should they now be in the other class?

https://chromiumcodereview.appspot.com/583373002/diff/20001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:75:
mNativeTabModelImpl = nativeInit(mIsIncognito);
Does this work?  Don't you hit reinterpret_cast<> bugs?

[gone, 2014-10-01 20:15:46]

https://chromiumcodereview.appspot.com/583373002/diff/20001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java
(right):

https://chromiumcodereview.appspot.com/583373002/diff/20001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:69:
mOrderController = orderController;
On 2014/09/19 22:20:33, David Trainor wrote:
> Should we be null checking mOrderController/mModelDelegate if we have a
> constructor that doesn't set them?  Should they now be in the other class?

I'm hoping to add versions of these for document modes soon (TM).  This is
dependent on making a proper TabModelSelector.  What about this style for the
constructor until then?

https://chromiumcodereview.appspot.com/583373002/diff/20001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:75:
mNativeTabModelImpl = nativeInit(mIsIncognito);
On 2014/09/19 22:20:33, David Trainor wrote:
> Does this work?  Don't you hit reinterpret_cast<> bugs?

Oddly, no.  Seems to be fine.  Subclass methods are delegated to properly.

[David Trainor- moved to gerrit, 2014-10-02 00:00:39]

https://chromiumcodereview.appspot.com/583373002/diff/40001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java
(right):

https://chromiumcodereview.appspot.com/583373002/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:88:
mNativeTabModelImpl = nativeInit(mIsIncognito);
Even if this works, it might just be because the vtables are aligned properly. 
Are you 100% sure this should work?  We're effectively doing:

reinterpret_cast<TabModelBase*>(reinterpret_cast<intptr>(DocumentTabList*))
which isn't defined to work properly AFAIK...


Look at Tab/ChromeTab to see an example of how to do this safely.

[gone, 2014-10-06 18:17:25]

This along the lines of what you were going for?

[gone, 2014-10-09 17:24:03]

/me performs a poke.

[David Trainor- moved to gerrit, 2014-10-09 20:31:53]

On 2014/10/09 17:24:03, dfalcantara wrote:
> /me performs a poke.

For some reason it stayed purple.  Sorry about that!  Looking now.

[David Trainor- moved to gerrit, 2014-10-09 20:43:42]

A few nits but in general lgtm.

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java
(right):

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:58:
mNativeTabModelImpl = nativeInit(incognito);
Want to drive this from the base class?  Like a initializeNative() method where
the base class can check to make sure it has a reference to it's pointer
afterwards?  Then we can enforce child classes to init themselves.

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelJniBridge.java
(right):

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelJniBridge.java:37:
public void destroy() {
Maybe add a comment here about how this will invalidate all other native
references to this object in child classes.

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelJniBridge.java:73:
protected void setIndex(int index) {
Can this be private?

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/browser/ui...
File chrome/browser/ui/android/tab_model/tab_model_base.h (right):

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/browser/ui...
chrome/browser/ui/android/tab_model/tab_model_base.h:27: class TabModelBase :
public TabModelJniBridge {
Can these last three methods go to the TabModelJniBridge?  They look like they
just facilitate communication between the native and java models.

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/browser/ui...
File chrome/browser/ui/android/tab_model/tab_model_jni_bridge.cc (right):

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/browser/ui...
chrome/browser/ui/android/tab_model/tab_model_jni_bridge.cc:47:
TabModelList::RemoveTabModel(this);
Should this go in the destructor?  Might make more sense there since it's added
in the constructor.

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/browser/ui...
File chrome/browser/ui/android/tab_model/tab_model_jni_bridge.h (right):

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/browser/ui...
chrome/browser/ui/android/tab_model/tab_model_jni_bridge.h:60: };
Disallow copy and assign?

[gone, 2014-10-10 22:39:19]

/me flips tables over the last devtools specific function.

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelJniBridge.java
(right):

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelJniBridge.java:37:
public void destroy() {
On 2014/10/09 20:43:42, David Trainor wrote:
> Maybe add a comment here about how this will invalidate all other native
> references to this object in child classes.

Done.

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelJniBridge.java:73:
protected void setIndex(int index) {
On 2014/10/09 20:43:42, David Trainor wrote:
> Can this be private?

This is called by TabModelBase.cancelTabClosure, so not currently.

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/browser/ui...
File chrome/browser/ui/android/tab_model/tab_model_base.h (right):

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/browser/ui...
chrome/browser/ui/android/tab_model/tab_model_base.h:27: class TabModelBase :
public TabModelJniBridge {
On 2014/10/09 20:43:42, David Trainor wrote:
> Can these last three methods go to the TabModelJniBridge?  They look like they
> just facilitate communication between the native and java models.

The way the DocumentTabList implements those three is different :/  Should be
fine if you're cool with overriding them in the DocumentTabList, though.

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/browser/ui...
File chrome/browser/ui/android/tab_model/tab_model_jni_bridge.cc (right):

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/browser/ui...
chrome/browser/ui/android/tab_model/tab_model_jni_bridge.cc:47:
TabModelList::RemoveTabModel(this);
On 2014/10/09 20:43:42, David Trainor wrote:
> Should this go in the destructor?  Might make more sense there since it's
added
> in the constructor.

Done.

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/browser/ui...
File chrome/browser/ui/android/tab_model/tab_model_jni_bridge.h (right):

https://chromiumcodereview.appspot.com/583373002/diff/60001/chrome/browser/ui...
chrome/browser/ui/android/tab_model/tab_model_jni_bridge.h:60: };
On 2014/10/09 20:43:42, David Trainor wrote:
> Disallow copy and assign?

Done.

[David Trainor- moved to gerrit, 2014-10-10 22:45:30]

https://chromiumcodereview.appspot.com/583373002/diff/270001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelJniBridge.java
(right):

https://chromiumcodereview.appspot.com/583373002/diff/270001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelJniBridge.java:74:
protected void setIndex(int index) {
Okay.  Ideally we'd use the TabModelUtils.setIndex() method instead of this
there.

https://chromiumcodereview.appspot.com/583373002/diff/270001/chrome/browser/u...
File chrome/browser/ui/android/tab_model/tab_model_base.h (right):

https://chromiumcodereview.appspot.com/583373002/diff/270001/chrome/browser/u...
chrome/browser/ui/android/tab_model/tab_model_base.h:27: class TabModelBase :
public TabModelJniBridge {
Can this whole file be deleted now?  We can drive creation like Tab.java does if
not a ChromeTab.

[gone, 2014-10-10 23:06:38]

https://chromiumcodereview.appspot.com/583373002/diff/270001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelJniBridge.java
(right):

https://chromiumcodereview.appspot.com/583373002/diff/270001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelJniBridge.java:74:
protected void setIndex(int index) {
On 2014/10/10 22:45:29, David Trainor wrote:
> Okay.  Ideally we'd use the TabModelUtils.setIndex() method instead of this
> there.

Done.

https://chromiumcodereview.appspot.com/583373002/diff/270001/chrome/browser/u...
File chrome/browser/ui/android/tab_model/tab_model_base.h (right):

https://chromiumcodereview.appspot.com/583373002/diff/270001/chrome/browser/u...
chrome/browser/ui/android/tab_model/tab_model_base.h:27: class TabModelBase :
public TabModelJniBridge {
On 2014/10/10 22:45:29, David Trainor wrote:
> Can this whole file be deleted now?  We can drive creation like Tab.java does
if
> not a ChromeTab.

Can this be a follow up?  DocumentTabList still has to change downstream to
match the updated interface.

[David Trainor- moved to gerrit, 2014-10-10 23:10:38]

lgtm

[gone, 2014-10-10 23:10:40]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2014-10-10 23:11:42]

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

[commit-bot: I haz the power, 2014-10-11 00:58:41]

Committed patchset #7 (id:520001)

[commit-bot: I haz the power, 2014-10-11 00:59:28]

Patchset 7 (id:??) landed as
https://crrev.com/4ecd374e81d8097d8107ec12267926c1281cee9d
Cr-Commit-Position: refs/heads/master@{#299223}