[Sync] Test SCF setting data types.

[Sync] Test SCF setting data types.

Add a test to confirm that data type selections are set when SCF is
closed rather than immediately when the selection is made.

BUG=480604
Committed: https://crrev.com/73e7b9a807f0edc0f62a4c13f25d2bcf62f808a6
Cr-Commit-Position: refs/heads/master@{#329944}

[maxbogue, 2015-05-11 23:58:16]

maxbogue@chromium.org changed reviewers:
+ maniscalco@chromium.org

[maxbogue, 2015-05-11 23:58:16]

Nick, PTAL. This is based off of http://crrev.com/1118833002, which is why the
bots are failing.

[maniscalco, 2015-05-12 20:39:17]

https://codereview.chromium.org/1140593002/diff/1/chrome/android/sync_shell/j...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java
(right):

https://codereview.chromium.org/1140593002/diff/1/chrome/android/sync_shell/j...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java:154:
expectedTypes.remove(ModelType.AUTOFILL);
We're removing AUTOFILL and PASSWORD because those were the items at index 0 and
3 returned by getDataTypes(), right  This coupling is fairly subtle.

One way you could improve it would be to have getDataTypes return a
Map<ModelType, CheckBoxPreference>.

The calls to togglePreference above could then become:

togglePreference(dataTypes.get(ModelType.AUTOFILL));
togglePreference(dataTypes.get(ModelType.PASSWORD));

The use of getDataTypes(fragment) elsewhere could become:

getDataTypes(fragment).values()

And SELECTABLE_DATA_TYPES could probably be replaced with:

getDataTypes(fragment).keySet()

WDYT?

[maxbogue, 2015-05-14 18:17:02]

https://codereview.chromium.org/1140593002/diff/1/chrome/android/sync_shell/j...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java
(right):

https://codereview.chromium.org/1140593002/diff/1/chrome/android/sync_shell/j...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java:154:
expectedTypes.remove(ModelType.AUTOFILL);
On 2015/05/12 20:39:17, maniscalco wrote:
> We're removing AUTOFILL and PASSWORD because those were the items at index 0
and
> 3 returned by getDataTypes(), right  This coupling is fairly subtle.
> 
> One way you could improve it would be to have getDataTypes return a
> Map<ModelType, CheckBoxPreference>.
> 
> The calls to togglePreference above could then become:
> 
> togglePreference(dataTypes.get(ModelType.AUTOFILL));
> togglePreference(dataTypes.get(ModelType.PASSWORD));
> 
> The use of getDataTypes(fragment) elsewhere could become:
> 
> getDataTypes(fragment).values()
> 
> And SELECTABLE_DATA_TYPES could probably be replaced with:
> 
> getDataTypes(fragment).keySet()
> 
> WDYT?
> 

I like it. Done, PTAL!

[maniscalco, 2015-05-14 19:01:43]

LGTM

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

https://codereview.chromium.org/1140593002/diff/40001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java:262:
// There is no Set.containsNone(), sadly.
Too bad about the lack of containsNone.  retainAll is kinda close to what you
want, but I think it's debatable as to whether it's an improvement in
readability.  Up to you.

actualDataTypes.retainAll(disabledDataTypes);
assertTrue(actualDataTypes.isEmpty());

[maxbogue, 2015-05-14 19:41:04]

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

https://codereview.chromium.org/1140593002/diff/40001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java:262:
// There is no Set.containsNone(), sadly.
On 2015/05/14 19:01:43, maniscalco wrote:
> Too bad about the lack of containsNone.  retainAll is kinda close to what you
> want, but I think it's debatable as to whether it's an improvement in
> readability.  Up to you.
> 
> actualDataTypes.retainAll(disabledDataTypes);
> assertTrue(actualDataTypes.isEmpty());

I think the existing code with the comment is clearer, and since it doesn't
modify actualDataTypes it's probably less likely to confuse anyone that might
change this test in the future.

[maxbogue, 2015-05-14 19:41:34]

maxbogue@chromium.org changed reviewers:
+ zea@chromium.org

[maxbogue, 2015-05-14 19:41:34]

+Nicolas for owner approval.

[Nicolas Zea, 2015-05-14 20:04:50]

lgtm

[maxbogue, 2015-05-14 21:53:34]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2015-05-14 21:55:36]

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

[commit-bot: I haz the power, 2015-05-14 22:02:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-14 22:03:20]

Patchset 3 (id:??) landed as
https://crrev.com/73e7b9a807f0edc0f62a4c13f25d2bcf62f808a6
Cr-Commit-Position: refs/heads/master@{#329944}