Make TabBase non abstract

Make TabBase non abstract

TabBase will build it's own native component if nobody overrides initializeNative().

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

[David Trainor- moved to gerrit, 2013-12-06 19:17:08]

ptal.  Over half the methods I'm stubbing will go away when NTP dies.

[Yaron, 2013-12-06 21:25:52]

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

https://codereview.chromium.org/108803002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:592: * Builds
the native counterpart to this class.  Meant to be overridden by subclasses to
build
Update the class-level comment to comment on this.

https://codereview.chromium.org/108803002/diff/1/chrome/android/testshell/tes...
File chrome/android/testshell/testshell_tab.cc (right):

https://codereview.chromium.org/108803002/diff/1/chrome/android/testshell/tes...
chrome/android/testshell/testshell_tab.cc:38: ScopedJavaLocalRef<jstring>
TestShellTab::FixupUrl(JNIEnv* env,
Can you just delete this whole file. I think we have a duplicate impl in
UrlUtilities.fixupUrl

https://codereview.chromium.org/108803002/diff/1/chrome/browser/android/tab_a...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/108803002/diff/1/chrome/browser/android/tab_a...
chrome/browser/android/tab_android.cc:232: void
TabAndroid::HandlePopupNavigation(chrome::NavigateParams* params) {
Can these be NOTREACHED so we don't accidentally hit them in the downstream
impl? I guess that's assuming you can't currently reach these via
testshell/instrumentation tests.

[David Trainor- moved to gerrit, 2013-12-17 19:53:07]

addressed comments

https://chromiumcodereview.appspot.com/108803002/diff/1/chrome/android/java/s...
File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):

https://chromiumcodereview.appspot.com/108803002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:592: * Builds
the native counterpart to this class.  Meant to be overridden by subclasses to
build
On 2013/12/06 21:25:52, Yaron wrote:
> Update the class-level comment to comment on this.

Done.

https://chromiumcodereview.appspot.com/108803002/diff/1/chrome/android/testsh...
File chrome/android/testshell/testshell_tab.cc (right):

https://chromiumcodereview.appspot.com/108803002/diff/1/chrome/android/testsh...
chrome/android/testshell/testshell_tab.cc:38: ScopedJavaLocalRef<jstring>
TestShellTab::FixupUrl(JNIEnv* env,
On 2013/12/06 21:25:52, Yaron wrote:
> Can you just delete this whole file. I think we have a duplicate impl in
> UrlUtilities.fixupUrl

Done.

https://chromiumcodereview.appspot.com/108803002/diff/1/chrome/browser/androi...
File chrome/browser/android/tab_android.cc (right):

https://chromiumcodereview.appspot.com/108803002/diff/1/chrome/browser/androi...
chrome/browser/android/tab_android.cc:232: void
TabAndroid::HandlePopupNavigation(chrome::NavigateParams* params) {
On 2013/12/06 21:25:52, Yaron wrote:
> Can these be NOTREACHED so we don't accidentally hit them in the downstream
> impl? I guess that's assuming you can't currently reach these via
> testshell/instrumentation tests.

I can do it for the NTP ones, the auth request/popup nav can still be reached
afaik.

[Yaron, 2013-12-18 02:15:54]

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

https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:599: * subclass
native counterparts instead.
Add comment per above: subclasses should not call this via super and instead
rely on the native class to create the JNI association (or something like that)

https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/browser/an...
File chrome/browser/android/tab_android.cc (right):

https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/browser/an...
chrome/browser/android/tab_android.cc:462: scoped_ptr<TabAndroid> tab(new
TabAndroid(env, obj));
Why bother with the scoped_ptr at all? Just do: new TabAndroid..

https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/chrome_and...
File chrome/chrome_android.gypi (right):

https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/chrome_and...
chrome/chrome_android.gypi:73: 'target_name': 'chromium_testshell_jni_headers',
Remove this target

[David Trainor- moved to gerrit, 2013-12-18 21:29:47]

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

https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:599: * subclass
native counterparts instead.
On 2013/12/18 02:15:55, Yaron wrote:
> Add comment per above: subclasses should not call this via super and instead
> rely on the native class to create the JNI association (or something like
that)

Done.

https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/browser/an...
File chrome/browser/android/tab_android.cc (right):

https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/browser/an...
chrome/browser/android/tab_android.cc:462: scoped_ptr<TabAndroid> tab(new
TabAndroid(env, obj));
On 2013/12/18 02:15:55, Yaron wrote:
> Why bother with the scoped_ptr at all? Just do: new TabAndroid..

This felt cleaner and a little more safe (if things throw exceptions which we
don't worry about here anyway).  If this method were to throw an exception
somewhere in the middle it would still clean up the TabAndroid object IIRC.  I
can get rid of it though.

https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/chrome_and...
File chrome/chrome_android.gypi (right):

https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/chrome_and...
chrome/chrome_android.gypi:73: 'target_name': 'chromium_testshell_jni_headers',
On 2013/12/18 02:15:55, Yaron wrote:
> Remove this target

Ah good catch!

[Yaron, 2013-12-18 21:48:23]

On 2013/12/18 21:29:47, David Trainor wrote:
>
https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/android/ja...
> File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):
> 
>
https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/android/ja...
> chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:599: *
subclass
> native counterparts instead.
> On 2013/12/18 02:15:55, Yaron wrote:
> > Add comment per above: subclasses should not call this via super and instead
> > rely on the native class to create the JNI association (or something like
> that)
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/browser/an...
> File chrome/browser/android/tab_android.cc (right):
> 
>
https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/browser/an...
> chrome/browser/android/tab_android.cc:462: scoped_ptr<TabAndroid> tab(new
> TabAndroid(env, obj));
> On 2013/12/18 02:15:55, Yaron wrote:
> > Why bother with the scoped_ptr at all? Just do: new TabAndroid..
> 
> This felt cleaner and a little more safe (if things throw exceptions which we
> don't worry about here anyway).  If this method were to throw an exception
> somewhere in the middle it would still clean up the TabAndroid object IIRC.  I
> can get rid of it though.
> 
>
https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/chrome_and...
> File chrome/chrome_android.gypi (right):
> 
>
https://chromiumcodereview.appspot.com/108803002/diff/60001/chrome/chrome_and...
> chrome/chrome_android.gypi:73: 'target_name':
'chromium_testshell_jni_headers',
> On 2013/12/18 02:15:55, Yaron wrote:
> > Remove this target
> 
> Ah good catch!

lgtm

[commit-bot: I haz the power, 2014-01-06 18:24:34]

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

[commit-bot: I haz the power, 2014-01-06 20:51:22]

Change committed as 243160