Integrate UI with authentication flow.

Integrate UI with authentication flow.

1. Integrate connect call into Blimp settings UI.

2. Polish the connect functions in java and c++, now identity_source is
responsible for handling GoogleServiceAuthError::REQUEST_CANCELED, since
the Java approach(AccountTrackerService listener) didn't work with
non-startup connect attempts. And code structure is cleaner in this way.

3. Removed BlimpSettingsCallback, and moved embedder functions into
BlimpClientContextDelegate. Also added testing classes for
BlimpClientContext and BlimpClientContextDelegate, but these classes can
only test shim Java layer.

4. Created BUILD.gn for core/settings, and put jni files there. Java
file is still in core/, for easier review diff. There will be future CL
to move these Java files.

5. Added BlimpClientContextInternal Java interface to decouple internal build
targets.

BUG=624457
Committed: https://crrev.com/e8dee94d09ad68c28f72ee1acabcbaab0b071fbe
Cr-Commit-Position: refs/heads/master@{#415511}

[xingliu, 2016-08-22 18:18:09]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 18:33:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 18:41:27]

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

[commit-bot: I haz the power, 2016-08-22 18:41:27]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xingliu, 2016-08-23 01:21:31]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 01:22:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 01:26:55]

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

[commit-bot: I haz the power, 2016-08-23 01:26:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xingliu, 2016-08-23 01:57:52]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 01:58:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 03:32:51]

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

[commit-bot: I haz the power, 2016-08-23 03:32:52]

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

[xingliu, 2016-08-23 16:44:23]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 16:44:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xingliu, 2016-08-23 16:47:50]

Description was changed from

==========
Integrate UI with authentication flow.

1. Integrate connect call into Blimp settings UI.

2. Polish the connect functions in java and c++, now identity_source is
responsible for handling GoogleServiceAuthError::REQUEST_CANCELED, since
the Java approach(AccountTrackerService listener) didn't work with
non-startup connect attempts. And code structure is cleaner in this way.

3. Removed BlimpSettingsCallback, and moved embedder functions into
BlimpClientContextDelegate. Also added testing classes for
BlimpClientContext and BlimpClientContextDelegate.

4. Moved PreferenecesUtil into blimp/client/core/common, moved
AboutBlimpPreferences and jni classes into blimp/client/core/settings,
and create its own BUILD.gn.

5. Added BlimpClientContextInternal interface to decouple internal build
targets.


BUG=624457
==========

to

==========
Integrate UI with authentication flow.

1. Integrate connect call into Blimp settings UI.

2. Polish the connect functions in java and c++, now identity_source is
responsible for handling GoogleServiceAuthError::REQUEST_CANCELED, since
the Java approach(AccountTrackerService listener) didn't work with
non-startup connect attempts. And code structure is cleaner in this way.

3. Removed BlimpSettingsCallback, and moved embedder functions into
BlimpClientContextDelegate. Also added testing classes for
BlimpClientContext and BlimpClientContextDelegate.

4. Created BUILD.gn for core/settings, and put jni files there. Java
file is still in core/, for easier review diff. There will be future CL
to move these Java files.

5. Added BlimpClientContextInternal interface to decouple internal build
targets, both in Java and c++.


BUG=624457
==========

[xingliu, 2016-08-23 16:50:18]

Description was changed from

==========
Integrate UI with authentication flow.

1. Integrate connect call into Blimp settings UI.

2. Polish the connect functions in java and c++, now identity_source is
responsible for handling GoogleServiceAuthError::REQUEST_CANCELED, since
the Java approach(AccountTrackerService listener) didn't work with
non-startup connect attempts. And code structure is cleaner in this way.

3. Removed BlimpSettingsCallback, and moved embedder functions into
BlimpClientContextDelegate. Also added testing classes for
BlimpClientContext and BlimpClientContextDelegate.

4. Created BUILD.gn for core/settings, and put jni files there. Java
file is still in core/, for easier review diff. There will be future CL
to move these Java files.

5. Added BlimpClientContextInternal interface to decouple internal build
targets, both in Java and c++.


BUG=624457
==========

to

==========
Integrate UI with authentication flow.

1. Integrate connect call into Blimp settings UI.

2. Polish the connect functions in java and c++, now identity_source is
responsible for handling GoogleServiceAuthError::REQUEST_CANCELED, since
the Java approach(AccountTrackerService listener) didn't work with
non-startup connect attempts. And code structure is cleaner in this way.

3. Removed BlimpSettingsCallback, and moved embedder functions into
BlimpClientContextDelegate. Also added testing classes for
BlimpClientContext and BlimpClientContextDelegate, but these classes can
only test shim Java layer.

4. Created BUILD.gn for core/settings, and put jni files there. Java
file is still in core/, for easier review diff. There will be future CL
to move these Java files.

5. Added BlimpClientContextInternal interface to decouple internal build
targets, both in Java and c++.


BUG=624457
==========

[xingliu, 2016-08-23 17:44:12]

xingliu@chromium.org changed reviewers:
+ dtrainor@chromium.org, nyquist@chromium.org

[xingliu, 2016-08-23 17:44:13]

This CL integrates UI with token flow. Please help review, thanks.

[xingliu, 2016-08-23 18:02:24]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 18:03:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 19:20:13]

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

[commit-bot: I haz the power, 2016-08-23 19:20:17]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2016-08-29 05:12:25]

looking good!  have a few questions around the internal class and what we can
share between linux and android!

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/app/andro...
File
blimp/client/app/android/javatests/src/org/chromium/blimp/core/MockBlimpClientContext.java
(right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/app/andro...
blimp/client/app/android/javatests/src/org/chromium/blimp/core/MockBlimpClientContext.java:15:
* Mock BlimpClientContext.
Add a comment describing that this isn't backed by a native object like the real
class.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
File blimp/client/core/android/blimp_client_context_impl_android.cc (right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/blimp_client_context_impl_android.cc:93:
settings_android->SetIdentitySource(GetIdentitySource());
A few questions around this:

- Why can't the base class just expose a GetIdentitySource() that this class
uses?
- Do we need BlimpClientContextInternal?

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
File
blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java
(right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java:79:
AboutBlimpPreferences.setContext(this);
What do you think about merging these methods?

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
File
blimp/client/core/android/java/src/org/chromium/blimp/core/common/BlimpClientContextInternal.java
(right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/common/BlimpClientContextInternal.java:17:
public interface BlimpClientContextInternal {
The Java AboutBlimpPreferences class still uses BlimpClientContext directly. 
Would it make more sense to pass a BlimpPreferencesDelegate or
BlimpPreferencesClient or something that BlimpClientContext implements and
passes into the settings screen?

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
File
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java
(right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:134:
* Show please sign in dialog, it will show AccountSigninView to let user to sign
in.
Probably don't need "please" in this flow :).

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:170:
listPreference.setValue(PreferencesUtil.DEFAULT_ASSIGNER_URL);
Can we use getLastUsedAssigner()?

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:240:
pref.setChecked(false);
If we were on should we alert the user and shut down blimp?

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:259:
protected void initNativeLayer() {
Maybe call this initializeNative or just initialize()?

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:266:
contextInternal.initSettingsPage(mNativeBlimpSettingsAndroid);
Just pass this class in and allow the native side to pull the pointer from the
jobject.

In native you can have a BlimpSettingsAndroid::FromJavaObject() that can return
it.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.h:15: #include
"blimp/client/core/blimp_client_context_internal.h"
Is this used here?

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_internal.h (right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_internal.h:19: virtual IdentitySource*
GetIdentitySource() = 0;
This looks like it's only implemented on Android, will this compile on linux? 
Could we just have an IdentitySourceProvider class in the settings package that
this can implement or build?  Or just pass the IdentitySource when we create the
dialog?

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sess...
File blimp/client/core/session/identity_source.cc (right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sess...
blimp/client/core/session/identity_source.cc:102: if (error.state() ==
GoogleServiceAuthError::State::REQUEST_CANCELED &&
Could you elaborate on when we hit this?  It looks like this means we've
requested the cancellation.  Why do we want to retry after that?

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sett...
File blimp/client/core/settings/android/blimp_settings_android.cc (right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sett...
blimp/client/core/settings/android/blimp_settings_android.cc:34:
identity_source_->RemoveObserver(this);
DCHECK(identity_source_)?

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sett...
blimp/client/core/settings/android/blimp_settings_android.cc:38: void
BlimpSettingsAndroid::SetIdentitySource(IdentitySource* identity_source) {
We actually want to call RemoveObserver on the old IdentitySource, not the new
one right?

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sett...
blimp/client/core/settings/android/blimp_settings_android.cc:58: OnSignedOut();
How much of this do we want to share with Linux?  It feels like we really want
to share most of this logic and flow but just expose the UX to the front end
right?

[xingliu, 2016-08-30 04:33:00]

Description was changed from

==========
Integrate UI with authentication flow.

1. Integrate connect call into Blimp settings UI.

2. Polish the connect functions in java and c++, now identity_source is
responsible for handling GoogleServiceAuthError::REQUEST_CANCELED, since
the Java approach(AccountTrackerService listener) didn't work with
non-startup connect attempts. And code structure is cleaner in this way.

3. Removed BlimpSettingsCallback, and moved embedder functions into
BlimpClientContextDelegate. Also added testing classes for
BlimpClientContext and BlimpClientContextDelegate, but these classes can
only test shim Java layer.

4. Created BUILD.gn for core/settings, and put jni files there. Java
file is still in core/, for easier review diff. There will be future CL
to move these Java files.

5. Added BlimpClientContextInternal interface to decouple internal build
targets, both in Java and c++.


BUG=624457
==========

to

==========
Integrate UI with authentication flow.

1. Integrate connect call into Blimp settings UI.

2. Polish the connect functions in java and c++, now identity_source is
responsible for handling GoogleServiceAuthError::REQUEST_CANCELED, since
the Java approach(AccountTrackerService listener) didn't work with
non-startup connect attempts. And code structure is cleaner in this way.

3. Removed BlimpSettingsCallback, and moved embedder functions into
BlimpClientContextDelegate. Also added testing classes for
BlimpClientContext and BlimpClientContextDelegate, but these classes can
only test shim Java layer.

4. Created BUILD.gn for core/settings, and put jni files there. Java
file is still in core/, for easier review diff. There will be future CL
to move these Java files.

5. Added BlimpClientContextInternal Java interface to decouple internal build
targets.


BUG=624457
==========

[xingliu, 2016-08-30 04:33:26]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 04:35:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xingliu, 2016-08-30 04:47:43]

Thanks for the review, update the patch to fix most of the issues. 

The only thing didn't do is alert the user when user sign out, this logic seems
need to be done in embedder delegate. Since pref UI fragment mostly will be
destroyed when sign out happens.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/app/andro...
File
blimp/client/app/android/javatests/src/org/chromium/blimp/core/MockBlimpClientContext.java
(right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/app/andro...
blimp/client/app/android/javatests/src/org/chromium/blimp/core/MockBlimpClientContext.java:15:
* Mock BlimpClientContext.
On 2016/08/29 05:12:24, David Trainor wrote:
> Add a comment describing that this isn't backed by a native object like the
real
> class.

Done. Also added same comment in MockBlimpClientContextDelegate.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
File blimp/client/core/android/blimp_client_context_impl_android.cc (right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/blimp_client_context_impl_android.cc:93:
settings_android->SetIdentitySource(GetIdentitySource());
On 2016/08/29 05:12:24, David Trainor wrote:
> A few questions around this:
> 
> - Why can't the base class just expose a GetIdentitySource() that this class
> uses?
> - Do we need BlimpClientContextInternal?

Moved this to base class, and rename BlimpClientContextInternal to
BlimpPreferencesDelegate in Java, removed c++ interface. The Java one is needed
because BlimpClientContextImpl depends on settings and glue things together, so
settings can't depend on context impl directly.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
File
blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java
(right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java:79:
AboutBlimpPreferences.setContext(this);
On 2016/08/29 05:12:24, David Trainor wrote:
> What do you think about merging these methods?

Done, there is one function to do these two things. But keep the smaller
function, for test purpose.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
File
blimp/client/core/android/java/src/org/chromium/blimp/core/common/BlimpClientContextInternal.java
(right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/common/BlimpClientContextInternal.java:17:
public interface BlimpClientContextInternal {
On 2016/08/29 05:12:24, David Trainor wrote:
> The Java AboutBlimpPreferences class still uses BlimpClientContext directly. 
> Would it make more sense to pass a BlimpPreferencesDelegate or
> BlimpPreferencesClient or something that BlimpClientContext implements and
> passes into the settings screen?
Changed it to BlimpPreferencesDelegate.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
File
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java
(right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:134:
* Show please sign in dialog, it will show AccountSigninView to let user to sign
in.
On 2016/08/29 05:12:24, David Trainor wrote:
> Probably don't need "please" in this flow :).

Done.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:170:
listPreference.setValue(PreferencesUtil.DEFAULT_ASSIGNER_URL);
On 2016/08/29 05:12:24, David Trainor wrote:
> Can we use getLastUsedAssigner()?

Done. Make sense.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:240:
pref.setChecked(false);
On 2016/08/29 05:12:24, David Trainor wrote:
> If we were on should we alert the user and shut down blimp?

This logic is mostly not get called since the UI is destroyed after come back
from sign out view.

Can I do it in separate CL? probably need to move this stuff into embedder
delegate. It seems to need an activity context with certain configuration in the
manifest.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:259:
protected void initNativeLayer() {
On 2016/08/29 05:12:24, David Trainor wrote:
> Maybe call this initializeNative or just initialize()?

Done.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:266:
contextInternal.initSettingsPage(mNativeBlimpSettingsAndroid);
On 2016/08/29 05:12:24, David Trainor wrote:
> Just pass this class in and allow the native side to pull the pointer from the
> jobject.
> 
> In native you can have a BlimpSettingsAndroid::FromJavaObject() that can
return
> it.

Done. Now, it's similar to other Jni bridges.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.h:15: #include
"blimp/client/core/blimp_client_context_internal.h"
On 2016/08/29 05:12:24, David Trainor wrote:
> Is this used here?

I first expose the identity source in the base class, later move to android
layer, now it's moved back to here.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_internal.h (right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_internal.h:19: virtual IdentitySource*
GetIdentitySource() = 0;
On 2016/08/29 05:12:24, David Trainor wrote:
> This looks like it's only implemented on Android, will this compile on linux? 
> Could we just have an IdentitySourceProvider class in the settings package
that
> this can implement or build?  Or just pass the IdentitySource when we create
the
> dialog?

Done, sorry this is my bad, I've done it in Java so in c++ it's already directly
pass the IdentitySource. So removed this file.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sess...
File blimp/client/core/session/identity_source.cc (right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sess...
blimp/client/core/session/identity_source.cc:102: if (error.state() ==
GoogleServiceAuthError::State::REQUEST_CANCELED &&
On 2016/08/29 05:12:24, David Trainor wrote:
> Could you elaborate on when we hit this?  It looks like this means we've
> requested the cancellation.  Why do we want to retry after that?

Added more comments here.

The embedder(android) may cancel the request without Blimp's control.

This happens when user login or change account.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sett...
File blimp/client/core/settings/android/blimp_settings_android.cc (right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sett...
blimp/client/core/settings/android/blimp_settings_android.cc:34:
identity_source_->RemoveObserver(this);
On 2016/08/29 05:12:24, David Trainor wrote:
> DCHECK(identity_source_)?

Done.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sett...
blimp/client/core/settings/android/blimp_settings_android.cc:38: void
BlimpSettingsAndroid::SetIdentitySource(IdentitySource* identity_source) {
On 2016/08/29 05:12:24, David Trainor wrote:
> We actually want to call RemoveObserver on the old IdentitySource, not the new
> one right?

This code is just to avoid SetIdentitySource called more than once.
But I'll remove the first RemoveObserver call, it seems not make too much sense.

There is only one identitySource. But the jni of this UI can be created and
destroyed.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sett...
blimp/client/core/settings/android/blimp_settings_android.cc:58: OnSignedOut();
On 2016/08/29 05:12:24, David Trainor wrote:
> How much of this do we want to share with Linux?  It feels like we really want
> to share most of this logic and flow but just expose the UX to the front end
> right?

The code here is only for UX, I'll expose the identity source to base class, so
linux can listen to sign in state change by adding itself as listener in context
impl.

[commit-bot: I haz the power, 2016-08-30 06:34:51]

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

[commit-bot: I haz the power, 2016-08-30 06:34:51]

Dry run: Exceeded global retry quota

[David Trainor- moved to gerrit, 2016-08-30 20:46:29]

lgtm % nits

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
File
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java
(right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:240:
pref.setChecked(false);
On 2016/08/30 04:47:42, xingliu wrote:
> On 2016/08/29 05:12:24, David Trainor wrote:
> > If we were on should we alert the user and shut down blimp?
> 
> This logic is mostly not get called since the UI is destroyed after come back
> from sign out view.
> 
> Can I do it in separate CL? probably need to move this stuff into embedder
> delegate. It seems to need an activity context with certain configuration in
the
> manifest.

Acknowledged.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sett...
File blimp/client/core/settings/android/blimp_settings_android.cc (right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sett...
blimp/client/core/settings/android/blimp_settings_android.cc:38: void
BlimpSettingsAndroid::SetIdentitySource(IdentitySource* identity_source) {
On 2016/08/30 04:47:42, xingliu wrote:
> On 2016/08/29 05:12:24, David Trainor wrote:
> > We actually want to call RemoveObserver on the old IdentitySource, not the
new
> > one right?
> 
> This code is just to avoid SetIdentitySource called more than once.
> But I'll remove the first RemoveObserver call, it seems not make too much
sense.
> 
> There is only one identitySource. But the jni of this UI can be created and
> destroyed.

I think the remove call is safe unless we add DCHECK(!identity_source_) before
we set it.  You could just do:

if (identity_source_)
  identity_source_->RemoveObserver(this);

before setting identity_source_ to the new identity_source.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sett...
blimp/client/core/settings/android/blimp_settings_android.cc:58: OnSignedOut();
On 2016/08/30 04:47:42, xingliu wrote:
> On 2016/08/29 05:12:24, David Trainor wrote:
> > How much of this do we want to share with Linux?  It feels like we really
want
> > to share most of this logic and flow but just expose the UX to the front end
> > right?
> 
> The code here is only for UX, I'll expose the identity source to base class,
so
> linux can listen to sign in state change by adding itself as listener in
context
> impl.
> 

Acknowledged.

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/andr...
File
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java
(right):

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:47:
PreferenceFragment fragment, BlimpClientContext blimpContext) {
Make this BlimpPreferencesDelegate?

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:89:
destroyNative();
Should we destroy native before calling super.onDestroy()?

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:130:
sPreferencesDelegate.connect();
assert sPreferencesDialog != null here too?

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.cc:96: // Lazy initialization of
IdentitySource.
Can this just call GetIdentitySource()->Connect()?

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/sess...
File blimp/client/core/session/identity_source.cc (right):

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/sess...
blimp/client/core/session/identity_source.cc:103: // Retry the request to avoid
this to happen.
remove "to happen"

[xingliu, 2016-08-30 22:27:34]

xingliu@chromium.org changed reviewers:
+ mikecase@chromium.org

[xingliu, 2016-08-30 22:27:37]

Fixes based on review.


@mikecase: Please review changes in 

src/build/android/lint/suppressions.xml.

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sett...
File blimp/client/core/settings/android/blimp_settings_android.cc (right):

https://codereview.chromium.org/2261273002/diff/100001/blimp/client/core/sett...
blimp/client/core/settings/android/blimp_settings_android.cc:38: void
BlimpSettingsAndroid::SetIdentitySource(IdentitySource* identity_source) {
On 2016/08/30 20:46:29, David Trainor wrote:
> On 2016/08/30 04:47:42, xingliu wrote:
> > On 2016/08/29 05:12:24, David Trainor wrote:
> > > We actually want to call RemoveObserver on the old IdentitySource, not the
> new
> > > one right?
> > 
> > This code is just to avoid SetIdentitySource called more than once.
> > But I'll remove the first RemoveObserver call, it seems not make too much
> sense.
> > 
> > There is only one identitySource. But the jni of this UI can be created and
> > destroyed.
> 
> I think the remove call is safe unless we add DCHECK(!identity_source_) before
> we set it.  You could just do:
> 
> if (identity_source_)
>   identity_source_->RemoveObserver(this);
> 
> before setting identity_source_ to the new identity_source.

Oh, I understand now. Added this before changing identity_source.

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/andr...
File
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java
(right):

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:47:
PreferenceFragment fragment, BlimpClientContext blimpContext) {
On 2016/08/30 20:46:29, David Trainor wrote:
> Make this BlimpPreferencesDelegate?

Make sense, fixed.

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:89:
destroyNative();
On 2016/08/30 20:46:29, David Trainor wrote:
> Should we destroy native before calling super.onDestroy()?

Done.

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/andr...
blimp/client/core/android/java/src/org/chromium/blimp/core/settings/AboutBlimpPreferences.java:130:
sPreferencesDelegate.connect();
On 2016/08/30 20:46:29, David Trainor wrote:
> assert sPreferencesDialog != null here too?

Done, also added in another call site.

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.cc:96: // Lazy initialization of
IdentitySource.
On 2016/08/30 20:46:29, David Trainor wrote:
> Can this just call GetIdentitySource()->Connect()?

Done.

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/sess...
File blimp/client/core/session/identity_source.cc (right):

https://codereview.chromium.org/2261273002/diff/160001/blimp/client/core/sess...
blimp/client/core/session/identity_source.cc:103: // Retry the request to avoid
this to happen.
On 2016/08/30 20:46:29, David Trainor wrote:
> remove "to happen"

Done.

[xingliu, 2016-08-30 22:27:49]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 22:28:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mikecase (-- gone --), 2016-08-30 22:50:15]

src/build/android/lint/suppressions.xml lgtm

[commit-bot: I haz the power, 2016-08-30 23:40:37]

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

[commit-bot: I haz the power, 2016-08-30 23:40:38]

Dry run: This issue passed the CQ dry run.

[xingliu, 2016-08-30 23:49:23]

The CQ bit was checked by xingliu@chromium.org

[xingliu, 2016-08-30 23:49:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2261273002/#ps180001
(title: "Misc fixes/")

[commit-bot: I haz the power, 2016-08-30 23:50:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 23:58:18]

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

[commit-bot: I haz the power, 2016-08-30 23:58:19]

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

[nyquist, 2016-08-31 00:18:51]

blimp/client/DEPS lgtm

[nyquist, 2016-08-31 00:18:56]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2016-08-31 00:19:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 00:25:16]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-08-31 00:27:06]

Description was changed from

==========
Integrate UI with authentication flow.

1. Integrate connect call into Blimp settings UI.

2. Polish the connect functions in java and c++, now identity_source is
responsible for handling GoogleServiceAuthError::REQUEST_CANCELED, since
the Java approach(AccountTrackerService listener) didn't work with
non-startup connect attempts. And code structure is cleaner in this way.

3. Removed BlimpSettingsCallback, and moved embedder functions into
BlimpClientContextDelegate. Also added testing classes for
BlimpClientContext and BlimpClientContextDelegate, but these classes can
only test shim Java layer.

4. Created BUILD.gn for core/settings, and put jni files there. Java
file is still in core/, for easier review diff. There will be future CL
to move these Java files.

5. Added BlimpClientContextInternal Java interface to decouple internal build
targets.


BUG=624457
==========

to

==========
Integrate UI with authentication flow.

1. Integrate connect call into Blimp settings UI.

2. Polish the connect functions in java and c++, now identity_source is
responsible for handling GoogleServiceAuthError::REQUEST_CANCELED, since
the Java approach(AccountTrackerService listener) didn't work with
non-startup connect attempts. And code structure is cleaner in this way.

3. Removed BlimpSettingsCallback, and moved embedder functions into
BlimpClientContextDelegate. Also added testing classes for
BlimpClientContext and BlimpClientContextDelegate, but these classes can
only test shim Java layer.

4. Created BUILD.gn for core/settings, and put jni files there. Java
file is still in core/, for easier review diff. There will be future CL
to move these Java files.

5. Added BlimpClientContextInternal Java interface to decouple internal build
targets.

BUG=624457

Committed: https://crrev.com/e8dee94d09ad68c28f72ee1acabcbaab0b071fbe
Cr-Commit-Position: refs/heads/master@{#415511}
==========

[commit-bot: I haz the power, 2016-08-31 00:27:08]

Patchset 10 (id:??) landed as
https://crrev.com/e8dee94d09ad68c28f72ee1acabcbaab0b071fbe
Cr-Commit-Position: refs/heads/master@{#415511}