Fix ChromiumTestShellTestBase.

Fix ChromiumTestShellTestBase.

- Needed to add a way to wait for the initial page to finish loading.
- Exposed isLoading() to TabBase
- Had to refactor the use of WebContentsDelegateChromeAndroid into a TabObserver
- Tied the toolbar into the TabObserver

BUG=


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

[Ted C, 2012-10-29 17:49:18]

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

https://codereview.chromium.org/11351002/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:159: for
(TabObserver observer : mObservers) {
It would be annoying (about Java not this change) if this caused performance
hiccups since this will be allocating a new iterator each time the progress is
changed.

I wonder if that's why people use lists more commonly than sets.  Oh well, we
can change this if it actually does become an issue.

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

https://codereview.chromium.org/11351002/diff/1/chrome/android/testshell/java...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:102:
public TabBase getActiveTab() {
javadoc

https://codereview.chromium.org/11351002/diff/1/chrome/android/testshell/java...
File
chrome/android/testshell/javatests/src/org/chromium/chrome/testshell/ChromiumTestShellTestBase.java
(right):

https://codereview.chromium.org/11351002/diff/1/chrome/android/testshell/java...
chrome/android/testshell/javatests/src/org/chromium/chrome/testshell/ChromiumTestShellTestBase.java:13:
import java.util.concurrent.atomic.AtomicBoolean;
I think java's go last.

http://source.android.com/source/code-style.html#order-import-statements

https://codereview.chromium.org/11351002/diff/1/chrome/android/testshell/java...
chrome/android/testshell/javatests/src/org/chromium/chrome/testshell/ChromiumTestShellTestBase.java:65:
TabBase tab = activity.getActiveTab();
Any thoughts on exposing the load complete from the TabObserver and then just do
a signal/wait call here?  Then you can avoid the polling logic here (which is
the less preferred approach).

[Yusuf, 2012-10-29 18:46:26]

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

https://codereview.chromium.org/11351002/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:156: private
class ChromeWebContentsDelegateAndroidImpl extends
ChromeWebContentsDelegateAndroid {
Not sure, but should we be careful about using Impl for inner class names? We
don't really have many naming conventions for Java, but on the native land this
is mostly used for platform specific implementations and there seems to be a bit
of a confusion there already.

[David Trainor- moved to gerrit, 2012-10-29 21:10:32]

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

https://codereview.chromium.org/11351002/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:156: private
class ChromeWebContentsDelegateAndroidImpl extends
ChromeWebContentsDelegateAndroid {
Fair enough!  Changed to TabBaseChromeWebContentsDelegateAndroid.

On 2012/10/29 18:46:26, Yusuf wrote:
> Not sure, but should we be careful about using Impl for inner class names? We
> don't really have many naming conventions for Java, but on the native land
this
> is mostly used for platform specific implementations and there seems to be a
bit
> of a confusion there already.

https://codereview.chromium.org/11351002/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:159: for
(TabObserver observer : mObservers) {
That is a good point.  I'll make this a list as add/delete are much less
frequent than notify.

On 2012/10/29 17:49:18, Ted C wrote:
> It would be annoying (about Java not this change) if this caused performance
> hiccups since this will be allocating a new iterator each time the progress is
> changed.
> 
> I wonder if that's why people use lists more commonly than sets.  Oh well, we
> can change this if it actually does become an issue.

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

https://codereview.chromium.org/11351002/diff/1/chrome/android/testshell/java...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:102:
public TabBase getActiveTab() {
On 2012/10/29 17:49:18, Ted C wrote:
> javadoc

Done.

https://codereview.chromium.org/11351002/diff/1/chrome/android/testshell/java...
File
chrome/android/testshell/javatests/src/org/chromium/chrome/testshell/ChromiumTestShellTestBase.java
(right):

https://codereview.chromium.org/11351002/diff/1/chrome/android/testshell/java...
chrome/android/testshell/javatests/src/org/chromium/chrome/testshell/ChromiumTestShellTestBase.java:13:
import java.util.concurrent.atomic.AtomicBoolean;
On 2012/10/29 17:49:18, Ted C wrote:
> I think java's go last.
> 
> http://source.android.com/source/code-style.html#order-import-statements

Done.

https://codereview.chromium.org/11351002/diff/1/chrome/android/testshell/java...
chrome/android/testshell/javatests/src/org/chromium/chrome/testshell/ChromiumTestShellTestBase.java:65:
TabBase tab = activity.getActiveTab();
Since TabBase could be NULL before the SurfaceView is initialized I can't
register the observer to wait.  I'd have to either build another observer for
the manager or poll anyway :(.

On 2012/10/29 17:49:18, Ted C wrote:
> Any thoughts on exposing the load complete from the TabObserver and then just
do
> a signal/wait call here?  Then you can avoid the polling logic here (which is
> the less preferred approach).

[Ted C, 2012-10-29 21:13:59]

lgtm

https://codereview.chromium.org/11351002/diff/1/chrome/android/testshell/java...
File
chrome/android/testshell/javatests/src/org/chromium/chrome/testshell/ChromiumTestShellTestBase.java
(right):

https://codereview.chromium.org/11351002/diff/1/chrome/android/testshell/java...
chrome/android/testshell/javatests/src/org/chromium/chrome/testshell/ChromiumTestShellTestBase.java:65:
TabBase tab = activity.getActiveTab();
On 2012/10/29 21:10:32, dtrainor wrote:
> Since TabBase could be NULL before the SurfaceView is initialized I can't
> register the observer to wait.  I'd have to either build another observer for
> the manager or poll anyway :(.
> 
> On 2012/10/29 17:49:18, Ted C wrote:
> > Any thoughts on exposing the load complete from the TabObserver and then
just
> do
> > a signal/wait call here?  Then you can avoid the polling logic here (which
is
> > the less preferred approach).
> 

Sadness...but ok

[Yusuf, 2012-10-29 21:20:12]

lgtm

[commit-bot: I haz the power, 2012-10-29 22:34:57]

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

[commit-bot: I haz the power, 2012-10-29 22:35:06]

Presubmit check for 11351002-7002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.4s to calculate.

[David Trainor- moved to gerrit, 2012-10-29 23:14:40]

Hey Scott, can I get a chrome/ owner LGTM?  Thanks! :)

[sky, 2012-10-29 23:33:56]

LGTM

[commit-bot: I haz the power, 2012-10-29 23:35:54]

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

[commit-bot: I haz the power, 2012-10-30 03:45:39]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-10-30 16:11:25]

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

[commit-bot: I haz the power, 2012-10-30 19:02:29]

Change committed as 164967