Implement additional parts of the new ui for CBD.

Implement additional parts of the new ui for CBD.

Change UI to use single-line time selector.
Move clear button to footer.
Fix RTL behavior of ViewPager.

All changes are behind the "tabs-in-cbd" flag

https://screenshot.googleplex.com/BwcNiqwedVf.png

BUG=681523
Review-Url: https://codereview.chromium.org/2646313005
Cr-Commit-Position: refs/heads/master@{#447733}
Committed: https://chromium.googlesource.com/chromium/src/+/9a5f9879b084901c23318218fdc99dac9cc37bad

[dullweber, 2017-01-24 15:39:24]

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

[commit-bot: I haz the power, 2017-01-24 15:39:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2017-01-24 15:41:12]

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

[dullweber, 2017-01-24 15:41:13]

Martin, please have a look at these changes

[commit-bot: I haz the power, 2017-01-24 15:43:31]

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

[commit-bot: I haz the power, 2017-01-24 15:43:32]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[msramek, 2017-01-25 15:24:24]

Hi! I took a first pass.

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/clear_browsing_data_tabs.xml (right):

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/clear_browsing_data_tabs.xml:19:
android:layoutDirection="ltr"
Maybe comment here that RTL is addressed in the Java code?

(Thanks for paying attention to this!)

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
(right):

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:730:
* Return the index of the tab last visited by the user in the CBD dialog.
Java-doc please.

The style dictates that public methods are java-documented. I know this is not
the case everywhere, but to my knowledge the Android team is trying to improve
that - let's help them.

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

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:241:
protected final EnumSet<DialogOption> getSelectedOptions() {
Please add a javadoc to methods where you increased visibility.

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:427:
protected int getPreferencesXml() {
nit: Maybe getLayoutResourceId() would be a better name?

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:486:
if (clearButton != null) {
This is a bit confusing. This class is associated with a XML file which clearly
does have a |clearButton|. If subclasses might not have it, we should address it
via inheritance. Subclasses should define their own onCreate() or helper methods
that will be injected here.

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java
(right):

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java:36:
protected int getPreferencesXml() {
@Override Here and below.

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java:40:
protected void updateButtonState() {
This method should do the same thing in all preferences - why do we need to
redefine it here?

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java
(right):

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:50:
private int fixIndex(int index) {
What does "fix" mean in this context? How about adjustIndexForDirectionality()?
This is Java, long descriptive names are fine :)

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:87:
private class ClearBrowsingDataPagerAdapter extends FragmentPagerAdapter {
This inner class could keep being static. There is nothing instance-dependent in
fixIndex() and CBD_TAB_COUNT.

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:127:
private class TabSelectListener implements TabLayout.OnTabSelectedListener {
Ditto, this inner class could be static.

https://codereview.chromium.org/2646313005/diff/1/chrome/browser/android/pref...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/2646313005/diff/1/chrome/browser/android/pref...
chrome/browser/android/preferences/pref_service_bridge.cc:602:
DCHECK_LE(tab_index, 1);
nit: I would prefer <2 rather than <=1. We cannot refer to the CBD_TABS_COUNT
constant directly, but let's at least use its value, which is 2.

[dullweber, 2017-01-25 17:22:06]

Thanks, I fixed most issues. We should discuss what would be the best way to
split the old and new behavior. I think the current approach will create a huge
amount of overrides and it will be very confusing what is actually happening.

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/clear_browsing_data_tabs.xml (right):

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/clear_browsing_data_tabs.xml:19:
android:layoutDirection="ltr"
On 2017/01/25 15:24:23, msramek wrote:
> Maybe comment here that RTL is addressed in the Java code?
> 
> (Thanks for paying attention to this!)

Done.

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
(right):

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:730:
* Return the index of the tab last visited by the user in the CBD dialog.
On 2017/01/25 15:24:23, msramek wrote:
> Java-doc please.
> 
> The style dictates that public methods are java-documented. I know this is not
> the case everywhere, but to my knowledge the Android team is trying to improve
> that - let's help them.

Done.

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

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:241:
protected final EnumSet<DialogOption> getSelectedOptions() {
On 2017/01/25 15:24:23, msramek wrote:
> Please add a javadoc to methods where you increased visibility.

Done.

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:427:
protected int getPreferencesXml() {
On 2017/01/25 15:24:23, msramek wrote:
> nit: Maybe getLayoutResourceId() would be a better name?

I changed it to getPreferenceXmlId because this is in R.xml.* and not R.layout.*

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:486:
if (clearButton != null) {
On 2017/01/25 15:24:23, msramek wrote:
> This is a bit confusing. This class is associated with a XML file which
clearly
> does have a |clearButton|. If subclasses might not have it, we should address
it
> via inheritance. Subclasses should define their own onCreate() or helper
methods
> that will be injected here.

I will look into a good solution for this and the summary texts tomorrow. I also
need to find out how I can integrate a clickable link into the checkbox
preference for history. I guess the code for the texts needs to change as well.

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java
(right):

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java:36:
protected int getPreferencesXml() {
On 2017/01/25 15:24:23, msramek wrote:
> @Override Here and below.

Done.

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java:40:
protected void updateButtonState() {
On 2017/01/25 15:24:23, msramek wrote:
> This method should do the same thing in all preferences - why do we need to
> redefine it here?

The current dialog is using a ButtonPreference that is part of the ListView
defined in the R.xml.* file. The new dialog uses a button in the footer. The
only way I found to implement this was to create a custom layout for the
preference site with an actual Button that works differently from the
ButtonPreference. So I need the  setOnClickListener and the updateButtonState()
method in this class to make the button work. I'm not sure if it would be
cleaner if I make a copy of ClearBrowsingDataPreference instead of lots
overrides. What do you think?

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java
(right):

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:50:
private int fixIndex(int index) {
On 2017/01/25 15:24:23, msramek wrote:
> What does "fix" mean in this context? How about
adjustIndexForDirectionality()?
> This is Java, long descriptive names are fine :)

Right, somehow I have to make use of the additional 20 characters that Java
allows me in each line :D

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:87:
private class ClearBrowsingDataPagerAdapter extends FragmentPagerAdapter {
On 2017/01/25 15:24:23, msramek wrote:
> This inner class could keep being static. There is nothing instance-dependent
in
> fixIndex() and CBD_TAB_COUNT.

fixIndex and inner classes are static now

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:127:
private class TabSelectListener implements TabLayout.OnTabSelectedListener {
On 2017/01/25 15:24:23, msramek wrote:
> Ditto, this inner class could be static.

Done.

https://codereview.chromium.org/2646313005/diff/1/chrome/browser/android/pref...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/2646313005/diff/1/chrome/browser/android/pref...
chrome/browser/android/preferences/pref_service_bridge.cc:602:
DCHECK_LE(tab_index, 1);
On 2017/01/25 15:24:23, msramek wrote:
> nit: I would prefer <2 rather than <=1. We cannot refer to the CBD_TABS_COUNT
> constant directly, but let's at least use its value, which is 2.

Done.

[msramek, 2017-01-26 11:04:07]

Thanks for the fixes. As discussed, can you please look into restructuring of
the onCreate() method in a more OOP way?

After that, the dialog parts LGTM. Note that I didn't review your changes to the
generic components like Spinner and Divider.

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

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:486:
if (clearButton != null) {
On 2017/01/25 17:22:06, dullweber wrote:
> On 2017/01/25 15:24:23, msramek wrote:
> > This is a bit confusing. This class is associated with a XML file which
> clearly
> > does have a |clearButton|. If subclasses might not have it, we should
address
> it
> > via inheritance. Subclasses should define their own onCreate() or helper
> methods
> > that will be injected here.
> 
> I will look into a good solution for this and the summary texts tomorrow. I
also
> need to find out how I can integrate a clickable link into the checkbox
> preference for history. I guess the code for the texts needs to change as
well.

As we just discussed offline, we could just extract "initializeClearButton",
"initializeGoogleFooter" etc. I still it is easier to understand if a subclass
@Overrides these methods and empty and comments that "I don't have this footer"
rather than one onCreate() method handling three different screens.

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java
(right):

https://codereview.chromium.org/2646313005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java:40:
protected void updateButtonState() {
On 2017/01/25 17:22:06, dullweber wrote:
> On 2017/01/25 15:24:23, msramek wrote:
> > This method should do the same thing in all preferences - why do we need to
> > redefine it here?
> 
> The current dialog is using a ButtonPreference that is part of the ListView
> defined in the R.xml.* file. The new dialog uses a button in the footer. The
> only way I found to implement this was to create a custom layout for the
> preference site with an actual Button that works differently from the
> ButtonPreference. So I need the  setOnClickListener and the
updateButtonState()
> method in this class to make the button work. I'm not sure if it would be
> cleaner if I make a copy of ClearBrowsingDataPreference instead of lots
> overrides. What do you think?

As discussed offline - let's not overcomplicate this and keep your current
implementation where the button is once a normal Button and once a Preference.
But please add a comment explaining what led us to this discrepancy.

[dullweber, 2017-01-26 13:28:52]

Description was changed from

==========
Implement parts of new ui for CBD.

Change UI to use single-line time selector.
Move button to footer.
Fix RTL behavior of ViewPager.

All changes are behind the "tabs-in-cbd" flag

https://screenshot.googleplex.com/BwcNiqwedVf.png

BUG=681523
==========

to

==========
Implement additional parts of the new ui for CBD.

Change UI to use single-line time selector.
Move clear button to footer.
Fix RTL behavior of ViewPager.

All changes are behind the "tabs-in-cbd" flag

https://screenshot.googleplex.com/BwcNiqwedVf.png

BUG=681523
==========

[dullweber, 2017-01-26 13:29:25]

dullweber@chromium.org changed reviewers:
+ twellington@chromium.org

[dullweber, 2017-01-26 13:29:26]

twellington@chromium.org: Please review these changes

[Theresa, 2017-01-27 03:30:11]

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

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/clear_browsing_data_tab_content.xml:14:
<FrameLayout
If you move android:layout_weight="1" ad android:layout_height="0dp" to the
child ListView and remove this FrameLayout, does the layout look the same?

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/clear_browsing_data_tab_content.xml:24:
<LinearLayout
This should probably be a Button with style="?android:attr/buttonBarButtonStyle"
to match our PasswordEntryEditor which has similar (hopefully exactly the same)
looking buttons.

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

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/preference_spinner_one_line.xml:1: <?xml
version="1.0" encoding="utf-8"?>
nit: make this preference_spinner_single_line.xml to match the attribute name

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/preference_spinner_one_line.xml:2: <!-- Copyright
2016 The Chromium Authors. All rights reserved.
s/2016/2017

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/res...
File chrome/android/java/res/values-v17/styles.xml (right):

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/res...
chrome/android/java/res/values-v17/styles.xml:170: <item
name="android:background">@color/google_grey_300</item>
Thanks for changing the hex to google_grey_300!

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/res...
File chrome/android/java/res/xml/clear_browsing_data_preferences_tab.xml
(right):

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/res...
chrome/android/java/res/xml/clear_browsing_data_preferences_tab.xml:2: <!--
Copyright 2016 The Chromium Authors. All rights reserved.
s/2016/2017

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

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:506:
protected void initButtonPreference() {
initClearButtonPreference() is a more descriptive name for this function

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java
(right):

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java:22:
// Replace default preference view with a view with a footer.
There are some extra words in this sentence.

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java
(right):

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:130:
PrefServiceBridge.getInstance().setLastSelectedClearBrowsingDataTab(tabIndex);
To check my understanding here:

LTR: Tab 0 = Basic   Tab 1 = Advanced
RTL: Tab 0 = Advanced   Tab 1 = Basic

Here's a scenario:
In LTR mode I select the "basic" tab (0); that gets stored. I close Chrome,
change my layout to RTL and reload. The selected index is adjusted: 2 - 1 - 0 =
1, so the "basic" tab is still selected. I toggle selection between "advanced"
and "basic"; an adjusted index gets stored: 2 - 1 - 1 = 0 is stored.

Is that correct?

[Theresa, 2017-01-27 03:31:45]

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java
(right):

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java:22:
// Replace default preference view with a view with a footer.
On 2017/01/27 03:30:11, Theresa wrote:
> There are some extra words in this sentence.

Or maybe there aren't? This may be more clear if it said "Replace default
preferences view with custom XML that contains a footer" or something like that.

[dullweber, 2017-01-30 10:40:26]

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

[commit-bot: I haz the power, 2017-01-30 10:40:33]

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, 2017-01-30 10:42:26]

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

[commit-bot: I haz the power, 2017-01-30 10:42:26]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dullweber, 2017-01-30 10:55:05]

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

[commit-bot: I haz the power, 2017-01-30 10:55:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2017-01-30 11:36:44]

thanks, I fixed the issues you mentioned

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

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/clear_browsing_data_tab_content.xml:14:
<FrameLayout
On 2017/01/27 03:30:11, Theresa wrote:
> If you move android:layout_weight="1" ad android:layout_height="0dp" to the
> child ListView and remove this FrameLayout, does the layout look the same?

yes

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/clear_browsing_data_tab_content.xml:24:
<LinearLayout
On 2017/01/27 03:30:11, Theresa wrote:
> This should probably be a Button with
style="?android:attr/buttonBarButtonStyle"
> to match our PasswordEntryEditor which has similar (hopefully exactly the
same)
> looking buttons.

Thanks, I changed the xml to be similar to the PasswordEntryEditor. It still
looks the same

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

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/preference_spinner_one_line.xml:1: <?xml
version="1.0" encoding="utf-8"?>
On 2017/01/27 03:30:11, Theresa wrote:
> nit: make this preference_spinner_single_line.xml to match the attribute name

Done.

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/preference_spinner_one_line.xml:2: <!-- Copyright
2016 The Chromium Authors. All rights reserved.
On 2017/01/27 03:30:11, Theresa wrote:
> s/2016/2017

Done.

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/res...
File chrome/android/java/res/xml/clear_browsing_data_preferences_tab.xml
(right):

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/res...
chrome/android/java/res/xml/clear_browsing_data_preferences_tab.xml:2: <!--
Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/27 03:30:11, Theresa wrote:
> s/2016/2017

Done.

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

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:506:
protected void initButtonPreference() {
On 2017/01/27 03:30:11, Theresa wrote:
> initClearButtonPreference() is a more descriptive name for this function

Done.

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java
(right):

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java:22:
// Replace default preference view with a view with a footer.
On 2017/01/27 03:31:45, Theresa wrote:
> On 2017/01/27 03:30:11, Theresa wrote:
> > There are some extra words in this sentence.
> 
> Or maybe there aren't? This may be more clear if it said "Replace default
> preferences view with custom XML that contains a footer" or something like
that.

thanks, that sounds better

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java
(right):

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:130:
PrefServiceBridge.getInstance().setLastSelectedClearBrowsingDataTab(tabIndex);
On 2017/01/27 03:30:11, Theresa wrote:
> To check my understanding here:
> 
> LTR: Tab 0 = Basic   Tab 1 = Advanced
> RTL: Tab 0 = Advanced   Tab 1 = Basic
> 
> Here's a scenario:
> In LTR mode I select the "basic" tab (0); that gets stored. I close Chrome,
> change my layout to RTL and reload. The selected index is adjusted: 2 - 1 - 0
=
> 1, so the "basic" tab is still selected. I toggle selection between "advanced"
> and "basic"; an adjusted index gets stored: 2 - 1 - 1 = 0 is stored.
> 
> Is that correct?

That's right. adjustIndexForDirectionality will make sure that we always store 0
for the basic tab and 1 for advanced. This was the most reliable solution I
found. 

I also tried several wrapper classes for the ViewPager that try to fix this but
they all had either problems to work together with the TabLayout, didn't show
the right tab when tab.select() was called or had broken scrolling. My attempts
at fixing this were not very successful, probably because I didn't find all
places that required a translation for RTL.
I tried this one
(https://android.googlesource.com/platform/packages/apps/DeskClock/+/master/sr...)
and a few others I found here: go/dkrjr

This project seems to have a proper fix but I don't know how to include it
properly without licensing issues:
https://github.com/diego-gomez-olvera/RtlViewPager

In general I think it is sad that the ViewPager doesn't work correctly although
a ticket was filed in 2013
(https://code.google.com/p/android/issues/detail?id=56831). Do you know if there
is anyone that could be asked about this?

[commit-bot: I haz the power, 2017-01-30 12:33:41]

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

[commit-bot: I haz the power, 2017-01-30 12:33:42]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-01-30 16:21:06]

This looks good overall, just a couple of small comments.

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java
(right):

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:130:
PrefServiceBridge.getInstance().setLastSelectedClearBrowsingDataTab(tabIndex);
On 2017/01/30 11:36:44, dullweber wrote:
> On 2017/01/27 03:30:11, Theresa wrote:
> > To check my understanding here:
> > 
> > LTR: Tab 0 = Basic   Tab 1 = Advanced
> > RTL: Tab 0 = Advanced   Tab 1 = Basic
> > 
> > Here's a scenario:
> > In LTR mode I select the "basic" tab (0); that gets stored. I close Chrome,
> > change my layout to RTL and reload. The selected index is adjusted: 2 - 1 -
0
> =
> > 1, so the "basic" tab is still selected. I toggle selection between
"advanced"
> > and "basic"; an adjusted index gets stored: 2 - 1 - 1 = 0 is stored.
> > 
> > Is that correct?
> 
> That's right. adjustIndexForDirectionality will make sure that we always store
0
> for the basic tab and 1 for advanced. This was the most reliable solution I
> found. 
> 
> I also tried several wrapper classes for the ViewPager that try to fix this
but
> they all had either problems to work together with the TabLayout, didn't show
> the right tab when tab.select() was called or had broken scrolling. My
attempts
> at fixing this were not very successful, probably because I didn't find all
> places that required a translation for RTL.
> I tried this one
>
(https://android.googlesource.com/platform/packages/apps/DeskClock/+/master/sr...)
> and a few others I found here: go/dkrjr
> 
> This project seems to have a proper fix but I don't know how to include it
> properly without licensing issues:
> https://github.com/diego-gomez-olvera/RtlViewPager
> 
> In general I think it is sad that the ViewPager doesn't work correctly
although
> a ticket was filed in 2013
> (https://code.google.com/p/android/issues/detail?id=56831). Do you know if
there
> is anyone that could be asked about this?

There are a couple of internal bugs that reference that external bug, as well as
others for ViewPager in RTL. It looks like the Android folks are aware but it
hasn't been a high priority. We could ping one of the internal bugs.


I think ideally, we would have a ChromeViewPagerCompat (or something like that)
in our ui/android/java/src/org/chromium/ui/widget/  that encapsulates this work
around. It doesn't look like we currently use have any other ViewPagers that
support swipe gesture in our code, so this could be a TODO for later.

https://codereview.chromium.org/2646313005/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/clear_browsing_data_tab_content.xml (right):

https://codereview.chromium.org/2646313005/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/clear_browsing_data_tab_content.xml:26:
android:orientation="horizontal">
This LinearLayout probably isn't necessary if the layout_height for the button
is "wrap_content"

[dullweber, 2017-02-01 11:47:57]

Thanks, I removed the LinearLayout

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java
(right):

https://codereview.chromium.org/2646313005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:130:
PrefServiceBridge.getInstance().setLastSelectedClearBrowsingDataTab(tabIndex);
On 2017/01/30 16:21:05, Theresa wrote:
> On 2017/01/30 11:36:44, dullweber wrote:
> > On 2017/01/27 03:30:11, Theresa wrote:
> > > To check my understanding here:
> > > 
> > > LTR: Tab 0 = Basic   Tab 1 = Advanced
> > > RTL: Tab 0 = Advanced   Tab 1 = Basic
> > > 
> > > Here's a scenario:
> > > In LTR mode I select the "basic" tab (0); that gets stored. I close
Chrome,
> > > change my layout to RTL and reload. The selected index is adjusted: 2 - 1
-
> 0
> > =
> > > 1, so the "basic" tab is still selected. I toggle selection between
> "advanced"
> > > and "basic"; an adjusted index gets stored: 2 - 1 - 1 = 0 is stored.
> > > 
> > > Is that correct?
> > 
> > That's right. adjustIndexForDirectionality will make sure that we always
store
> 0
> > for the basic tab and 1 for advanced. This was the most reliable solution I
> > found. 
> > 
> > I also tried several wrapper classes for the ViewPager that try to fix this
> but
> > they all had either problems to work together with the TabLayout, didn't
show
> > the right tab when tab.select() was called or had broken scrolling. My
> attempts
> > at fixing this were not very successful, probably because I didn't find all
> > places that required a translation for RTL.
> > I tried this one
> >
>
(https://android.googlesource.com/platform/packages/apps/DeskClock/+/master/sr...)
> > and a few others I found here: go/dkrjr
> > 
> > This project seems to have a proper fix but I don't know how to include it
> > properly without licensing issues:
> > https://github.com/diego-gomez-olvera/RtlViewPager
> > 
> > In general I think it is sad that the ViewPager doesn't work correctly
> although
> > a ticket was filed in 2013
> > (https://code.google.com/p/android/issues/detail?id=56831). Do you know if
> there
> > is anyone that could be asked about this?
> 
> There are a couple of internal bugs that reference that external bug, as well
as
> others for ViewPager in RTL. It looks like the Android folks are aware but it
> hasn't been a high priority. We could ping one of the internal bugs.
> 
> 
> I think ideally, we would have a ChromeViewPagerCompat (or something like
that)
> in our ui/android/java/src/org/chromium/ui/widget/  that encapsulates this
work
> around. It doesn't look like we currently use have any other ViewPagers that
> support swipe gesture in our code, so this could be a TODO for later.

Ok, I added a TODO

https://codereview.chromium.org/2646313005/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/clear_browsing_data_tab_content.xml (right):

https://codereview.chromium.org/2646313005/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/clear_browsing_data_tab_content.xml:26:
android:orientation="horizontal">
On 2017/01/30 16:21:06, Theresa wrote:
> This LinearLayout probably isn't necessary if the layout_height for the button
> is "wrap_content"

Yes, that works.

[Theresa, 2017-02-01 16:03:08]

lgtm

[dullweber, 2017-02-01 18:55:36]

dullweber@chromium.org changed reviewers:
+ tedchoc@chromium.org

[dullweber, 2017-02-01 18:55:37]

tedchoc@chromium.org: Please review changes in
chrome/browser/android/preferences/pref_service_bridge.cc

[Ted C, 2017-02-01 21:16:33]

On 2017/02/01 18:55:37, dullweber wrote:
> mailto:tedchoc@chromium.org: Please review changes in
> chrome/browser/android/preferences/pref_service_bridge.cc

pref_service_bridge.cc - lgtm

[dullweber, 2017-02-02 08:50:16]

The CQ bit was checked by dullweber@chromium.org

[dullweber, 2017-02-02 08:50:16]

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

[commit-bot: I haz the power, 2017-02-02 08:50:30]

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, 2017-02-02 09:58:09]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486025416269120,
"parent_rev": "0e38f6a240a50e71d3a280deacf36dfbb8c65ee1", "commit_rev":
"9a5f9879b084901c23318218fdc99dac9cc37bad"}

[commit-bot: I haz the power, 2017-02-02 09:58:43]

Description was changed from

==========
Implement additional parts of the new ui for CBD.

Change UI to use single-line time selector.
Move clear button to footer.
Fix RTL behavior of ViewPager.

All changes are behind the "tabs-in-cbd" flag

https://screenshot.googleplex.com/BwcNiqwedVf.png

BUG=681523
==========

to

==========
Implement additional parts of the new ui for CBD.

Change UI to use single-line time selector.
Move clear button to footer.
Fix RTL behavior of ViewPager.

All changes are behind the "tabs-in-cbd" flag

https://screenshot.googleplex.com/BwcNiqwedVf.png

BUG=681523

Review-Url: https://codereview.chromium.org/2646313005
Cr-Commit-Position: refs/heads/master@{#447733}
Committed:
https://chromium.googlesource.com/chromium/src/+/9a5f9879b084901c23318218fdc9...
==========

[commit-bot: I haz the power, 2017-02-02 09:58:45]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9a5f9879b084901c23318218fdc9...