Add a basic UI to the Android Blimp client.

Add a basic UI to the Android Blimp client.

- Hook up a toolbar with a reload button and a url entry field.
- Hook up the phone back button to route through the toolbar.
- Tie the toolbar code into native where it can hit the network layer.
- Mutate the navigation proto to match the design.

BUG=552576
Committed: https://crrev.com/13d14b93584e3cdd6205b6fc3302c21abf0a239a
Cr-Commit-Position: refs/heads/master@{#359186}

[David Trainor- moved to gerrit, 2015-11-06 23:03:44]

dtrainor@chromium.org changed reviewers:
+ khushalsagar@chromium.org, kmarshall@chromium.org, nyquist@chromium.org

[David Trainor- moved to gerrit, 2015-11-06 23:03:45]

nyquist@/khushal@: client/
kmarshall@: common/ and blimp_toolbar.cc to see where I plan on hitting a
specific class which can build/receive protos.

I'm thinking it'll look something like:

Platform Specific FE       To/From Protos, has Delegate   Actual Network Layers
                           to talk to FE

BlimpToolbar (Android) <-> RemoteNavigationController  ->
BlimpMessageMultiplexer
BlimpToolbar (Linux)                                   <- BlimpMessageDispatcher

I made the proto built into a component lib, which made it easier to include
from the client/ folder.  I originally added a NavigationControllerReceiver that
implemented the BlimpMessageReceiver, but I'll work on that in a follow up CL.

I also added the NAVIGATION BlimpMessage type as that's what we kind of had in
the Helium spec.

[David Trainor- moved to gerrit, 2015-11-06 23:04:18]

On 2015/11/06 23:03:45, David Trainor wrote:
> nyquist@/khushal@: client/
> kmarshall@: common/ and blimp_toolbar.cc to see where I plan on hitting a
> specific class which can build/receive protos.
> 
> I'm thinking it'll look something like:
> 
> Platform Specific FE       To/From Protos, has Delegate   Actual Network
Layers
>                            to talk to FE
> 
> BlimpToolbar (Android) <-> RemoteNavigationController  ->
> BlimpMessageMultiplexer
> BlimpToolbar (Linux)                                   <-
BlimpMessageDispatcher
> 
> I made the proto built into a component lib, which made it easier to include
> from the client/ folder.  I originally added a NavigationControllerReceiver
that
> implemented the BlimpMessageReceiver, but I'll work on that in a follow up CL.
> 
> I also added the NAVIGATION BlimpMessage type as that's what we kind of had in
> the Helium spec.

Aww that got cut off... assume BlimpMessageMultiplexer is on the line above :(.

[David Trainor- moved to gerrit, 2015-11-06 23:08:36]

dtrainor@chromium.org changed reviewers:
+ haibinlu@chromium.org

[David Trainor- moved to gerrit, 2015-11-06 23:08:37]

+haibinlu@ for where to hook into the UI.  We'll need some class that does the
proto marshalling so both Android and Linux front ends can hit it/hear from it. 
Then that can talk to the message dispatcher/multiplexer on the network layer.

[nyquist, 2015-11-09 01:58:45]

lgtm

https://codereview.chromium.org/1422363008/diff/20001/blimp/client/android/bl...
File blimp/client/android/blimp_toolbar.h (right):

https://codereview.chromium.org/1422363008/diff/20001/blimp/client/android/bl...
blimp/client/android/blimp_toolbar.h:32: // To be called by a the content lite
NavigationController.  Should probably
Nit: Should this be a TODO?

https://codereview.chromium.org/1422363008/diff/20001/blimp/client/android/ja...
File blimp/client/android/java/src/org/chromium/blimp/toolbar/UrlBar.java
(right):

https://codereview.chromium.org/1422363008/diff/20001/blimp/client/android/ja...
blimp/client/android/java/src/org/chromium/blimp/toolbar/UrlBar.java:33: private
ObserverList<UrlBarObserver> mObservers = new ObserverList<UrlBarObserver>();
Nit: final

https://codereview.chromium.org/1422363008/diff/20001/blimp/client/android/ja...
blimp/client/android/java/src/org/chromium/blimp/toolbar/UrlBar.java:49: return
getEditableText() != null ? getEditableText().toString() : "";
Optional nit: Flip the != to have the positive case first?

[David Trainor- moved to gerrit, 2015-11-09 17:17:46]

https://chromiumcodereview.appspot.com/1422363008/diff/20001/blimp/client/and...
File blimp/client/android/blimp_toolbar.h (right):

https://chromiumcodereview.appspot.com/1422363008/diff/20001/blimp/client/and...
blimp/client/android/blimp_toolbar.h:32: // To be called by a the content lite
NavigationController.  Should probably
On 2015/11/09 01:58:45, nyquist wrote:
> Nit: Should this be a TODO?

Done.

https://chromiumcodereview.appspot.com/1422363008/diff/20001/blimp/client/and...
File blimp/client/android/java/src/org/chromium/blimp/toolbar/UrlBar.java
(right):

https://chromiumcodereview.appspot.com/1422363008/diff/20001/blimp/client/and...
blimp/client/android/java/src/org/chromium/blimp/toolbar/UrlBar.java:33: private
ObserverList<UrlBarObserver> mObservers = new ObserverList<UrlBarObserver>();
On 2015/11/09 01:58:45, nyquist wrote:
> Nit: final

Done.

https://chromiumcodereview.appspot.com/1422363008/diff/20001/blimp/client/and...
blimp/client/android/java/src/org/chromium/blimp/toolbar/UrlBar.java:49: return
getEditableText() != null ? getEditableText().toString() : "";
On 2015/11/09 01:58:45, nyquist wrote:
> Optional nit: Flip the != to have the positive case first?

Done.

[David Trainor- moved to gerrit, 2015-11-09 17:19:09]

dtrainor@chromium.org changed reviewers:
+ brettw@chromium.org

[David Trainor- moved to gerrit, 2015-11-09 17:19:10]

brettw@: ptal at blimp/client/DEPS review (added url)

Thanks!

[haibinlu, 2015-11-09 18:58:43]

https://codereview.chromium.org/1422363008/diff/40001/blimp/client/android/ja...
File blimp/client/android/java/src/org/chromium/blimp/toolbar/BlimpToolbar.java
(right):

https://codereview.chromium.org/1422363008/diff/40001/blimp/client/android/ja...
blimp/client/android/java/src/org/chromium/blimp/toolbar/BlimpToolbar.java:36:
private String mUrlToLoad;
is it possible that "there is a URL to load before native is ready to receive
any URL" ?

https://codereview.chromium.org/1422363008/diff/40001/blimp/client/android/ja...
blimp/client/android/java/src/org/chromium/blimp/toolbar/BlimpToolbar.java:48: *
To be called when the native library is loaded so that this class can initialize
it's native
it's -> its

https://codereview.chromium.org/1422363008/diff/40001/blimp/client/android/ja...
blimp/client/android/java/src/org/chromium/blimp/toolbar/BlimpToolbar.java:74:
public void loadUrl(String text) {
is this for testing only? Is there a convention to use suffix "ForTesting" as in
cc code?

https://codereview.chromium.org/1422363008/diff/40001/blimp/client/android/ja...
File blimp/client/android/java/src/org/chromium/blimp/toolbar/UrlBar.java
(right):

https://codereview.chromium.org/1422363008/diff/40001/blimp/client/android/ja...
blimp/client/android/java/src/org/chromium/blimp/toolbar/UrlBar.java:18: * A
{@link View} that a;lows users to enter text and URLs.
a;lows -> allows

https://codereview.chromium.org/1422363008/diff/40001/blimp/common/proto/navi...
File blimp/common/proto/navigation.proto (right):

https://codereview.chromium.org/1422363008/diff/40001/blimp/common/proto/navi...
blimp/common/proto/navigation.proto:14: // The text to generate and create a URL
from.  This might be a URL or might
could you elaborate? maybe give some examples.

https://codereview.chromium.org/1422363008/diff/40001/blimp/common/proto/navi...
blimp/common/proto/navigation.proto:38: GO_BACK = 3;
add FORWARD too?

[haibinlu, 2015-11-09 19:13:05]

https://codereview.chromium.org/1422363008/diff/40001/blimp/common/proto/cont...
File blimp/common/proto/control.proto (left):

https://codereview.chromium.org/1422363008/diff/40001/blimp/common/proto/cont...
blimp/common/proto/control.proto:18: CREATE_TAB = 1;
under current design (Kevin can comment more here),  client side needs to send
create_tab first, followed by load_url.  

Shall we merge navigation commands into the control message here? I do not feel
strongly about this though.

[David Trainor- moved to gerrit, 2015-11-09 19:27:18]

https://codereview.chromium.org/1422363008/diff/40001/blimp/client/android/ja...
File blimp/client/android/java/src/org/chromium/blimp/toolbar/BlimpToolbar.java
(right):

https://codereview.chromium.org/1422363008/diff/40001/blimp/client/android/ja...
blimp/client/android/java/src/org/chromium/blimp/toolbar/BlimpToolbar.java:36:
private String mUrlToLoad;
On 2015/11/09 18:58:43, haibinlu wrote:
> is it possible that "there is a URL to load before native is ready to receive
> any URL" ?

Sure.  The user could have typed something before the native library loads
(would be difficult, but possible).  Also we could load a URL from an intent and
want to set it early as well.

Right now the Activity code loads the Android views right as the native library
loads, but it probably shouldn't and I'll probably change it soon.

https://codereview.chromium.org/1422363008/diff/40001/blimp/client/android/ja...
blimp/client/android/java/src/org/chromium/blimp/toolbar/BlimpToolbar.java:74:
public void loadUrl(String text) {
On 2015/11/09 18:58:43, haibinlu wrote:
> is this for testing only? Is there a convention to use suffix "ForTesting" as
in
> cc code?

Nope.  We'll probably need to use this for three:
1. To set a default URL to load.
2. If we load from an intent.
3. Testing! :)

Depending on how we structure the other code, we might end up tweaking this
though.

https://codereview.chromium.org/1422363008/diff/40001/blimp/common/proto/cont...
File blimp/common/proto/control.proto (left):

https://codereview.chromium.org/1422363008/diff/40001/blimp/common/proto/cont...
blimp/common/proto/control.proto:18: CREATE_TAB = 1;
On 2015/11/09 19:13:05, haibinlu wrote:
> under current design (Kevin can comment more here),  client side needs to send
> create_tab first, followed by load_url.  
> 
> Shall we merge navigation commands into the control message here? I do not
feel
> strongly about this though.  

Oh I missed that.  I think we want to distinguish between events that take an
action on a particular tab and events that are actions between two tabs.  So I
pulled out the navigation state into a separate proto.  But if people feel
strongly I can change it :).

https://codereview.chromium.org/1422363008/diff/40001/blimp/common/proto/navi...
File blimp/common/proto/navigation.proto (right):

https://codereview.chromium.org/1422363008/diff/40001/blimp/common/proto/navi...
blimp/common/proto/navigation.proto:14: // The text to generate and create a URL
from.  This might be a URL or might
On 2015/11/09 18:58:43, haibinlu wrote:
> could you elaborate? maybe give some examples.

I'll put some examples here.  Thanks :).

https://codereview.chromium.org/1422363008/diff/40001/blimp/common/proto/navi...
blimp/common/proto/navigation.proto:38: GO_BACK = 3;
On 2015/11/09 18:58:43, haibinlu wrote:
> add FORWARD too?

Good point, although I'm not going to bother adding any UI for that right now
(short of making a pretty UI it'll just clutter the toolbar).

[David Trainor- moved to gerrit, 2015-11-10 16:14:51]

Responded to comments/addressed nits.  Thanks!

https://chromiumcodereview.appspot.com/1422363008/diff/40001/blimp/client/and...
File blimp/client/android/java/src/org/chromium/blimp/toolbar/BlimpToolbar.java
(right):

https://chromiumcodereview.appspot.com/1422363008/diff/40001/blimp/client/and...
blimp/client/android/java/src/org/chromium/blimp/toolbar/BlimpToolbar.java:48: *
To be called when the native library is loaded so that this class can initialize
it's native
On 2015/11/09 18:58:43, haibinlu wrote:
> it's -> its

Done.

https://chromiumcodereview.appspot.com/1422363008/diff/40001/blimp/client/and...
File blimp/client/android/java/src/org/chromium/blimp/toolbar/UrlBar.java
(right):

https://chromiumcodereview.appspot.com/1422363008/diff/40001/blimp/client/and...
blimp/client/android/java/src/org/chromium/blimp/toolbar/UrlBar.java:18: * A
{@link View} that a;lows users to enter text and URLs.
On 2015/11/09 18:58:43, haibinlu wrote:
> a;lows -> allows

Done.

[haibinlu, 2015-11-10 18:18:22]

lgtm

[brettw, 2015-11-10 21:40:27]

DEPS LGTM

[Kevin M, 2015-11-11 19:38:38]

lgtm

[Khushal, 2015-11-11 19:39:51]

On 2015/11/11 19:38:38, Kevin M wrote:
> lgtm

lgtm

[David Trainor- moved to gerrit, 2015-11-11 19:53:12]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2015-11-11 19:53:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, khushalsagar@chromium.org, brettw@chromium.org,
kmarshall@chromium.org, haibinlu@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1422363008/#ps80001
(title: "Renamed BlimpToolbar to Toolbar after discussion with Kevin.")

[commit-bot: I haz the power, 2015-11-11 19:54:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422363008/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422363008/80001

[commit-bot: I haz the power, 2015-11-11 20:18:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-11 20:18:14]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haibinlu, 2015-11-11 22:49:57]

lgtm

[David Trainor- moved to gerrit, 2015-11-11 22:59:37]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2015-11-11 22:59:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
khushalsagar@chromium.org, nyquist@chromium.org, brettw@chromium.org,
kmarshall@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1422363008/#ps120001 (title: "Made sure
to clear old control data")

[commit-bot: I haz the power, 2015-11-11 23:01:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422363008/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422363008/120001

[commit-bot: I haz the power, 2015-11-11 23:41:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-12 20:01:45]

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