Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1530123002:
Prepare ClearBrowsingDataDialogFragment for browsing data counters. (Closed)
|
Created:
5 years ago by msramek Modified:
4 years, 11 months ago CC:
chromium-reviews, maxbogue+watch_chromium.org, plaree+watch_chromium.org, pvalenzuela+watch_chromium.org, tim+watch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@utils Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrepare ClearBrowsingDataDialogFragment for browsing data counters.
Following the desktop implementation (crbug.com/510028), we proceed to implement browsing data counters on Android (crbug.com/569870).
In this CL, we:
1. Add the ability to create, hold and destroy browsing data counters from the android UI (ClearBrowsingDataDialogFragment).
2. Refactor ClearBrowsingDataDialogFragment so that all properties of a dialog item (label, whether it's checked etc.) and methods related to the counter are held together by an inner class "Item".
3. Remember the checkbox state in a preference; this is necessary to manage the counters (that always check the preference). However, this also fixes the bug crbug.com/549172 that requests the labels to be sticky.
4. Add a new layout that holds the CheckedTextView together with the counter text; note that unless the experiment is enabled (see AreCountersEnabled() method), the counter text will be set to Visibility.GONE. In other words, this CL introduces no visual changes unless the user explicitly enables the experiment.
In this CL, we don't:
5. Polish the UI. The mocks are ready (https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1), but this will be done in another CL.
BUG=569870, 549172
BASE=1527653004
Committed: https://crrev.com/7984da63c93bb6328a3eeede7bf7e76855470995
Cr-Commit-Position: refs/heads/master@{#370004}
Patch Set 1 : #
Total comments: 22
Patch Set 2 : Rebase. #Patch Set 3 : Added *Bridge class, shared enum #
Total comments: 45
Patch Set 4 : Rebase, addressed comments #
Total comments: 10
Patch Set 5 : Nits. #Patch Set 6 : isOptionSelectedByDefault #
Total comments: 6
Patch Set 7 : s/default/NUM_TYPES/ #Messages
Total messages: 27 (10 generated)
Patchset #1 (id:1) has been deleted
msramek@chromium.org changed reviewers: + melandory@chromium.org, newt@chromium.org
Hi Newt and Tanja, can you please have a look at this? It's been over a month since I promised Newt to move forward with crbug.com/549172, but didn't have time to polish the CL until now. Thanks, Martin
Thanks! https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/checked_text_view_with_footer.xml (right): https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/checked_text_view_with_footer.xml:1: <?xml version="1.0" encoding="utf-8"?> realistically, this layout won't be reused anywhere else, so I'd name it something specific to the clear browsing data dialog, e.g. clear_browsing_data_dialog_item.xml https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/checked_text_view_with_footer.xml:17: android:id="@+id/footer" In Android preferences, this is called the "summary" (as opposed to the "title"). It might be nice to use the same terminology here. https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/checked_text_view_with_footer.xml:23: android:textAppearance="?android:attr/textAppearanceSmall" /> should you add android:textColor="?android:attr/textColorSecondary" ? Take a look at preference.xml in the support library if you're trying to create something that looks (somewhat) like a preference. https://android.googlesource.com/platform/frameworks/support/+/master/v7/pref... https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:634: public boolean getBrowsingDataDeletionPreference(int index) { Methods related to clearing browsing data should be pulled out into a separate class. PrefServiceBridge is intended for simple forwarding between Java and the C++ PrefService and for one-off cases where it's not worth creating a new class. The clear browsing data methods don't fit either of these criteria. I think that putting them in a separate class will create a clearer and more focused API. For an example of how to create a new Java class with native methods, see FeatureUtilities.java and feature_utilities.cc. In particular, see how those classes are used in chrome_jni_registrar.cc and in chrome_browser.gypi and note the RegisterFeatureUtilities() method in feature_utilities.cc. https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:671: public long createBrowsingDataCounter( Code at this level shouldn't know about or depend on ClearBrowsingDataDialogFragment. Instead, I'd define an interface, say, ClearBrowsingDataCounterCallback, with a single method onCounterFinished(), and make ClearBrowsingDataDialogFragment.Item implement that interface. https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:48: public class Item { Make this static and private. (See comment about adding a ClearBrowsingDataCounterCallback interface in PrefServiceBridge.) https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:95: public boolean isDisabled() { I'd flip this around to isEnabled() for consistency with other APIs that you're interacting with. It also avoids double negatives, i.e. if (!isDisabled()) https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:114: protected void finalize() throws Throwable { Avoid finalize(). finalize() reduces garbage collection efficiency, runs on a different thread (which is the most serious problem here), and swallows all exceptions silently. Also see https://google.github.io/styleguide/javaguide.html#s6.4-finalizers Instead, use the DialogFragment's onDestroy() method to clean up resources. E.g. add a destroy() method to Item and call it from DialogFragment.onDestroy(). https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:209: PrefServiceBridge.getInstance().getBrowsingDataDeletionPreference(0), Please refactor to avoid magic numbers. I think it makes sense to add an "index" field to DialogOption that corresponds to the indices of the pref_names array defined in pref_service_bridge.cc. (You could also consider used a shared C++/Java enum for this purposes. Grep for GENERATED_JAVA_ENUM_PACKAGE in most_visited_sites.cc for an example.) Also, since you're already using indices to communicate across the language boundary, you might as well make PrefServiceBridge.clearBrowsingData() take a List<Integer> indicating which data types to clear, rather than taking a bunch of booleans, whose order could easily be mixed up. (in C++ you can use a method in jni_array.h to convert List<Integer> to a vector<int>) https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:296: PrefServiceBridge.getInstance().getBrowsingDataDeletionPreference(i), This line assumes that items in the "options" have the same order as the pref_array has items at the same index as the pref_names array in pref_service_bridge.cc, which is a dangerous assumption. This could also be fixed by adding an "index" field to DialogOptions, as mentioned above. https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ClearSyncDataDialogFragment.java (right): https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ClearSyncDataDialogFragment.java:16: * Modal dialog for clearing sync data. This allows the user to clear browsing data as well as In a later CL, you'll probably want to merge this class into ClearBrowsingDataDialogFragment.
Patchset #3 (id:60001) has been deleted
Thanks for all the guidance, Newt! PTAL! https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/checked_text_view_with_footer.xml (right): https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/checked_text_view_with_footer.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2015/12/17 22:54:58, newt (OOO until 1-4) wrote: > realistically, this layout won't be reused anywhere else, so I'd name it > something specific to the clear browsing data dialog, e.g. > clear_browsing_data_dialog_item.xml Done. https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/checked_text_view_with_footer.xml:17: android:id="@+id/footer" On 2015/12/17 22:54:58, newt (OOO until 1-4) wrote: > In Android preferences, this is called the "summary" (as opposed to the > "title"). It might be nice to use the same terminology here. Done. Also renamed "text" above to "title". https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/checked_text_view_with_footer.xml:23: android:textAppearance="?android:attr/textAppearanceSmall" /> On 2015/12/17 22:54:58, newt (OOO until 1-4) wrote: > should you add android:textColor="?android:attr/textColorSecondary" ? > > Take a look at preference.xml in the support library if you're trying to create > something that looks (somewhat) like a preference. > > https://android.googlesource.com/platform/frameworks/support/+/master/v7/pref... Thanks! I'll definitely take inspiration from that file. But I'm expecting that the styling to match the mocks is going to take longer, so I'd still leave it for a separate CL. https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:634: public boolean getBrowsingDataDeletionPreference(int index) { On 2015/12/17 22:54:58, newt (OOO until 1-4) wrote: > Methods related to clearing browsing data should be pulled out into a separate > class. > > PrefServiceBridge is intended for simple forwarding between Java and the C++ > PrefService and for one-off cases where it's not worth creating a new class. The > clear browsing data methods don't fit either of these criteria. I think that > putting them in a separate class will create a clearer and more focused API. > > For an example of how to create a new Java class with native methods, see > FeatureUtilities.java and feature_utilities.cc. In particular, see how those > classes are used in chrome_jni_registrar.cc and in chrome_browser.gypi and note > the RegisterFeatureUtilities() method in feature_utilities.cc. Done. Thanks for the explanation, this is very helpful! https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:671: public long createBrowsingDataCounter( On 2015/12/17 22:54:58, newt wrote: > Code at this level shouldn't know about or depend on > ClearBrowsingDataDialogFragment. Instead, I'd define an interface, say, > ClearBrowsingDataCounterCallback, with a single method onCounterFinished(), and > make ClearBrowsingDataDialogFragment.Item implement that interface. Done. https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:48: public class Item { On 2015/12/17 22:54:59, newt (OOO until 1-4) wrote: > Make this static and private. (See comment about adding a > ClearBrowsingDataCounterCallback interface in PrefServiceBridge.) Done. https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:95: public boolean isDisabled() { On 2015/12/17 22:54:58, newt (OOO until 1-4) wrote: > I'd flip this around to isEnabled() for consistency with other APIs that you're > interacting with. It also avoids double negatives, i.e. if (!isDisabled()) Done. https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:114: protected void finalize() throws Throwable { On 2015/12/17 22:54:58, newt (OOO until 1-4) wrote: > Avoid finalize(). finalize() reduces garbage collection efficiency, runs on a > different thread (which is the most serious problem here), and swallows all > exceptions silently. > > Also see https://google.github.io/styleguide/javaguide.html#s6.4-finalizers > > Instead, use the DialogFragment's onDestroy() method to clean up resources. E.g. > add a destroy() method to Item and call it from DialogFragment.onDestroy(). Done. https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:209: PrefServiceBridge.getInstance().getBrowsingDataDeletionPreference(0), On 2015/12/17 22:54:59, newt (OOO until 1-4) wrote: > Please refactor to avoid magic numbers. I think it makes sense to add an "index" > field to DialogOption that corresponds to the indices of the pref_names array > defined in pref_service_bridge.cc. (You could also consider used a shared > C++/Java enum for this purposes. Grep for GENERATED_JAVA_ENUM_PACKAGE in > most_visited_sites.cc for an example.) > > Also, since you're already using indices to communicate across the language > boundary, you might as well make PrefServiceBridge.clearBrowsingData() take a > List<Integer> indicating which data types to clear, rather than taking a bunch > of booleans, whose order could easily be mixed up. (in C++ you can use a method > in jni_array.h to convert List<Integer> to a vector<int>) Done. Shared enum between C++ and Java is pretty much what I wanted to do, I just didn't know it was possible. Added enum ClankBrowsingDataType. I'm a bit unhappy that we're doing a DialogOption (Java) -> int/ClankBrowsingDataType (JNI) -> pref (C++) translations everywhere, basically having three ways to represent the same thing, but I at least tried to polish it so that variable names make it clear which one of the three is needed where. https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:296: PrefServiceBridge.getInstance().getBrowsingDataDeletionPreference(i), On 2015/12/17 22:54:59, newt wrote: > This line assumes that items in the "options" have the same order as the > pref_array has items at the same index as the pref_names array in > pref_service_bridge.cc, which is a dangerous assumption. This could also be > fixed by adding an "index" field to DialogOptions, as mentioned above. Done. I called it "datatype" instead, so the semantics are clearer: the index of a DialogOption in the menu is purely a matter of the Java UI. Therefore, what is sent over the language boundary is not called "index", but "datatype", as it describes which browsing data type is bound to that option. In practice, it's still an int (and it still is the index, as I kept the ordering) but the options now can be visually rearranged without breaking anything. https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ClearSyncDataDialogFragment.java (right): https://codereview.chromium.org/1530123002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ClearSyncDataDialogFragment.java:16: * Modal dialog for clearing sync data. This allows the user to clear browsing data as well as On 2015/12/17 22:54:59, newt (OOO until 1-4) wrote: > In a later CL, you'll probably want to merge this class into > ClearBrowsingDataDialogFragment. SGTM.
Thanks for the follow-up. Another round of comments :) https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/clear_browsing_data_dialog_item.xml (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/clear_browsing_data_dialog_item.xml:2: <!-- Copyright 2015 The Chromium Authors. All rights reserved. 2016 :) https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:634: public boolean getBrowsingDataDeletionPreference(int dataType) { public methods need javadoc. https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:638: public void setBrowsingDataDeletionPreference(int dataType, boolean value) { javadoc https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:643: * Clear the specified types of browsing data asynchronously. Explain what the valid values for dataTypes are https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java:14: private BrowsingDataCounterCallback mOwner; I'd be consistent with terminology and rename mOwner to mCallback. https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java:16: public BrowsingDataCounterBridge(BrowsingDataCounterCallback owner, int dataType) { javadoc for all public methods https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java:23: nativeDestroy(mNativeBrowsingDataCounterBridge); set mNativeBrowsingDataCounterBridge to 0 after destroying it to prevent accidental use-after-free https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java:32: private native long nativeInit(int index); s/index/dataType/ https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java:33: private static native void nativeDestroy(long nativeBrowsingDataCounterBridge); make this non-static https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterCallback.java (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterCallback.java:10: public interface BrowsingDataCounterCallback { since this is only used by BrowsingDataCounterBridge, I'd probably move this interface definition inside BrowsingDataCounterBridge as a nested interface. https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:314: options[i] != DialogOption.CLEAR_HISTORY || mCanDeleteBrowsingHistory); I'd make this line more readable by first creating a variable called "enabled" and passing that in here: boolean enabled = options[i] != DialogOption.CLEAR_HISTORY || mCanDeleteBrowsingHistory; mItems[i] = new Item(..., enabled); https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... File chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc:48: ScopedJavaLocalRef<jstring> resultString = s/resultString/result_string/ https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc:61: bool RegisterBrowsingDataCounterBridge(JNIEnv* env) { add a "// static" comment above this method https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... File chrome/browser/android/browsing_data/browsing_data_counter_bridge.h (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... chrome/browser/android/browsing_data/browsing_data_counter_bridge.h:18: BrowsingDataCounterBridge( Document this. In particular, what are valid values for dataType? https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... chrome/browser/android/browsing_data/browsing_data_counter_bridge.h:28: void Callback(scoped_ptr<BrowsingDataCounter::Result> result); Should this method be private? Also, Callback is a very vague name; how about giving this the same name as the corresponding Java method: OnBrowsingDataCounterFinished()? https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... chrome/browser/android/browsing_data/browsing_data_counter_bridge.h:30: bool valid() { return counter_; } Where is this method called? https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:553: for (const int data_type : data_types_vector) { Rather than converting from ClankBrowsingDataType values to the BrowsingDataRemover values, why not just generate a Java enum from BrowsingDataRemover and get rid of ClankBrowsingDataType altogether? Then this loop could simply OR together all the values. Or you could pass a single int containing the bitwise OR of all the desired BrowsingDataRemover values to clear. Like you mentioned, it would be nice to reduce the number of different ways of representing these same values. https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_counter_utils.h (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_utils.h:11: // Whether the browsing data counters experiment is enabled. Tangential comment: Perhaps these static methods should live inside a namespace. Without a namespace, it's quite difficult to figure out where, say, "AreCountersEnabled()" is defined when reading other code that uses these functions. This could be a follow-up CL if you plan to change this. See https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a... https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_utils.h:22: enum ClankBrowsingDataType { Don't use the word "clank" in code. How about just "BrowsingDataType" (or "BrowsingDataTypeAndroid" if this isn't applicable to other platforms)? https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_utils.h:29: NUM_TYPES = BOOKMARKS remove " = BOOKMARKS". In this case, BOOKMARKS will be 5, and NUM_TYPES should be 6.
Patchset #4 (id:100001) has been deleted
PTAL. Some changes from a rebase creeped in, but I guess it's still easier to compare patchsets if the comments are in the previous one as opposed to if I inserted a rebase-only patchset between them. Or let me know what works for you. https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/clear_browsing_data_dialog_item.xml (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/clear_browsing_data_dialog_item.xml:2: <!-- Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/12 18:55:26, newt wrote: > 2016 :) Done. https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:634: public boolean getBrowsingDataDeletionPreference(int dataType) { On 2016/01/12 18:55:26, newt wrote: > public methods need javadoc. Done. (Hmm, there is a presubmit warning for class javadoc, but apparently none for methods; and it's missing for a lot of methods in this file!) https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:638: public void setBrowsingDataDeletionPreference(int dataType, boolean value) { On 2016/01/12 18:55:26, newt wrote: > javadoc Done. https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:643: * Clear the specified types of browsing data asynchronously. On 2016/01/12 18:55:26, newt wrote: > Explain what the valid values for dataTypes are Done. https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java:14: private BrowsingDataCounterCallback mOwner; On 2016/01/12 18:55:26, newt wrote: > I'd be consistent with terminology and rename mOwner to mCallback. Done. https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java:16: public BrowsingDataCounterBridge(BrowsingDataCounterCallback owner, int dataType) { On 2016/01/12 18:55:26, newt wrote: > javadoc for all public methods Done. https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java:23: nativeDestroy(mNativeBrowsingDataCounterBridge); On 2016/01/12 18:55:26, newt wrote: > set mNativeBrowsingDataCounterBridge to 0 after destroying it to prevent > accidental use-after-free Done. https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java:32: private native long nativeInit(int index); On 2016/01/12 18:55:26, newt wrote: > s/index/dataType/ Done. https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java:33: private static native void nativeDestroy(long nativeBrowsingDataCounterBridge); On 2016/01/12 18:55:26, newt wrote: > make this non-static Done. https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterCallback.java (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterCallback.java:10: public interface BrowsingDataCounterCallback { On 2016/01/12 18:55:26, newt wrote: > since this is only used by BrowsingDataCounterBridge, I'd probably move this > interface definition inside BrowsingDataCounterBridge as a nested interface. Done. https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:314: options[i] != DialogOption.CLEAR_HISTORY || mCanDeleteBrowsingHistory); On 2016/01/12 18:55:26, newt wrote: > I'd make this line more readable by first creating a variable called "enabled" > and passing that in here: > > boolean enabled = options[i] != DialogOption.CLEAR_HISTORY || > mCanDeleteBrowsingHistory; > > mItems[i] = new Item(..., enabled); Done. https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... File chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc:48: ScopedJavaLocalRef<jstring> resultString = On 2016/01/12 18:55:26, newt wrote: > s/resultString/result_string/ Done. https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc:61: bool RegisterBrowsingDataCounterBridge(JNIEnv* env) { On 2016/01/12 18:55:26, newt wrote: > add a "// static" comment above this method This actually wasn't a static class member, but a standalone method. I followed an example of one of the methods in the JNI registrar which does it that way. But static Register method is apparently also popular, and much more tidy, so let's do it that way. https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... File chrome/browser/android/browsing_data/browsing_data_counter_bridge.h (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... chrome/browser/android/browsing_data/browsing_data_counter_bridge.h:18: BrowsingDataCounterBridge( On 2016/01/12 18:55:26, newt wrote: > Document this. In particular, what are valid values for dataType? Done. Also, renamed to |data_type|, since this is C++ :) https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... chrome/browser/android/browsing_data/browsing_data_counter_bridge.h:28: void Callback(scoped_ptr<BrowsingDataCounter::Result> result); On 2016/01/12 18:55:26, newt wrote: > Should this method be private? Also, Callback is a very vague name; how about > giving this the same name as the corresponding Java method: > OnBrowsingDataCounterFinished()? Done. Renamed to |OnCounterFinished|, as that is the name of the Java method, and the "browsing data" part is implied by the name of the class. And yes, it should be private. https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... chrome/browser/android/browsing_data/browsing_data_counter_bridge.h:30: bool valid() { return counter_; } On 2016/01/12 18:55:26, newt wrote: > Where is this method called? Hmm, good catch. I know that I added here to test if a counter is non-NULL, i.e. if it exists for the given data type, but this actually isn't called anywhere. If a counter doesn't exist, the constructor returns early, and no callbacks are ever called. No need for external checks right now. https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:553: for (const int data_type : data_types_vector) { On 2016/01/12 18:55:26, newt wrote: > Rather than converting from ClankBrowsingDataType values to the > BrowsingDataRemover values, why not just generate a Java enum from > BrowsingDataRemover and get rid of ClankBrowsingDataType altogether? Then this > loop could simply OR together all the values. Or you could pass a single int > containing the bitwise OR of all the desired BrowsingDataRemover values to > clear. > > Like you mentioned, it would be nice to reduce the number of different ways of > representing these same values. Hmm, that's a bit tricky. I would like to have reasonable keys (e.g. enum values) for the data types that user sees in the UI. On desktop, the UI has checkboxes that are tied to prefs::kDelete* boolean preferences, so I mostly used those preferences as keys (you can see that BrowsingDataCounter takes these prefs in the constructor). I really *should have* defined such an enum in the beginning, because now it's getting awkward. So this time, I wanted to get it right at least on Clank by defining such an enum there (ClankBrowsingDataType). I think we should simplify things by reusing the same enum on Desktop and Android (and later on iOS). There are some differences (DOWNLOADS only exists on Desktop; BOOKMARKS only on Clank) that I'm willing to live with, but most data types are the same. I added a TODO to reuse BrowsingDataType in the Desktop UI. That shouldn't be a part of this CL though, as it requires some refactoring. Regarding the use of BrowsingDataRemover::RemoveDataMask, I'm not so sure right now. First, it's not a one-to-one correspondence to the user-facing options, since cookies are represented by REMOVE_COOKIES and REMOVE_SITE_DATA, and the latter is in fact a convenience bitwise OR of several other options. We would still have to add stuff like BOOKMARKS, which are currently not defined there (as they are deleted directly from the Java code on Android). Also, semantics - we want to enumerate the data types and count them, but not remove them; we'd have to rename it and remove it from BrowsingDataRemover. I'm not entirely opposed to that, but it will require more thought and again, some refactoring. I filed crbug.com/577198. https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_counter_utils.h (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_utils.h:11: // Whether the browsing data counters experiment is enabled. On 2016/01/12 18:55:27, newt wrote: > Tangential comment: Perhaps these static methods should live inside a namespace. > Without a namespace, it's quite difficult to figure out where, say, > "AreCountersEnabled()" is defined when reading other code that uses these > functions. This could be a follow-up CL if you plan to change this. > > See > https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a... I originally did have a namespace :-) See the code review: https://codereview.chromium.org/1527653004/diff/20001/chrome/browser/browsing... https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_utils.h:22: enum ClankBrowsingDataType { On 2016/01/12 18:55:26, newt wrote: > Don't use the word "clank" in code. How about just "BrowsingDataType" (or > "BrowsingDataTypeAndroid" if this isn't applicable to other platforms)? Done. Yes, I wasn't sure about that, but I found some occurrences of "Clank" in the code, so I assumed it was OK. I'll remove the "Android" part altogether to reference that this should be reused on Desktop and later iOS as well (see the comment in pref_service_bridge.cc). https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_utils.h:29: NUM_TYPES = BOOKMARKS On 2016/01/12 18:55:26, newt wrote: > remove " = BOOKMARKS". In this case, BOOKMARKS will be 5, and NUM_TYPES should > be 6. Ah, of course.
A few nits, then lgtm. Thanks! https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:634: public boolean getBrowsingDataDeletionPreference(int dataType) { On 2016/01/13 15:27:53, msramek wrote: > On 2016/01/12 18:55:26, newt wrote: > > public methods need javadoc. > > Done. (Hmm, there is a presubmit warning for class javadoc, but apparently none > for methods; and it's missing for a lot of methods in this file!) We haven't enabled the presubmit warning for methods (yet) because there are too many methods still missing javadocs, and checkstyle checks the entire class for issues, not just the changed lines. Thank you for helping to reduce the number of methods missing javadoc :) https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:553: for (const int data_type : data_types_vector) { On 2016/01/13 15:27:53, msramek wrote: > On 2016/01/12 18:55:26, newt wrote: > > Rather than converting from ClankBrowsingDataType values to the > > BrowsingDataRemover values, why not just generate a Java enum from > > BrowsingDataRemover and get rid of ClankBrowsingDataType altogether? Then this > > loop could simply OR together all the values. Or you could pass a single int > > containing the bitwise OR of all the desired BrowsingDataRemover values to > > clear. > > > > Like you mentioned, it would be nice to reduce the number of different ways of > > representing these same values. > > Hmm, that's a bit tricky. > > I would like to have reasonable keys (e.g. enum values) for the data types that > user sees in the UI. > > On desktop, the UI has checkboxes that are tied to prefs::kDelete* boolean > preferences, so I mostly used those preferences as keys (you can see that > BrowsingDataCounter takes these prefs in the constructor). I really *should > have* defined such an enum in the beginning, because now it's getting awkward. > > So this time, I wanted to get it right at least on Clank by defining such an > enum there (ClankBrowsingDataType). I think we should simplify things by reusing > the same enum on Desktop and Android (and later on iOS). There are some > differences (DOWNLOADS only exists on Desktop; BOOKMARKS only on Clank) that I'm > willing to live with, but most data types are the same. > > I added a TODO to reuse BrowsingDataType in the Desktop UI. That shouldn't be a > part of this CL though, as it requires some refactoring. > > Regarding the use of BrowsingDataRemover::RemoveDataMask, I'm not so sure right > now. First, it's not a one-to-one correspondence to the user-facing options, > since cookies are represented by REMOVE_COOKIES and REMOVE_SITE_DATA, and the > latter is in fact a convenience bitwise OR of several other options. We would > still have to add stuff like BOOKMARKS, which are currently not defined there > (as they are deleted directly from the Java code on Android). Also, semantics - > we want to enumerate the data types and count them, but not remove them; we'd > have to rename it and remove it from BrowsingDataRemover. I'm not entirely > opposed to that, but it will require more thought and again, some refactoring. > > I filed crbug.com/577198. Thanks for the detailed explanation. I hadn't looked closely at BrowsingDataRemover; now I see that it doesn't correspond well to the user-visible data types. Reusing BrowsingDataType on other platforms sounds great. As for bookmarks being handled separately on Android: I would love to unify the deletion of bookmarks on Android with other platforms. In particular, on Android we currently use ChromeBrowserProviderClient.removeAllUserBookmarks() in ClearSyncDataDialogFragment to delete bookmarks, but the ChromeBrowserProviderClient API is deprecated and will eventually be removed. It looks like bookmark deletion isn't currently handled by BrowsingDataRemover, though? Is it not possible to delete all bookmarks on other platforms? https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_counter_utils.h (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_utils.h:11: // Whether the browsing data counters experiment is enabled. On 2016/01/13 15:27:53, msramek wrote: > On 2016/01/12 18:55:27, newt wrote: > > Tangential comment: Perhaps these static methods should live inside a > namespace. > > Without a namespace, it's quite difficult to figure out where, say, > > "AreCountersEnabled()" is defined when reading other code that uses these > > functions. This could be a follow-up CL if you plan to change this. > > > > See > > > https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a... > > I originally did have a namespace :-) See the code review: > https://codereview.chromium.org/1527653004/diff/20001/chrome/browser/browsing... Ah, well I respectfully submit my opposing opinion :) I think this case is a bit different because the methods aren't enclosed in a class. Anyway, the decision is up to you. https://codereview.chromium.org/1530123002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1530123002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:643: * @param dataType the requested browsing data type (from the shared enum nit: capitalize the first letter of the description, e.g. "The requested..." https://codereview.chromium.org/1530123002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java (right): https://codereview.chromium.org/1530123002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java:19: * @param result the result of the counter. how about: "A string describing how much storage space will be reclaimed by clearing this data type." https://codereview.chromium.org/1530123002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/1530123002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:49: private static class Item implements BrowsingDataCounterBridge.BrowsingDataCounterCallback { If a nested class has a nice descriptive name, it's fine to import the inner class directly. https://codereview.chromium.org/1530123002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:166: CLEAR_HISTORY( can these lines be unwrapped? https://codereview.chromium.org/1530123002/diff/120001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1530123002/diff/120001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:547: // ClearBrowsingDataObserver removes itself when |browsing_data_remover| is s/removes/deletes/ ?
Patchset #6 (id:160001) has been deleted
Thanks, Newt! I addressed the nits in Patchset #5, but made one last small change in Patchset #6, so feel free to still have another look :-) https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:553: for (const int data_type : data_types_vector) { On 2016/01/13 20:01:49, newt wrote: > On 2016/01/13 15:27:53, msramek wrote: > > On 2016/01/12 18:55:26, newt wrote: > > > Rather than converting from ClankBrowsingDataType values to the > > > BrowsingDataRemover values, why not just generate a Java enum from > > > BrowsingDataRemover and get rid of ClankBrowsingDataType altogether? Then > this > > > loop could simply OR together all the values. Or you could pass a single int > > > containing the bitwise OR of all the desired BrowsingDataRemover values to > > > clear. > > > > > > Like you mentioned, it would be nice to reduce the number of different ways > of > > > representing these same values. > > > > Hmm, that's a bit tricky. > > > > I would like to have reasonable keys (e.g. enum values) for the data types > that > > user sees in the UI. > > > > On desktop, the UI has checkboxes that are tied to prefs::kDelete* boolean > > preferences, so I mostly used those preferences as keys (you can see that > > BrowsingDataCounter takes these prefs in the constructor). I really *should > > have* defined such an enum in the beginning, because now it's getting awkward. > > > > So this time, I wanted to get it right at least on Clank by defining such an > > enum there (ClankBrowsingDataType). I think we should simplify things by > reusing > > the same enum on Desktop and Android (and later on iOS). There are some > > differences (DOWNLOADS only exists on Desktop; BOOKMARKS only on Clank) that > I'm > > willing to live with, but most data types are the same. > > > > I added a TODO to reuse BrowsingDataType in the Desktop UI. That shouldn't be > a > > part of this CL though, as it requires some refactoring. > > > > Regarding the use of BrowsingDataRemover::RemoveDataMask, I'm not so sure > right > > now. First, it's not a one-to-one correspondence to the user-facing options, > > since cookies are represented by REMOVE_COOKIES and REMOVE_SITE_DATA, and the > > latter is in fact a convenience bitwise OR of several other options. We would > > still have to add stuff like BOOKMARKS, which are currently not defined there > > (as they are deleted directly from the Java code on Android). Also, semantics > - > > we want to enumerate the data types and count them, but not remove them; we'd > > have to rename it and remove it from BrowsingDataRemover. I'm not entirely > > opposed to that, but it will require more thought and again, some refactoring. > > > > I filed crbug.com/577198. > > Thanks for the detailed explanation. > > I hadn't looked closely at BrowsingDataRemover; now I see that it doesn't > correspond well to the user-visible data types. > > Reusing BrowsingDataType on other platforms sounds great. > > As for bookmarks being handled separately on Android: I would love to unify the > deletion of bookmarks on Android with other platforms. In particular, on Android > we currently use ChromeBrowserProviderClient.removeAllUserBookmarks() in > ClearSyncDataDialogFragment to delete bookmarks, but the > ChromeBrowserProviderClient API is deprecated and will eventually be removed. It > looks like bookmark deletion isn't currently handled by BrowsingDataRemover, > though? Is it not possible to delete all bookmarks on other platforms? On Android, we show bookmarks only in the ClearSyncDataDialogFragment, which is a special case of the CBD dialog that we show to users when they disconnect one profile and connect another. In that case, we suggest them to delete everything from the old profile to avoid merging the old and new data. In the regular CBD dialog, on all platforms, bookmarks are not included. I think the idea is that bookmarks are somewhat more valuable, so you don't want to clean them often. Also, unlike most other browsing data types, they are not automatically recorded, but very explicitly set. But I agree that the deletion could be handled by BrowsingDataRemover, so that everything is in one place. I can take care of that soon-ish. https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_counter_utils.h (right): https://codereview.chromium.org/1530123002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_utils.h:11: // Whether the browsing data counters experiment is enabled. On 2016/01/13 20:01:49, newt wrote: > On 2016/01/13 15:27:53, msramek wrote: > > On 2016/01/12 18:55:27, newt wrote: > > > Tangential comment: Perhaps these static methods should live inside a > > namespace. > > > Without a namespace, it's quite difficult to figure out where, say, > > > "AreCountersEnabled()" is defined when reading other code that uses these > > > functions. This could be a follow-up CL if you plan to change this. > > > > > > See > > > > > > https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a... > > > > I originally did have a namespace :-) See the code review: > > > https://codereview.chromium.org/1527653004/diff/20001/chrome/browser/browsing... > > Ah, well I respectfully submit my opposing opinion :) I think this case is a > bit different because the methods aren't enclosed in a class. Anyway, the > decision is up to you. Very well :) Then I think it's best if we go with the other option from the linked review, and that is to define a very narrow namespace that matches the file name. That is roughly equivalent to encapsulating them into a static class, but AFAIK, the latter is a Java approach, the former is more acceptable in C++. I created a follow-up CL https://codereview.chromium.org/1581173004. https://codereview.chromium.org/1530123002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1530123002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:643: * @param dataType the requested browsing data type (from the shared enum On 2016/01/13 20:01:49, newt wrote: > nit: capitalize the first letter of the description, e.g. "The requested..." Done. Here and elsewhere. I actually capitalized it at first, but then noticed that most javadocs in this file don't do that. https://codereview.chromium.org/1530123002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java (right): https://codereview.chromium.org/1530123002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java:19: * @param result the result of the counter. On 2016/01/13 20:01:49, newt wrote: > how about: "A string describing how much storage space will be reclaimed by > clearing this data type." Done. https://codereview.chromium.org/1530123002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/1530123002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:49: private static class Item implements BrowsingDataCounterBridge.BrowsingDataCounterCallback { On 2016/01/13 20:01:49, newt wrote: > If a nested class has a nice descriptive name, it's fine to import the inner > class directly. Done. https://codereview.chromium.org/1530123002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:166: CLEAR_HISTORY( On 2016/01/13 20:01:49, newt wrote: > can these lines be unwrapped? Done. Mostly yes. https://codereview.chromium.org/1530123002/diff/120001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1530123002/diff/120001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:547: // ClearBrowsingDataObserver removes itself when |browsing_data_remover| is On 2016/01/13 20:01:49, newt wrote: > s/removes/deletes/ ? Done. Although this is from the rebase, not mine :) https://codereview.chromium.org/1530123002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/1530123002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:299: isOptionSelectedByDefault(options[i]), One last change: isOptionSelectedByDefault, that provided the default selection before, now wasn't used at all. So I replaced this line with a call to isOptionSelectedByDefault() and then changed isOptionSelectedByDefault() to read the preference. No functionality change in this file, but there are two implications: 1. It fixed the failing test which was was overriding this method to get the default selection 2. This method is still used in ClearSyncDataDialogFragment, which was overidding it to preselect *all* data types. Given that we show this dialog to the user when they're changing their syncing account and we recommend them to delete *all* their data, I think it is desirable to keep that behavior. This preselection is not done through changing preferences, so if the user cancels the account change and returns back to Chrome and opens the CBD dialog, the configuration will stay as it was. Unless they touched something in ClearSyncDataDialogFragment, then that will still be sticky. It could be more elegant, but I need to sit down with our PM and UX to discuss these special cases, and I think this interim state is acceptable. https://codereview.chromium.org/1530123002/diff/180001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1530123002/diff/180001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:517: if (!GetDeletionPreferenceFromDataType( Oops! This should be negated. Fortunately caught while testing.
msramek@chromium.org changed reviewers: + bauerb@chromium.org
Hi Bernhard, can you please review browsing_data_counter_utils.[cc|h] as an owner? Furthermore, in one of his comments, Newt suggested that the files should have a namespace - and we talked about that in a previous code review together. So I made a fix in a separate CL https://codereview.chromium.org/1581173004. PTAL at that one as well :) Thanks, Martin
https://codereview.chromium.org/1530123002/diff/180001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_counter_utils.cc (right): https://codereview.chromium.org/1530123002/diff/180001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_utils.cc:193: default: Replace this with NUM_TYPES please, so that the compiler will check that you've covered all the cases ☺
https://codereview.chromium.org/1530123002/diff/180001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_counter_utils.cc (right): https://codereview.chromium.org/1530123002/diff/180001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_utils.cc:193: default: On 2016/01/14 16:12:43, Bernhard Bauer wrote: > Replace this with NUM_TYPES please, so that the compiler will check that you've > covered all the cases ☺ Done. Unfortunately, since |data_type| can possibly be out of range, this requires additional return after the switch.
lgtm https://codereview.chromium.org/1530123002/diff/180001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_counter_utils.cc (right): https://codereview.chromium.org/1530123002/diff/180001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_utils.cc:193: default: On 2016/01/14 18:03:33, msramek wrote: > On 2016/01/14 16:12:43, Bernhard Bauer wrote: > > Replace this with NUM_TYPES please, so that the compiler will check that > you've > > covered all the cases ☺ > > Done. Unfortunately, since |data_type| can possibly be out of range, this > requires additional return after the switch. Yes, but that's what NOTREACHED() is for. :) (I think you could probably fall out of the switch statement and only use one NOTREACHED()).
msramek@chromium.org changed reviewers: + jochen@chromium.org
+Jochen, please review chrome/chrome.gyp. Thanks! https://codereview.chromium.org/1530123002/diff/180001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_counter_utils.cc (right): https://codereview.chromium.org/1530123002/diff/180001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_utils.cc:193: default: On 2016/01/14 18:23:00, Bernhard Bauer wrote: > On 2016/01/14 18:03:33, msramek wrote: > > On 2016/01/14 16:12:43, Bernhard Bauer wrote: > > > Replace this with NUM_TYPES please, so that the compiler will check that > > you've > > > covered all the cases ☺ > > > > Done. Unfortunately, since |data_type| can possibly be out of range, this > > requires additional return after the switch. > > Yes, but that's what NOTREACHED() is for. :) > > (I think you could probably fall out of the switch statement and only use one > NOTREACHED()). It came to my mind, but I decided for a separate NOTREACHED() in the NUM_TYPES case, so that the case is self-contained and doesn't rely on code outside the switch.
lgtm
Thanks, folks! It's safely after branchpoint, so let me land this :)
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org Link to the patchset: https://codereview.chromium.org/1530123002/#ps200001 (title: "s/default/NUM_TYPES/")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1530123002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1530123002/200001
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Prepare ClearBrowsingDataDialogFragment for browsing data counters. Following the desktop implementation (crbug.com/510028), we proceed to implement browsing data counters on Android (crbug.com/569870). In this CL, we: 1. Add the ability to create, hold and destroy browsing data counters from the android UI (ClearBrowsingDataDialogFragment). 2. Refactor ClearBrowsingDataDialogFragment so that all properties of a dialog item (label, whether it's checked etc.) and methods related to the counter are held together by an inner class "Item". 3. Remember the checkbox state in a preference; this is necessary to manage the counters (that always check the preference). However, this also fixes the bug crbug.com/549172 that requests the labels to be sticky. 4. Add a new layout that holds the CheckedTextView together with the counter text; note that unless the experiment is enabled (see AreCountersEnabled() method), the counter text will be set to Visibility.GONE. In other words, this CL introduces no visual changes unless the user explicitly enables the experiment. In this CL, we don't: 5. Polish the UI. The mocks are ready (https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1), but this will be done in another CL. BUG=569870,549172 BASE=1527653004 ========== to ========== Prepare ClearBrowsingDataDialogFragment for browsing data counters. Following the desktop implementation (crbug.com/510028), we proceed to implement browsing data counters on Android (crbug.com/569870). In this CL, we: 1. Add the ability to create, hold and destroy browsing data counters from the android UI (ClearBrowsingDataDialogFragment). 2. Refactor ClearBrowsingDataDialogFragment so that all properties of a dialog item (label, whether it's checked etc.) and methods related to the counter are held together by an inner class "Item". 3. Remember the checkbox state in a preference; this is necessary to manage the counters (that always check the preference). However, this also fixes the bug crbug.com/549172 that requests the labels to be sticky. 4. Add a new layout that holds the CheckedTextView together with the counter text; note that unless the experiment is enabled (see AreCountersEnabled() method), the counter text will be set to Visibility.GONE. In other words, this CL introduces no visual changes unless the user explicitly enables the experiment. In this CL, we don't: 5. Polish the UI. The mocks are ready (https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20Android/V1), but this will be done in another CL. BUG=569870,549172 BASE=1527653004 Committed: https://crrev.com/7984da63c93bb6328a3eeede7bf7e76855470995 Cr-Commit-Position: refs/heads/master@{#370004} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7984da63c93bb6328a3eeede7bf7e76855470995 Cr-Commit-Position: refs/heads/master@{#370004} |
