Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2730703003:
Change CBD layout and texts (Closed)
|
Created:
3 years, 9 months ago by dullweber Modified:
3 years, 8 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, msramek+watch_chromium.org, michaelpg+watch-options_chromium.org, michaelpg+watch-md-settings_chromium.org, agrieve+watch_chromium.org, markusheintz_, srahim+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange layout and texts for new CBD dialog.
- Improved spinner layout
- New spinner texts (1 day -> 24 hours, 1 week -> 7 days)
- Implement a way to show different counter texts for basic and advanced mode
- New counter texts for basic mode
- Clickable myactivity.google.com link in checkbox summary
- Add instrumentation test to verify that the right string is shown depending on sign in and sync state.
Screenshots for default display size (Nexus 6P):
https://screenshot.googleplex.com/P1hFk9v8V3k.png
https://screenshot.googleplex.com/zVjhJJWnXUo.png
Screenshots on largest display size (Nexus 6P):
https://screenshot.googleplex.com/m9nBDbQ5ra5.png
https://screenshot.googleplex.com/Gb6vA38rfOH.png
BUG=681523
Review-Url: https://codereview.chromium.org/2730703003
Cr-Commit-Position: refs/heads/master@{#462406}
Committed: https://chromium.googlesource.com/chromium/src/+/53bf4e3a7fca5f9faa16e56ba7b8a3c280324057
Patch Set 1 #Patch Set 2 : add clickable link, improve layout #Patch Set 3 : only show history text when synced #Patch Set 4 : add browsertest for checkbox summaries #
Total comments: 1
Patch Set 5 : try fix tests #
Total comments: 5
Patch Set 6 : remove CBDTab enum usage from counter utils #Patch Set 7 : remove hamcrest matcher #Patch Set 8 : readd hamcrest conditions #
Total comments: 9
Patch Set 9 : fixes #Patch Set 10 : cleanup test #
Total comments: 28
Patch Set 11 : fix comments #Patch Set 12 : rebase #Patch Set 13 : remove checkbox gravity #Patch Set 14 : rebase #
Total comments: 10
Patch Set 15 : Remove padding #
Total comments: 2
Patch Set 16 : add teardown method to test #Patch Set 17 : rebase #Dependent Patchsets: Messages
Total messages: 83 (63 generated)
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== Change CBD layout and texts Improve spinner layout Add new spinner text Show different counter text for basic and advanced mode Add new counter texts BUG=681523 ========== to ========== Change layout and texts for new CBD dialog. - Improved spinner layout - New spinner texts - Shows different counter text for basic and advanced mode - New counter texts - Improved CheckboxPreference Layout to match mockups - Clickable myactivity.google.com link in checkbox summary Screenshots for default display size: https://screenshot.googleplex.com/0jjGhHys7ZP.png https://screenshot.googleplex.com/oQLWJWGtNW3.png Screenshots on largest display size: https://screenshot.googleplex.com/8dQoaW9NyWn.png https://screenshot.googleplex.com/wEsZomWRktr.png BUG=681523 ==========
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Change layout and texts for new CBD dialog. - Improved spinner layout - New spinner texts - Shows different counter text for basic and advanced mode - New counter texts - Improved CheckboxPreference Layout to match mockups - Clickable myactivity.google.com link in checkbox summary Screenshots for default display size: https://screenshot.googleplex.com/0jjGhHys7ZP.png https://screenshot.googleplex.com/oQLWJWGtNW3.png Screenshots on largest display size: https://screenshot.googleplex.com/8dQoaW9NyWn.png https://screenshot.googleplex.com/wEsZomWRktr.png BUG=681523 ========== to ========== Change layout and texts for new CBD dialog. - Improved spinner layout - New spinner texts - Shows different counter text for basic and advanced mode - New counter texts - Improved CheckboxPreference Layout to match mockups - Clickable myactivity.google.com link in checkbox summary - Instrumentation Test to verify that the right string is shown depending on sign in and sync state. Screenshots for default display size: https://screenshot.googleplex.com/0jjGhHys7ZP.png https://screenshot.googleplex.com/oQLWJWGtNW3.png Screenshots on largest display size: https://screenshot.googleplex.com/8dQoaW9NyWn.png https://screenshot.googleplex.com/wEsZomWRktr.png BUG=681523 ==========
Description was changed from ========== Change layout and texts for new CBD dialog. - Improved spinner layout - New spinner texts - Shows different counter text for basic and advanced mode - New counter texts - Improved CheckboxPreference Layout to match mockups - Clickable myactivity.google.com link in checkbox summary - Instrumentation Test to verify that the right string is shown depending on sign in and sync state. Screenshots for default display size: https://screenshot.googleplex.com/0jjGhHys7ZP.png https://screenshot.googleplex.com/oQLWJWGtNW3.png Screenshots on largest display size: https://screenshot.googleplex.com/8dQoaW9NyWn.png https://screenshot.googleplex.com/wEsZomWRktr.png BUG=681523 ========== to ========== Change layout and texts for new CBD dialog. - Improved spinner layout - New spinner texts (1 day -> 24 hours, 1 week -> 7 days) - Implement a way to show different counter texts for basic and advanced mode - New counter texts for basic mode - Modified CheckboxPreference Layout to match mockups - Clickable myactivity.google.com link in checkbox summary - Add instrumentation test to verify that the right string is shown depending on sign in and sync state. Screenshots for default display size (Nexus 6P): https://screenshot.googleplex.com/0jjGhHys7ZP.png https://screenshot.googleplex.com/oQLWJWGtNW3.png Screenshots on largest display size (Nexus 6P): https://screenshot.googleplex.com/8dQoaW9NyWn.png https://screenshot.googleplex.com/wEsZomWRktr.png BUG=681523 ==========
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dullweber@chromium.org changed reviewers: + msramek@chromium.org
Hi Martin, please take a look at this cl. https://codereview.chromium.org/2730703003/diff/60001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2730703003/diff/60001/chrome/android/BUILD.gn... chrome/android/BUILD.gn:455: "//third_party/junit", Hamcrest and junit are already indirect dependencies of //base:base_java_test_support but I couldn't use static imports of assertThat() or any of the hamcrest matchers like containsString() without these explicit dependencies. Is it ok to add them here? The chrome_junit_tests use these assertions too but they only have the hamcrest dependency.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Let me ask a question before I start reviewing this further, as the answer might reshape this CL. https://codereview.chromium.org/2730703003/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_counter_utils.h (right): https://codereview.chromium.org/2730703003/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_utils.h:18: // Constructs the text to be displayed by a counter from the given |result|. Please comment on the new parameter. https://codereview.chromium.org/2730703003/diff/80001/components/browsing_dat... File components/browsing_data/core/browsing_data_utils.cc (right): https://codereview.chromium.org/2730703003/diff/80001/components/browsing_dat... components/browsing_data/core/browsing_data_utils.cc:91: pref_name == browsing_data::prefs::kDeleteBrowsingHistoryBasic) { High-level question: Why do we need |cbd_tab| when the preference already clearly distinguishes between them? If I called this function with browsing_data::prefs::kDeleteBrowsingHistoryBasic and ClearBrowsingDataTab::ADVANCED, I'll get a valid result. And arguably, I shouldn't. https://codereview.chromium.org/2730703003/diff/80001/components/browsing_dat... components/browsing_data/core/browsing_data_utils.cc:111: case browsing_data::ClearBrowsingDataTab::NUM_TYPES: nit: No need for the namespace. https://codereview.chromium.org/2730703003/diff/80001/components/browsing_dat... File components/browsing_data/core/browsing_data_utils.h (right): https://codereview.chromium.org/2730703003/diff/80001/components/browsing_dat... components/browsing_data/core/browsing_data_utils.h:58: ClearBrowsingDataTab cbd_tab); Ditto.
I removed the CBDTab enum parameter from BrowsingDataUtils and BrowsingDataCounterUtils https://codereview.chromium.org/2730703003/diff/80001/components/browsing_dat... File components/browsing_data/core/browsing_data_utils.cc (right): https://codereview.chromium.org/2730703003/diff/80001/components/browsing_dat... components/browsing_data/core/browsing_data_utils.cc:91: pref_name == browsing_data::prefs::kDeleteBrowsingHistoryBasic) { On 2017/03/15 10:23:35, msramek wrote: > High-level question: Why do we need |cbd_tab| when the preference already > clearly distinguishes between them? > > If I called this function with browsing_data::prefs::kDeleteBrowsingHistoryBasic > and ClearBrowsingDataTab::ADVANCED, I'll get a valid result. And arguably, I > shouldn't. We actually don't need it here. Thanks!
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good, just a few more comments :) https://codereview.chromium.org/2730703003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/2730703003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:124: if (ClearBrowsingDataTabsFragment.isFeatureEnabled()) { Please add a comment explaining why we branch here. https://codereview.chromium.org/2730703003/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java (right): https://codereview.chromium.org/2730703003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java:121: * Tests that for users who are signed in but don't have sync activated, only the message about This doesn't seem right. The message about other forms of history should only be shown if PrefServiceBridge#OtherFormsOfBrowsingHistoryListener#showNoticeAboutOtherFormsOfBrowsingHistory(). In practice, that means signed in, syncing history without a custom passphrase, and have "Include Chrome history..." in the account's activity controls. In other words, it holds that OTHER_ACTIVITY => SIGNED_IN_DEVICES => GOOGLE_ACCOUNT. https://codereview.chromium.org/2730703003/diff/140001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_counter_factory.cc (left): https://codereview.chromium.org/2730703003/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_factory.cc:51: pref_name == browsing_data::prefs::kDeleteBrowsingHistoryBasic) { If there's no counter on the basic tab, we should return nullptr. https://codereview.chromium.org/2730703003/diff/140001/components/browsing_da... File components/browsing_data_strings.grdp (right): https://codereview.chromium.org/2730703003/diff/140001/components/browsing_da... components/browsing_data_strings.grdp:25: <message name="IDS_DEL_CACHE_COUNTER_BASIC" desc="A counter showing the size of the users's cache including either 'x MB' or 'less than x MB' as $1."> nit: ... and explaining that the cache is used to speed up the loading of websites.
https://codereview.chromium.org/2730703003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/2730703003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:124: if (ClearBrowsingDataTabsFragment.isFeatureEnabled()) { On 2017/03/22 14:40:21, msramek wrote: > Please add a comment explaining why we branch here. Done. https://codereview.chromium.org/2730703003/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java (right): https://codereview.chromium.org/2730703003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java:121: * Tests that for users who are signed in but don't have sync activated, only the message about On 2017/03/22 14:40:21, msramek wrote: > This doesn't seem right. The message about other forms of history should only be > shown if > PrefServiceBridge#OtherFormsOfBrowsingHistoryListener#showNoticeAboutOtherFormsOfBrowsingHistory(). > In practice, that means signed in, syncing history without a custom passphrase, > and have "Include Chrome history..." in the account's activity controls. > > In other words, it holds that OTHER_ACTIVITY => SIGNED_IN_DEVICES => > GOOGLE_ACCOUNT. As just discussed: I will show the "Other Activity" string always when a user is signed in because there will be other kinds of non-chrome activity. It also simplifies the logic because all strings can be set from the same place and the choice only depends on the signed-in and sync state, not on a passphrase or "include chrome history". We should think about this again if the current string gets changed at some point. https://codereview.chromium.org/2730703003/diff/140001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_counter_factory.cc (left): https://codereview.chromium.org/2730703003/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_factory.cc:51: pref_name == browsing_data::prefs::kDeleteBrowsingHistoryBasic) { On 2017/03/22 14:40:21, msramek wrote: > If there's no counter on the basic tab, we should return nullptr. There is already the "return nullptr" at the end of the method but I added explicit cases for history and cookies https://codereview.chromium.org/2730703003/diff/140001/components/browsing_da... File components/browsing_data_strings.grdp (right): https://codereview.chromium.org/2730703003/diff/140001/components/browsing_da... components/browsing_data_strings.grdp:25: <message name="IDS_DEL_CACHE_COUNTER_BASIC" desc="A counter showing the size of the users's cache including either 'x MB' or 'less than x MB' as $1."> On 2017/03/22 14:40:21, msramek wrote: > nit: ... and explaining that the cache is used to speed up the loading of > websites. Done.
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dullweber@chromium.org changed reviewers: + twellington@chromium.org
twellington@chromium.org: Please review these changes. After this cl the new UI for CBD should be nearly done :)
https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/divider_preference_with_bottom_padding.xml (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/divider_preference_with_bottom_padding.xml:11: <View style="@style/Divider"/> Does this need to be wrapped in a LinearLayout? You should be able to add padding directly to the View tag. https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/preference_spinner_custom.xml (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/preference_spinner_custom.xml:6: <!-- Layout used by SpinnerPreference if customLayout is set to true. --> nit "... is set to true and the device's smallest width is less than 360dp." https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/re... File chrome/android/java/res/values/attrs.xml (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/re... chrome/android/java/res/values/attrs.xml:74: <attr name="customLayout" format="boolean" /> The name "customLayout" implies that I can set a layout using the customLayout attribute in XML. If we're going with customLayout, let's make this a "reference" instead of a boolean so that in XML we can do this: chrome:customLayout="@layout/clear_browsing_data_preference_spinner" Alternatively, singleLine feels more re-usable to be than a "custom" layout. I think it's okay if the singleLine layout adapts on small screens when there isn't enough space as long as it's well documented. https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:55: // Create custom onTouch listener to be able to respond to click events inside the summary. Is the desired UX that when you click anywhere besides the ClickableSpan the preference's checkbox is toggled, and when you click on the ClickableSpan the link is taken? I think that's probably not going to work from an accessibility standpoint (see next comment), because accessibility mode does not give focus to the ClickableSpan separate from the rest of the summary string. Who are you working with from UX on this? I'm happy to get looped into a conversation if needed. https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:58: @SuppressLint("ClickableViewAccessibility") We need to test this with TalkBack on to make sure that the links in the summary text are clickable when accessibility mode is enabled. Typically, the expected behavior for text views that contain clickable spans is that the text is read out when tapped and the clickable span is activated/clicked on double tap. This current CBD implementation probably doesn't work with Talkback because ClickableSpan isn't accessible by default. We have a Chromium subclass, TextViewWithClickableSpans, that you'll probably need to use: https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/widg... You may need to create a custom preference layout that's used for the preferences with clickable spans in their summary strings. We had to do this for the existing footers on the CBD preference screen -- see TextMessageWithLinkAndIconPreference: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:92: (int) (6 * density), ApiCompatibilityUtils.getPaddingEnd(view), You should define a dimension with a value of "6" in dimens.xml and use Resources#getDimensionPixelSize() rather than multiplying 6 by the density here. https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:100: ApiCompatibilityUtils.getPaddingEnd(textLayout), 0); If possible, it is highly preferable to avoid this custom padding because it adds additional engineering overhead and maintenance, particularly when we go through another style redesign (happens every few years). We have a broad team effort to make our UI more consistent to reduce the burden on UX and eng and make the product feel more consistent overall. We use checkbox preferences with longer summary strings in our "Privacy" settings screen. Is there a reason that the padding here needs to be different than it is there? What does it look like if we don't adjust the padding and gravity? https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasic.java (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasic.java:30: public static final String COOKIES_CHECKBOX = "clear_cookies_checkbox"; Can the superclass versions of these be made public and visible for testing rather than duplicating the strings? https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasic.java:52: if (ChromeSigninController.get(getActivity()).isSignedIn()) { The sign in state can change, possibly in a separate window while in Android N+ multi-window. You should consider adding a SignInStateObserver to monitor when state changes and update the summary accordingly. https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java:57: activity.getString(R.string.clear_browsing_data_tab_period_everything))}; Can you please ask UX if we can just replace these strings in the existing UI rather than creating a fork in the code here? https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:592: Last 4 weeks If I'm reading the spec correctly, this should be "Last 30 days" https://codereview.chromium.org/2730703003/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java:29: @RetryOnFailure Are these tests flaky? Our general policy is that we don't add @RetryOnFailure to new tests, only old tests where it's not obvious when they regressed and started flaking or how to fix them. There needs to be a good justification for landing new tests with this annotation.
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review, I fixed or responded to the comments. Sadly I forgot a rebase and now the diff to the last commit contains a few other changes. https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/divider_preference_with_bottom_padding.xml (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/divider_preference_with_bottom_padding.xml:11: <View style="@style/Divider"/> On 2017/03/24 17:15:36, Theresa wrote: > Does this need to be wrapped in a LinearLayout? You should be able to add > padding directly to the View tag. I tried paddingBottom directly on the View but it isn't applied and the Browsing history checkbox gets too close to the divider. https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/preference_spinner_custom.xml (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/preference_spinner_custom.xml:6: <!-- Layout used by SpinnerPreference if customLayout is set to true. --> On 2017/03/24 17:15:36, Theresa wrote: > nit "... is set to true and the device's smallest width is less than 360dp." Done. https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/re... File chrome/android/java/res/values/attrs.xml (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/re... chrome/android/java/res/values/attrs.xml:74: <attr name="customLayout" format="boolean" /> On 2017/03/24 17:15:36, Theresa wrote: > The name "customLayout" implies that I can set a layout using the customLayout > attribute in XML. If we're going with customLayout, let's make this a > "reference" instead of a boolean so that in XML we can do this: > > chrome:customLayout="@layout/clear_browsing_data_preference_spinner" > > > Alternatively, singleLine feels more re-usable to be than a "custom" layout. I > think it's okay if the singleLine layout adapts on small screens when there > isn't enough space as long as it's well documented. OK, I will change it back to "singleLine". I was unsure if that is still a good name if the control uses two lines on a small screen. https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:55: // Create custom onTouch listener to be able to respond to click events inside the summary. On 2017/03/24 17:15:37, Theresa wrote: > Is the desired UX that when you click anywhere besides the ClickableSpan the > preference's checkbox is toggled, and when you click on the ClickableSpan the > link is taken? > > I think that's probably not going to work from an accessibility standpoint (see > next comment), because accessibility mode does not give focus to the > ClickableSpan separate from the rest of the summary string. Who are you working > with from UX on this? I'm happy to get looped into a conversation if needed. My goal was that clicking the text should check or uncheck the checkbox. Only direct clicks on the myactivity.google.com link should trigger the LinkClickDelegate. maxwalker@ is doing the UX for this. Discussion about the UI is on these slides: https://docs.google.com/a/google.com/presentation/d/1NI9SR2DpFKAsAmvB0-Bn1Qyd.... I tried to use the UI with talkback and I think it would be best if myactivity.google.com would not be clickable because it is only a small target and most users would want to check or uncheck the checkbox. Users who want to go to their activity can go to myactivity.google.com by manually entering the address. Is that an acceptable behavior? Otherwise I would try to use the TextViewWithClickableSpans with a custom layout for the CheckBoxPreference. https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:92: (int) (6 * density), ApiCompatibilityUtils.getPaddingEnd(view), On 2017/03/24 17:15:36, Theresa wrote: > You should define a dimension with a value of "6" in dimens.xml and use > Resources#getDimensionPixelSize() rather than multiplying 6 by the density here. Done, I also improved the documentation of why I change each part of the layout. https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:100: ApiCompatibilityUtils.getPaddingEnd(textLayout), 0); On 2017/03/24 17:15:37, Theresa wrote: > If possible, it is highly preferable to avoid this custom padding because it > adds additional engineering overhead and maintenance, particularly when we go > through another style redesign (happens every few years). We have a broad team > effort to make our UI more consistent to reduce the burden on UX and eng and > make the product feel more consistent overall. > > We use checkbox preferences with longer summary strings in our "Privacy" > settings screen. Is there a reason that the padding here needs to be different > than it is there? What does it look like if we don't adjust the padding and > gravity? I made these layout changes to align the checkbox with the top of the title instead of the center of the component. Max asked me to this because the summary text can be It is documented in the specs that the checkbox should be 16dp from the top and right, independent of the size of the summary text. https://screenshot.googleplex.com/HazfXCSSL7p.png here is a screenshot with the padding and Gravity.TOP disabled. I noticed that the first Gravity.TOP entry was useless and removed it. Only the gravity on the widget_frame is needed to pull the checkbox up https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasic.java (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasic.java:30: public static final String COOKIES_CHECKBOX = "clear_cookies_checkbox"; On 2017/03/24 17:15:37, Theresa wrote: > Can the superclass versions of these be made public and visible for testing > rather than duplicating the strings? yes, I will do that https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasic.java:52: if (ChromeSigninController.get(getActivity()).isSignedIn()) { On 2017/03/24 17:15:37, Theresa wrote: > The sign in state can change, possibly in a separate window while in Android N+ > multi-window. You should consider adding a SignInStateObserver to monitor when > state changes and update the summary accordingly. > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... The old ui uses a similar check to decide which footer to show. Is it ok to do the same and not observe signin, sync and sync datatypes? How would you change the signin state with multiwindow? Open chrome and signout your account in the Android settings? https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java:57: activity.getString(R.string.clear_browsing_data_tab_period_everything))}; On 2017/03/24 17:15:37, Theresa wrote: > Can you please ask UX if we can just replace these strings in the existing UI > rather than creating a fork in the code here? I think that doesn't work because the old ui starts with "Clear data from the" and then the sentence is completed by an option like "past week" or "beginning of time". The new text for everything is "All time". The reason to change this is that in many languages this sentence structure and word order doesn't work. The German text for example has to use a somewhat weird construction. Also the new texts are upper case while the old ones begin with a lower case because they complete a sentence. https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:592: Last 4 weeks On 2017/03/24 17:15:37, Theresa wrote: > If I'm reading the spec correctly, this should be "Last 30 days" That's right. The issue is that 4 weeks are 28 days and the new text is for 30 days. I already created a document for this problem and my current solution is to keep 4 weeks until we can change the text in both UIs to 30 days or remove the old ui. https://docs.google.com/a/google.com/document/d/1w9GHkI2iYzEfimj589fHeSzjb9Eo... https://codereview.chromium.org/2730703003/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java:29: @RetryOnFailure On 2017/03/24 17:15:37, Theresa wrote: > Are these tests flaky? > > Our general policy is that we don't add @RetryOnFailure to new tests, only old > tests where it's not obvious when they regressed and started flaking or how to > fix them. There needs to be a good justification for landing new tests with this > annotation. Thanks, I didn't know what this is exactly for. I removed RetryOnFailure and will run the test a few times but I think it should be stable.
https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:55: // Create custom onTouch listener to be able to respond to click events inside the summary. On 2017/03/27 14:11:04, dullweber wrote: > On 2017/03/24 17:15:37, Theresa wrote: > > Is the desired UX that when you click anywhere besides the ClickableSpan the > > preference's checkbox is toggled, and when you click on the ClickableSpan the > > link is taken? > > > > I think that's probably not going to work from an accessibility standpoint > (see > > next comment), because accessibility mode does not give focus to the > > ClickableSpan separate from the rest of the summary string. Who are you > working > > with from UX on this? I'm happy to get looped into a conversation if needed. > > My goal was that clicking the text should check or uncheck the checkbox. Only > direct clicks on the http://myactivity.google.com link should trigger the > LinkClickDelegate. maxwalker@ is doing the UX for this. Discussion about the UI > is on these slides: > https://docs.google.com/a/google.com/presentation/d/1NI9SR2DpFKAsAmvB0-Bn1Qyd.... > > I tried to use the UI with talkback and I think it would be best if > http://myactivity.google.com would not be clickable because it is only a small target > and most users would want to check or uncheck the checkbox. > Users who want to go to their activity can go to http://myactivity.google.com by > manually entering the address. Is that an acceptable behavior? Otherwise I would > try to use the TextViewWithClickableSpans with a custom layout for the > CheckBoxPreference. I don't think that is acceptable behavior, but we can always reach out to our accessibility team for consultation on how this should behave. It's better to find out now than during launch review once the feature is complete. Here are a couple of other options. 1) When talk back is off, a direct tap on the URL activates the link and a tap anywhere else toggles the checkbox. When talk b ack is on, a double tap on any of the summary text activates the link and taps on the checkbox toggle the preference. 2) Re-introduce the footer that is in the clear browsing data preference screen today so that the text view containing the link can be displayed separate from a preference. https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:100: ApiCompatibilityUtils.getPaddingEnd(textLayout), 0); On 2017/03/27 14:11:04, dullweber wrote: > On 2017/03/24 17:15:37, Theresa wrote: > > If possible, it is highly preferable to avoid this custom padding because it > > adds additional engineering overhead and maintenance, particularly when we go > > through another style redesign (happens every few years). We have a broad team > > effort to make our UI more consistent to reduce the burden on UX and eng and > > make the product feel more consistent overall. > > > > We use checkbox preferences with longer summary strings in our "Privacy" > > settings screen. Is there a reason that the padding here needs to be different > > than it is there? What does it look like if we don't adjust the padding and > > gravity? > > I made these layout changes to align the checkbox with the top of the title > instead of the center of the component. Max asked me to this because the summary > text can be > It is documented in the specs that the checkbox should be 16dp from the top and > right, independent of the size of the summary text. > https://screenshot.googleplex.com/HazfXCSSL7p.png here is a screenshot with the > padding and Gravity.TOP disabled. I noticed that the first Gravity.TOP entry was > useless and removed it. Only the gravity on the widget_frame is needed to pull > the checkbox up The UX decision either needs to be consistent with our existing preference styles or apply to all preferences (ideally they would all change at the same time). It's part of our job as eng to enforce consistency when the design guideline differs from what is implemented in other parts of the product. https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasic.java (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasic.java:52: if (ChromeSigninController.get(getActivity()).isSignedIn()) { On 2017/03/27 14:11:04, dullweber wrote: > On 2017/03/24 17:15:37, Theresa wrote: > > The sign in state can change, possibly in a separate window while in Android > N+ > > multi-window. You should consider adding a SignInStateObserver to monitor when > > state changes and update the summary accordingly. > > > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > The old ui uses a similar check to decide which footer to show. Is it ok to do > the same and not observe signin, sync and sync datatypes? How would you change > the signin state with multiwindow? Open chrome and signout your account in the > Android settings? Yes, signing out in Android settings would do it. If this has parity with the old UI then it's okay to leave as-is. It's an edge case, anyway, since we only allow the user to have one instance of settings open at any given time even if they have two Chrome windows side-by-side. https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTab.java:57: activity.getString(R.string.clear_browsing_data_tab_period_everything))}; On 2017/03/27 14:11:04, dullweber wrote: > On 2017/03/24 17:15:37, Theresa wrote: > > Can you please ask UX if we can just replace these strings in the existing UI > > rather than creating a fork in the code here? > > I think that doesn't work because the old ui starts with "Clear data from the" > and then the sentence is completed by an option like "past week" or "beginning > of time". The new text for everything is "All time". The reason to change this > is that in many languages this sentence structure and word order doesn't work. > The German text for example has to use a somewhat weird construction. > Also the new texts are upper case while the old ones begin with a lower case > because they complete a sentence. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM when Theresa is happy. https://codereview.chromium.org/2730703003/diff/140001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_counter_factory.cc (left): https://codereview.chromium.org/2730703003/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_factory.cc:51: pref_name == browsing_data::prefs::kDeleteBrowsingHistoryBasic) { On 2017/03/23 10:22:34, dullweber wrote: > On 2017/03/22 14:40:21, msramek wrote: > > If there's no counter on the basic tab, we should return nullptr. > > There is already the "return nullptr" at the end of the method but I added > explicit cases for history and cookies For the record, I am fine with kDeleteBrowsingHistoryBasic being handled implicitly by the nullptr at the end of method; we don't need an explicit case for that. I was just complaining that in this particular case, a live code was reached instead of the nullptr.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Change layout and texts for new CBD dialog. - Improved spinner layout - New spinner texts (1 day -> 24 hours, 1 week -> 7 days) - Implement a way to show different counter texts for basic and advanced mode - New counter texts for basic mode - Modified CheckboxPreference Layout to match mockups - Clickable myactivity.google.com link in checkbox summary - Add instrumentation test to verify that the right string is shown depending on sign in and sync state. Screenshots for default display size (Nexus 6P): https://screenshot.googleplex.com/0jjGhHys7ZP.png https://screenshot.googleplex.com/oQLWJWGtNW3.png Screenshots on largest display size (Nexus 6P): https://screenshot.googleplex.com/8dQoaW9NyWn.png https://screenshot.googleplex.com/wEsZomWRktr.png BUG=681523 ========== to ========== Change layout and texts for new CBD dialog. - Improved spinner layout - New spinner texts (1 day -> 24 hours, 1 week -> 7 days) - Implement a way to show different counter texts for basic and advanced mode - New counter texts for basic mode - Clickable myactivity.google.com link in checkbox summary - Add instrumentation test to verify that the right string is shown depending on sign in and sync state. Screenshots for default display size (Nexus 6P): https://screenshot.googleplex.com/hVjWKpEJoBq.png https://screenshot.googleplex.com/YfOJhRiDTNH.png Screenshots on largest display size (Nexus 6P): https://screenshot.googleplex.com/wXzgPMVDi5E.png https://screenshot.googleplex.com/wXzgPMVDi5E.png BUG=681523 ==========
Description was changed from ========== Change layout and texts for new CBD dialog. - Improved spinner layout - New spinner texts (1 day -> 24 hours, 1 week -> 7 days) - Implement a way to show different counter texts for basic and advanced mode - New counter texts for basic mode - Clickable myactivity.google.com link in checkbox summary - Add instrumentation test to verify that the right string is shown depending on sign in and sync state. Screenshots for default display size (Nexus 6P): https://screenshot.googleplex.com/hVjWKpEJoBq.png https://screenshot.googleplex.com/YfOJhRiDTNH.png Screenshots on largest display size (Nexus 6P): https://screenshot.googleplex.com/wXzgPMVDi5E.png https://screenshot.googleplex.com/wXzgPMVDi5E.png BUG=681523 ========== to ========== Change layout and texts for new CBD dialog. - Improved spinner layout - New spinner texts (1 day -> 24 hours, 1 week -> 7 days) - Implement a way to show different counter texts for basic and advanced mode - New counter texts for basic mode - Clickable myactivity.google.com link in checkbox summary - Add instrumentation test to verify that the right string is shown depending on sign in and sync state. Screenshots for default display size (Nexus 6P): https://screenshot.googleplex.com/hVjWKpEJoBq.png https://screenshot.googleplex.com/YfOJhRiDTNH.png Screenshots on largest display size (Nexus 6P): https://screenshot.googleplex.com/wXzgPMVDi5E.png https://screenshot.googleplex.com/KuZ1XKFC36W.png BUG=681523 ==========
https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java (right): https://codereview.chromium.org/2730703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:100: ApiCompatibilityUtils.getPaddingEnd(textLayout), 0); On 2017/03/27 15:24:52, Theresa wrote: > On 2017/03/27 14:11:04, dullweber wrote: > > On 2017/03/24 17:15:37, Theresa wrote: > > > If possible, it is highly preferable to avoid this custom padding because it > > > adds additional engineering overhead and maintenance, particularly when we > go > > > through another style redesign (happens every few years). We have a broad > team > > > effort to make our UI more consistent to reduce the burden on UX and eng and > > > make the product feel more consistent overall. > > > > > > We use checkbox preferences with longer summary strings in our "Privacy" > > > settings screen. Is there a reason that the padding here needs to be > different > > > than it is there? What does it look like if we don't adjust the padding and > > > gravity? > > > > I made these layout changes to align the checkbox with the top of the title > > instead of the center of the component. Max asked me to this because the > summary > > text can be > > It is documented in the specs that the checkbox should be 16dp from the top > and > > right, independent of the size of the summary text. > > https://screenshot.googleplex.com/HazfXCSSL7p.png here is a screenshot with > the > > padding and Gravity.TOP disabled. I noticed that the first Gravity.TOP entry > was > > useless and removed it. Only the gravity on the widget_frame is needed to pull > > the checkbox up > > The UX decision either needs to be consistent with our existing preference > styles or apply to all preferences (ideally they would all change at the same > time). It's part of our job as eng to enforce consistency when the design > guideline differs from what is implemented in other parts of the product. I removed the custom gravity on the checkbox. This way we can submit this cl and talk about the checkbox ui and accessibility later.
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2730703003/diff/260001/chrome/android/java/re... File chrome/android/java/res/values/attrs.xml (right): https://codereview.chromium.org/2730703003/diff/260001/chrome/android/java/re... chrome/android/java/res/values/attrs.xml:74: <!-- Used to change the SpinnerPreference to displays the TextView and the Spinner on the nit: s/displays/display https://codereview.chromium.org/2730703003/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java (right): https://codereview.chromium.org/2730703003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:55: textView.setOnTouchListener(new View.OnTouchListener() { I'm really not happy leaving this as is. Can we at least add a TODO to rethink this before launching the feature past the Dev channel? https://codereview.chromium.org/2730703003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:93: ApiCompatibilityUtils.getPaddingEnd(textLayout), checkBoxPadding); Can we remove this custom padding for now too please. https://codereview.chromium.org/2730703003/diff/260001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java (right): https://codereview.chromium.org/2730703003/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java:60: Does whether Chrome sync is enabled or disabled need to be set back at the end of the tests?
Description was changed from ========== Change layout and texts for new CBD dialog. - Improved spinner layout - New spinner texts (1 day -> 24 hours, 1 week -> 7 days) - Implement a way to show different counter texts for basic and advanced mode - New counter texts for basic mode - Clickable myactivity.google.com link in checkbox summary - Add instrumentation test to verify that the right string is shown depending on sign in and sync state. Screenshots for default display size (Nexus 6P): https://screenshot.googleplex.com/hVjWKpEJoBq.png https://screenshot.googleplex.com/YfOJhRiDTNH.png Screenshots on largest display size (Nexus 6P): https://screenshot.googleplex.com/wXzgPMVDi5E.png https://screenshot.googleplex.com/KuZ1XKFC36W.png BUG=681523 ========== to ========== Change layout and texts for new CBD dialog. - Improved spinner layout - New spinner texts (1 day -> 24 hours, 1 week -> 7 days) - Implement a way to show different counter texts for basic and advanced mode - New counter texts for basic mode - Clickable myactivity.google.com link in checkbox summary - Add instrumentation test to verify that the right string is shown depending on sign in and sync state. Screenshots for default display size (Nexus 6P): https://screenshot.googleplex.com/P1hFk9v8V3k.png https://screenshot.googleplex.com/zVjhJJWnXUo.png Screenshots on largest display size (Nexus 6P): https://screenshot.googleplex.com/m9nBDbQ5ra5.png https://screenshot.googleplex.com/Gb6vA38rfOH.png BUG=681523 ==========
Thanks! I removed the padding and updated the screenshots. https://codereview.chromium.org/2730703003/diff/260001/chrome/android/java/re... File chrome/android/java/res/values/attrs.xml (right): https://codereview.chromium.org/2730703003/diff/260001/chrome/android/java/re... chrome/android/java/res/values/attrs.xml:74: <!-- Used to change the SpinnerPreference to displays the TextView and the Spinner on the On 2017/03/30 21:57:47, Theresa wrote: > nit: s/displays/display Done. https://codereview.chromium.org/2730703003/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java (right): https://codereview.chromium.org/2730703003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:55: textView.setOnTouchListener(new View.OnTouchListener() { On 2017/03/30 21:57:47, Theresa wrote: > I'm really not happy leaving this as is. Can we at least add a TODO to rethink > this before launching the feature past the Dev channel? I added a TODO. https://codereview.chromium.org/2730703003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:93: ApiCompatibilityUtils.getPaddingEnd(textLayout), checkBoxPadding); On 2017/03/30 21:57:47, Theresa wrote: > Can we remove this custom padding for now too please. I removed the padding and updated the screenshots. I think the smaller device wastes too much space now but we can keep it this way for now. https://codereview.chromium.org/2730703003/diff/260001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java (right): https://codereview.chromium.org/2730703003/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java:60: On 2017/03/30 21:57:47, Theresa wrote: > Does whether Chrome sync is enabled or disabled need to be set back at the end > of the tests? I thought that every testcase in an instrumentation test gets a new process, so all static variables should be reset? AndroidSyncSettings.overrideForTests is never reset in any test and ProfileSyncService.overrideForTests is set to null in some tests but it actually can't be properly reverted because sInitialized is set to true and can't be set back to false. I think I can leave it this way if my assumption about how these tests work is true :) I also tried the following: private static int foo = 0; in setup(): assertEquals(0, foo); foo = 1; and it works in a test with multiple testcases.
lgtm https://codereview.chromium.org/2730703003/diff/260001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java (right): https://codereview.chromium.org/2730703003/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java:60: On 2017/03/31 10:42:42, dullweber wrote: > On 2017/03/30 21:57:47, Theresa wrote: > > Does whether Chrome sync is enabled or disabled need to be set back at the end > > of the tests? > > I thought that every testcase in an instrumentation test gets a new process, so > all static variables should be reset? > AndroidSyncSettings.overrideForTests is never reset in any test and > ProfileSyncService.overrideForTests is set to null in some tests but it actually > can't be properly reverted because sInitialized is set to true and can't be set > back to false. > > I think I can leave it this way if my assumption about how these tests work is > true :) > > I also tried the following: > > private static int foo = 0; > > in setup(): > assertEquals(0, foo); > foo = 1; > > and it works in a test with multiple testcases. Instrumentation tests are supposed to clear preferences that are set but we've seen odd behavior in the past where that's not always the case. I think it's a best practice to reset state that you've changed during your tests but it's up to you if you want to change this. https://codereview.chromium.org/2730703003/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java (right): https://codereview.chromium.org/2730703003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:51: // TODO(dullweber): Rething how the link can be made accessible to TalkBack before launch. s/Rething/Rethink
Thanks! https://codereview.chromium.org/2730703003/diff/260001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java (right): https://codereview.chromium.org/2730703003/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesBasicTest.java:60: On 2017/03/31 15:29:05, Theresa wrote: > On 2017/03/31 10:42:42, dullweber wrote: > > On 2017/03/30 21:57:47, Theresa wrote: > > > Does whether Chrome sync is enabled or disabled need to be set back at the > end > > > of the tests? > > > > I thought that every testcase in an instrumentation test gets a new process, > so > > all static variables should be reset? > > AndroidSyncSettings.overrideForTests is never reset in any test and > > ProfileSyncService.overrideForTests is set to null in some tests but it > actually > > can't be properly reverted because sInitialized is set to true and can't be > set > > back to false. > > > > I think I can leave it this way if my assumption about how these tests work is > > true :) > > > > I also tried the following: > > > > private static int foo = 0; > > > > in setup(): > > assertEquals(0, foo); > > foo = 1; > > > > and it works in a test with multiple testcases. > > Instrumentation tests are supposed to clear preferences that are set but we've > seen odd behavior in the past where that's not always the case. I think it's a > best practice to reset state that you've changed during your tests but it's up > to you if you want to change this. Done. https://codereview.chromium.org/2730703003/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java (right): https://codereview.chromium.org/2730703003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java:51: // TODO(dullweber): Rething how the link can be made accessible to TalkBack before launch. On 2017/03/31 15:29:06, Theresa wrote: > s/Rething/Rethink Done.
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dullweber@chromium.org changed reviewers: + miguelg@chromium.org
miguelg@chromium.org: Please review changes in chrome/android/java/strings/android_chrome_strings.grd I added additional strings for a redesigned clear browsing data dialog. Slides with mockups are linked in the bug. All changes are currently behind a flag (tabs-in-cbd).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dullweber@chromium.org changed reviewers: - miguelg@chromium.org
dullweber@chromium.org changed reviewers: + mariakhomenko@chromium.org
mariakhomenko@chromium.org: Please review changes in chrome/android/java/strings/android_chrome_strings.grd I added additional strings for a redesigned clear browsing data dialog. Slides with mockups are linked in the bug. All changes are currently behind a flag (tabs-in-cbd).
On 2017/04/05 11:57:16, dullweber wrote: > mailto:mariakhomenko@chromium.org: Please review changes in > chrome/android/java/strings/android_chrome_strings.grd > > I added additional strings for a redesigned clear browsing data dialog. Slides > with mockups are linked in the bug. All changes are currently behind a flag > (tabs-in-cbd). lgtm for chrome/android/java/strings/android_chrome_strings.grd
The CQ bit was checked by dullweber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, twellington@chromium.org Link to the patchset: https://codereview.chromium.org/2730703003/#ps320001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1491466458280460,
"parent_rev": "7d2f2a94dcea8910c419b6afca5e1c9cf5329cd7", "commit_rev":
"53bf4e3a7fca5f9faa16e56ba7b8a3c280324057"}
Message was sent while issue was closed.
Description was changed from ========== Change layout and texts for new CBD dialog. - Improved spinner layout - New spinner texts (1 day -> 24 hours, 1 week -> 7 days) - Implement a way to show different counter texts for basic and advanced mode - New counter texts for basic mode - Clickable myactivity.google.com link in checkbox summary - Add instrumentation test to verify that the right string is shown depending on sign in and sync state. Screenshots for default display size (Nexus 6P): https://screenshot.googleplex.com/P1hFk9v8V3k.png https://screenshot.googleplex.com/zVjhJJWnXUo.png Screenshots on largest display size (Nexus 6P): https://screenshot.googleplex.com/m9nBDbQ5ra5.png https://screenshot.googleplex.com/Gb6vA38rfOH.png BUG=681523 ========== to ========== Change layout and texts for new CBD dialog. - Improved spinner layout - New spinner texts (1 day -> 24 hours, 1 week -> 7 days) - Implement a way to show different counter texts for basic and advanced mode - New counter texts for basic mode - Clickable myactivity.google.com link in checkbox summary - Add instrumentation test to verify that the right string is shown depending on sign in and sync state. Screenshots for default display size (Nexus 6P): https://screenshot.googleplex.com/P1hFk9v8V3k.png https://screenshot.googleplex.com/zVjhJJWnXUo.png Screenshots on largest display size (Nexus 6P): https://screenshot.googleplex.com/m9nBDbQ5ra5.png https://screenshot.googleplex.com/Gb6vA38rfOH.png BUG=681523 Review-Url: https://codereview.chromium.org/2730703003 Cr-Commit-Position: refs/heads/master@{#462406} Committed: https://chromium.googlesource.com/chromium/src/+/53bf4e3a7fca5f9faa16e56ba7b8... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/53bf4e3a7fca5f9faa16e56ba7b8... |
