[Sync] Add bookmark move tests.

[Sync] Add bookmark move tests.

These tests required the following new infrastructure:

- A hack in SyncTestUtil.extractSpecifics to give bookmark specifics.
  the parent ID value.
- FakeServer::ModifyBookmarkEntity to allow setting the parent ID.
- JNI bindings for GetSyncEntitiesByModelType.

BUG=480604
Committed: https://crrev.com/546119359aee306e2ce94f22b998ea5e85bfa555
Cr-Commit-Position: refs/heads/master@{#338567}

[maxbogue, 2015-07-10 00:33:24]

maxbogue@chromium.org changed reviewers:
+ nyquist@chromium.org, pvalenzuela@chromium.org

[maxbogue, 2015-07-10 00:33:25]

Patrick, could you review the tests and fake server changes here?
Tommy, could you review the new JNI?

[pval...(no longer on Chromium), 2015-07-10 01:48:08]

https://codereview.chromium.org/1232003003/diff/20001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java
(right):

https://codereview.chromium.org/1232003003/diff/20001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java:157:
assertEquals("The bookmark was not moved.", "s" + folder.id, bookmark.parentId);
make a note as to why the "s" is there. also, what do you think about adding a
sanitizeId method that will strip the s?

https://codereview.chromium.org/1232003003/diff/20001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java:277:
assertFalse(bookmark.isFolder());
not sure this line (and the one after) are necessary. what do you think about
removing them?

https://codereview.chromium.org/1232003003/diff/20001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java:419:
mBookmarksBridge.moveBookmark(id, newParentId, 0);
it's not clear (from the call here) what the 0 is for. maybe add a /* comment */
after the param?

https://codereview.chromium.org/1232003003/diff/20001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FakeServerHelper.java
(right):

https://codereview.chromium.org/1232003003/diff/20001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FakeServerHelper.java:175:
public List<SyncEntity> getSyncEntitiesByModelType(final ModelType modelType) {
Please add Javadoc

https://codereview.chromium.org/1232003003/diff/20001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FakeServerHelper.java:189:
Log.e(TAG, "Error parsing SyncEntity.", e);
this seems like an error that's worse that simply logging and moving on. perhaps
throw an exception here?

https://codereview.chromium.org/1232003003/diff/20001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java
(right):

https://codereview.chromium.org/1232003003/diff/20001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java:405:
bookmarkSpecifics.put("parent_id", node.getString("PARENT_ID"));
as you mentioned in the description, this is definitely a hack. any other
options you considered before going with this?

https://codereview.chromium.org/1232003003/diff/20001/sync/test/fake_server/a...
File sync/test/fake_server/android/fake_server_helper_android.cc (right):

https://codereview.chromium.org/1232003003/diff/20001/sync/test/fake_server/a...
sync/test/fake_server/android/fake_server_helper_android.cc:122:
base::android::ConvertJavaStringToUTF8(env, model_type_string),
should be indented 4 spaces

[maxbogue, 2015-07-10 17:52:11]

https://codereview.chromium.org/1232003003/diff/20001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java
(right):

https://codereview.chromium.org/1232003003/diff/20001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java:157:
assertEquals("The bookmark was not moved.", "s" + folder.id, bookmark.parentId);
On 2015/07/10 01:48:07, pvalenzuela wrote:
> make a note as to why the "s" is there. also, what do you think about adding a
> sanitizeId method that will strip the s?

Done. Not gonna bother with sanitizeId now because this is the only place it's
come up.

https://codereview.chromium.org/1232003003/diff/20001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java:277:
assertFalse(bookmark.isFolder());
On 2015/07/10 01:48:07, pvalenzuela wrote:
> not sure this line (and the one after) are necessary. what do you think about
> removing them?

Done. They were from before I'd done the if block.

https://codereview.chromium.org/1232003003/diff/20001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java:419:
mBookmarksBridge.moveBookmark(id, newParentId, 0);
On 2015/07/10 01:48:07, pvalenzuela wrote:
> it's not clear (from the call here) what the 0 is for. maybe add a /* comment
*/
> after the param?

Done.

https://codereview.chromium.org/1232003003/diff/20001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FakeServerHelper.java
(right):

https://codereview.chromium.org/1232003003/diff/20001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FakeServerHelper.java:175:
public List<SyncEntity> getSyncEntitiesByModelType(final ModelType modelType) {
On 2015/07/10 01:48:07, pvalenzuela wrote:
> Please add Javadoc

Done.

https://codereview.chromium.org/1232003003/diff/20001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FakeServerHelper.java:189:
Log.e(TAG, "Error parsing SyncEntity.", e);
On 2015/07/10 01:48:07, pvalenzuela wrote:
> this seems like an error that's worse that simply logging and moving on.
perhaps
> throw an exception here?

Done.

https://codereview.chromium.org/1232003003/diff/20001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java
(right):

https://codereview.chromium.org/1232003003/diff/20001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java:405:
bookmarkSpecifics.put("parent_id", node.getString("PARENT_ID"));
On 2015/07/10 01:48:07, pvalenzuela wrote:
> as you mentioned in the description, this is definitely a hack. any other
> options you considered before going with this?

The only solution I would prefer to this would be just passing the entire
SyncEntity protos from the client, but even if that's feasible I don't want to
do it in this CL.

https://codereview.chromium.org/1232003003/diff/20001/sync/test/fake_server/a...
File sync/test/fake_server/android/fake_server_helper_android.cc (right):

https://codereview.chromium.org/1232003003/diff/20001/sync/test/fake_server/a...
sync/test/fake_server/android/fake_server_helper_android.cc:122:
base::android::ConvertJavaStringToUTF8(env, model_type_string),
On 2015/07/10 01:48:07, pvalenzuela wrote:
> should be indented 4 spaces

Done.

[pval...(no longer on Chromium), 2015-07-10 20:24:58]

lgtm

https://codereview.chromium.org/1232003003/diff/40001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FakeServerHelper.java
(right):

https://codereview.chromium.org/1232003003/diff/40001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FakeServerHelper.java:189:
throws Exception {
I think just throwing InvalidProtocolBuffferNanoException would be better here
since it's a little more clear what could go wrong. Note that the current
calling code can stay the same since it throws Exception.

[maxbogue, 2015-07-10 21:54:16]

https://codereview.chromium.org/1232003003/diff/40001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FakeServerHelper.java
(right):

https://codereview.chromium.org/1232003003/diff/40001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FakeServerHelper.java:189:
throws Exception {
On 2015/07/10 20:24:57, pvalenzuela wrote:
> I think just throwing InvalidProtocolBuffferNanoException would be better here
> since it's a little more clear what could go wrong. Note that the current
> calling code can stay the same since it throws Exception.

Changed it to ExecutionException per our offline discussion.

[nyquist, 2015-07-13 18:52:46]

https://codereview.chromium.org/1232003003/diff/60001/sync/test/fake_server/a...
File sync/test/fake_server/android/fake_server_helper_android.cc (right):

https://codereview.chromium.org/1232003003/diff/60001/sync/test/fake_server/a...
sync/test/fake_server/android/fake_server_helper_android.cc:143: jbyteArray
FakeServerHelperAndroid::ProtoToJbyteArray(
Could you instead use MessageLite::SerializeToString
(https://developers.google.com/protocol-buffers/docs/reference/cpp/google.prot...)
and base::android::ToJavaArrayOfByteArray ?

https://codereview.chromium.org/1232003003/diff/60001/sync/test/fake_server/a...
File sync/test/fake_server/android/fake_server_helper_android.h (right):

https://codereview.chromium.org/1232003003/diff/60001/sync/test/fake_server/a...
sync/test/fake_server/android/fake_server_helper_android.h:56: jstring
model_type_string);
I wish someone took it on them to auto-generate the ModelType enum using the
C++-based enum generator for Java? As in; it generates a bunch of public static
int ... values that can be used across Java and C++. Care to file a bug about
that?

[maxbogue, 2015-07-13 19:44:54]

https://codereview.chromium.org/1232003003/diff/60001/sync/test/fake_server/a...
File sync/test/fake_server/android/fake_server_helper_android.cc (right):

https://codereview.chromium.org/1232003003/diff/60001/sync/test/fake_server/a...
sync/test/fake_server/android/fake_server_helper_android.cc:143: jbyteArray
FakeServerHelperAndroid::ProtoToJbyteArray(
On 2015/07/13 18:52:46, nyquist wrote:
> Could you instead use MessageLite::SerializeToString
>
(https://developers.google.com/protocol-buffers/docs/reference/cpp/google.prot...)
> and base::android::ToJavaArrayOfByteArray ?

Done.

https://codereview.chromium.org/1232003003/diff/60001/sync/test/fake_server/a...
File sync/test/fake_server/android/fake_server_helper_android.h (right):

https://codereview.chromium.org/1232003003/diff/60001/sync/test/fake_server/a...
sync/test/fake_server/android/fake_server_helper_android.h:56: jstring
model_type_string);
On 2015/07/13 18:52:46, nyquist wrote:
> I wish someone took it on them to auto-generate the ModelType enum using the
> C++-based enum generator for Java? As in; it generates a bunch of public
static
> int ... values that can be used across Java and C++. Care to file a bug about
> that?

Filed http://crbug.com/509788.

[nyquist, 2015-07-13 20:07:01]

jni lgtm

[maxbogue, 2015-07-13 20:09:44]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2015-07-13 20:09:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
pvalenzuela@chromium.org
Link to the patchset: https://codereview.chromium.org/1232003003/#ps80001
(title: "Use a couple existing JNI methods instead.")

[commit-bot: I haz the power, 2015-07-13 20:10:31]

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

[commit-bot: I haz the power, 2015-07-13 21:43:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-13 21:45:31]

Patchset 5 (id:??) landed as
https://crrev.com/546119359aee306e2ce94f22b998ea5e85bfa555
Cr-Commit-Position: refs/heads/master@{#338567}