Add Settings[, Oberser] java code and JNI bridge.

Add Settings[, Oberser] java code and JNI bridge.

This CL addes the Settings[, Oberser] java code, a compound observer
proxy SettingsObserverProxy, and the JNI bridge between the java and
native code. Users of the java API should be blimp internal code,
calling getSettings() on the BlimpClientContextImpl to get the Settings
java object.

BUG=647848
Committed: https://crrev.com/c2a0d1de87e89bbc1836e7dc083731108aeec22e
Cr-Commit-Position: refs/heads/master@{#430524}

[Menglin, 2016-11-01 17:36:48]

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

[Menglin, 2016-11-01 17:36:48]

HI David, ptal. thanks!

Menglin

[David Trainor- moved to gerrit, 2016-11-03 03:05:10]

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/context/a...
File
blimp/client/core/context/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java
(right):

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/context/a...
blimp/client/core/context/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java:65:
public Settings getSettings() {
Add javadoc

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/context/a...
blimp/client/core/context/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java:66:
return nativeGetSettings(mNativeBlimpClientContextImplAndroid);
Assert that mNativeBlimpCLientContextImplAndroid != 0?

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/context/b...
File blimp/client/core/context/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/context/b...
blimp/client/core/context/blimp_client_context_impl.cc:105: #if
defined(OS_ANDROID)
Can we pass the settings object in the constructor and just build it in the
create calls?  Then we don't have to pass local_state in.

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/settings/...
File
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java
(right):

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/settings/...
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java:25:
private SettingsObserverProxy mObserverProxy;
Who cleans up the observer proxy?

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/settings/...
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java:32:
private long getNativePtr() {
Put this down below near clearNativePtr

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/settings/...
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java:37:
public void addObserver(SettingsObserver observer) {
Javadocs on some of these?  Specifically on destroy where it's important to note
that the class shouldn't be used afterwards and any method calls will have no
effect.

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/settings/...
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java:48:
public void destroy() {
Do we need this method?  I feel like this is going to be destroyed by native as
it's a unique_ptr.  We might want to be careful/explicit about the lifecycle
here.

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/settings/...
File blimp/client/core/settings/android/settings_android.h (right):

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/settings/...
blimp/client/core/settings/android/settings_android.h:17: class SettingsAndroid
: public Settings {
We should think about whether this should be a subclass or a wrapper class. 
Wrapper might make setup a bit easier, but subclass seems to make more sense
from the object standpoint...

[Menglin, 2016-11-03 23:16:18]

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

[Menglin, 2016-11-03 23:17:01]

hi David, new patch uploaded. ptal. Thanks!

Menglin

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/context/a...
File
blimp/client/core/context/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java
(right):

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/context/a...
blimp/client/core/context/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java:65:
public Settings getSettings() {
On 2016/11/03 03:05:10, David Trainor wrote:
> Add javadoc

Done.

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/context/a...
blimp/client/core/context/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java:66:
return nativeGetSettings(mNativeBlimpClientContextImplAndroid);
On 2016/11/03 03:05:10, David Trainor wrote:
> Assert that mNativeBlimpCLientContextImplAndroid != 0?

Done.

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/context/b...
File blimp/client/core/context/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/context/b...
blimp/client/core/context/blimp_client_context_impl.cc:105: #if
defined(OS_ANDROID)
On 2016/11/03 03:05:10, David Trainor wrote:
> Can we pass the settings object in the constructor and just build it in the
> create calls?  Then we don't have to pass local_state in.

yes we can do that to avoid #if in the constructor

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/settings/...
File
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java
(right):

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/settings/...
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java:25:
private SettingsObserverProxy mObserverProxy;
On 2016/11/03 03:05:10, David Trainor wrote:
> Who cleans up the observer proxy?

it should be cleaned up in clearNativePtr() when Settings' native object is
destroyed.

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/settings/...
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java:32:
private long getNativePtr() {
On 2016/11/03 03:05:10, David Trainor wrote:
> Put this down below near clearNativePtr

Done.

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/settings/...
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java:37:
public void addObserver(SettingsObserver observer) {
On 2016/11/03 03:05:10, David Trainor wrote:
> Javadocs on some of these?  Specifically on destroy where it's important to
note
> that the class shouldn't be used afterwards and any method calls will have no
> effect.

Done.

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/settings/...
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java:48:
public void destroy() {
On 2016/11/03 03:05:10, David Trainor wrote:
> Do we need this method?  I feel like this is going to be destroyed by native
as
> it's a unique_ptr.  We might want to be careful/explicit about the lifecycle
> here.

that's a good point. ObserverProxy should be destroyed when the Settings native
object is destroyed. But the Settings java object wouldn't need be destroyed
from java because its native object is owned by blimp_client_context_impl . I
will remove the destroy method here and add destroying the observer proxy to
clearNativePtr

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/settings/...
File blimp/client/core/settings/android/settings_android.h (right):

https://codereview.chromium.org/2463423002/diff/1/blimp/client/core/settings/...
blimp/client/core/settings/android/settings_android.h:17: class SettingsAndroid
: public Settings {
On 2016/11/03 03:05:10, David Trainor wrote:
> We should think about whether this should be a subclass or a wrapper class. 
> Wrapper might make setup a bit easier, but subclass seems to make more sense
> from the object standpoint...

I'll just keep it as the subclass. have to pass the Settings in the
BlimpClientContextImpl's constructor, but i think it's fine to change that

[commit-bot: I haz the power, 2016-11-03 23:17:49]

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-11-04 03:22:37]

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

[commit-bot: I haz the power, 2016-11-04 03:22:38]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[David Trainor- moved to gerrit, 2016-11-07 18:30:58]

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
File blimp/client/core/context/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
blimp/client/core/context/blimp_client_context_impl.cc:66: auto settings =
base::MakeUnique<SettingsAndroid>(local_state);
Maybe just inline this?  I'm fine either way though.

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
blimp/client/core/context/blimp_client_context_impl.cc:154: DCHECK(!delegate_ ||
!delegate);
Do we support clearing the delegate?  I might just dcheck (!delegate_ &&
delegate)

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
blimp/client/core/context/blimp_client_context_impl.cc:180:
DCHECK(assignment_fetcher_);
<< "Make sure SetDelegate is called with a valid delegate before using this
BlimpClientContext"?

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
blimp/client/core/context/blimp_client_context_impl.cc:198:
DCHECK(assignment_fetcher_);
<< "Make sure SetDelegate is called with a valid delegate before using this
BlimpClientContext."

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
File blimp/client/core/context/blimp_client_context_impl_unittest.cc (right):

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
blimp/client/core/context/blimp_client_context_impl_unittest.cc:71: auto
settings = base::MakeUnique<SettingsAndroid>(&prefs);
Don't make the Android one here.  We don't build the
BlimpClientContextImplAndroid, so we shouldn't build the SettingsAndroid

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/setti...
File
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java
(right):

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/setti...
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java:72:
private void clearNativePtr() {
Can we call this onNativeDestroyed() since it's doing more than just clearing
the native pointer?  I know typically we use clearNativePtr, but that's just
because it's usually all it does.

[Menglin, 2016-11-07 19:11:56]

new patch uploaded. ptal thanks!

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
File blimp/client/core/context/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
blimp/client/core/context/blimp_client_context_impl.cc:66: auto settings =
base::MakeUnique<SettingsAndroid>(local_state);
On 2016/11/07 18:30:58, David Trainor wrote:
> Maybe just inline this?  I'm fine either way though.

i'll just keep it this way then :)

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
blimp/client/core/context/blimp_client_context_impl.cc:154: DCHECK(!delegate_ ||
!delegate);
On 2016/11/07 18:30:58, David Trainor wrote:
> Do we support clearing the delegate?  I might just dcheck (!delegate_ &&
> delegate)

No currently we don't support clearing the delegate.

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
blimp/client/core/context/blimp_client_context_impl.cc:180:
DCHECK(assignment_fetcher_);
On 2016/11/07 18:30:58, David Trainor wrote:
> << "Make sure SetDelegate is called with a valid delegate before using this
> BlimpClientContext"?

in BlimpClientContextImpl::SetDelegate (line 159), the assignment_fetcher_ will
only be set when the delegate is valid, so I think it's ok to just check the
assignment_fetcher_ here?

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
blimp/client/core/context/blimp_client_context_impl.cc:198:
DCHECK(assignment_fetcher_);
On 2016/11/07 18:30:58, David Trainor wrote:
> << "Make sure SetDelegate is called with a valid delegate before using this
> BlimpClientContext."

same as the above?

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
File blimp/client/core/context/blimp_client_context_impl_unittest.cc (right):

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
blimp/client/core/context/blimp_client_context_impl_unittest.cc:71: auto
settings = base::MakeUnique<SettingsAndroid>(&prefs);
On 2016/11/07 18:30:58, David Trainor wrote:
> Don't make the Android one here.  We don't build the
> BlimpClientContextImplAndroid, so we shouldn't build the SettingsAndroid

Done.

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/setti...
File
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java
(right):

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/setti...
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java:72:
private void clearNativePtr() {
On 2016/11/07 18:30:58, David Trainor wrote:
> Can we call this onNativeDestroyed() since it's doing more than just clearing
> the native pointer?  I know typically we use clearNativePtr, but that's just
> because it's usually all it does.

sure. good point

[David Trainor- moved to gerrit, 2016-11-08 00:14:49]

lgtm % nits below.

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
File blimp/client/core/context/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
blimp/client/core/context/blimp_client_context_impl.cc:180:
DCHECK(assignment_fetcher_);
On 2016/11/07 19:11:55, Menglin wrote:
> On 2016/11/07 18:30:58, David Trainor wrote:
> > << "Make sure SetDelegate is called with a valid delegate before using this
> > BlimpClientContext"?
> 
> in BlimpClientContextImpl::SetDelegate (line 159), the assignment_fetcher_
will
> only be set when the delegate is valid, so I think it's ok to just check the
> assignment_fetcher_ here?

That should be fine, the error message just makes it clear why it failed.

https://codereview.chromium.org/2463423002/diff/40001/blimp/client/core/setti...
File
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java
(right):

https://codereview.chromium.org/2463423002/diff/40001/blimp/client/core/setti...
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java:86:
private native void nativeDestroy(long nativeSettingsAndroid);
Not used?

https://codereview.chromium.org/2463423002/diff/40001/blimp/client/core/setti...
File blimp/client/core/settings/android/settings_android.cc (right):

https://codereview.chromium.org/2463423002/diff/40001/blimp/client/core/setti...
blimp/client/core/settings/android/settings_android.cc:42: void
SettingsAndroid::Destroy(JNIEnv* env, jobject jobj) {
Delete this?

[Menglin, 2016-11-08 00:36:46]

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

[commit-bot: I haz the power, 2016-11-08 00:37:08]

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-11-08 01:24:51]

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

[commit-bot: I haz the power, 2016-11-08 01:24:52]

Dry run: This issue passed the CQ dry run.

[Menglin, 2016-11-08 05:03:05]

The CQ bit was checked by mlliu@chromium.org

[Menglin, 2016-11-08 05:03:05]

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/2463423002/#ps60001
(title: "nits and sync to head")

[Menglin, 2016-11-08 05:03:23]

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
File blimp/client/core/context/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
blimp/client/core/context/blimp_client_context_impl.cc:180:
DCHECK(assignment_fetcher_);
On 2016/11/08 00:14:49, David Trainor wrote:
> On 2016/11/07 19:11:55, Menglin wrote:
> > On 2016/11/07 18:30:58, David Trainor wrote:
> > > << "Make sure SetDelegate is called with a valid delegate before using
this
> > > BlimpClientContext"?
> > 
> > in BlimpClientContextImpl::SetDelegate (line 159), the assignment_fetcher_
> will
> > only be set when the delegate is valid, so I think it's ok to just check the
> > assignment_fetcher_ here?
> 
> That should be fine, the error message just makes it clear why it failed.

ok

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
blimp/client/core/context/blimp_client_context_impl.cc:198:
DCHECK(assignment_fetcher_);
On 2016/11/07 19:11:55, Menglin wrote:
> On 2016/11/07 18:30:58, David Trainor wrote:
> > << "Make sure SetDelegate is called with a valid delegate before using this
> > BlimpClientContext."
> 
> same as the above?

Done.

https://codereview.chromium.org/2463423002/diff/40001/blimp/client/core/setti...
File
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java
(right):

https://codereview.chromium.org/2463423002/diff/40001/blimp/client/core/setti...
blimp/client/core/settings/android/java/src/org/chromium/blimp/core/settings/Settings.java:86:
private native void nativeDestroy(long nativeSettingsAndroid);
On 2016/11/08 00:14:49, David Trainor wrote:
> Not used?

ah, i should've removed this!

https://codereview.chromium.org/2463423002/diff/40001/blimp/client/core/setti...
File blimp/client/core/settings/android/settings_android.cc (right):

https://codereview.chromium.org/2463423002/diff/40001/blimp/client/core/setti...
blimp/client/core/settings/android/settings_android.cc:42: void
SettingsAndroid::Destroy(JNIEnv* env, jobject jobj) {
On 2016/11/08 00:14:49, David Trainor wrote:
> Delete this?

same here. i should've removed this!

[commit-bot: I haz the power, 2016-11-08 05:03:32]

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-11-08 05:09:08]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-08 05:11:27]

Description was changed from

==========
Add Settings[, Oberser] java code and JNI bridge.

This CL addes the Settings[, Oberser] java code, a compound observer
proxy SettingsObserverProxy, and the JNI bridge between the java and
native code. Users of the java API should be blimp internal code,
calling getSettings() on the BlimpClientContextImpl to get the Settings
java object.

BUG=647848
==========

to

==========
Add Settings[, Oberser] java code and JNI bridge.

This CL addes the Settings[, Oberser] java code, a compound observer
proxy SettingsObserverProxy, and the JNI bridge between the java and
native code. Users of the java API should be blimp internal code,
calling getSettings() on the BlimpClientContextImpl to get the Settings
java object.

BUG=647848

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

[commit-bot: I haz the power, 2016-11-08 05:11:29]

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

[Khushal, 2016-11-09 01:15:39]

khushalsagar@chromium.org changed reviewers:
+ khushalsagar@chromium.org

[Khushal, 2016-11-09 01:15:40]

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
File blimp/client/core/context/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
blimp/client/core/context/blimp_client_context_impl.cc:154: DCHECK(!delegate_ ||
!delegate);
On 2016/11/07 19:11:55, Menglin wrote:
> On 2016/11/07 18:30:58, David Trainor wrote:
> > Do we support clearing the delegate?  I might just dcheck (!delegate_ &&
> > delegate)
> 
> No currently we don't support clearing the delegate.

We do clear the delegate,
https://cs.chromium.org/chromium/src/chrome/browser/android/blimp/chrome_blim....

[Menglin, 2016-11-11 19:29:02]

On 2016/11/09 01:15:40, Khushal wrote:
>
https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
> File blimp/client/core/context/blimp_client_context_impl.cc (right):
> 
>
https://codereview.chromium.org/2463423002/diff/20001/blimp/client/core/conte...
> blimp/client/core/context/blimp_client_context_impl.cc:154: DCHECK(!delegate_
||
> !delegate);
> On 2016/11/07 19:11:55, Menglin wrote:
> > On 2016/11/07 18:30:58, David Trainor wrote:
> > > Do we support clearing the delegate?  I might just dcheck (!delegate_ &&
> > > delegate)
> > 
> > No currently we don't support clearing the delegate.
> 
> We do clear the delegate,
>
https://cs.chromium.org/chromium/src/chrome/browser/android/blimp/chrome_blim....

Thanks for pointing it out! The fix was checked in
https://codereview.chromium.org/2485323002/