Change the CBD dialog on Android to a PreferenceFragment

Change the CBD dialog on Android to a PreferenceFragment

According to the mocks (https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1#%2F00%20-%20Default%20Selection.png), the CBD dialog should look like a standard preferences page.

Therefore, in this CL, we:
1. Make ClearBrowsingDataDialogFragment inherit from PreferencesFragment instead of DialogFragment
2. Following that change, replace CheckedTextView with CheckboxPreference, Button with ButtonPreference etc.
3. The dialog layout is now defined in an XML file instead of being generating through an Adapter.
4. Every ClearBrowsingDataDialogFragment.Item now corresponds to a CheckboxPreference, and thus it can delegate most of its logic (e.g. checked state, enabled state etc.) to that CheckboxPreference.

Furthermore, to match the mocks, we:
5. Change the support string at the bottom of the dialog according to the mocks, and add a "Learn more" link.

What is still different from the mocks and will be done in followup CLs (as they require separate Preference subclasses):
6. The time range combobox is not yet added
7. The support string is not of the correct color.
8. CheckboxPreference-s still have 1px border on top and bottom.

BUG=569870
Committed: https://crrev.com/910dffdd8810791e3627fad75a318c14022d7e76
Cr-Commit-Position: refs/heads/master@{#373203}

[msramek, 2016-01-22 17:14:14]

Description was changed from

==========
Change the CBD dialog on Android to a PreferenceFragment

According to the mocks
(https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1#%2F0...),
the CBD dialog should look like a standard preferences page.

Therefore, in this CL, we:
1. Make ClearBrowsingDataDialogFragment inherit from PreferencesFragment instead
of DialogFragment
2. Following that change, replace CheckedTextView with CheckboxPreference,
Button with ButtonPreference etc.
3. The dialog layout is now defined in an XML file instead of being generating
through an Adapter.
4. Every ClearBrowsingDataDialogFragment.Item now corresponds to a
CheckboxPreference, and thus it can delegate most of its logic (e.g. checked
state, enabled state etc.) to that CheckboxPreference.

Furthermore, to match the mocks, we:
5. Change the support string at the bottom of the dialog according to the mocks.

What is still different from the mocks and will be done in followup CLs (as they
require separate Preference subclasses):
6. The time range combobox is not yet added
7. The support string is not of the correct color.
8. CheckboxPreference-s still have 1px border on top and bottom.

BUG=549172
==========

to

==========
Change the CBD dialog on Android to a PreferenceFragment

According to the mocks
(https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1#%2F0...),
the CBD dialog should look like a standard preferences page.

Therefore, in this CL, we:
1. Make ClearBrowsingDataDialogFragment inherit from PreferencesFragment instead
of DialogFragment
2. Following that change, replace CheckedTextView with CheckboxPreference,
Button with ButtonPreference etc.
3. The dialog layout is now defined in an XML file instead of being generating
through an Adapter.
4. Every ClearBrowsingDataDialogFragment.Item now corresponds to a
CheckboxPreference, and thus it can delegate most of its logic (e.g. checked
state, enabled state etc.) to that CheckboxPreference.

Furthermore, to match the mocks, we:
5. Change the support string at the bottom of the dialog according to the mocks,
and add a "Learn more" link.

What is still different from the mocks and will be done in followup CLs (as they
require separate Preference subclasses):
6. The time range combobox is not yet added
7. The support string is not of the correct color.
8. CheckboxPreference-s still have 1px border on top and bottom.

BUG=549172
==========

[msramek, 2016-01-22 17:47:38]

Patchset #1 (id:1) has been deleted

[msramek, 2016-01-22 17:52:48]

Description was changed from

==========
Change the CBD dialog on Android to a PreferenceFragment

According to the mocks
(https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1#%2F0...),
the CBD dialog should look like a standard preferences page.

Therefore, in this CL, we:
1. Make ClearBrowsingDataDialogFragment inherit from PreferencesFragment instead
of DialogFragment
2. Following that change, replace CheckedTextView with CheckboxPreference,
Button with ButtonPreference etc.
3. The dialog layout is now defined in an XML file instead of being generating
through an Adapter.
4. Every ClearBrowsingDataDialogFragment.Item now corresponds to a
CheckboxPreference, and thus it can delegate most of its logic (e.g. checked
state, enabled state etc.) to that CheckboxPreference.

Furthermore, to match the mocks, we:
5. Change the support string at the bottom of the dialog according to the mocks,
and add a "Learn more" link.

What is still different from the mocks and will be done in followup CLs (as they
require separate Preference subclasses):
6. The time range combobox is not yet added
7. The support string is not of the correct color.
8. CheckboxPreference-s still have 1px border on top and bottom.

BUG=549172
==========

to

==========
Change the CBD dialog on Android to a PreferenceFragment

According to the mocks
(https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1#%2F0...),
the CBD dialog should look like a standard preferences page.

Therefore, in this CL, we:
1. Make ClearBrowsingDataDialogFragment inherit from PreferencesFragment instead
of DialogFragment
2. Following that change, replace CheckedTextView with CheckboxPreference,
Button with ButtonPreference etc.
3. The dialog layout is now defined in an XML file instead of being generating
through an Adapter.
4. Every ClearBrowsingDataDialogFragment.Item now corresponds to a
CheckboxPreference, and thus it can delegate most of its logic (e.g. checked
state, enabled state etc.) to that CheckboxPreference.

Furthermore, to match the mocks, we:
5. Change the support string at the bottom of the dialog according to the mocks,
and add a "Learn more" link.

What is still different from the mocks and will be done in followup CLs (as they
require separate Preference subclasses):
6. The time range combobox is not yet added
7. The support string is not of the correct color.
8. CheckboxPreference-s still have 1px border on top and bottom.

BUG=569870
==========

[msramek, 2016-01-22 17:59:43]

msramek@chromium.org changed reviewers:
+ melandory@chromium.org, newt@chromium.org

[msramek, 2016-01-22 17:59:43]

Hi Newt and Tanja,

Please have a look! This is a follow-up on
https://codereview.chromium.org/1530123002/, where I continue to change the UI.
It turns out that in the previous CL I partially moved in the wrong direction,
as PreferenceFragment has a quite different paradigm of doing things.

I still need to fix the tests, but otherwise I think the CL is ready for your
attention :)

Thanks,
Martin

[msramek, 2016-01-22 18:46:30]

+cc dmurph@ To keep you in the loop, in case you're already starting to
implement the UI. Nothing should change in the fact that the starting point for
your UI is ClearBrowsingDataDialog.onOptionSelected(), but I want you to be
aware that I'm still refactoring the dialog itself to avoid merge conflicts :)

[dmurph, 2016-01-22 18:48:44]

On 2016/01/22 at 18:46:30, msramek wrote:
> +cc dmurph@ To keep you in the loop, in case you're already starting to
implement the UI. Nothing should change in the fact that the starting point for
your UI is ClearBrowsingDataDialog.onOptionSelected(), but I want you to be
aware that I'm still refactoring the dialog itself to avoid merge conflicts :)

Thanks! It'll be great to know when this goes in.

[msramek, 2016-01-28 18:43:48]

msramek@chromium.org changed required reviewers:
+ newt@chromium.org

[msramek, 2016-01-28 18:43:49]

I made some more changes after a UX discussion we had today, pointed them out in
comments in the latest patchset.

Also, I finally set up the environment to test everything locally, so even tests
should now pass.

Newt, can you please have a look?

Please start at the latest patchset (the previous ones were just blind attempts
to find the test failures), and see both the CL description and my comments for
context.

Thanks,
Martin

https://codereview.chromium.org/1618413002/diff/100001/chrome/android/java/re...
File chrome/android/java/res/xml/privacy_preferences.xml (right):

https://codereview.chromium.org/1618413002/diff/100001/chrome/android/java/re...
chrome/android/java/res/xml/privacy_preferences.xml:70: <Preference
I had a discussion with our UX designer today and we decided to replace the
button with a simple preference, since it was a special case and didn't fit into
our settings UI.

https://codereview.chromium.org/1618413002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java
(left):

https://codereview.chromium.org/1618413002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:527:
PrivacyPreferences.class.getName());
Previously, we opened PrivacyPreferences and gave it a parameter to open
ClearBrowsingDataDialogFragment in its onCreate().

However, since the CBD dialog is now its own PreferenceFragment (as opposed to a
DialogFragment opened atop of PreferenceFragment), we can now open it directly.

This method is currently only called from the history tab. The positive effect
of this change is that opening the CBD dialog and tapping "Back" will lead you
back to the history page, not to PrivacyPreferences.

https://codereview.chromium.org/1618413002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ClearSyncDataDialogFragment.java
(right):

https://codereview.chromium.org/1618413002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ClearSyncDataDialogFragment.java:23:
public void onAttach(Context context) {
onAttach(Activity activity) is deprecated, and onAttach(Context context) is the
replacement. However, this is a recent change - on Lollipop, only
onAttach(Activity activity) is called.

I left both versions for compatibility, but I'm not sure what is the correct
approach in this case. I expect that we do want to still support Lollipop?

https://codereview.chromium.org/1618413002/diff/100001/chrome/android/javates...
File chrome/android/javatests/src/org/chromium/chrome/browser/HistoryUITest.java
(right):

https://codereview.chromium.org/1618413002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/HistoryUITest.java:240:
PreferenceScreen screen =
Since clicking the "Clear" button in the dialog closes it immediately, and
returns right to the history tab (see the comment in ChromeApplication.java),
this test had to be restructured.

Clicking the button is now the trigger for ActivityUtils.waitForActivity, and
the "clearBrowsingFragment.isVisible()" check had to move below.

[msramek, 2016-01-28 18:55:50]

Sigh... Patchset 6 - removed a unused variable, as it didn't compile on trybots.
Whatever I said about "last patchset" is now true of "Patchset 5" :)

[newt (away), 2016-01-29 20:16:02]

Thanks for this! Looks generally good -- mostly minor comments inline.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/re...
File chrome/android/java/res/values/values.xml (right):

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/re...
chrome/android/java/res/values/values.xml:58: <string
name="help_context_clear_browsing_data">clear_browsing_data</string>
Could you update the go/mobilehelprecs doc? (see the "Instructions" tab and the
"Chrome" tab in that doc)

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/re...
File chrome/android/java/res/xml/clear_browsing_data_dialog.xml (right):

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/re...
chrome/android/java/res/xml/clear_browsing_data_dialog.xml:1: <?xml
version="1.0" encoding="utf-8"?>
This should be called clear_browsing_data_preferences.xml to follow the existing
file naming pattern

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/re...
chrome/android/java/res/xml/clear_browsing_data_dialog.xml:11:
android:key="clear_browsing_data_history_checkbox"
These keys don't need to be globally unique. They could probably be terser, say,
"clear_history_checkbox" or even "clear_history".

I'd also add android:persistent="false" to each of these. That'll ensure that
Android doesn't read and store the values of these checkboxes in shared prefs,
which is unnecessary, duplicates knowledge, and -- the best part -- will cause
ClassCastExceptions if the type of these preferences ever changes (e.g. from a
checkbox to a multiple-choice pref).

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/re...
chrome/android/java/res/xml/clear_browsing_data_dialog.xml:28:
android:title="@string/clear_formdata_title" />
might as well spell this "form_data" instead of "formdata" for consistency

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/re...
chrome/android/java/res/xml/clear_browsing_data_dialog.xml:47:
android:key="clear_browsing_data_learn_more"
No need to add keys for Preferences that aren't referenced from code. The
absence of "key" indicates at a glance that this preference isn't modified
dynamically, which makes understanding this file a bit easier.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java
(right):

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:28:
public class ClearBrowsingDataDialogFragment extends PreferenceFragment
I'd call this "ClearBrowsingDataPreferences" to parallel the names of all the
other screens in preferences (MainPreferences, AutofillPreferences, etc).

(Please do the renaming as a separate patch after other comments have been
addressed so that reviewing is easier.)

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:55:
mCheckbox.setPersistent(false);
This might be too late to set persistent to false; at this point Android may
already have tried to read the default value from shared prefs. I'd probably set
android:persistent="false" directly in the XML.

(Side-note: I'd like to have a general solution to this issue. If you have
ideas, I'm all ears.)

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:59:
mCheckbox.setChecked(enabled && selected);
Are there any cases where a checkbox could be force-checked? i.e. enabled=false
and selected=true? This line wouldn't handle that case correctly.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:210:
return true;
you should only return true if the click was handled

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:245:
EnumSet<DialogOption> unbound_options = EnumSet.allOf(DialogOption.class);
s/upbound_options/unboundOptions/

(checkstyle should have complained about this on git cl upload...)

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:256:
// Only one footnote should be shown, the unsynced or synced version, depending
on whether
Rather than removing one of these two preferences, I'd use a single
TextMessagePreference and set its text programmatically here. Inflating views is
quite expensive (typically several ms), so it's best to avoid inflating views
that won't be used.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:265:
getActivity().setTitle(R.string.clear_browsing_data_title);
nit: move this to near the top of onCreate()

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ClearSyncDataDialogFragment.java
(right):

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ClearSyncDataDialogFragment.java:25:
mApplicationContext = context;
s/context/context.getApplicationContext()/

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ClearSyncDataDialogFragment.java:29:
public void onAttach(Activity activity) {
IIUC, onAttach(Context context) is not being called on pre-M devices because
this class extends android.app.Fragment (via
android.preference.PreferenceFragment). Typically, we'd extend
android.support.v4.app.Fragment, which ensures that onAttach(Context context) is
called on all devices. However, there wasn't a support version of
PreferenceFragment available until recently and we haven't switched over to
using it yet. So, the current code is OK for now.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmAccountChangeFragment.java
(right):

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmAccountChangeFragment.java:112:
private void showClearSyncDataDialogFragment() {
update this method name

[msramek, 2016-02-02 10:04:10]

Patchset #7 (id:140001) has been deleted

[msramek, 2016-02-02 12:19:51]

Hi Newt,

PTAL! I split the renaming into a separate patchset. The diffs are still shown
somewhat weirdly (e.g. the 8..9 diff in one of the renamed files still
highlights one 7..9 change), but it should be reviewable.

Martin

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/re...
File chrome/android/java/res/values/values.xml (right):

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/re...
chrome/android/java/res/values/values.xml:58: <string
name="help_context_clear_browsing_data">clear_browsing_data</string>
On 2016/01/29 20:16:02, newt wrote:
> Could you update the go/mobilehelprecs doc? (see the "Instructions" tab and
the
> "Chrome" tab in that doc)

Done. Added an entry, contacted the TPM.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/re...
File chrome/android/java/res/xml/clear_browsing_data_dialog.xml (right):

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/re...
chrome/android/java/res/xml/clear_browsing_data_dialog.xml:1: <?xml
version="1.0" encoding="utf-8"?>
On 2016/01/29 20:16:02, newt wrote:
> This should be called clear_browsing_data_preferences.xml to follow the
existing
> file naming pattern

Done.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/re...
chrome/android/java/res/xml/clear_browsing_data_dialog.xml:11:
android:key="clear_browsing_data_history_checkbox"
On 2016/01/29 20:16:02, newt wrote:
> These keys don't need to be globally unique. They could probably be terser,
say,
> "clear_history_checkbox" or even "clear_history".
> 
> I'd also add android:persistent="false" to each of these. That'll ensure that
> Android doesn't read and store the values of these checkboxes in shared prefs,
> which is unnecessary, duplicates knowledge, and -- the best part -- will cause
> ClassCastExceptions if the type of these preferences ever changes (e.g. from a
> checkbox to a multiple-choice pref).

Done. I'll keep the "checkbox" part, so that it doesn't sound like
"clear_history" and "clear_button" are the same category.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/re...
chrome/android/java/res/xml/clear_browsing_data_dialog.xml:28:
android:title="@string/clear_formdata_title" />
On 2016/01/29 20:16:02, newt wrote:
> might as well spell this "form_data" instead of "formdata" for consistency

Agreed, changed it. It was this way in android_chrome_strings.grd.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/re...
chrome/android/java/res/xml/clear_browsing_data_dialog.xml:47:
android:key="clear_browsing_data_learn_more"
On 2016/01/29 20:16:02, newt wrote:
> No need to add keys for Preferences that aren't referenced from code. The
> absence of "key" indicates at a glance that this preference isn't modified
> dynamically, which makes understanding this file a bit easier.

Done. Makes sense. I thought it was a good practice to name everything :)

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java
(right):

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:28:
public class ClearBrowsingDataDialogFragment extends PreferenceFragment
On 2016/01/29 20:16:02, newt wrote:
> I'd call this "ClearBrowsingDataPreferences" to parallel the names of all the
> other screens in preferences (MainPreferences, AutofillPreferences, etc).
> 
> (Please do the renaming as a separate patch after other comments have been
> addressed so that reviewing is easier.)

Done. Renamed this, the corresponding XML, and also ClearSyncDataDialogFragment.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:55:
mCheckbox.setPersistent(false);
On 2016/01/29 20:16:02, newt wrote:
> This might be too late to set persistent to false; at this point Android may
> already have tried to read the default value from shared prefs. I'd probably
set
> android:persistent="false" directly in the XML.
> 
> (Side-note: I'd like to have a general solution to this issue. If you have
> ideas, I'm all ears.)

Done.

I heard some vague plans about syncing preferences between desktop and mobile.
I'm not sure what is the current status. But if that happens, basically every
preference in Chrome's settings will have to defer to the native PrefService. So
the Sync team might have some ideas.

Perhaps inherit all preferences from a custom ChromePreference, where
ChromePreference.getSharedPreferences() will return ChromeSharedPreferences, a
subclass of SharedPreferences, that in fact delegates everything to the native
PrefService? SharedPreferences seems to have all methods abstract, so that
should be possible. Then put in some DCHECKS so that nobody uses
android.preference.Preference.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:59:
mCheckbox.setChecked(enabled && selected);
On 2016/01/29 20:16:02, newt wrote:
> Are there any cases where a checkbox could be force-checked? i.e.
enabled=false
> and selected=true? This line wouldn't handle that case correctly.

The only case for a disabled checkbox should be a supervised user that is not
allowed to delete their history, in which case the checkbox should be also
unchecked.

However, good that you pointed this out! My handling was incorrect. If it was
the case that the deletion preference was true, but history deletion was
disabled, this would simply show the checkbox as unchecked, but would not toggle
the deletion preference to false.

I removed this, and instead moved the handling to where
mCanDeleteBrowsingHistory is evaluated.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:210:
return true;
On 2016/01/29 20:16:02, newt wrote:
> you should only return true if the click was handled

Done.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:245:
EnumSet<DialogOption> unbound_options = EnumSet.allOf(DialogOption.class);
On 2016/01/29 20:16:02, newt wrote:
> s/upbound_options/unboundOptions/
> 
> (checkstyle should have complained about this on git cl upload...)

Done. If it's just a warning, I might have missed it.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:256:
// Only one footnote should be shown, the unsynced or synced version, depending
on whether
On 2016/01/29 20:16:02, newt wrote:
> Rather than removing one of these two preferences, I'd use a single
> TextMessagePreference and set its text programmatically here. Inflating views
is
> quite expensive (typically several ms), so it's best to avoid inflating views
> that won't be used.

Done. And actually, now that there's just one, let me rename it to "summary".

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:265:
getActivity().setTitle(R.string.clear_browsing_data_title);
On 2016/01/29 20:16:02, newt wrote:
> nit: move this to near the top of onCreate()

Done.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ClearSyncDataDialogFragment.java
(right):

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ClearSyncDataDialogFragment.java:25:
mApplicationContext = context;
On 2016/01/29 20:16:02, newt wrote:
> s/context/context.getApplicationContext()/

Done.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ClearSyncDataDialogFragment.java:29:
public void onAttach(Activity activity) {
On 2016/01/29 20:16:02, newt wrote:
> IIUC, onAttach(Context context) is not being called on pre-M devices because
> this class extends android.app.Fragment (via
> android.preference.PreferenceFragment). Typically, we'd extend
> android.support.v4.app.Fragment, which ensures that onAttach(Context context)
is
> called on all devices. However, there wasn't a support version of
> PreferenceFragment available until recently and we haven't switched over to
> using it yet. So, the current code is OK for now.

Acknowledged.

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmAccountChangeFragment.java
(right):

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmAccountChangeFragment.java:112:
private void showClearSyncDataDialogFragment() {
On 2016/01/29 20:16:02, newt wrote:
> update this method name

Done.

[msramek, 2016-02-02 12:33:59]

On 2016/02/02 12:19:51, msramek wrote:
> PTAL! I split the renaming into a separate patchset. The diffs are still shown
> somewhat weirdly (e.g. the 8..9 diff in one of the renamed files still
> highlights one 7..9 change), but it should be reviewable.
> 

Ah, nevermind, sorry for the confusion. For a renamed file, it just always shows
the diff with the original file at the base of the CL, not the with the original
file in the given patchset.

[newt (away), 2016-02-02 19:35:03]

lgtm. Thanks!

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java
(right):

https://codereview.chromium.org/1618413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:55:
mCheckbox.setPersistent(false);
On 2016/02/02 12:19:51, msramek wrote:
> On 2016/01/29 20:16:02, newt wrote:
> > This might be too late to set persistent to false; at this point Android may
> > already have tried to read the default value from shared prefs. I'd probably
> set
> > android:persistent="false" directly in the XML.
> > 
> > (Side-note: I'd like to have a general solution to this issue. If you have
> > ideas, I'm all ears.)
> 
> Done.
> 
> I heard some vague plans about syncing preferences between desktop and mobile.
> I'm not sure what is the current status. But if that happens, basically every
> preference in Chrome's settings will have to defer to the native PrefService.
So
> the Sync team might have some ideas.
> 
> Perhaps inherit all preferences from a custom ChromePreference, where
> ChromePreference.getSharedPreferences() will return ChromeSharedPreferences, a
> subclass of SharedPreferences, that in fact delegates everything to the native
> PrefService? SharedPreferences seems to have all methods abstract, so that
> should be possible. Then put in some DCHECKS so that nobody uses
> android.preference.Preference.

These are good ideas. We already have Chrome-specific base classes for most
preference types (e.g. ChromeBaseCheckBoxPreference). We could setPersistent()
false in those, or possibly override the PreferenceManager or the
SharedPreferences, either at the Activity level or at the individual Preference
level.

[msramek, 2016-02-03 09:22:46]

Thanks, Newt!

[msramek, 2016-02-03 09:22:51]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2016-02-03 09:23:53]

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

[commit-bot: I haz the power, 2016-02-03 09:29:53]

Description was changed from

==========
Change the CBD dialog on Android to a PreferenceFragment

According to the mocks
(https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1#%2F0...),
the CBD dialog should look like a standard preferences page.

Therefore, in this CL, we:
1. Make ClearBrowsingDataDialogFragment inherit from PreferencesFragment instead
of DialogFragment
2. Following that change, replace CheckedTextView with CheckboxPreference,
Button with ButtonPreference etc.
3. The dialog layout is now defined in an XML file instead of being generating
through an Adapter.
4. Every ClearBrowsingDataDialogFragment.Item now corresponds to a
CheckboxPreference, and thus it can delegate most of its logic (e.g. checked
state, enabled state etc.) to that CheckboxPreference.

Furthermore, to match the mocks, we:
5. Change the support string at the bottom of the dialog according to the mocks,
and add a "Learn more" link.

What is still different from the mocks and will be done in followup CLs (as they
require separate Preference subclasses):
6. The time range combobox is not yet added
7. The support string is not of the correct color.
8. CheckboxPreference-s still have 1px border on top and bottom.

BUG=569870
==========

to

==========
Change the CBD dialog on Android to a PreferenceFragment

According to the mocks
(https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1#%2F0...),
the CBD dialog should look like a standard preferences page.

Therefore, in this CL, we:
1. Make ClearBrowsingDataDialogFragment inherit from PreferencesFragment instead
of DialogFragment
2. Following that change, replace CheckedTextView with CheckboxPreference,
Button with ButtonPreference etc.
3. The dialog layout is now defined in an XML file instead of being generating
through an Adapter.
4. Every ClearBrowsingDataDialogFragment.Item now corresponds to a
CheckboxPreference, and thus it can delegate most of its logic (e.g. checked
state, enabled state etc.) to that CheckboxPreference.

Furthermore, to match the mocks, we:
5. Change the support string at the bottom of the dialog according to the mocks,
and add a "Learn more" link.

What is still different from the mocks and will be done in followup CLs (as they
require separate Preference subclasses):
6. The time range combobox is not yet added
7. The support string is not of the correct color.
8. CheckboxPreference-s still have 1px border on top and bottom.

BUG=569870
==========

[commit-bot: I haz the power, 2016-02-03 09:29:55]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2016-02-03 09:31:05]

Description was changed from

==========
Change the CBD dialog on Android to a PreferenceFragment

According to the mocks
(https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1#%2F0...),
the CBD dialog should look like a standard preferences page.

Therefore, in this CL, we:
1. Make ClearBrowsingDataDialogFragment inherit from PreferencesFragment instead
of DialogFragment
2. Following that change, replace CheckedTextView with CheckboxPreference,
Button with ButtonPreference etc.
3. The dialog layout is now defined in an XML file instead of being generating
through an Adapter.
4. Every ClearBrowsingDataDialogFragment.Item now corresponds to a
CheckboxPreference, and thus it can delegate most of its logic (e.g. checked
state, enabled state etc.) to that CheckboxPreference.

Furthermore, to match the mocks, we:
5. Change the support string at the bottom of the dialog according to the mocks,
and add a "Learn more" link.

What is still different from the mocks and will be done in followup CLs (as they
require separate Preference subclasses):
6. The time range combobox is not yet added
7. The support string is not of the correct color.
8. CheckboxPreference-s still have 1px border on top and bottom.

BUG=569870
==========

to

==========
Change the CBD dialog on Android to a PreferenceFragment

According to the mocks
(https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1#%2F0...),
the CBD dialog should look like a standard preferences page.

Therefore, in this CL, we:
1. Make ClearBrowsingDataDialogFragment inherit from PreferencesFragment instead
of DialogFragment
2. Following that change, replace CheckedTextView with CheckboxPreference,
Button with ButtonPreference etc.
3. The dialog layout is now defined in an XML file instead of being generating
through an Adapter.
4. Every ClearBrowsingDataDialogFragment.Item now corresponds to a
CheckboxPreference, and thus it can delegate most of its logic (e.g. checked
state, enabled state etc.) to that CheckboxPreference.

Furthermore, to match the mocks, we:
5. Change the support string at the bottom of the dialog according to the mocks,
and add a "Learn more" link.

What is still different from the mocks and will be done in followup CLs (as they
require separate Preference subclasses):
6. The time range combobox is not yet added
7. The support string is not of the correct color.
8. CheckboxPreference-s still have 1px border on top and bottom.

BUG=569870

Committed: https://crrev.com/910dffdd8810791e3627fad75a318c14022d7e76
Cr-Commit-Position: refs/heads/master@{#373203}
==========

[commit-bot: I haz the power, 2016-02-03 09:31:06]

Patchset 9 (id:??) landed as
https://crrev.com/910dffdd8810791e3627fad75a318c14022d7e76
Cr-Commit-Position: refs/heads/master@{#373203}