Add rendering support to the TestShell.

Add rendering support to the TestShell.

- Tie the Compositor into the TestShell.
- Tie a really basic Tab into the TestShell.

BUG=http://crbug.com/136786


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158654

[David Trainor- moved to gerrit, 2012-09-20 06:34:36]

Initial upstream pass at getting this to render.  There is a crash when I
actually build a tab and load a URL upstream, but I addressed Ted's comments
downstream and will be looking at the crash today.

[Ted C, 2012-09-20 17:23:58]

https://codereview.chromium.org/10968003/diff/1/chrome/android/testshell/DEPS
File chrome/android/testshell/DEPS (right):

https://codereview.chromium.org/10968003/diff/1/chrome/android/testshell/DEPS...
chrome/android/testshell/DEPS:4: "+content/public/browser/android/compositor.h",
I think having just content/public/browser/android should be ok.  Specifying
individual headers seems like a maintenance annoyance.

https://codereview.chromium.org/10968003/diff/1/chrome/android/testshell/java...
File chrome/android/testshell/java/chromium_testshell_apk.xml (right):

https://codereview.chromium.org/10968003/diff/1/chrome/android/testshell/java...
chrome/android/testshell/java/chromium_testshell_apk.xml:18: <property
name="resource.absolute.dir" value="${RESOURCE_DIR}"/>
tabs...cough...tabs

down below as well.

https://codereview.chromium.org/10968003/diff/1/chrome/android/testshell/java...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(right):

https://codereview.chromium.org/10968003/diff/1/chrome/android/testshell/java...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:119:
AppResource.DRAWABLE_LINK_PREVIEW_POPUP_OVERLAY =
R.drawable.popup_zoomer_overlay;
you'll need to dcommit these drawables before you can land this change as the
trybots won't use the binary file.

https://codereview.chromium.org/10968003/diff/1/chrome/android/testshell/java...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/TabManager.java
(right):

https://codereview.chromium.org/10968003/diff/1/chrome/android/testshell/java...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/TabManager.java:119:
private class TabManagerWebContentsDelegateAndroid extends
ChromeWebContentsDelegateAndroid {
hmm...any reason why this isn't part of showTab on toolbar?  Would be nice to
isolate all of the updating inside of that one file.

https://codereview.chromium.org/10968003/diff/1/chrome/android/testshell/java...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/TabManager.java:122:
mToolbar.onLoadProgressChanged(progress/100.f);
spaces around the /

https://codereview.chromium.org/10968003/diff/1/chrome/android/testshell/java...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/Toolbar.java
(right):

https://codereview.chromium.org/10968003/diff/1/chrome/android/testshell/java...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/Toolbar.java:28:
public class Toolbar extends LinearLayout {
Maybe we want to call this TestShellToolbar just to have fewer files with the
name Toolbar around?

https://codereview.chromium.org/10968003/diff/1/chrome/android/testshell/tab_...
File chrome/android/testshell/tab_manager.h (right):

https://codereview.chromium.org/10968003/diff/1/chrome/android/testshell/tab_...
chrome/android/testshell/tab_manager.h:11: class WebLayer;
??? Why is this here?

https://codereview.chromium.org/10968003/diff/1/chrome/browser/android/tab_ba...
File chrome/browser/android/tab_base_android_impl.cc (right):

https://codereview.chromium.org/10968003/diff/1/chrome/browser/android/tab_ba...
chrome/browser/android/tab_base_android_impl.cc:127:
DCHECK(tab_contents_.get());
should this also be resetting tab_contents_?

https://codereview.chromium.org/10968003/diff/1/chrome/browser/android/tab_ba...
File chrome/browser/android/tab_base_android_impl.h (right):

https://codereview.chromium.org/10968003/diff/1/chrome/browser/android/tab_ba...
chrome/browser/android/tab_base_android_impl.h:43: const string16& host,
indent off

https://codereview.chromium.org/10968003/diff/1/chrome/chrome_android.gypi
File chrome/chrome_android.gypi (right):

https://codereview.chromium.org/10968003/diff/1/chrome/chrome_android.gypi#ne...
chrome/chrome_android.gypi:50: '../ui/ui.gyp:ui_java',
same thing as the other CL about trimming the list and automatically pulling in
dependencies...where applicable

[David Trainor- moved to gerrit, 2012-09-21 22:59:10]

Fixed Ted's comments.  Fixed rendering bug.

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/android/testshe...
File chrome/android/testshell/DEPS (right):

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/android/testshe...
chrome/android/testshell/DEPS:4: "+content/public/browser/android/compositor.h",
On 2012/09/20 17:23:59, Ted C wrote:
> I think having just content/public/browser/android should be ok.  Specifying
> individual headers seems like a maintenance annoyance. 

Done.

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/android/testshe...
File chrome/android/testshell/java/chromium_testshell_apk.xml (right):

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/android/testshe...
chrome/android/testshell/java/chromium_testshell_apk.xml:18: <property
name="resource.absolute.dir" value="${RESOURCE_DIR}"/>
On 2012/09/20 17:23:59, Ted C wrote:
> tabs...cough...tabs
> 
> down below as well.

Done.

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/android/testshe...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(right):

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/android/testshe...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:119:
AppResource.DRAWABLE_LINK_PREVIEW_POPUP_OVERLAY =
R.drawable.popup_zoomer_overlay;
:'(

On 2012/09/20 17:23:59, Ted C wrote:
> you'll need to dcommit these drawables before you can land this change as the
> trybots won't use the binary file.

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/android/testshe...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/TabManager.java
(right):

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/android/testshe...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/TabManager.java:119:
private class TabManagerWebContentsDelegateAndroid extends
ChromeWebContentsDelegateAndroid {
Has to be passed in on the constructor, or at least created before the
ContentView is initialized.

On 2012/09/20 17:23:59, Ted C wrote:
> hmm...any reason why this isn't part of showTab on toolbar?  Would be nice to
> isolate all of the updating inside of that one file.

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/android/testshe...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/TabManager.java:122:
mToolbar.onLoadProgressChanged(progress/100.f);
On 2012/09/20 17:23:59, Ted C wrote:
> spaces around the /

Done.

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/android/testshe...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/Toolbar.java
(right):

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/android/testshe...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/Toolbar.java:28:
public class Toolbar extends LinearLayout {
On 2012/09/20 17:23:59, Ted C wrote:
> Maybe we want to call this TestShellToolbar just to have fewer files with the
> name Toolbar around?

Done.

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/android/testshe...
File chrome/android/testshell/tab_manager.h (right):

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/android/testshe...
chrome/android/testshell/tab_manager.h:11: class WebLayer;
                    _________________________________________________
            /|     |                                                 |
            ||     |                                                 |
       .----|-----,|                    OOPS                         |
       ||  ||   ==||                                                 |
  .-----'--'|   ==||                                                 |
  |)-      ~|     ||_________________________________________________|
  | ___     |     |____...==..._  >\______________________________|
 [_/.-.\"--"-------- //.-.  .-.\\/   |/            \\ .-.  .-. //
   ( o )`==="""""""""`( o )( o )     o              `( o )( o )` 
    '-'                '-'  '-'                       '-'  '-'

On 2012/09/20 17:23:59, Ted C wrote:
> ??? Why is this here?

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/browser/android...
File chrome/browser/android/tab_base_android_impl.cc (right):

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/browser/android...
chrome/browser/android/tab_base_android_impl.cc:127:
DCHECK(tab_contents_.get());
Good catch thanks!

On 2012/09/20 17:23:59, Ted C wrote:
> should this also be resetting tab_contents_?

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/browser/android...
File chrome/browser/android/tab_base_android_impl.h (right):

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/browser/android...
chrome/browser/android/tab_base_android_impl.h:43: const string16& host,
On 2012/09/20 17:23:59, Ted C wrote:
> indent off

Done.

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/chrome_android....
File chrome/chrome_android.gypi (right):

https://chromiumcodereview.appspot.com/10968003/diff/1/chrome/chrome_android....
chrome/chrome_android.gypi:50: '../ui/ui.gyp:ui_java',
On 2012/09/20 17:23:59, Ted C wrote:
> same thing as the other CL about trimming the list and automatically pulling
in
> dependencies...where applicable

Done.

[Yaron, 2012-09-24 17:20:28]

Jay, as Ted is now OOO, can you help review

[Yaron, 2012-09-24 20:11:32]

Focused mostly on native code which seems fine aside from TabContents
construction.

http://codereview.chromium.org/10968003/diff/9027/chrome/android/testshell/ja...
File chrome/android/testshell/java/chromium_testshell_apk.xml (right):

http://codereview.chromium.org/10968003/diff/9027/chrome/android/testshell/ja...
chrome/android/testshell/java/chromium_testshell_apk.xml:18: <!-- Set the output
directory for the final apk to the ${apks.dir}. -->
Revert this file (same code is below)

http://codereview.chromium.org/10968003/diff/9027/chrome/android/testshell/ja...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(right):

http://codereview.chromium.org/10968003/diff/9027/chrome/android/testshell/ja...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:46:
mWindow = new NativeWindow(this);
Should this be ActivityNativeWindow?

http://codereview.chromium.org/10968003/diff/9027/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/10968003/diff/9027/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents.h:132: friend class TabAndroid;
You may want to fix (see comment above). You can ask avi@ about this or I can
help look.

[nilesh, 2012-09-24 20:46:40]

Mostly nits.

http://codereview.chromium.org/10968003/diff/9027/chrome/android/testshell/ja...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(right):

http://codereview.chromium.org/10968003/diff/9027/chrome/android/testshell/ja...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:41:
LibraryLoader.loadAndInitSync();
I may be wrong, but looks to me that ContentView.initChromiumBrowserProcess will
do a LibraryLoader.loadAdnInitSync (if library is not already loaded).Are we
trying to load another library with an explicit call to loadAndInitSync?

http://codereview.chromium.org/10968003/diff/9027/chrome/android/testshell/ja...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/TabManager.java
(right):

http://codereview.chromium.org/10968003/diff/9027/chrome/android/testshell/ja...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/TabManager.java:95:
TabBase tab = new TabBase(getContext(), url, mWindow,
no line wrap needed?

http://codereview.chromium.org/10968003/diff/9027/chrome/android/testshell/ja...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/TestShellToolbar.java
(right):

http://codereview.chromium.org/10968003/diff/9027/chrome/android/testshell/ja...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/TestShellToolbar.java:80:
if (progress == 1.0) postDelayed(mClearProgressRunnable,
COMPLETED_PROGRESS_TIMEOUT_MS);
Is progress capped at 1.0? 
Can we just change the argument to int?

http://codereview.chromium.org/10968003/diff/9027/chrome/android/testshell/re...
File chrome/android/testshell/res/drawable/progress.xml (right):

http://codereview.chromium.org/10968003/diff/9027/chrome/android/testshell/re...
chrome/android/testshell/res/drawable/progress.xml:13: <<<<<<< HEAD
Merge markers?

http://codereview.chromium.org/10968003/diff/9027/chrome/browser/android/tab_...
File chrome/browser/android/tab_base_android_impl.cc (right):

http://codereview.chromium.org/10968003/diff/9027/chrome/browser/android/tab_...
chrome/browser/android/tab_base_android_impl.cc:68: const string16& host,
fix indentation.

http://codereview.chromium.org/10968003/diff/9027/chrome/browser/android/tab_...
chrome/browser/android/tab_base_android_impl.cc:104: jobject
web_contents_delegate) {
fix indentation

http://codereview.chromium.org/10968003/diff/9027/chrome/browser/android/tab_...
chrome/browser/android/tab_base_android_impl.cc:112: // If this is a pop-up, a
TabContents may have been created
This comment does not seem relevant here. There is no check before we try to
create a tab_contents

Maybe it is relevant in TabAndroid::GetOrCreateTabContents

http://codereview.chromium.org/10968003/diff/9027/chrome/browser/android/tab_...
chrome/browser/android/tab_base_android_impl.cc:148: // Native JNI methods
I think we can remove this comment. We have JNI methods above too.

http://codereview.chromium.org/10968003/diff/9027/chrome/browser/android/tab_...
File chrome/browser/android/tab_base_android_impl.h (right):

http://codereview.chromium.org/10968003/diff/9027/chrome/browser/android/tab_...
chrome/browser/android/tab_base_android_impl.h:18: }  // namespace browser_sync
No need for this comment for small namespace blocks.

[Jay Civelli, 2012-09-24 22:43:08]

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

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/java...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:99: if
(mContentView != null) {
Nit: you could do
if (mContentView == null) return;
like you are doing below.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
File chrome/android/testshell/tab_manager.cc (right):

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:28:
base::android::ScopedJavaGlobalRef<jobject> jobj_;
No "_" for struct members, it should be:

base::android::ScopedJavaGlobalRef<jobject> jobj;
scoped_ptr<content::Compositor> compositor;
scoped_ptr<WebKit::WebLayer> root_layer;

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:29: scoped_ptr<content::Compositor>
compositor_;
Nit: no need for fully qualified name, you are "using" it above. Same below.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:65: g_global_state.Get().jobj_.Reset(
I don't understand why we need a GlobalRef on that guy instead of a
JavaObjectWeakGlobalRef.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:66:
base::android::ScopedJavaLocalRef<jobject>(env, obj));
Nit: no need for the full qualified name, you are "using" it above.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:79: JNIEnv* env, jclass clazz, jobject
jsurface) {
Doesn't that fit on the prev line?

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:100: JNIEnv* env, jclass clazz, jint
jtab) {
Nit: Should fit on previous line, right?

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:101: if (!GetCompositor()) return;
C++ style code says return should be on next line. (no curly braces needed).
Same in next method.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:108: JNIEnv* env, jclass clazz, jint
jtab) {
Nit: Would fit on prev line.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
File chrome/browser/android/tab_base_android_impl.cc (right):

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.cc:46: private:
Nit: 1 blank line before private:

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.cc:52: : TabAndroid() {
Is calling TabAndroid() needed?

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
File chrome/browser/android/tab_base_android_impl.h (right):

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.h:35: WebKit::WebLayer* tab_layer()
{ return tab_layer_.get(); }
Should be const.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.h:49: const
content::ContextMenuParams& params,
Indent should be 4 spaces.
Same on methods below.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.h:63: // register the Tab's native
methods through jni
PITA Jay says comments should start with a capital and end with a full-stop (and
JNI should be all caps).

[David Trainor- moved to gerrit, 2012-09-24 23:39:57]

Addressed comments.  Removed TabContents* from TabBase (we only need
WebContents* atm).  PTAL.

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

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/java...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:99: if
(mContentView != null) {
On 2012/09/24 22:43:08, Jay Civelli wrote:
> Nit: you could do
> if (mContentView == null) return;
> like you are doing below.

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
File chrome/android/testshell/java/chromium_testshell_apk.xml (right):

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/java/chromium_testshell_apk.xml:18: <!-- Set the output
directory for the final apk to the ${apks.dir}. -->
On 2012/09/24 20:11:32, Yaron wrote:
> Revert this file (same code is below)

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(right):

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:41:
LibraryLoader.loadAndInitSync();
Ah good catch.

On 2012/09/24 20:46:41, nileshagrawal1 wrote:
> I may be wrong, but looks to me that ContentView.initChromiumBrowserProcess
will
> do a LibraryLoader.loadAdnInitSync (if library is not already loaded).Are we
> trying to load another library with an explicit call to loadAndInitSync?

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:46:
mWindow = new NativeWindow(this);
On 2012/09/24 20:11:32, Yaron wrote:
> Should this be ActivityNativeWindow?

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/TabManager.java
(right):

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/TabManager.java:95:
TabBase tab = new TabBase(getContext(), url, mWindow,
Goes over 100 chars :(.

On 2012/09/24 20:46:41, nileshagrawal1 wrote:
> no line wrap needed?

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/TestShellToolbar.java
(right):

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/TestShellToolbar.java:80:
if (progress == 1.0) postDelayed(mClearProgressRunnable,
COMPLETED_PROGRESS_TIMEOUT_MS);
I'll use 0 - 100.  I don't want to expose 10,000 because that's the View
Drawable level we can set to get a progress indication, and I don't want to
expose that to the caller.

On 2012/09/24 20:46:41, nileshagrawal1 wrote:
> Is progress capped at 1.0? 
> Can we just change the argument to int?

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
File chrome/android/testshell/res/drawable/progress.xml (right):

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/res/drawable/progress.xml:13: <<<<<<< HEAD
On 2012/09/24 20:46:41, nileshagrawal1 wrote:
> Merge markers?

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
File chrome/android/testshell/tab_manager.cc (right):

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:28:
base::android::ScopedJavaGlobalRef<jobject> jobj_;
On 2012/09/24 22:43:08, Jay Civelli wrote:
> No "_" for struct members, it should be:
> 
> base::android::ScopedJavaGlobalRef<jobject> jobj;
> scoped_ptr<content::Compositor> compositor;
> scoped_ptr<WebKit::WebLayer> root_layer;

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:29: scoped_ptr<content::Compositor>
compositor_;
On 2012/09/24 22:43:08, Jay Civelli wrote:
> Nit: no need for fully qualified name, you are "using" it above. Same below.

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:65: g_global_state.Get().jobj_.Reset(
After refactoring I think I don't need this at all.  Left in for earlier code.

On 2012/09/24 22:43:08, Jay Civelli wrote:
> I don't understand why we need a GlobalRef on that guy instead of a
> JavaObjectWeakGlobalRef.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:66:
base::android::ScopedJavaLocalRef<jobject>(env, obj));
On 2012/09/24 22:43:08, Jay Civelli wrote:
> Nit: no need for the full qualified name, you are "using" it above.

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:79: JNIEnv* env, jclass clazz, jobject
jsurface) {
On 2012/09/24 22:43:08, Jay Civelli wrote:
> Doesn't that fit on the prev line?

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:100: JNIEnv* env, jclass clazz, jint
jtab) {
On 2012/09/24 22:43:08, Jay Civelli wrote:
> Nit: Should fit on previous line, right?

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:101: if (!GetCompositor()) return;
On 2012/09/24 22:43:08, Jay Civelli wrote:
> C++ style code says return should be on next line. (no curly braces needed).
> Same in next method.

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/android/test...
chrome/android/testshell/tab_manager.cc:108: JNIEnv* env, jclass clazz, jint
jtab) {
On 2012/09/24 22:43:08, Jay Civelli wrote:
> Nit: Would fit on prev line.

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
File chrome/browser/android/tab_base_android_impl.cc (right):

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.cc:46: private:
On 2012/09/24 22:43:08, Jay Civelli wrote:
> Nit: 1 blank line before private:

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.cc:52: : TabAndroid() {
No.  Refactor bug sorry.

On 2012/09/24 22:43:08, Jay Civelli wrote:
> Is calling TabAndroid() needed?

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.cc:68: const string16& host,
On 2012/09/24 20:46:41, nileshagrawal1 wrote:
> fix indentation.

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.cc:104: jobject
web_contents_delegate) {
On 2012/09/24 20:46:41, nileshagrawal1 wrote:
> fix indentation

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.cc:112: // If this is a pop-up, a
TabContents may have been created
Redid how this works.  Removed most of this.

On 2012/09/24 20:46:41, nileshagrawal1 wrote:
> This comment does not seem relevant here. There is no check before we try to
> create a tab_contents
> 
> Maybe it is relevant in TabAndroid::GetOrCreateTabContents

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.cc:148: // Native JNI methods
On 2012/09/24 20:46:41, nileshagrawal1 wrote:
> I think we can remove this comment. We have JNI methods above too.

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
File chrome/browser/android/tab_base_android_impl.h (right):

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.h:18: }  // namespace browser_sync
On 2012/09/24 20:46:41, nileshagrawal1 wrote:
> No need for this comment for small namespace blocks.

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.h:35: WebKit::WebLayer* tab_layer()
{ return tab_layer_.get(); }
On 2012/09/24 22:43:08, Jay Civelli wrote:
> Should be const.

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.h:49: const
content::ContextMenuParams& params,
On 2012/09/24 22:43:08, Jay Civelli wrote:
> Indent should be 4 spaces.
> Same on methods below.

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.h:63: // register the Tab's native
methods through jni
On 2012/09/24 22:43:08, Jay Civelli wrote:
> PITA Jay says comments should start with a capital and end with a full-stop
(and
> JNI should be all caps).

Done.

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/ui/t...
File chrome/browser/ui/tab_contents/tab_contents.h (right):

https://chromiumcodereview.appspot.com/10968003/diff/9027/chrome/browser/ui/t...
chrome/browser/ui/tab_contents/tab_contents.h:132: friend class TabAndroid;
Talked to Avi.  Got rid of our reliance on a TabContents, now we store and use
the WebContents pointer directly.
On 2012/09/24 20:11:32, Yaron wrote:
> You may want to fix (see comment above). You can ask avi@ about this or I can
> help look.

[David Trainor- moved to gerrit, 2012-09-24 23:42:19]

Scott, again this is pretty android specific, but if you want could you take a
look at chrome_content_browser_client.cc, tab_base_android_impl.h/cc and the gyp
changes?

Thanks!

[Avi (use Gerrit), 2012-09-24 23:43:48]

https://chromiumcodereview.appspot.com/10968003/diff/16004/chrome/browser/and...
File chrome/browser/android/tab_base_android_impl.h (right):

https://chromiumcodereview.appspot.com/10968003/diff/16004/chrome/browser/and...
chrome/browser/android/tab_base_android_impl.h:14: class TabContents;
Do you need this any more?

https://chromiumcodereview.appspot.com/10968003/diff/16004/chrome/browser/and...
chrome/browser/android/tab_base_android_impl.h:87:
scoped_ptr<content::WebContents> web_contents_;
Yeah!

[Yaron, 2012-09-24 23:59:04]

lgtt but please wait for others

https://chromiumcodereview.appspot.com/10968003/diff/16004/chrome/browser/and...
File chrome/browser/android/tab_android.h (right):

https://chromiumcodereview.appspot.com/10968003/diff/16004/chrome/browser/and...
chrome/browser/android/tab_android.h:17: class TabContents;
Revert this.

[Yaron, 2012-09-24 23:59:10]

lgtm but please wait for others

[Jay Civelli, 2012-09-25 00:01:49]

lgtm

https://chromiumcodereview.appspot.com/10968003/diff/16004/chrome/browser/and...
File chrome/browser/android/tab_base_android_impl.cc (right):

https://chromiumcodereview.appspot.com/10968003/diff/16004/chrome/browser/and...
chrome/browser/android/tab_base_android_impl.cc:111: new
ChromeWebContentsDelegateRenderAndroid(this,
indent should be 4 spaces in C++ land.

[David Trainor- moved to gerrit, 2012-09-25 00:34:31]

addressed nits.

https://codereview.chromium.org/10968003/diff/16004/chrome/browser/android/ta...
File chrome/browser/android/tab_android.h (right):

https://codereview.chromium.org/10968003/diff/16004/chrome/browser/android/ta...
chrome/browser/android/tab_android.h:17: class TabContents;
On 2012/09/24 23:59:04, Yaron wrote:
> Revert this.

Done.

https://codereview.chromium.org/10968003/diff/16004/chrome/browser/android/ta...
File chrome/browser/android/tab_base_android_impl.cc (right):

https://codereview.chromium.org/10968003/diff/16004/chrome/browser/android/ta...
chrome/browser/android/tab_base_android_impl.cc:111: new
ChromeWebContentsDelegateRenderAndroid(this,
On 2012/09/25 00:01:49, Jay Civelli wrote:
> indent should be 4 spaces in C++ land.

Done.

https://codereview.chromium.org/10968003/diff/16004/chrome/browser/android/ta...
File chrome/browser/android/tab_base_android_impl.h (right):

https://codereview.chromium.org/10968003/diff/16004/chrome/browser/android/ta...
chrome/browser/android/tab_base_android_impl.h:14: class TabContents;
On 2012/09/24 23:43:49, Avi wrote:
> Do you need this any more?

Done.

https://codereview.chromium.org/10968003/diff/16004/chrome/browser/android/ta...
chrome/browser/android/tab_base_android_impl.h:87:
scoped_ptr<content::WebContents> web_contents_;
RAWR I EATZ TABCONTENTZ!
					O_
				       /  >
				      -  >   ^\
				     /   >  ^ /   
				    (O)  > ^ /   / / /  
       _____                        |            \\|//
      /  __ \                      _/      /     / _/
     /  /  | |                    /       /     / /
   _/  |___/ /			_/      ------_/ / 
 ==_|  \____/    	      _/       /  ______/
     \   \                 __/           |\
      |   \_          ____/              / \      _                    
       \    \________/                  |\  \----/_V
        \_                              / \_______ V
          \__                /       \ /          V
             \               \        \
              \______         \_       \
                     \__________\_      \ 
                        /    /    \_    | 
                       |   _/       \   |
                      /  _/          \  |
                     |  /            |  |
                     \  \__          |   \__
                     /\____=\       /\_____=\

On 2012/09/24 23:43:49, Avi wrote:
> Yeah!

[no sievers, 2012-09-25 10:21:53]

https://codereview.chromium.org/10968003/diff/17005/chrome/android/testshell/...
File chrome/android/testshell/tab_manager.cc (right):

https://codereview.chromium.org/10968003/diff/17005/chrome/android/testshell/...
chrome/android/testshell/tab_manager.cc:98:
GetCompositor()->OnSurfaceUpdated(base::Bind(&DummyCallback));
Are you sure? :)
How would this ever render then?

[no sievers, 2012-09-25 10:24:04]

https://codereview.chromium.org/10968003/diff/17005/chrome/android/testshell/...
File chrome/android/testshell/tab_manager.cc (right):

https://codereview.chromium.org/10968003/diff/17005/chrome/android/testshell/...
chrome/android/testshell/tab_manager.cc:98:
GetCompositor()->OnSurfaceUpdated(base::Bind(&DummyCallback));
Oh this is to force a draw when you switch tabs, ok.
I think I need to rename this method to Composite().

Can you do OnSurfaceUpdated(base::Closure()) or something to avoid the empty
function definition?

On 2012/09/25 10:21:53, Daniel Sievers wrote:
> Are you sure? :)
> How would this ever render then?

[sky, 2012-09-25 15:57:38]

LGTM

https://chromiumcodereview.appspot.com/10968003/diff/17005/chrome/browser/and...
File chrome/browser/android/tab_base_android_impl.cc (right):

https://chromiumcodereview.appspot.com/10968003/diff/17005/chrome/browser/and...
chrome/browser/android/tab_base_android_impl.cc:28: JNIEnv* env, jobject obj)
each param on own line.

https://chromiumcodereview.appspot.com/10968003/diff/17005/chrome/browser/and...
chrome/browser/android/tab_base_android_impl.cc:48: };
DISALLOW_...

https://chromiumcodereview.appspot.com/10968003/diff/17005/chrome/browser/and...
File chrome/browser/android/tab_base_android_impl.h (right):

https://chromiumcodereview.appspot.com/10968003/diff/17005/chrome/browser/and...
chrome/browser/android/tab_base_android_impl.h:60: const GURL& url, const
string16& title, const SkBitmap& skbitmap,
each param on its own line.

[David Trainor- moved to gerrit, 2012-09-25 19:07:12]

https://chromiumcodereview.appspot.com/10968003/diff/17005/chrome/android/tes...
File chrome/android/testshell/tab_manager.cc (right):

https://chromiumcodereview.appspot.com/10968003/diff/17005/chrome/android/tes...
chrome/android/testshell/tab_manager.cc:98:
GetCompositor()->OnSurfaceUpdated(base::Bind(&DummyCallback));
On 2012/09/25 10:24:04, Daniel Sievers wrote:
> Oh this is to force a draw when you switch tabs, ok.
> I think I need to rename this method to Composite().
> 
> Can you do OnSurfaceUpdated(base::Closure()) or something to avoid the empty
> function definition?
> 
> On 2012/09/25 10:21:53, Daniel Sievers wrote:
> > Are you sure? :)
> > How would this ever render then?
> 

Talked with Daniel, not changing this for now.

https://chromiumcodereview.appspot.com/10968003/diff/17005/chrome/browser/and...
File chrome/browser/android/tab_base_android_impl.cc (right):

https://chromiumcodereview.appspot.com/10968003/diff/17005/chrome/browser/and...
chrome/browser/android/tab_base_android_impl.cc:28: JNIEnv* env, jobject obj)
On 2012/09/25 15:57:39, sky wrote:
> each param on own line.

Done.

https://chromiumcodereview.appspot.com/10968003/diff/17005/chrome/browser/and...
chrome/browser/android/tab_base_android_impl.cc:48: };
On 2012/09/25 15:57:39, sky wrote:
> DISALLOW_...

Done.

https://chromiumcodereview.appspot.com/10968003/diff/17005/chrome/browser/and...
File chrome/browser/android/tab_base_android_impl.h (right):

https://chromiumcodereview.appspot.com/10968003/diff/17005/chrome/browser/and...
chrome/browser/android/tab_base_android_impl.h:60: const GURL& url, const
string16& title, const SkBitmap& skbitmap,
On 2012/09/25 15:57:39, sky wrote:
> each param on its own line.

Done.

[commit-bot: I haz the power, 2012-09-25 19:07:45]

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

[commit-bot: I haz the power, 2012-09-25 21:17:01]

Change committed as 158654