Add a time period dropdown to the CBD dialog on Android.

Add a time period dropdown to the CBD dialog on Android.

1. Define SpinnerPreference, a subclass of Preference that represents a dropdown of any Object-s.
2. Add an instance of SpinnerPreference to the CBD dialog.
3. Bind the dropdown values to BrowsingDataRemover::TimePeriod through PrefServiceBridge.

Older mocks for reference: https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1#%2F00%20-%20Default%20Selection.png

We don't match the mocks perfectly yet, as the style and strings are still being discussed.

BUG=569870
Committed: https://crrev.com/0c66f43c2e7acfc0749e54008932ff81c7c2c38c
Cr-Commit-Position: refs/heads/master@{#376449}

[msramek, 2016-02-04 14:01:15]

Description was changed from

==========
Add a time period dropdown to the CBD dialog on Android.

1. Define SpinnerPreference, a subclass of Preference that represents a dropdown
of any Object-s.
2. Add an instance of SpinnerPreference to the CBD dialog.
3. Bind the dropdown values to BrowsingDataRemover::TimePeriod through
PrefServiceBridge.

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

to

==========
Add a time period dropdown to the CBD dialog on Android.

1. Define SpinnerPreference, a subclass of Preference that represents a dropdown
of any Object-s.
2. Add an instance of SpinnerPreference to the CBD dialog.
3. Bind the dropdown values to BrowsingDataRemover::TimePeriod through
PrefServiceBridge.

Older mocks for reference:
https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1#%2F0...

We don't match the mocks perfectly yet, as the style and strings are still being
discussed.

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

[msramek, 2016-02-04 14:06:25]

Patchset #1 (id:1) has been deleted

[msramek, 2016-02-04 14:19:52]

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

[msramek, 2016-02-04 14:19:53]

Hi Newt and Bernhard,

here comes another change to the CBD dialog on Android. Please have a look!

Thanks,
Martin

[Bernhard Bauer, 2016-02-08 16:15:06]

https://codereview.chromium.org/1663983003/diff/20001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1663983003/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:463: <message
name="IDS_CLEAR_BROWSING_DATA_PERIOD_HOUR" desc="The option to delete browsing
from the past hour. This is the direct object of the sentence 'Clear data from
the past hour'. Keep the first letter capitalized, as the options are shown
standalone.">
The first letter in this message isn't capitalized.

Your description only mentions older mocks -- do you have mocks for this
version?

[Bernhard Bauer, 2016-02-08 16:15:06]

https://codereview.chromium.org/1663983003/diff/20001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1663983003/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:463: <message
name="IDS_CLEAR_BROWSING_DATA_PERIOD_HOUR" desc="The option to delete browsing
from the past hour. This is the direct object of the sentence 'Clear data from
the past hour'. Keep the first letter capitalized, as the options are shown
standalone.">
The first letter in this message isn't capitalized.

Your description only mentions older mocks -- do you have mocks for this
version?

[msramek, 2016-02-09 10:39:54]

https://codereview.chromium.org/1663983003/diff/20001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1663983003/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:463: <message
name="IDS_CLEAR_BROWSING_DATA_PERIOD_HOUR" desc="The option to delete browsing
from the past hour. This is the direct object of the sentence 'Clear data from
the past hour'. Keep the first letter capitalized, as the options are shown
standalone.">
On 2016/02/08 16:15:05, Bernhard Bauer wrote:
> The first letter in this message isn't capitalized.
> 
> Your description only mentions older mocks -- do you have mocks for this
> version?

Thanks for catching :) This is an illustration of how the design kept changing
while I was writing this CL.

These slides
https://docs.google.com/presentation/d/1k4_lZEnXn2e-U_3BQwvdZvtge3Jgld3bNM-do...
were OK'd by the UI review. They already show the non-capitalized options. The
dropdown should now look correctly, except that it uses the same popup
background view as other dropdowns in Chrome (slightly less fancy and
material-designish than in the mocks). IIUC, this is android:popupBackground
that is set globally in styles.xml, so in the end we decided not to override it
for this particular instance.

[newt (away), 2016-02-11 23:46:11]

Looks generally good. I haven't looked at ClearBrowsingDataPreferences.java yet.
Sorry for the delays, I've been OOO and traveling. I'll finish up tomorrow.

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/preference_spinner.xml (right):

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/preference_spinner.xml:16: <TextView
Is this the right style? color, size, etc?

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/preference_spinner.xml:21: <Spinner
You can just add style="@style/Widget.AppCompat.Spinner.Underlined" to the
Spinner and you can remove the UnderlineView below

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java
(right):

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:24:
private int mInitialIndex;
s/Initial/Selected/ ?

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:34:
public void setOptions(Object[] options, int initialIndex) {
javadoc for public methods

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:37:
mAdapter.setDropDownViewResource(android.R.layout.simple_spinner_dropdown_item);
Does this work on pre-L devices (i.e. on Holo devices)? You should probably use
the equivalent resources from the support library, assuming it exists. i.e.
"R.layout.simple_spinner_dropdown_item" without the "android." prefix. Same for
simple_spinner_item above.

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:50:
((TextView) view.findViewById(R.id.title)).setText(this.getTitle());
remove "this."

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:56:
mSpinner.setOnItemSelectedListener(new AdapterView.OnItemSelectedListener() {
Since onBindView() can be called many times, it'd be best to store and reuse the
OnItemSelectedListener rather than create a new once each time.

You could just construct it as a member variable:

  private OnItemSelectedListener mListener = new OnItemSelectedListener() { ...
};

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:68:
if (getOnPreferenceChangeListener() != null) {
Does this method get called in practice? The javadoc says "The selection can
disappear for instance when touch is activated". We probably don't want to
update listeners when the user has simply opened the dropdown.

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:463: <message
name="IDS_CLEAR_BROWSING_DATA_PERIOD_HOUR" desc="The option to delete browsing
data from the past hour. This is the direct object of the sentence 'Clear data
from the past hour'.">
Can we reuse the strings from C++? If these are already defined in
generated_resources.grd, just add formatter_data="android_java" to the strings
in generated_resources.grd and they'll be automagically available in Java, just
like strings defined in android_chrome_strings.grd.

[msramek, 2016-02-12 15:10:49]

No worries, I know, I stalked your calendar :) I really appreciate that you had
time to look at it all.

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/preference_spinner.xml (right):

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/preference_spinner.xml:16: <TextView
On 2016/02/11 23:46:11, newt (slow) wrote:
> Is this the right style? color, size, etc?

Yes, the label is correct as it is. I tweaked the padding on the Spinner though.

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/preference_spinner.xml:21: <Spinner
On 2016/02/11 23:46:11, newt (slow) wrote:
> You can just add mailto:style="@style/Widget.AppCompat.Spinner.Underlined" to
the
> Spinner and you can remove the UnderlineView below

I kept the PreferenceSpinnerUnderlineView, as
Widget.AppCompat.Spinner.Underlined retains its left padding (even after I set
it to zero on the Spinner itself), and has a different color.
PreferenceSpinnerUnderlineView matches the mocks out of the box.

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java
(right):

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:24:
private int mInitialIndex;
On 2016/02/11 23:46:11, newt (slow) wrote:
> s/Initial/Selected/ ?

Done.

This was originally meant to only mean the initial value. But now I noticed that
I kept resetting this value on onBindView(), so I renamed it and update it in
the listener.

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:34:
public void setOptions(Object[] options, int initialIndex) {
On 2016/02/11 23:46:11, newt (slow) wrote:
> javadoc for public methods

Done.

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:37:
mAdapter.setDropDownViewResource(android.R.layout.simple_spinner_dropdown_item);
On 2016/02/11 23:46:11, newt (slow) wrote:
> Does this work on pre-L devices (i.e. on Holo devices)? You should probably
use
> the equivalent resources from the support library, assuming it exists. i.e.
> "R.layout.simple_spinner_dropdown_item" without the "android." prefix. Same
for
> simple_spinner_item above.

I searched the code for other instances of spinners:

https://code.google.com/p/chromium/codesearch#search/&q=%22setDropDownViewRes...

It seems that we use android.R.layout.simple_spinner_dropdown_item and
android.R.layout.simple_spinner_item almost everywhere, so I'm at least not
decreasing the compatibility. I can try to flash Holo on Monday to see how it
looks like.

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:50:
((TextView) view.findViewById(R.id.title)).setText(this.getTitle());
On 2016/02/11 23:46:11, newt (slow) wrote:
> remove "this."

Done.

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:56:
mSpinner.setOnItemSelectedListener(new AdapterView.OnItemSelectedListener() {
On 2016/02/11 23:46:11, newt (slow) wrote:
> Since onBindView() can be called many times, it'd be best to store and reuse
the
> OnItemSelectedListener rather than create a new once each time.
> 
> You could just construct it as a member variable:
> 
>   private OnItemSelectedListener mListener = new OnItemSelectedListener() {
...
> };

Actually, let me split this into onCreateView() with the initialization of
everything that remains constant, and onBindView() with things that could change
(that's mSpinner.setAdapter() and mSpinner.setSelection(), since those can
change through setOptions()).

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:68:
if (getOnPreferenceChangeListener() != null) {
On 2016/02/11 23:46:11, newt (slow) wrote:
> Does this method get called in practice? The javadoc says "The selection can
> disappear for instance when touch is activated". We probably don't want to
> update listeners when the user has simply opened the dropdown.

Hmm, I wasn't able to reproduce that. Either that doesn't happen on all
versions, or the javadoc is incorrect.

If it does happen under some circumstances, then I agree that we don't want to
update the listeners, so I removed the call. We lose the edge case when someone
passes in an empty array of options, but we can address that at other time if
and when this class is reused elsewhere.

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:463: <message
name="IDS_CLEAR_BROWSING_DATA_PERIOD_HOUR" desc="The option to delete browsing
data from the past hour. This is the direct object of the sentence 'Clear data
from the past hour'.">
On 2016/02/11 23:46:11, newt (slow) wrote:
> Can we reuse the strings from C++? If these are already defined in
> generated_resources.grd, just add formatter_data="android_java" to the strings
> in generated_resources.grd and they'll be automagically available in Java,
just
> like strings defined in android_chrome_strings.grd.

Unfortunately, we currently can't. The C++ strings are prepended with "the",
i.e. "the past hour", "the past day", etc. When working on this design, we
actually had a long discussion how to present the option (capitalized or not,
with the article or not, ...) and ended up with something different then we
currently do on desktop. I believe that we *do* want to consolidate it across
platforms though, so I filed crbug.com/586486 to keep track of that.

[newt (away), 2016-02-12 17:58:04]

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/preference_spinner.xml (right):

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/preference_spinner.xml:21: <Spinner
On 2016/02/12 15:10:49, msramek wrote:
> On 2016/02/11 23:46:11, newt (slow) wrote:
> > You can just add mailto:style="@style/Widget.AppCompat.Spinner.Underlined"
to
> the
> > Spinner and you can remove the UnderlineView below
> 
> I kept the PreferenceSpinnerUnderlineView, as
> Widget.AppCompat.Spinner.Underlined retains its left padding (even after I set
> it to zero on the Spinner itself), and has a different color.
> PreferenceSpinnerUnderlineView matches the mocks out of the box.

Fair enough :)

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java
(right):

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:37:
mAdapter.setDropDownViewResource(android.R.layout.simple_spinner_dropdown_item);
On 2016/02/12 15:10:49, msramek wrote:
> On 2016/02/11 23:46:11, newt (slow) wrote:
> > Does this work on pre-L devices (i.e. on Holo devices)? You should probably
> use
> > the equivalent resources from the support library, assuming it exists. i.e.
> > "R.layout.simple_spinner_dropdown_item" without the "android." prefix. Same
> for
> > simple_spinner_item above.
> 
> I searched the code for other instances of spinners:
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=%22setDropDownViewRes...
> 
> It seems that we use android.R.layout.simple_spinner_dropdown_item and
> android.R.layout.simple_spinner_item almost everywhere, so I'm at least not
> decreasing the compatibility. I can try to flash Holo on Monday to see how it
> looks like.

I checked on a KitKat device and it looks correct, like the mocks. Also,
simple_spinner_item and simple_spinner_dropdown_item aren't even included in the
support library anyway.

https://codereview.chromium.org/1663983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:56:
mSpinner.setOnItemSelectedListener(new AdapterView.OnItemSelectedListener() {
On 2016/02/12 15:10:49, msramek wrote:
> On 2016/02/11 23:46:11, newt (slow) wrote:
> > Since onBindView() can be called many times, it'd be best to store and reuse
> the
> > OnItemSelectedListener rather than create a new once each time.
> > 
> > You could just construct it as a member variable:
> > 
> >   private OnItemSelectedListener mListener = new OnItemSelectedListener() {
> ...
> > };
> 
> Actually, let me split this into onCreateView() with the initialization of
> everything that remains constant, and onBindView() with things that could
change
> (that's mSpinner.setAdapter() and mSpinner.setSelection(), since those can
> change through setOptions()).

Even better :)

https://codereview.chromium.org/1663983003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java
(right):

https://codereview.chromium.org/1663983003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:49:
* @return Object The currently selected option.
nit: @return doesn't take a type such as "Object" since the signature already
specifies the type

You can just write "@return The currently selected option" or "Returns the
currently selected option"

https://codereview.chromium.org/1663983003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
(right):

https://codereview.chromium.org/1663983003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:140:
public enum TimePeriodSpinnerOption {
does this need to be public?

https://codereview.chromium.org/1663983003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:152:
int mTimePeriod;
private

https://codereview.chromium.org/1663983003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:174:
mStringRepresentation = activity.getString(mStringResource);
Changing the value of an enum's member variable is a bit questionable. This will
also cause those strings to be kept in memory for the lifetime of the process (a
minor issue).

On the other hand, using an enum here does make the code pretty simple.

Two options for making this a little better:
 1. Rename this method to init() and change the param to a Context.
 2. Change this to a class and require a Context in the constructor. Instead of
defining enum objects called LAST_HOUR, etc, just construct all the needed
TimePeriodSpinnerOptions inside of getTimePeriodSpinnerOptions().

https://codereview.chromium.org/1663983003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:234:
protected TimePeriodSpinnerOption[] getTimePeriodSpinnerOptions() {
make this private (unless you need to override it in ClearSyncDataPreferences,
but that doesn't seem to be the case)

[msramek, 2016-02-15 18:56:28]

https://codereview.chromium.org/1663983003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java
(right):

https://codereview.chromium.org/1663983003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SpinnerPreference.java:49:
* @return Object The currently selected option.
On 2016/02/12 17:58:04, newt wrote:
> nit: @return doesn't take a type such as "Object" since the signature already
> specifies the type
> 
> You can just write "@return The currently selected option" or "Returns the
> currently selected option"

Done.

https://codereview.chromium.org/1663983003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
(right):

https://codereview.chromium.org/1663983003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:140:
public enum TimePeriodSpinnerOption {
On 2016/02/12 17:58:04, newt wrote:
> does this need to be public?

Nope. Done.

https://codereview.chromium.org/1663983003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:152:
int mTimePeriod;
On 2016/02/12 17:58:04, newt wrote:
> private

Done.

https://codereview.chromium.org/1663983003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:174:
mStringRepresentation = activity.getString(mStringResource);
On 2016/02/12 17:58:04, newt wrote:
> Changing the value of an enum's member variable is a bit questionable. This
will
> also cause those strings to be kept in memory for the lifetime of the process
(a
> minor issue).
> 
> On the other hand, using an enum here does make the code pretty simple.
> 
> Two options for making this a little better:
>  1. Rename this method to init() and change the param to a Context.
>  2. Change this to a class and require a Context in the constructor. Instead
of
> defining enum objects called LAST_HOUR, etc, just construct all the needed
> TimePeriodSpinnerOptions inside of getTimePeriodSpinnerOptions().

Done. I did #2, as it's really cleaner if this isn't an enum. I guess I just
wanted to copy the style of DialogOption too much to realize this :)

https://codereview.chromium.org/1663983003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:234:
protected TimePeriodSpinnerOption[] getTimePeriodSpinnerOptions() {
On 2016/02/12 17:58:04, newt wrote:
> make this private (unless you need to override it in ClearSyncDataPreferences,
> but that doesn't seem to be the case)

Done.

https://codereview.chromium.org/1663983003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:336:
PrefServiceBridge.getInstance().getBrowsingDataDeletionTimePeriod());
I changed this, as I realized it's incorrect. Of course, options in the dialog
have indices 0..4 that exactly match the preference values 0..4, but they could
theoretically appear in a different order. I changed the code to actually find
an instance of TimePeriodSpinnerOption whose getTimePeriod() ==
getBrowsingDataDeletionTimePeriod().

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

lgtm after comment. Looks good!

https://codereview.chromium.org/1663983003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
(right):

https://codereview.chromium.org/1663983003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:140:
private class TimePeriodSpinnerOption {
make this static. As a rule, always make inner classes static unless there's a
good reason for it to be non-static. That helps prevent memory leaks and reduce
circular dependencies among other benefits.

[msramek, 2016-02-17 10:55:47]

msramek@chromium.org changed reviewers:
+ jochen@chromium.org

[msramek, 2016-02-17 10:55:47]

Bernhard, can you please look at browsing_data_remover.h?

And +Jochen, please have a look at chrome/chrome.gyp.

Thanks!
Martin

https://codereview.chromium.org/1663983003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
(right):

https://codereview.chromium.org/1663983003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:140:
private class TimePeriodSpinnerOption {
On 2016/02/16 19:35:07, newt wrote:
> make this static. As a rule, always make inner classes static unless there's a
> good reason for it to be non-static. That helps prevent memory leaks and
reduce
> circular dependencies among other benefits.

Acknowledged and done.

[Bernhard Bauer, 2016-02-18 16:26:10]

lgtm

[jochen (gone - plz use gerrit), 2016-02-19 14:41:11]

lgtm

[msramek, 2016-02-19 15:07:54]

Thanks, guys!

[msramek, 2016-02-19 15:07:59]

The CQ bit was checked by msramek@chromium.org

[msramek, 2016-02-19 15:07:59]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1663983003/#ps100001
(title: "static")

[commit-bot: I haz the power, 2016-02-19 15:08:36]

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

[commit-bot: I haz the power, 2016-02-19 16:12:05]

Description was changed from

==========
Add a time period dropdown to the CBD dialog on Android.

1. Define SpinnerPreference, a subclass of Preference that represents a dropdown
of any Object-s.
2. Add an instance of SpinnerPreference to the CBD dialog.
3. Bind the dropdown values to BrowsingDataRemover::TimePeriod through
PrefServiceBridge.

Older mocks for reference:
https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1#%2F0...

We don't match the mocks perfectly yet, as the style and strings are still being
discussed.

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

to

==========
Add a time period dropdown to the CBD dialog on Android.

1. Define SpinnerPreference, a subclass of Preference that represents a dropdown
of any Object-s.
2. Add an instance of SpinnerPreference to the CBD dialog.
3. Bind the dropdown values to BrowsingDataRemover::TimePeriod through
PrefServiceBridge.

Older mocks for reference:
https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1#%2F0...

We don't match the mocks perfectly yet, as the style and strings are still being
discussed.

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

[commit-bot: I haz the power, 2016-02-19 16:12:06]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-02-19 16:13:12]

Description was changed from

==========
Add a time period dropdown to the CBD dialog on Android.

1. Define SpinnerPreference, a subclass of Preference that represents a dropdown
of any Object-s.
2. Add an instance of SpinnerPreference to the CBD dialog.
3. Bind the dropdown values to BrowsingDataRemover::TimePeriod through
PrefServiceBridge.

Older mocks for reference:
https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1#%2F0...

We don't match the mocks perfectly yet, as the style and strings are still being
discussed.

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

to

==========
Add a time period dropdown to the CBD dialog on Android.

1. Define SpinnerPreference, a subclass of Preference that represents a dropdown
of any Object-s.
2. Add an instance of SpinnerPreference to the CBD dialog.
3. Bind the dropdown values to BrowsingDataRemover::TimePeriod through
PrefServiceBridge.

Older mocks for reference:
https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1#%2F0...

We don't match the mocks perfectly yet, as the style and strings are still being
discussed.

BUG=569870

Committed: https://crrev.com/0c66f43c2e7acfc0749e54008932ff81c7c2c38c
Cr-Commit-Position: refs/heads/master@{#376449}
==========

[commit-bot: I haz the power, 2016-02-19 16:13:13]

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