Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1465363002:
[Storage] Android - ManageSpace UI, Important Origins, and CBD Dialog (Closed)
Description[Storage] Android - ManageSpace UI, Important Origins, and CBD Dialog
See the screenshots in the bug.
This patch does the following:
* Adds an activity for the manageSpaceActivity target in the
AndroidManifest.xml which gives options for managing or clearing
storage.
NOTE: This is only hooked up in API >= KLP, as the API for
clearing an application's app data isn't available until then.
* We have a Dialog protecting the Reset App functionality.
* Tested for clearing unimportant storage.
* Adds a Clear All button to the bottom of the Storage site settings
view. This involves a layout that's almost an exact copy of the
default layout, except we have one button.
* Protected again by a dialog.
* Adds a dialog to the Clear Browsing Data screen, that only shows
up when the user has 'important' origins.
* The user is prompted about these sites, and can uncheck them.
* Test is included, which also double checks that cookies are
not deleted.
BUG=560549, 579763
Committed: https://crrev.com/05781339eb7e4153776b66bd6353d4b7ba24d0a6
Cr-Commit-Position: refs/heads/master@{#399584}
Patch Set 1 #
Total comments: 56
Patch Set 2 : comments, splitting off dialog logic, etc #Patch Set 3 : comments, splitting off dialog logic, etc #
Total comments: 16
Patch Set 4 : comments #Patch Set 5 : Redo, ManageSpaceActivity for clearing app data, and simplified Storage clear button #
Total comments: 2
Patch Set 6 : Android Java Hookup #
Total comments: 55
Patch Set 7 : almost working! #Patch Set 8 : Working #Patch Set 9 : Comments, working #
Total comments: 144
Patch Set 10 : comments #
Total comments: 2
Patch Set 11 : comments, cleanup, etc #
Total comments: 14
Patch Set 12 : comments #Patch Set 13 : successful async startup. #
Total comments: 10
Patch Set 14 : comments, and tests #
Total comments: 13
Patch Set 15 : Added row layout #
Total comments: 19
Patch Set 16 : ManageSpaceActivity test, and important site check fix #
Total comments: 2
Patch Set 17 : whoops, forgot files #
Total comments: 54
Patch Set 18 : comments, test split up, cleanup #
Total comments: 12
Patch Set 19 : removed frame layout #
Total comments: 41
Patch Set 20 : comments #
Total comments: 21
Patch Set 21 : comments, style fix, progress dialog change #
Total comments: 8
Patch Set 22 : rebase #Patch Set 23 : comments #Patch Set 24 : style changes #Patch Set 25 : Favicon support, and scroll indicators #
Total comments: 6
Patch Set 26 : Dialog layout change to make message always appear, and comments #
Total comments: 10
Patch Set 27 : rebase #
Total comments: 16
Patch Set 28 : comments, and removed progress dialog while fetching important sites (after clear button) #
Total comments: 5
Patch Set 29 : comment & compile #Patch Set 30 : rebase #
Total comments: 2
Patch Set 31 : compile & comments #
Total comments: 50
Patch Set 32 : comments #
Total comments: 2
Patch Set 33 : test, browserstartupcontroller mods #
Total comments: 2
Patch Set 34 : rebase #Patch Set 35 : fixed comment #Patch Set 36 : mistake in test #Patch Set 37 : rebase #Messages
Total messages: 100 (22 generated)
dmurph@chromium.org changed reviewers: + finnur@chromium.org
Hi finnur, I'm not sure who I should get reviews from for this guy. It has all of the functionality but it's not clean/I'm not sure knowledgable of Clank. Can you add people that normally review clank changes? Also, sorry about the formatting issues, I used 'git cl format' a while ago and it made everything different. Daniel
finnur@chromium.org changed reviewers: + newt@chromium.org
Newton is owner for this dir, I didn't get a chance to take a look at this toady, but will tomorrow morning.
Actually, I know how annoying it is to have nothing to work against when you show up in the morning, so I conjured up a few comments in a somewhat hasty review, so as to give you at least something to think about. :) https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/storage_preference_fragment.xml:1: <?xml version="1.0" encoding="utf-8"?> Would be good to get a screenshot of this UI. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:339: for (WebsitePreference preference : mWebsites) { I'm a little concerned about deleting from a potentially stale list. I did wonder if the action of deleting should go off and fetch a new list, or implementing the lookup in the WebsitePermissionsFetcher object somehow. I need to run, though, so I don't have much time to finish that thought. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:346: new DialogInterface.OnClickListener() { I think the code would benefit from having the handlers be separate functions. It feel a little cluttered to have this all in one block. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:423: } Most of this probably belongs in a helper function.
100% agree on putting the code elsewhere, I wasn't sure what the norm was (different classes or same class w/ helpers etc) so I just shoved it all in one spot for now. Thanks! On Tue, Nov 24, 2015, 8:39 AM <finnur@chromium.org> wrote: > Actually, I know how annoying it is to have nothing to work against when > you > show up in the morning, so I conjured up a few comments in a somewhat hasty > review, so as to give you at least something to think about. :) > > > > https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... > File chrome/android/java/res/layout/storage_preference_fragment.xml > (right): > > > https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... > chrome/android/java/res/layout/storage_preference_fragment.xml:1 > <https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay...>: > <?xml > version="1.0" encoding="utf-8"?> > Would be good to get a screenshot of this UI. > > > https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... > File > > chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java > (right): > > > https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... > > chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:339 > <https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org...> > : > for (WebsitePreference preference : mWebsites) { > I'm a little concerned about deleting from a potentially stale list. I > did wonder if the action of deleting should go off and fetch a new list, > or implementing the lookup in the WebsitePermissionsFetcher object > somehow. I need to run, though, so I don't have much time to finish that > thought. > > > https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... > > chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:346 > <https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org...> > : > new DialogInterface.OnClickListener() { > I think the code would benefit from having the handlers be separate > functions. It feel a little cluttered to have this all in one block. > > > https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... > > chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:423 > <https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org...> > : > } > Most of this probably belongs in a helper function. > > https://codereview.chromium.org/1465363002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Some additional comments. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/storage_preference_fragment.xml:18: android:layout_height="0dp" Unites are irrelevant with 0, just use "0", same with below. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/storage_preference_fragment.xml:19: android:layout_weight="1" But don't you want the unit here? https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:330: listView.setDivider(null); nit: line break after this. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:337: return; nit: line break after this. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:341: } nit: line break after this? https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:408: AlertDialog dialog2 = builder.create(); Oh, man. So many dialogs. It's hard to review with all these nested dialogs. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:420: dialog.show(); builder.create().show()? https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:423: } Or, actually, perhaps the whole dialog structure can live in a separate class... https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:671: } The indentation here and below seems incorrect to me. https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... File content/public/android/java/strings/android_content_strings.grd (right): https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:104: </message> Is this a dialog with two destructive actions and no cancel button? https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:115: All this app's data will be deleted permanently. This includes all files, settings, accounts, databases, etc. 'This app' seems a bit generic. Is that the string you were requested to use? https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:116: </message> This is not the file I would have expected to see these strings in... Why pick this over android_chrome_strings.grd, for example? Also, this file seems ordered alphabetically, except for these new strings.
I looked at everything except the Java code in detail. Like Finnur mentioned, I think the clear storage UI should live in a separate file, and I'd appreciate mocks before looking too closely at that code. Thanks! https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:278: <activity android:name="org.chromium.chrome.browser.preferences.website.StoragePreferenceActivity" nit: I'd put this just below Preferences, since it's less used/important https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/storage_preference_fragment.xml:10: <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" standard indentation for Android xml files looks like this: <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" android:orientation="vertical" ... /> <FrameLayout android:layout_width="match_parent" ... /> <ListView android:id="@android:id/list" ... /> </FrameLayout> </LinearLayout> https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/storage_preference_fragment.xml:18: android:layout_height="0dp" On 2015/11/25 11:30:22, Finnur wrote: > Unites are irrelevant with 0, just use "0", same with below. Actually, you need units even when the value is 0 (unlike CSS) https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/storage_preference_fragment.xml:19: android:layout_weight="1" On 2015/11/25 11:30:22, Finnur wrote: > But don't you want the unit here? layout_weight doesn't actually have units https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:325: public void onActivityCreated(Bundle savedInstanceState) { Too much nesting. Please split this out into a separate file and separate functions, and reduce nesting. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:719: (ChromeBaseCheckBoxPreference) getPreferenceScreen().findPreference( I think it's better to avoid formatting changes unless you're making a clear improvement. Some of these formatting changes are wrong (e.g. all the else-if clauses above), and many of them are arbitrary: the formatting was correct before and it's still correct, just different. I'd revert all the formatting changes except perhaps unwrapping "findPreference(ALLOWED/BLOCKED_GROUP)" https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/StoragePreferenceActivity.java (right): https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/StoragePreferenceActivity.java:14: public class StoragePreferenceActivity extends Activity { javadoc. Explain what this class does, why it exists, how to get to this UI, etc. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/StoragePreferenceActivity.java:18: Intent intent = new Intent(this, Preferences.class); use PreferencesLauncher.createIntentForSettingsPage() to create this intent. It'll set up the Intent in the standard way, and you can add extras as needed. https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... File content/public/android/java/strings/android_content_strings.grd (right): https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:103: This will clear <ph name="SIZE_IN_KB">%1$s<ex>101kb</ex></ph> of storage from sites not marked as important. To clear all app storage, reset Chrome to its factory default. What does "marked as important" mean? Do we expect users to know what this means? (We shouldn't; I don't know what this means) If this simply means sites that are saved offline, or some other simple-to-enumerate category, I think it would be more useful to call it out specifically. https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:105: <message name="IDS_STORAGE_CLEAR_DIALOG_RESET_APP_OPTION" desc=""> needs a description; same below https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:106: Reset App Use "Sentence case", i.e. only capitalize the first word of each title. This is the convention used throughout Android UI. https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:109: Clear Site Storage sentence case https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:116: </message> On 2015/11/25 11:30:22, Finnur wrote: > This is not the file I would have expected to see these strings in... Why pick > this over android_chrome_strings.grd, for example? > > Also, this file seems ordered alphabetically, except for these new strings. Right, these strings should be in android_chrome_strings.grd, since they're used only in src/chrome/android code. Please group them with relevant strings in android_chrome_strings.grd
Description was changed from
==========
[Storage] Implementing a clear-all button in the Clank Storage UI.
NOTE: I implemented this in a pretty ugly way because I'm not sure what
the team wants w/ code style. So I just put everything in the same spot.
This also hooks up the Storage UI w/ the Manage Storage button in
the Android settings.
BUG=560549
==========
to
==========
[Storage] Implementing a clear-all button in the Clank Storage UI.
See the screenshots in the bug.
This patch does the following:
* Adds an activity for the manageSpaceActivity target in the
AndroidManifest.xml which redirects us to the Storage category
of site settings. (This hooks up Manage Space in App Settings)
* Adds a Clear button to the bottom of the Storage site settings
view. This involves a custom layout XML, as we need the button
to be visible when the list is empty as well.
* Adds 2 dialogs:
* When clear button is pressed, we prompt user if they want to
clear the site data, or all app data.
* When the user selects to clear all app data, we preset a
confirmation dialog.
* Clears all app data in a SDK-version-friendly-way
BUG=560549
==========
I added screenshots in the bug, and Rebecca also added her mocks. Let me know what you think! Thanks for reviewing :) https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:278: <activity android:name="org.chromium.chrome.browser.preferences.website.StoragePreferenceActivity" On 2015/11/25 at 15:56:25, newt wrote: > nit: I'd put this just below Preferences, since it's less used/important Done. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/storage_preference_fragment.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2015/11/24 at 16:39:53, Finnur wrote: > Would be good to get a screenshot of this UI. Put screenshot in bug. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/storage_preference_fragment.xml:10: <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" On 2015/11/25 at 15:56:25, newt wrote: > standard indentation for Android xml files looks like this: > > <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" > android:orientation="vertical" > ... /> > > <FrameLayout > android:layout_width="match_parent" > ... /> > > <ListView android:id="@android:id/list" ... /> > > </FrameLayout> > > </LinearLayout> So this means I'm adding a couple of newlines, right? https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:325: public void onActivityCreated(Bundle savedInstanceState) { On 2015/11/25 at 15:56:25, newt wrote: > Too much nesting. Please split this out into a separate file and separate functions, and reduce nesting. Definitely. I mentioned this a couple times. I'll put the dialog parts in a separate class, and keep the website clearing logic & app reset logic in this class. Let me know if I should break it up more. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:330: listView.setDivider(null); On 2015/11/25 at 11:30:22, Finnur wrote: > nit: line break after this. Done. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:337: return; On 2015/11/25 at 11:30:22, Finnur wrote: > nit: line break after this. Done. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:339: for (WebsitePreference preference : mWebsites) { On 2015/11/24 at 16:39:53, Finnur wrote: > I'm a little concerned about deleting from a potentially stale list. I did wonder if the action of deleting should go off and fetch a new list, or implementing the lookup in the WebsitePermissionsFetcher object somehow. I need to run, though, so I don't have much time to finish that thought. Done. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:341: } On 2015/11/25 at 11:30:22, Finnur wrote: > nit: line break after this? Done. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:346: new DialogInterface.OnClickListener() { On 2015/11/24 at 16:39:53, Finnur wrote: > I think the code would benefit from having the handlers be separate functions. It feel a little cluttered to have this all in one block. Yes definitely. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:408: AlertDialog dialog2 = builder.create(); On 2015/11/25 at 11:30:22, Finnur wrote: > Oh, man. So many dialogs. It's hard to review with all these nested dialogs. Moved to a separate file. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:420: dialog.show(); On 2015/11/25 at 11:30:22, Finnur wrote: > builder.create().show()? Done. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:423: } On 2015/11/25 at 11:30:22, Finnur wrote: > Or, actually, perhaps the whole dialog structure can live in a separate class... Done. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:671: } On 2015/11/25 at 11:30:22, Finnur wrote: > The indentation here and below seems incorrect to me. This was done with git cl format without me realizing how many parts this would change. I reverted all of these parts. I mentioned this in email, https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:719: (ChromeBaseCheckBoxPreference) getPreferenceScreen().findPreference( On 2015/11/25 at 15:56:25, newt wrote: > I think it's better to avoid formatting changes unless you're making a clear improvement. Some of these formatting changes are wrong (e.g. all the else-if clauses above), and many of them are arbitrary: the formatting was correct before and it's still correct, just different. I'd revert all the formatting changes except perhaps unwrapping "findPreference(ALLOWED/BLOCKED_GROUP)" As I mentioned in the email, this was a mistake where I assumed that running git cl format wouldn't mess all this up. Reverted for new patch. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/StoragePreferenceActivity.java (right): https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/StoragePreferenceActivity.java:14: public class StoragePreferenceActivity extends Activity { On 2015/11/25 at 15:56:25, newt wrote: > javadoc. Explain what this class does, why it exists, how to get to this UI, etc. Done. https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/StoragePreferenceActivity.java:18: Intent intent = new Intent(this, Preferences.class); On 2015/11/25 at 15:56:25, newt wrote: > use PreferencesLauncher.createIntentForSettingsPage() to create this intent. It'll set up the Intent in the standard way, and you can add extras as needed. Done. https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... File content/public/android/java/strings/android_content_strings.grd (right): https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:103: This will clear <ph name="SIZE_IN_KB">%1$s<ex>101kb</ex></ph> of storage from sites not marked as important. To clear all app storage, reset Chrome to its factory default. On 2015/11/25 at 15:56:25, newt wrote: > What does "marked as important" mean? Do we expect users to know what this means? (We shouldn't; I don't know what this means) > > If this simply means sites that are saved offline, or some other simple-to-enumerate category, I think it would be more useful to call it out specifically. Whoops, this is a fragment from the future! Changed to be the current meaning. https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:104: </message> On 2015/11/25 at 11:30:22, Finnur wrote: > Is this a dialog with two destructive actions and no cancel button? I use the R.id.cancel resource. Is that alright? https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:105: <message name="IDS_STORAGE_CLEAR_DIALOG_RESET_APP_OPTION" desc=""> On 2015/11/25 at 15:56:25, newt wrote: > needs a description; same below Done. https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:106: Reset App On 2015/11/25 at 15:56:25, newt wrote: > Use "Sentence case", i.e. only capitalize the first word of each title. This is the convention used throughout Android UI. For buttons? ok. https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:109: Clear Site Storage On 2015/11/25 at 15:56:25, newt wrote: > sentence case Done. https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:115: All this app's data will be deleted permanently. This includes all files, settings, accounts, databases, etc. On 2015/11/25 at 11:30:22, Finnur wrote: > 'This app' seems a bit generic. Is that the string you were requested to use? I just copied it from the mocks. Changed to Chrome. https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:116: </message> On 2015/11/25 at 15:56:25, newt wrote: > On 2015/11/25 11:30:22, Finnur wrote: > > This is not the file I would have expected to see these strings in... Why pick > > this over android_chrome_strings.grd, for example? > > > > Also, this file seems ordered alphabetically, except for these new strings. > > Right, these strings should be in android_chrome_strings.grd, since they're used only in src/chrome/android code. Please group them with relevant strings in android_chrome_strings.grd Done.
https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/storage_preference_fragment.xml:18: android:layout_height="0dp" I've been working too much in Polymer lately. :) https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/storage_preference_fragment.xml:19: android:layout_weight="1" Oh. I mistook weight for width. Sorry. :) https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/storage_preference_fragment.xml:39: nit: Remove line break. https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/storage_preference_fragment.xml:40: </FrameLayout> Indentation seems off. https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/storage_preference_fragment.xml:53: nit: Remove line break. https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ClearAllStorageAction.java (right): https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ClearAllStorageAction.java:48: // Add the buttons nit: Missing period. https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:309: public static boolean deleteDir(File dir) { nit: Document these functions. Also (Newton), is there a more central location for a function like this? It seems generic enough to be needed elsewhere, potentially. https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:338: if (!s.equals("lib")) { I don't know what the file structure looks like on Android. What can we expect to get deleted/not deleted (e.g. why exclude lib)? Do we need to worry about accidentally deleting something we didn't intend? https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:339: deleteDir(new File(appDir, s)); I notice we ignore errors here. That can leave Chrome in a partially un-installed state, right? I don't know if a retry is a good strategy, but perhaps at a minimum we could throw up a message saying "failed to delete, please uninstall", or something. https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2331: Clear Missing the '...' suffix, as per mocks.
https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/storage_preference_fragment.xml:39: On 2015/11/26 at 15:54:41, Finnur wrote: > nit: Remove line break. Done. https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/storage_preference_fragment.xml:40: </FrameLayout> On 2015/11/26 at 15:54:41, Finnur wrote: > Indentation seems off. Done. https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/storage_preference_fragment.xml:53: On 2015/11/26 at 15:54:41, Finnur wrote: > nit: Remove line break. https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ClearAllStorageAction.java (right): https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ClearAllStorageAction.java:48: // Add the buttons On 2015/11/26 at 15:54:41, Finnur wrote: > nit: Missing period. Done. https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:309: public static boolean deleteDir(File dir) { On 2015/11/26 at 15:54:41, Finnur wrote: > nit: Document these functions. > > Also (Newton), is there a more central location for a function like this? It seems generic enough to be needed elsewhere, potentially. Done. Yes, please let me know where I should put this stuff. I'll cc you guys on the email to clank-dev. https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:338: if (!s.equals("lib")) { On 2015/11/26 at 15:54:41, Finnur wrote: > I don't know what the file structure looks like on Android. > What can we expect to get deleted/not deleted (e.g. why exclude lib)? Do we need to worry about accidentally deleting something we didn't intend? I emailed Dianne Hackborn (hackbod@google) to see if she knows a better way. Other possibility is to disable this functionality for devices < kitkat. https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:339: deleteDir(new File(appDir, s)); On 2015/11/26 at 15:54:41, Finnur wrote: > I notice we ignore errors here. That can leave Chrome in a partially un-installed state, right? I don't know if a retry is a good strategy, but perhaps at a minimum we could throw up a message saying "failed to delete, please uninstall", or something. Yes, that sounds good. I'll chat w/ Rebecca and clank-dev to figure out the best solution. https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1465363002/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2331: Clear On 2015/11/26 at 15:54:41, Finnur wrote: > Missing the '...' suffix, as per mocks. Done.
Description was changed from
==========
[Storage] Implementing a clear-all button in the Clank Storage UI.
See the screenshots in the bug.
This patch does the following:
* Adds an activity for the manageSpaceActivity target in the
AndroidManifest.xml which redirects us to the Storage category
of site settings. (This hooks up Manage Space in App Settings)
* Adds a Clear button to the bottom of the Storage site settings
view. This involves a custom layout XML, as we need the button
to be visible when the list is empty as well.
* Adds 2 dialogs:
* When clear button is pressed, we prompt user if they want to
clear the site data, or all app data.
* When the user selects to clear all app data, we preset a
confirmation dialog.
* Clears all app data in a SDK-version-friendly-way
BUG=560549
==========
to
==========
[Storage] Implementing a clear-all button in the Clank Storage UI.
See the screenshots in the bug.
This patch does the following:
* Adds an activity for the manageSpaceActivity target in the
AndroidManifest.xml which gives options for managing or clearing
storage.
NOTE: This is only hooked up in API >= KLP, as the API for
clearing an application's app data isn't available until then.
* Adds a Clear button to the bottom of the Storage site settings
view. This involves a layout that's almost an exact copy of the
default layout, except we have one button.
* Adds 2 dialogs:
* When clear button is pressed, we prompt user if they want to
clear all 'X MB' of site data.
* When the user selects to clear all app data, we preset a
confirmation dialog.
BUG=560549
==========
Hey guys, So this is a re-do, and I would love feedback just on one thing: In the ManageSpaceActivity, I need to make sure the native libraries are loaded to do the to use the WebsitePermissionsFetcher (as it calls into CPP). I use the BrowserStartupController. Is this correct? I tested that I'm able to use the app before & after I launch this activity (and also launch this activity before I ever launch the app). Thanks so much! Daniel
I haven't faced that problem, so I don't know the answer. Newton probably knows.
https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:671: } On 2015/11/25 23:47:46, dmurph wrote: > On 2015/11/25 at 11:30:22, Finnur wrote: > > The indentation here and below seems incorrect to me. > > This was done with git cl format without me realizing how many parts this would > change. I reverted all of these parts. I mentioned this in email, Unfortunately, "git cl format" doesn't really work with Java code. It's using clang format, with some hacks to make it possible to sort of parse Java, but as you can see, it doesn't always get things right. https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... File content/public/android/java/strings/android_content_strings.grd (right): https://codereview.chromium.org/1465363002/diff/1/content/public/android/java... content/public/android/java/strings/android_content_strings.grd:106: Reset App On 2015/11/25 23:47:46, dmurph wrote: > On 2015/11/25 at 15:56:25, newt wrote: > > Use "Sentence case", i.e. only capitalize the first word of each title. This > is the convention used throughout Android UI. > > For buttons? ok. Yes, sentence case for everything on Android. (except sometimes chrome:// pages, where we're lazy and use the same string across all platforms) https://codereview.chromium.org/1465363002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:65: BrowserStartupController controller = Currently, BrowserStartupController.startBrowserProcessesAsync() is closely tied to the code in AsyncInitializationActivity. One (untested) option is to extend AsyncInitializationActivity. Currently, only ChromeActivity (the activity that houses the main web browsing session) extends AsyncInitializationActivity. You may or may not run into bugs / trickiness when extending AsyncInitializationActivity. Alternatively, you can follow the pattern used in other activities that depend on native code: use ChromeApplication.startBrowserProcessesSync(). See Preferences.java for an example: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
Description was changed from
==========
[Storage] Implementing a clear-all button in the Clank Storage UI.
See the screenshots in the bug.
This patch does the following:
* Adds an activity for the manageSpaceActivity target in the
AndroidManifest.xml which gives options for managing or clearing
storage.
NOTE: This is only hooked up in API >= KLP, as the API for
clearing an application's app data isn't available until then.
* Adds a Clear button to the bottom of the Storage site settings
view. This involves a layout that's almost an exact copy of the
default layout, except we have one button.
* Adds 2 dialogs:
* When clear button is pressed, we prompt user if they want to
clear all 'X MB' of site data.
* When the user selects to clear all app data, we preset a
confirmation dialog.
BUG=560549
==========
to
==========
[Storage] Implementing a clear-all button in the Clank Storage UI.
See the screenshots in the bug.
This patch does the following:
* Adds an activity for the manageSpaceActivity target in the
AndroidManifest.xml which gives options for managing or clearing
storage.
NOTE: This is only hooked up in API >= KLP, as the API for
clearing an application's app data isn't available until then.
* Adds a Clear button to the bottom of the Storage site settings
view. This involves a layout that's almost an exact copy of the
default layout, except we have one button.
* Adds 2 dialogs:
* When clear button is pressed, we prompt user if they want to
clear all 'X MB' of site data.
* When the user selects to clear all app data, we preset a
confirmation dialog.
BUG=560549,579763
==========
Description was changed from
==========
[Storage] Implementing a clear-all button in the Clank Storage UI.
See the screenshots in the bug.
This patch does the following:
* Adds an activity for the manageSpaceActivity target in the
AndroidManifest.xml which gives options for managing or clearing
storage.
NOTE: This is only hooked up in API >= KLP, as the API for
clearing an application's app data isn't available until then.
* Adds a Clear button to the bottom of the Storage site settings
view. This involves a layout that's almost an exact copy of the
default layout, except we have one button.
* Adds 2 dialogs:
* When clear button is pressed, we prompt user if they want to
clear all 'X MB' of site data.
* When the user selects to clear all app data, we preset a
confirmation dialog.
BUG=560549,579763
==========
to
==========
[Storage] Android - ManageSpace UI, Important Origins, and CBD Dialog
See the screenshots in the bug.
This patch does the following:
* Adds an activity for the manageSpaceActivity target in the
AndroidManifest.xml which gives options for managing or clearing
storage.
NOTE: This is only hooked up in API >= KLP, as the API for
clearing an application's app data isn't available until then.
* We have a Dialog protecting the Reset App functionality.
* Adds a Clear All button to the bottom of the Storage site settings
view. This involves a layout that's almost an exact copy of the
default layout, except we have one button.
* Protected again by a dialog.
* Adds a dialog to the Clear Browsing Data screen, that only shows
up when the user has 'important' origins.
* The user is prompted about these sites, and can uncheck them.
STILL TODO:
* Finalize the CPP side of important origins, where we want to
check permissions and other things.
* Hook up the BrowsingDataRemover so that we actually do the origin
filtering
BUG=560549,579763
==========
Hey guys, I emailed you earlier about this patch, I now have something for you to review :) This isn't TOTALLY complete. I'm looking for guidance in the following areas: 1. I'm putting the 'important origin' idea in two spots through the cpp bridge. One pipes it through w/ the storage lookup, and one is separate and just fetches the list of important origins (with extra data, but I'm only currently using the url). Any advice about this? Knowing if the origin is important or not on the storage site helps w/ the clear all button, but I could just call both instead of putting it into the current API. 2. How I added dialogs, the UI in general (mostly similar to before). 3. How do I test these things well? Full integration test style. The actual origin filtering isn't hooked up yet, pending this patch: https://codereview.chromium.org/1741123002 and the bridge bindings. Thanks so much! Daniel
My first impression of this was: Wow, the UI has evolved quite a bit since I was involved with this last. Not in a negative way, just interesting to see how far we've come and the mocks look good. > 1. I'm putting the 'important origin' idea in two spots through the cpp bridge. > One pipes it through w/ the storage lookup, and one is separate and just > fetches the list of important origins (with extra data, but I'm only > currently > using the url). Any advice about this? Knowing if the origin is important or > not on the storage site helps w/ the clear all button, but I could just call > both instead of putting it into the current API. I'm somewhat on the fence here. You don't always need the ranking data, so it slows things down for the general case (fetching local data to show). If you can (without jumping through hoops too much) fetch it on demand for both cases then I suppose that would be better. > 2. How I added dialogs, the UI in general (mostly similar to before). Newton can probably assist you better than I with most of this. Looks like he might be away for a few days, though, but Ted Choc is a good resource also if you don't want to wait. > 3. How do I test these things well? Full integration test style. I've only done one complex dialog on Android and the test for that is here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... ... in case you want to use it as inspiration. More specific review comments follow.
https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:2: <!-- Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:25: <!-- ======== Unimportant Storage Info ======== --> nit: Single quotes would probably help here: 'Unimportant Storage' Info ... to differentiate from: Unimportant 'Storage Info'. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:68: Nit: Delete extra line here and below. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:2: <!-- Copyright 2015 The Chromium Authors. All rights reserved. Nit: 2016 https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... File chrome/android/java/res/values-v19/constants.xml (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... chrome/android/java/res/values-v19/constants.xml:2: <!-- Copyright 2015 The Chromium Authors. All rights reserved. Nit: 2016 https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... chrome/android/java/res/values-v19/constants.xml:5: DO NOT TRANSLATE --> Do not translate? I don't recall seeing this convention before. Is this intentional? https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... File chrome/android/java/res/values/constants.xml (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... chrome/android/java/res/values/constants.xml:5: DO NOT TRANSLATE --> Same question here. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:186: private boolean mWaitingForOrigins = false; Probably clearer to add a private function WaitingForOrigins() that returns mSortedImportantOrigins == null. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:188: private Map<String, ImportantOriginInfo> mImportantOriginMap; Unused, right? Well, I guess you said the CL wasn't fully complete, but it would help reviewers to have a comment explaining how you'll use the data to facilitate the cleanup once the CL is complete, so we can get a clearer sense of how it will work. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:372: WebsitePreferenceBridge.fetchImportantOriginInfo(this); nit: Line break above. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:419: * @return if we needed to show the dialog. s/if/whether/ https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:453: if (!needed) { You can collapse these two into one line. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. Nit: 2016 (elsewhere too) https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:67: mClearAllDataSection.setVisibility(View.GONE); Maybe explain a little why? https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:75: BrowserStartupController.get(this, LibraryProcessType.PROCESS_BROWSER); It is not immediately clear to me why this and the rest of this function is needed. This may be obvious to people doing Android development day in day out, but at least I'd appreciate a comment. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:106: public void onClick(View v) { s/v/view/ https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:180: long importantSize = 0; This variable name looks a bit weird. Important Size... what is that? :) useageOfImportantSites perhaps? totalForImportantSites? https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:186: + ", " + site.getLocalStorageInfo().getSiteEngagementScore()); Remember to take this and all other debug log statements out when ready for final review. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:196: private class UnimportantDataClearer As you've seen, I have a mild aversion to Important-Unimportant being used as prefix. It always looks like it refers to what comes *after* Data (UnimportantClearer). Maybe it is just me. Don't have a concrete suggestion though, feel free to ignore. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:218: final int[] numLeft = new int[1]; Same here. Why is this an array, am I missing something? https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:313: getInfoForOrigins(); I guess this can be argued either way, but just to get some discussion going... This will actually clear potentially more sites than the user saw on screen (if a new site is added between showing the list and clicking clear, right)? Is that a concern? https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:315: final int[] numLeft = new int[1]; Why is this an array? https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:36: private final List<String> mImportantOrigins = new ArrayList<>(); Nit: Document. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:54: * @return the mImportantOrigins nit: This doesn't really add anything to what you get from reading the code. :) https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:125: // Important sites nit: End with period. https://codereview.chromium.org/1465363002/diff/100001/chrome/browser/android... File chrome/browser/android/preferences/website_preference_bridge.cc (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/browser/android... chrome/browser/android/preferences/website_preference_bridge.cc:725: std::vector<std::pair<GURL, double>> ranked_origins; Shouldn't this be named top_ranking_origins, or something? https://codereview.chromium.org/1465363002/diff/100001/chrome/browser/android... chrome/browser/android/preferences/website_preference_bridge.cc:741: ranked_origins.resize(std::min(ranked_origins.size(), 5u)); Um... this is non-obvious. Can you add a comment on line 736 explaining what you are doing, for example: // Produce a list of the top five highest ranked origins. ... or something. I also don't quite understand why we have a limit of 5, can you elaborate?
On 2016/03/02 23:10:49, dmurph wrote: > Hey guys, > > I emailed you earlier about this patch, I now have something for you to review > :) > > This isn't TOTALLY complete. I'm looking for guidance in the following areas: > 1. I'm putting the 'important origin' idea in two spots through the cpp bridge. > One pipes it through w/ the storage lookup, and one is separate and just > fetches the list of important origins (with extra data, but I'm only > currently > using the url). Any advice about this? Knowing if the origin is important or > not on the storage site helps w/ the clear all button, but I could just call > both instead of putting it into the current API. How expensive is getting the engagement score? If it's somewhat expensive, then you could add a boolean parameter to WebsiteSettingsBridge.fetchLocalStorageInfo() that controls whether to look up the engagement score. If the parameter is false, -1 (or some invalid value) would be returned for all the engagement scores. > 2. How I added dialogs, the UI in general (mostly similar to before). > 3. How do I test these things well? Full integration test style. > > The actual origin filtering isn't hooked up yet, pending this patch: > https://codereview.chromium.org/1741123002 > and the bridge bindings. > > Thanks so much! > > Daniel
On 2016/03/02 23:10:49, dmurph wrote: > Hey guys, > > I emailed you earlier about this patch, I now have something for you to review > :) > > This isn't TOTALLY complete. I'm looking for guidance in the following areas: > 1. I'm putting the 'important origin' idea in two spots through the cpp bridge. > One pipes it through w/ the storage lookup, and one is separate and just > fetches the list of important origins (with extra data, but I'm only > currently > using the url). Any advice about this? Knowing if the origin is important or > not on the storage site helps w/ the clear all button, but I could just call > both instead of putting it into the current API. How expensive is getting the engagement score? If it's somewhat expensive, then you could add a boolean parameter to WebsiteSettingsBridge.fetchLocalStorageInfo() that controls whether to look up the engagement score. If the parameter is false, -1 (or some invalid value) would be returned for all the engagement scores. > 2. How I added dialogs, the UI in general (mostly similar to before). > 3. How do I test these things well? Full integration test style. > > The actual origin filtering isn't hooked up yet, pending this patch: > https://codereview.chromium.org/1741123002 > and the bridge bindings. > > Thanks so much! > > Daniel
Description was changed from ========== [Storage] Android - ManageSpace UI, Important Origins, and CBD Dialog See the screenshots in the bug. This patch does the following: * Adds an activity for the manageSpaceActivity target in the AndroidManifest.xml which gives options for managing or clearing storage. NOTE: This is only hooked up in API >= KLP, as the API for clearing an application's app data isn't available until then. * We have a Dialog protecting the Reset App functionality. * Adds a Clear All button to the bottom of the Storage site settings view. This involves a layout that's almost an exact copy of the default layout, except we have one button. * Protected again by a dialog. * Adds a dialog to the Clear Browsing Data screen, that only shows up when the user has 'important' origins. * The user is prompted about these sites, and can uncheck them. STILL TODO: * Finalize the CPP side of important origins, where we want to check permissions and other things. * Hook up the BrowsingDataRemover so that we actually do the origin filtering BUG=560549,579763 ========== to ========== [Storage] Android - ManageSpace UI, Important Origins, and CBD Dialog See the screenshots in the bug. This patch does the following: * Adds an activity for the manageSpaceActivity target in the AndroidManifest.xml which gives options for managing or clearing storage. NOTE: This is only hooked up in API >= KLP, as the API for clearing an application's app data isn't available until then. * We have a Dialog protecting the Reset App functionality. * Adds a Clear All button to the bottom of the Storage site settings view. This involves a layout that's almost an exact copy of the default layout, except we have one button. * Protected again by a dialog. * Adds a dialog to the Clear Browsing Data screen, that only shows up when the user has 'important' origins. * The user is prompted about these sites, and can uncheck them. STILL TODO: * Write tests. * Possibly remove the ClearAllStorageAction now that it's small, and use the onActivityResult methodology. BUG=560549,579763 ==========
dmurph@chromium.org changed reviewers: + tedchoc@chromium.org, twellington@chromium.org - newt@chromium.org
Hello Ted, Finnur, and Theresa, This is the Android UI patch for the Important Sites feature. Part of it is behind a feature flag (the CBD dialog bit), and part is going out immediately (the ManageSpaceActivity and the ClearAll button in the storage preference). (go/importantstorage) I still need to remove the following from this patch: * Enabling the feature by default, that's just for testing * Logging And I still need to add: * Testing * Possibly remove the ClearAllStorageAction class now that it's simple, and use the onActivityResult methodology to return data there. I'm looking for feedback on: * General architecture & Android UI implementation, and * how to test stuff like this (I'm unsure how testing normally works here) Thanks so much! Daniel https://codereview.chromium.org/1465363002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:65: BrowserStartupController controller = On 2015/12/09 at 01:00:44, newt (away) wrote: > Currently, BrowserStartupController.startBrowserProcessesAsync() is closely tied to the code in AsyncInitializationActivity. > > One (untested) option is to extend AsyncInitializationActivity. Currently, only ChromeActivity (the activity that houses the main web browsing session) extends AsyncInitializationActivity. You may or may not run into bugs / trickiness when extending AsyncInitializationActivity. > > Alternatively, you can follow the pattern used in other activities that depend on native code: use ChromeApplication.startBrowserProcessesSync(). See Preferences.java for an example: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... Done. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:2: <!-- Copyright 2015 The Chromium Authors. All rights reserved. On 2016/03/03 at 14:15:42, Finnur wrote: > nit: 2016 Done. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:25: <!-- ======== Unimportant Storage Info ======== --> On 2016/03/03 at 14:15:42, Finnur wrote: > nit: Single quotes would probably help here: > 'Unimportant Storage' Info > > ... to differentiate from: > Unimportant 'Storage Info'. Done. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:68: On 2016/03/03 at 14:15:42, Finnur wrote: > Nit: Delete extra line here and below. Done. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:2: <!-- Copyright 2015 The Chromium Authors. All rights reserved. On 2016/03/03 at 14:15:42, Finnur wrote: > Nit: 2016 Done. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... File chrome/android/java/res/values-v19/constants.xml (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... chrome/android/java/res/values-v19/constants.xml:2: <!-- Copyright 2015 The Chromium Authors. All rights reserved. On 2016/03/03 at 14:15:43, Finnur wrote: > Nit: 2016 Done. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... chrome/android/java/res/values-v19/constants.xml:5: DO NOT TRANSLATE --> On 2016/03/03 at 14:15:43, Finnur wrote: > Do not translate? I don't recall seeing this convention before. Is this intentional? Removed. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... File chrome/android/java/res/values/constants.xml (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/re... chrome/android/java/res/values/constants.xml:5: DO NOT TRANSLATE --> On 2016/03/03 at 14:15:43, Finnur wrote: > Same question here. Done. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:186: private boolean mWaitingForOrigins = false; On 2016/03/03 at 14:15:43, Finnur wrote: > Probably clearer to add a private function WaitingForOrigins() that returns mSortedImportantOrigins == null. I cleaned this up a bit. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:188: private Map<String, ImportantOriginInfo> mImportantOriginMap; On 2016/03/03 at 14:15:43, Finnur wrote: > Unused, right? > > Well, I guess you said the CL wasn't fully complete, but it would help reviewers to have a comment explaining how you'll use the data to facilitate the cleanup once the CL is complete, so we can get a clearer sense of how it will work. REmoved. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:372: WebsitePreferenceBridge.fetchImportantOriginInfo(this); On 2016/03/03 at 14:15:43, Finnur wrote: > nit: Line break above. Done. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:419: * @return if we needed to show the dialog. On 2016/03/03 at 14:15:43, Finnur wrote: > s/if/whether/ Removed https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:453: if (!needed) { On 2016/03/03 at 14:15:43, Finnur wrote: > You can collapse these two into one line. REmoved https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:67: mClearAllDataSection.setVisibility(View.GONE); On 2016/03/03 at 14:15:43, Finnur wrote: > Maybe explain a little why? Done. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:75: BrowserStartupController.get(this, LibraryProcessType.PROCESS_BROWSER); On 2016/03/03 at 14:15:43, Finnur wrote: > It is not immediately clear to me why this and the rest of this function is needed. This may be obvious to people doing Android development day in day out, but at least I'd appreciate a comment. Done. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:106: public void onClick(View v) { On 2016/03/03 at 14:15:43, Finnur wrote: > s/v/view/ Done. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:180: long importantSize = 0; On 2016/03/03 at 14:15:43, Finnur wrote: > This variable name looks a bit weird. Important Size... what is that? :) > useageOfImportantSites perhaps? > totalForImportantSites? Done. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:186: + ", " + site.getLocalStorageInfo().getSiteEngagementScore()); On 2016/03/03 at 14:15:43, Finnur wrote: > Remember to take this and all other debug log statements out when ready for final review. Done. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:218: final int[] numLeft = new int[1]; On 2016/03/03 at 14:15:43, Finnur wrote: > Same here. Why is this an array, am I missing something? So, we need to make the argument final so that we can reference it in the anonymous class. I don't know a better way of doing this, and it looks pretty bad. any suggestions? https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:313: getInfoForOrigins(); On 2016/03/03 at 14:15:44, Finnur wrote: > I guess this can be argued either way, but just to get some discussion going... > > This will actually clear potentially more sites than the user saw on screen (if a new site is added between showing the list and clicking clear, right)? Is that a concern? No, that's fine. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:315: final int[] numLeft = new int[1]; On 2016/03/03 at 14:15:44, Finnur wrote: > Why is this an array? There's no way of having a mutable value referenced in the callback w/o having it be a final array. It has to be final. I don't know a better way of doing this :( https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:36: private final List<String> mImportantOrigins = new ArrayList<>(); On 2016/03/03 at 14:15:44, Finnur wrote: > Nit: Document. Removed. https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:54: * @return the mImportantOrigins On 2016/03/03 at 14:15:44, Finnur wrote: > nit: This doesn't really add anything to what you get from reading the code. :) Removed https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:125: // Important sites On 2016/03/03 at 14:15:44, Finnur wrote: > nit: End with period. Removed. https://codereview.chromium.org/1465363002/diff/100001/chrome/browser/android... File chrome/browser/android/preferences/website_preference_bridge.cc (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/browser/android... chrome/browser/android/preferences/website_preference_bridge.cc:725: std::vector<std::pair<GURL, double>> ranked_origins; On 2016/03/03 at 14:15:44, Finnur wrote: > Shouldn't this be named top_ranking_origins, or something? chagned. https://codereview.chromium.org/1465363002/diff/100001/chrome/browser/android... chrome/browser/android/preferences/website_preference_bridge.cc:741: ranked_origins.resize(std::min(ranked_origins.size(), 5u)); On 2016/03/03 at 14:15:44, Finnur wrote: > Um... this is non-obvious. Can you add a comment on line 736 explaining what you are doing, for example: > // Produce a list of the top five highest ranked origins. > > ... or something. > > I also don't quite understand why we have a limit of 5, can you elaborate? Moved to a separate class in previous patch.
Ted (and probably Theresa) is a better candidate than me for some of those questions (I don't consider myself an Android expert). https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:218: final int[] numLeft = new int[1]; I don't have a problem with this being |final|. I'm just confused as to why it has to be an array of int, especially since you never use anything but the first item in the array? Why not just final int numLeft? https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:315: final int[] numLeft = new int[1]; I guess this is a question for Ted. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:160: nit: Delete extra line (same below). https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... File chrome/android/java/res/values/constants.xml (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/values/constants.xml:6: <!-- Our manage space activity. Default pre-KLP. --> I'm drawing a blank... KLP is what again? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:139: /** Used for the onActivityResult pattern */ Nit: Missing punctuation (the period). But, I'd rather see the comment describe what the const is, than where it is used (which is prone to become out of date). https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:213: private ConfirmImportantSitesDialogFragment mConfirmImportantSitesDialog; nit: Document these two members. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:242: Log.i(TAG, "We're blacklisting some domains."); Nit: Remember to remove this debug statement before checking in. Same below. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:265: DialogOption.CLEAR_PASSWORDS, DialogOption.CLEAR_FORM_DATA}; Much less readable, in my opinion... https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:532: public static String strJoin(String[] aArr, String sSep) { Is this a debug function? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:170: Log.i(FRAGMENT_TAG, "Creating dialog"); nit: Debug statement. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:188: /* Transforms a domain into an origin for favicon lookup */ Nit: Missing period at end. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:193: private String[] mImportantDomains; nit: I'd go ahead and document those members and functions below and above. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:205: mFaviconCache = new FaviconCache(5); Why 5? Seems kind of low to me, but is there a method behind the selection of cache size here? How does this work in practice? Which sites generally end up in the cache after a run? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:219: } The code here looks identical to the function above. Maybe factor out to helper? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:245: // http://securityintelligence.com/new-vulnerability-android-framework-fragment-... I would move this comment above the function, then you won't have the >100 col problem.
Thanks! https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:218: final int[] numLeft = new int[1]; On 2016/04/27 at 13:49:15, Finnur wrote: > I don't have a problem with this being |final|. I'm just confused as to why it has to be an array of int, especially since you never use anything but the first item in the array? Why not just final int numLeft? Because then I couldn't modify it (as it's final) ;) https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:160: On 2016/04/27 at 13:49:15, Finnur wrote: > nit: Delete extra line (same below). Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... File chrome/android/java/res/values/constants.xml (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/values/constants.xml:6: <!-- Our manage space activity. Default pre-KLP. --> On 2016/04/27 at 13:49:15, Finnur wrote: > I'm drawing a blank... KLP is what again? Switched to KitKat ;) https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:139: /** Used for the onActivityResult pattern */ On 2016/04/27 at 13:49:15, Finnur wrote: > Nit: Missing punctuation (the period). > > But, I'd rather see the comment describe what the const is, than where it is used (which is prone to become out of date). Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:213: private ConfirmImportantSitesDialogFragment mConfirmImportantSitesDialog; On 2016/04/27 at 13:49:15, Finnur wrote: > nit: Document these two members. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:242: Log.i(TAG, "We're blacklisting some domains."); On 2016/04/27 at 13:49:15, Finnur wrote: > Nit: Remember to remove this debug statement before checking in. Same below. Yep, as mentioned in the email I still have to remove all of these. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:265: DialogOption.CLEAR_PASSWORDS, DialogOption.CLEAR_FORM_DATA}; On 2016/04/27 at 13:49:15, Finnur wrote: > Much less readable, in my opinion... Sorry, clang format fodder https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:532: public static String strJoin(String[] aArr, String sSep) { On 2016/04/27 at 13:49:15, Finnur wrote: > Is this a debug function? Yeah, I forgot to remove it. done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:170: Log.i(FRAGMENT_TAG, "Creating dialog"); On 2016/04/27 at 13:49:15, Finnur wrote: > nit: Debug statement. removed. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:188: /* Transforms a domain into an origin for favicon lookup */ On 2016/04/27 at 13:49:15, Finnur wrote: > Nit: Missing period at end. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:193: private String[] mImportantDomains; On 2016/04/27 at 13:49:15, Finnur wrote: > nit: I'd go ahead and document those members and functions below and above. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:205: mFaviconCache = new FaviconCache(5); On 2016/04/27 at 13:49:15, Finnur wrote: > Why 5? Seems kind of low to me, but is there a method behind the selection of cache size here? How does this work in practice? Which sites generally end up in the cache after a run? We currently only have 5 items max, so this will be sufficient. I can add a const in the class, and I'll increase it to 7 just in case. you want to have at least as many in the cache as are on screen. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:219: } On 2016/04/27 at 13:49:16, Finnur wrote: > The code here looks identical to the function above. Maybe factor out to helper? Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:245: // http://securityintelligence.com/new-vulnerability-android-framework-fragment-... On 2016/04/27 at 13:49:15, Finnur wrote: > I would move this comment above the function, then you won't have the >100 col problem. Done.
https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:328: android:configChanges="orientation|keyboardHidden|keyboard|screenSize|mcc|mnc" This should also handle sreenLayout and smallestScreenSize config changes so that the activity doesn't get restarted if it's resized in Android N multi-window (unless there's a good reason it can't handle those config changes). https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:6: <!-- Based on Play Services' manage_space_activity.xml layout file. --> Play Service's xml file probably isn't public. I would leave this comment out. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:17: android:layout_marginStart="16dip" Our xml files use "dp" rather than "dip" (they're interchangeable) https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:39: android:paddingTop="-1dip"> Negative padding is odd. The field right below has +6dp.. can you make that +5dp and drop this padding? Or just drop it all together? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:54: android:layout_height="1px" Is the dotted line only 1px on xxxhdpi devices as well? Or should this be 1dp? I couldn't figure out how to comment on the image files directly, so some other thoughts: - including 480px in the file name is odd - missing an asset for xxxhdpi - make sure to run optimize-png-files.sh on the new png's - can you you use a 1 dot + padding image & create an xml drawable w/ width 480dp android:tileMode set to repeat the dot rather than include a long dotted line? Or maybe, given where this dotted line occurs, the drawable could fill in all of the space between the text and MB instead of having a specific width. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:63: android:paddingTop="6dip" Define a style for the TextView's in this file since they share many parameters (paddingTop, textAppearance, layout_height, and layout_width) https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:134: android:paddingEnd="32dp" /> Declare a style for the buttons in this file to share. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:149: android:paddingTop="18dip" /> Why does this text view have so much padding compared to the others? Does it make sense to move the padding to its parent LinearLayout? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/select_dialog_listview.xml (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/select_dialog_listview.xml:6: <!-- Based on device/apps/common/res/layout/select_dialog.xml --> Again, I think this comment is unnecessary. If it's exactly the same, could you just use AlertDialog instead of declaring custom xml? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/select_dialog_listview.xml:11: android:layout_marginTop="5px" Use dp instead of px https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/select_dialog_listview.xml:12: android:cacheColorHint="@null" Are you intending for the list background to be transparent? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:7: platform_frameworks_base/core/res/res/layout/preference_list_fragment.xml This path doesn't exist in our code base. I would drop this line as well. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:19: android:paddingTop="0dip" replace dip with dp (here and below) https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:22: android:drawSelectorOnTop="false" This defaults to false. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:24: android:scrollbarAlwaysDrawVerticalTrack="true" /> Does this need to be set to true? If there's no vertical scroll, it seems odd to show the track anyway. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:33: /> Put this on the line above https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:45: /> Put this on the line above. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... File chrome/android/java/res/values-v19/constants.xml (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/values-v19/constants.xml:6: <string name="manage_space_activity" translatable="false">org.chromium.chrome.browser.preferences.website.ManageSpaceActivity</string> Why is this in a new constants.xml instead of values.xml? I think the translatable="false" is unnecssary since this is in chrome/android/java/res (based on the comment in res/values/strings.xml). https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... File chrome/android/java/res/values/constants.xml (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/values/constants.xml:6: <!-- Our manage space activity. Default pre-KLP. --> On 2016/04/27 13:49:15, Finnur wrote: > I'm drawing a blank... KLP is what again? I think KitKat, since the activity is for API level 19+. I would replace this string with pre-KitKat and expand this comment to include the info in the CL description about the API needed not being available pre-KitKat. I'm (pleasantly) surprised Android handles empty strings in the manifest correctly. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java:32: public static final String IMPORTANT_SITES_IN_CBD = "ImportantSitesInCBD"; I don't think CBD is a well known acronym. Maybe add a comment so it's clear. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:225: * @param options The dialog options whose corresponding data should be deleted. Add a @param for blacklistedDomains. nit: in this file the param is blacklistedDomains and in PreferServiceBridge it's blacklistDomains. It'd be nice if they matched. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:326: if (mSortedImportantDomains == null) { Should we call clearBrowsingData(getSelectedOptions(), null); directly if the experiment is disabled? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:453: PrefServiceBridge.fetchImportantSites(this); Should we only fetch if the experiment is enabled? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:558: clearBrowsingData(getSelectedOptions(), deselectedDomains); Can you add a comment explaining what selected vs de-selected means? It's not clear (without looking at other files) that the dialog prompts you to clear data for checked sites. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:97: viewHolder.textView.setChecked(true); Using a CheckedTextView for this seems weird. I think what you really want is a layout with a checkbox, text, and image. You can set drawables on TextView's (see android:drawableLeft). Or you may be able to do this with just the Android CheckBox class by adding a drawableRight and drawableLeft. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:131: private Drawable faviconDrawable(Bitmap image) { nit: getFaviconDrawable(Bitmap favicon)? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:138: private static class FaviconCache { What's the expected flow for this dialog? I would expect that the user opens it, clears data and then is done. If that's the case, won't the cache just be empty every time? What's the point of caching the favicons inside this class? Also, take a look at LargeIconBridge#getLargeIconForUrl() -- that class has built in cache. It's what we use for NTP thumbnails and the bookmarks manager. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:159: private static class ViewHolder { Consider using a different class name. ViewHolder already exists as a public class in RecyclerView.java. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:205: mFaviconCache = new FaviconCache(5); On 2016/04/27 13:49:15, Finnur wrote: > Why 5? Seems kind of low to me, but is there a method behind the selection of > cache size here? How does this work in practice? Which sites generally end up in > the cache after a run? The maximum important sites to fetch in pref_service_bridge.cc is 5 (see kMaxImportantSites in that file). This value needs to be declared as a constant. Ideally somewhere that's shared by Java and native. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:233: // We create our own listview, as AlertDialog doesn't let us set a message and a list nit: ListView https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:251: // OnClickListener is registered manually. This comment seems a little out of place here since it goes with .setPositiveButton and .setNegativeButton above. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ClearAllStorageAction.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ClearAllStorageAction.java:16: * This class handles showing clear storage dialogs to the user. I'm having trouble working out where certain classes are used (without tracing through all of the code). Can you add comments about when this class is used? (e.g. "This class handles showing a clear storage dialog in website's preferences." In this vein, in the future it'd be good to split this out into at least two CLs - one for the changes to Chrome's settings and one for the new ManageStorageActivity setting. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ClearAllStorageAction.java:22: public static interface CanClearStorage { void clearStorage(); } This is an odd interface name. Maybe ClearStorageDelegate? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ClearAllStorageAction.java:27: * @param storageClearer is used to either clear all website storage or clear unimportant Nit: capitalize "Is" https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ClearAllStorageAction.java:34: public void showClearAllDialog(final Activity activity, long totalWebsiteUsage) { javadocs for this method and the one below https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:35: * configured in the AndroidManifest.xml file by setting the android:manageSpaceActivity. nit: s/the AndroidManifest.xml file/AndroidManifest.xml s/the android:manageSpaceActivity/ android:manageSpaceActivity for the application. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:58: ChromeBrowserInitializer.getInstance(this).handleSynchronousStartup(); This should be done asynchronously, so that the UI doesn't block while Chrome is starting up. Cold start of Chrome can take a while. Doing this synchronously may cause ANRs. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:61: // This can only ever happen, if at all, when the activity is started from an Android While this is true for Preferences.java (where this was copied), it's probably not true for this class. If this fails, the browser process is in a bad way, but the user should be able to clear Chrome data without the browser process starting up. Is the browser process only needed to get local storage data size or is being used in other ways? We should have a fallback here rather than exiting. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:72: mSiteDataSizeText = (TextView) findViewById(R.id.site_data_storage_size_text); This is for all sites? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:89: // We actually shouldn't even be exposed if we're < KITKAT, as we only set this activity If we can't get into this situation, would it be bette to just assert that android.os.Build.VERSION_SDK_INT >= android.os.Build.VERSION_CODES.KITKAT? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:97: super.onResume(); Do we need to refetch this every time the activity is resumed? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:106: WebsitePermissionsFetcher fetcher = Does it make sense to have a new method in UnimportantSiteDataClearer#clearData() that handles this line and the next so that it's more obvious what's happening? A comment explaining that data will be cleared once sites that use storage are fetched would be helpful. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:137: if (android.os.Build.VERSION.SDK_INT < android.os.Build.VERSION_CODES.KITKAT) { Why are we checking this here again? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:182: if (!sites.contains(site)) { Is this check needed? HashSet should be smart enough to to add things that already exist in the set. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:186: } nit: Add a comment here? And maybe some spaces to break up the sections? This is personal preference, so just a suggestion. I like doing something like this: // 1. Add sites from origins. ... // 2. Add sites only accessible by host name. ... // 3. Calculate total storage size. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:219: } On 2016/04/27 13:49:16, Finnur wrote: > The code here looks identical to the function above. Maybe factor out to helper? Or even better, can this whole thing be factored out? e.g. Save the list of all sites and list of unimportant sites and just clear those when the corresponding button is clicked, rather than refetching site info. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:244: // If Preferences is exported, then it's vulnerable to a fragment injection exploit: Replace Preferences w/ the name of this class. Or elaborate. This isn't actually checking that Preferences doesn't get exported. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:317: // The goal is to refresh the info for origins again after we've cleared all of them. Expand on this comment explaining how we're achieving that goal. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:318: final int[] numLeft = new int[1]; numWebsitesLeft? This variable name is a little unclear. Also, why are we using an int[] with size 1 instead of just an int? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:347: getInfoForOrigins(); We fetch origins in onResume, correct? Do we need to fetch them again when the clear button is clicked? Same question for first call to getInfoForOrigins() in the clearStorage() method. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2257: Google Chrome storage There's probably not a great way to do this, but it'd be nice if we used the actual product name here so that Chrome Canary/Dev/Beta used the right string. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2269: Clear site storage? This string exists twice. Condense to one shared string. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2271: <message name="IDS_STORAGE_MANAGEMENT_CLEAR_UNIMPORTANT_DIALOG_TEXT" desc="Text of the reset app dialoag in the storage UI."> Is "reset app dialog" correct? Maybe "delete app data that Chrome doesn't think is important"? https://codereview.chromium.org/1465363002/diff/160001/chrome/browser/android... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/browser/android... chrome/browser/android/chrome_feature_list.cc:45: base::FEATURE_ENABLED_BY_DEFAULT}; This is launching? https://codereview.chromium.org/1465363002/diff/160001/chrome/browser/android... File chrome/browser/android/preferences/website_preference_bridge.cc (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/browser/android... chrome/browser/android/preferences/website_preference_bridge.cc:50: const int kMaxImportantSites = 10; Why is this different from the value in pref_service_bridge? https://codereview.chromium.org/1465363002/diff/160001/chrome/browser/android... chrome/browser/android/preferences/website_preference_bridge.cc:674: if (std::find(important_domains.begin(), important_domains.end(), Does 'important' info need to be added to each website or can the Java side get the list of important domains separately and compare the website's domain to that list? I know the list is expected to be short, but it seems like a waste to fetch it for every single website.
https://codereview.chromium.org/1465363002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:146: activityManager.clearApplicationUserData(); Lint will you probably complain about this. You may need an @TargetApi somewhere (maybe at the top of the activity itself).
Alrighty! Two outstanding issues/questions: 1. I still need to figure out the image tiling work. This should be done early next week. 2. In ManageSpaceActivity, I fetch the number in onResume because it doesn't update if you do app switching, which I do a lot. We can also fetch the numbers just in onCreate. It just gets stale if you app switch, or if you press 'back' from the Manage storage page. Actually, given that last case, I'd rather keep in in onResume so it's consistant. Thanks for your feedback! It looks waaaay better. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:328: android:configChanges="orientation|keyboardHidden|keyboard|screenSize|mcc|mnc" On 2016/04/27 at 21:10:57, Theresa Wellington wrote: > This should also handle sreenLayout and smallestScreenSize config changes so that the activity doesn't get restarted if it's resized in Android N multi-window (unless there's a good reason it can't handle those config changes). Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:6: <!-- Based on Play Services' manage_space_activity.xml layout file. --> On 2016/04/27 at 21:10:57, Theresa Wellington wrote: > Play Service's xml file probably isn't public. I would leave this comment out. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:17: android:layout_marginStart="16dip" On 2016/04/27 at 21:10:57, Theresa Wellington wrote: > Our xml files use "dp" rather than "dip" (they're interchangeable) Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:39: android:paddingTop="-1dip"> On 2016/04/27 at 21:10:57, Theresa Wellington wrote: > Negative padding is odd. The field right below has +6dp.. can you make that +5dp and drop this padding? Or just drop it all together? Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:54: android:layout_height="1px" On 2016/04/27 at 21:10:57, Theresa Wellington wrote: > Is the dotted line only 1px on xxxhdpi devices as well? Or should this be 1dp? > > > I couldn't figure out how to comment on the image files directly, so some other thoughts: > - including 480px in the file name is odd > - missing an asset for xxxhdpi > - make sure to run optimize-png-files.sh on the new png's > - can you you use a 1 dot + padding image & create an xml drawable w/ width 480dp android:tileMode set to repeat the dot rather than include a long dotted line? Or maybe, given where this dotted line occurs, the drawable could fill in all of the space between the text and MB instead of having a specific width. Ok, I'm looking into getting this right. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:63: android:paddingTop="6dip" On 2016/04/27 at 21:10:57, Theresa Wellington wrote: > Define a style for the TextView's in this file since they share many parameters (paddingTop, textAppearance, layout_height, and layout_width) Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:134: android:paddingEnd="32dp" /> On 2016/04/27 at 21:10:57, Theresa Wellington wrote: > Declare a style for the buttons in this file to share. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:149: android:paddingTop="18dip" /> On 2016/04/27 at 21:10:57, Theresa Wellington wrote: > Why does this text view have so much padding compared to the others? Does it make sense to move the padding to its parent LinearLayout? Done. It's for making it a little separate. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/select_dialog_listview.xml (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/select_dialog_listview.xml:6: <!-- Based on device/apps/common/res/layout/select_dialog.xml --> On 2016/04/27 at 21:10:57, Theresa Wellington wrote: > Again, I think this comment is unnecessary. If it's exactly the same, could you just use AlertDialog instead of declaring custom xml? It's the same but it's not haha. I'm using a list view here, instead of special view class, and I can't use the AlertDialog built in list features as it doesn't let me include a 'message' field then. Ugh android is annoying. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/select_dialog_listview.xml:11: android:layout_marginTop="5px" On 2016/04/27 at 21:10:57, Theresa Wellington wrote: > Use dp instead of px Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/select_dialog_listview.xml:12: android:cacheColorHint="@null" On 2016/04/27 at 21:10:57, Theresa Wellington wrote: > Are you intending for the list background to be transparent? Copied. I'll remove. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:7: platform_frameworks_base/core/res/res/layout/preference_list_fragment.xml On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > This path doesn't exist in our code base. I would drop this line as well. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:19: android:paddingTop="0dip" On 2016/04/27 at 21:10:57, Theresa Wellington wrote: > replace dip with dp (here and below) Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:22: android:drawSelectorOnTop="false" On 2016/04/27 at 21:10:57, Theresa Wellington wrote: > This defaults to false. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:24: android:scrollbarAlwaysDrawVerticalTrack="true" /> On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > Does this need to be set to true? If there's no vertical scroll, it seems odd to show the track anyway. I don't know. Seems to still work. This file was copied. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:33: /> On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > Put this on the line above Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:45: /> On 2016/04/27 at 21:10:57, Theresa Wellington wrote: > Put this on the line above. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... File chrome/android/java/res/values-v19/constants.xml (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/re... chrome/android/java/res/values-v19/constants.xml:6: <string name="manage_space_activity" translatable="false">org.chromium.chrome.browser.preferences.website.ManageSpaceActivity</string> On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > Why is this in a new constants.xml instead of values.xml? I think the translatable="false" is unnecssary since this is in chrome/android/java/res (based on the comment in res/values/strings.xml). Frankly I don't even know the difference. I can use values.xml, that's fine. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java:32: public static final String IMPORTANT_SITES_IN_CBD = "ImportantSitesInCBD"; On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > I don't think CBD is a well known acronym. Maybe add a comment so it's clear. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:326: if (mSortedImportantDomains == null) { On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > Should we call clearBrowsingData(getSelectedOptions(), null); directly if the experiment is disabled? Yes. Thanks, cleaned up. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:453: PrefServiceBridge.fetchImportantSites(this); On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > Should we only fetch if the experiment is enabled? Yep, thanks, fixed. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:558: clearBrowsingData(getSelectedOptions(), deselectedDomains); On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > Can you add a comment explaining what selected vs de-selected means? It's not clear (without looking at other files) that the dialog prompts you to clear data for checked sites. I added a comment here and on the tag. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:97: viewHolder.textView.setChecked(true); On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > Using a CheckedTextView for this seems weird. I think what you really want is a layout with a checkbox, text, and image. You can set drawables on TextView's (see android:drawableLeft). Or you may be able to do this with just the Android CheckBox class by adding a drawableRight and drawableLeft. So this acts weird because it's part of a dialog in a list that supports checking. I'm not sure why, but playing with these values did weird things, like having two checkboxes on the left and other strange things. This ended up working nicely, is it alright if we keep it this way? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:131: private Drawable faviconDrawable(Bitmap image) { On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > nit: getFaviconDrawable(Bitmap favicon)? Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:138: private static class FaviconCache { On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > What's the expected flow for this dialog? > > I would expect that the user opens it, clears data and then is done. If that's the case, won't the cache just be empty every time? What's the point of caching the favicons inside this class? > > Also, take a look at LargeIconBridge#getLargeIconForUrl() -- that class has built in cache. It's what we use for NTP thumbnails and the bookmarks manager. Yes, it'll be empty. I'm not using that class (I don't want a large icon), and if I used it it would be the same case as now -- it's one cache per instance. This is mostly here because I noticed flickering, especially if there was scrolling, of the icons. The list view also likes asking for a ton of the same list row, not sure why. So this helps cut down on repeated loads of the favicon. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:159: private static class ViewHolder { On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > Consider using a different class name. ViewHolder already exists as a public class in RecyclerView.java. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:205: mFaviconCache = new FaviconCache(5); On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > On 2016/04/27 13:49:15, Finnur wrote: > > Why 5? Seems kind of low to me, but is there a method behind the selection of > > cache size here? How does this work in practice? Which sites generally end up in > > the cache after a run? > > The maximum important sites to fetch in pref_service_bridge.cc is 5 (see kMaxImportantSites in that file). > > This value needs to be declared as a constant. Ideally somewhere that's shared by Java and native. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:233: // We create our own listview, as AlertDialog doesn't let us set a message and a list On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > nit: ListView Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:251: // OnClickListener is registered manually. On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > This comment seems a little out of place here since it goes with .setPositiveButton and .setNegativeButton above. Removed, it was old. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ClearAllStorageAction.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ClearAllStorageAction.java:16: * This class handles showing clear storage dialogs to the user. On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > I'm having trouble working out where certain classes are used (without tracing through all of the code). Can you add comments about when this class is used? (e.g. "This class handles showing a clear storage dialog in website's preferences." > > In this vein, in the future it'd be good to split this out into at least two CLs - one for the changes to Chrome's settings and one for the new ManageStorageActivity setting. I removed this. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ClearAllStorageAction.java:22: public static interface CanClearStorage { void clearStorage(); } On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > This is an odd interface name. Maybe ClearStorageDelegate? Removed. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ClearAllStorageAction.java:27: * @param storageClearer is used to either clear all website storage or clear unimportant On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > Nit: capitalize "Is" REmoved. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ClearAllStorageAction.java:34: public void showClearAllDialog(final Activity activity, long totalWebsiteUsage) { On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > javadocs for this method and the one below Removed. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:35: * configured in the AndroidManifest.xml file by setting the android:manageSpaceActivity. On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > nit: s/the AndroidManifest.xml file/AndroidManifest.xml > s/the android:manageSpaceActivity/ android:manageSpaceActivity for the application. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:58: ChromeBrowserInitializer.getInstance(this).handleSynchronousStartup(); On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > This should be done asynchronously, so that the UI doesn't block while Chrome is starting up. Cold start of Chrome can take a while. Doing this synchronously may cause ANRs. I was doing this with BrowserStartupController#startBrowserProcessesAsync, but that was kind of flaky (it worked initially, but now it doesn't work). Newt said to use this, as it is what Preferences does I think? Let me know if you know a better way of doing this, I can't find a common pattern. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:61: // This can only ever happen, if at all, when the activity is started from an Android On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > While this is true for Preferences.java (where this was copied), it's probably not true for this class. If this fails, the browser process is in a bad way, but the user should be able to clear Chrome data without the browser process starting up. Is the browser process only needed to get local storage data size or is being used in other ways? We should have a fallback here rather than exiting. Yeah we need it to have functionality for the first two buttons (clear unimportant, manage storage) to work. We could just disable those and still have the third one - Clear all data - enabled. How does that sound? I did that. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:72: mSiteDataSizeText = (TextView) findViewById(R.id.site_data_storage_size_text); On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > This is for all sites? Yes. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:89: // We actually shouldn't even be exposed if we're < KITKAT, as we only set this activity On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > If we can't get into this situation, would it be bette to just assert that android.os.Build.VERSION_SDK_INT >= android.os.Build.VERSION_CODES.KITKAT? Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:97: super.onResume(); On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > Do we need to refetch this every time the activity is resumed? If you're using the app switcher, then this works correctly. Do you have any other opinions? I can just do this on activity create. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:106: WebsitePermissionsFetcher fetcher = On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > Does it make sense to have a new method in UnimportantSiteDataClearer#clearData() that handles this line and the next so that it's more obvious what's happening? A comment explaining that data will be cleared once sites that use storage are fetched would be helpful. Huh. Ok, done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:137: if (android.os.Build.VERSION.SDK_INT < android.os.Build.VERSION_CODES.KITKAT) { On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > Why are we checking this here again? Safety. Removed. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:182: if (!sites.contains(site)) { On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > Is this check needed? HashSet should be smart enough to to add things that already exist in the set. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:186: } On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > nit: Add a comment here? And maybe some spaces to break up the sections? This is personal preference, so just a suggestion. I like doing something like this: > > // 1. Add sites from origins. > ... > > // 2. Add sites only accessible by host name. > ... > > // 3. Calculate total storage size. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:244: // If Preferences is exported, then it's vulnerable to a fragment injection exploit: On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > Replace Preferences w/ the name of this class. Or elaborate. This isn't actually checking that Preferences doesn't get exported. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:317: // The goal is to refresh the info for origins again after we've cleared all of them. On 2016/04/27 at 21:11:00, Theresa Wellington wrote: > Expand on this comment explaining how we're achieving that goal. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:318: final int[] numLeft = new int[1]; On 2016/04/27 at 21:11:00, Theresa Wellington wrote: > numWebsitesLeft? This variable name is a little unclear. > > Also, why are we using an int[] with size 1 instead of just an int? Please tell me there's a better way to do this haha. I need to pass a modifiable fariable to the inner anon class here so I can keep track of the number or requests I have left. In order to use it, it has to be 'final'. In order for me to be able to modify it, it has to be an array (as I can't modify a final int). https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:347: getInfoForOrigins(); On 2016/04/27 at 21:11:00, Theresa Wellington wrote: > We fetch origins in onResume, correct? Do we need to fetch them again when the clear button is clicked? Same question for first call to getInfoForOrigins() in the clearStorage() method. Yeah, probably not. I was just trying to be thorough, but I think it won't make sense if our list is different than what we clear. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2257: Google Chrome storage On 2016/04/27 at 21:11:00, Theresa Wellington wrote: > There's probably not a great way to do this, but it'd be nice if we used the actual product name here so that Chrome Canary/Dev/Beta used the right string. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2269: Clear site storage? On 2016/04/27 at 21:11:00, Theresa Wellington wrote: > This string exists twice. Condense to one shared string. Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2271: <message name="IDS_STORAGE_MANAGEMENT_CLEAR_UNIMPORTANT_DIALOG_TEXT" desc="Text of the reset app dialoag in the storage UI."> On 2016/04/27 at 21:11:00, Theresa Wellington wrote: > Is "reset app dialog" correct? Maybe "delete app data that Chrome doesn't think is important"? Done. https://codereview.chromium.org/1465363002/diff/160001/chrome/browser/android... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/browser/android... chrome/browser/android/chrome_feature_list.cc:45: base::FEATURE_ENABLED_BY_DEFAULT}; On 2016/04/27 at 21:11:00, Theresa Wellington wrote: > This is launching? No, I just have this enabled for testing. Sorry! https://codereview.chromium.org/1465363002/diff/160001/chrome/browser/android... File chrome/browser/android/preferences/website_preference_bridge.cc (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/browser/android... chrome/browser/android/preferences/website_preference_bridge.cc:50: const int kMaxImportantSites = 10; On 2016/04/27 at 21:11:00, Theresa Wellington wrote: > Why is this different from the value in pref_service_bridge? I chatted with Dru (the PM on this) and we decided we were cool with a larger value here because the aren't being shown to the user, they're just getting protected from the 'clear all unimportant' usage. I'll document. https://codereview.chromium.org/1465363002/diff/160001/chrome/browser/android... chrome/browser/android/preferences/website_preference_bridge.cc:674: if (std::find(important_domains.begin(), important_domains.end(), On 2016/04/27 at 21:11:00, Theresa Wellington wrote: > Does 'important' info need to be added to each website or can the Java side get the list of important domains separately and compare the website's domain to that list? > > I know the list is expected to be short, but it seems like a waste to fetch it for every single website. We don't fetch it for every website (we just use this callback once), and we're iterating a small vector. https://codereview.chromium.org/1465363002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:146: activityManager.clearApplicationUserData(); On 2016/04/27 at 22:27:24, Theresa Wellington wrote: > Lint will you probably complain about this. You may need an @TargetApi somewhere (maybe at the top of the activity itself). Cool, thanks, I wasn't sure how to get rid of that error.
Also, I'll start looking in the corresponding javatests directories to figure out how I'm going to be doing that.
https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:58: ChromeBrowserInitializer.getInstance(this).handleSynchronousStartup(); On 2016/04/29 23:53:49, dmurph wrote: > On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > > This should be done asynchronously, so that the UI doesn't block while Chrome > is starting up. Cold start of Chrome can take a while. Doing this synchronously > may cause ANRs. > > I was doing this with BrowserStartupController#startBrowserProcessesAsync, but > that was kind of flaky (it worked initially, but now it doesn't work). Newt said > to use this, as it is what Preferences does I think? Let me know if you know a > better way of doing this, I can't find a common pattern. startBrowserProcessesAsync is what you want. The sync startup is fine for preferences since it is really hard to get to preferences w/o first going through Chrome, which will trigger the browser process initialization already. Here, we can trigger this from a general Android entry point that has no previous Chrome paths, so I think the async thing is much more important. In general, I want to rid the sync path entirely for UI components, but first we need to stop adding more. What about it was flaky? Also, I think there are a couple ways you can handle the async path: 1.) <the easy less visually pleasant approach> Move everything after your super.onCreate call into the StartupCallback#onSuccess. 2.) Inflate your UI, disable all your buttons initially. Wait for onSuccess and enable them and add your click listeners. Then in onFailure, you do what you suggest in your next comment and only enable the third/non-browser process requiring button.
https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:97: viewHolder.textView.setChecked(true); On 2016/04/29 23:53:48, dmurph wrote: > On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > > Using a CheckedTextView for this seems weird. I think what you really want is > a layout with a checkbox, text, and image. You can set drawables on TextView's > (see android:drawableLeft). Or you may be able to do this with just the Android > CheckBox class by adding a drawableRight and drawableLeft. > > So this acts weird because it's part of a dialog in a list that supports > checking. I'm not sure why, but playing with these values did weird things, like > having two checkboxes on the left and other strange things. This ended up > working nicely, is it alright if we keep it this way? Instead of using select_dialog_multichoice_material, we could use custom xml to define a horizontal LinearLayout with a CheckBox and an ImageView if drawableRight and drawableLeft don't work. That feels a lot more natural to me than using a CheckedTextView. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:138: private static class FaviconCache { On 2016/04/29 23:53:48, dmurph wrote: > On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > > What's the expected flow for this dialog? > > > > I would expect that the user opens it, clears data and then is done. If that's > the case, won't the cache just be empty every time? What's the point of caching > the favicons inside this class? > > > > Also, take a look at LargeIconBridge#getLargeIconForUrl() -- that class has > built in cache. It's what we use for NTP thumbnails and the bookmarks manager. > > Yes, it'll be empty. I'm not using that class (I don't want a large icon), and > if I used it it would be the same case as now -- it's one cache per instance. > This is mostly here because I noticed flickering, especially if there was > scrolling, of the icons. The list view also likes asking for a ton of the same > list row, not sure why. So this helps cut down on repeated loads of the favicon. Caching for scrolling makes sense. The favicons we get for bookmarks end up being displayed at 16dp (the same as default_favicon_size used in this class). The size you're requesting, 16dp is large in terms of pixels on devices with higher density. Any page that hasn't been visited on the current device will most likely only have a 16px favicon available, which is the minimum we use for bookmarks. I can send you some screenshots of what this looks in bookmarks when the large size is available vs not available. I think using the regular getLocalFaviconImageForUrl() vs LargeIconService will end up looking the same assuming all of the sites have been visited on the local device. The code is a bit different; I'm not sure which one performs better, but since you're only getting 5 sites it probably doesn't matter. The nice thing about the favicon service is that it provides a fallback color to use if the favicon isn't available at the minimum size requested or above. As an aside, the comment for FaviconHelper#getLocalFaviconImageForURL says it only retrieves favicons for pages the user has visited on the current device. Skimming through the code, I'm not sure that's actually true. Will the favicons shown here always be for pages the user visited locally? Or if I'm signed in does my desktop browsing affect the important sites calculation? https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:61: // This can only ever happen, if at all, when the activity is started from an Android On 2016/04/29 23:53:49, dmurph wrote: > On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > > While this is true for Preferences.java (where this was copied), it's probably > not true for this class. If this fails, the browser process is in a bad way, but > the user should be able to clear Chrome data without the browser process > starting up. Is the browser process only needed to get local storage data size > or is being used in other ways? We should have a fallback here rather than > exiting. > > Yeah we need it to have functionality for the first two buttons (clear > unimportant, manage storage) to work. We could just disable those and still have > the third one - Clear all data - enabled. How does that sound? I did that. That sounds great. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:97: super.onResume(); On 2016/04/29 23:53:49, dmurph wrote: > On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > > Do we need to refetch this every time the activity is resumed? > > If you're using the app switcher, then this works correctly. Do you have any > other opinions? I can just do this on activity create. onResume() may be triggered a lot in multi-window -- this is probably a side case that we don't really need to worry about, so I'm okay with leaving the number refresh here. That way if the user leaves this activity and does something in Chrome that changes the size calculations, the numbers get updated correctly. https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:106: WebsitePermissionsFetcher fetcher = On 2016/04/29 23:53:49, dmurph wrote: > On 2016/04/27 at 21:10:59, Theresa Wellington wrote: > > Does it make sense to have a new method in > UnimportantSiteDataClearer#clearData() that handles this line and the next so > that it's more obvious what's happening? A comment explaining that data will be > cleared once sites that use storage are fetched would be helpful. > > Huh. Ok, done. Thanks! https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2257: Google Chrome storage On 2016/04/29 23:53:49, dmurph wrote: > On 2016/04/27 at 21:11:00, Theresa Wellington wrote: > > There's probably not a great way to do this, but it'd be nice if we used the > actual product name here so that Chrome Canary/Dev/Beta used the right string. > > Done. Thanks for figuring this out. I learned something today. https://codereview.chromium.org/1465363002/diff/160001/chrome/browser/android... File chrome/browser/android/preferences/website_preference_bridge.cc (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/browser/android... chrome/browser/android/preferences/website_preference_bridge.cc:674: if (std::find(important_domains.begin(), important_domains.end(), On 2016/04/29 23:53:49, dmurph wrote: > On 2016/04/27 at 21:11:00, Theresa Wellington wrote: > > Does 'important' info need to be added to each website or can the Java side > get the list of important domains separately and compare the website's domain to > that list? > > > > I know the list is expected to be short, but it seems like a waste to fetch it > for every single website. > > We don't fetch it for every website (we just use this callback once), and we're > iterating a small vector. sg. https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:328: android:configChanges="smallestScreenSize|screenSize|orientation|keyboardHidden|keyboard|screenSize|mcc|mnc" nit: move smallestScreenSize|screenSize to the end to match the rest of the configChanges in this file. https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:35: android:paddingTop="-1dp"> I'm still not a fan of the negative top padding. How does this look with out it? Are the designers okay with leaving it out? https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:43: android:src="@drawable/dotted_line_480px" This ImageView could use a declared style as well. Or a separate layout that can be used with <include> since it's the exact same ImageView is used 3 times. http://developer.android.com/training/improving-layouts/reusing-layouts.html https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/re... File chrome/android/java/res/values/constants.xml (right): https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/constants.xml:1: <?xml version="1.0" encoding="utf-8"?> This file can be deleted now that the string is in values.xml https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/re... File chrome/android/java/res/values/values.xml (right): https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/values.xml:59: <!-- Our manage space activity. Default pre-KLP to be nothing. --> s/pre-KLP/pre-KitKat https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:39: @TargetApi(value = android.os.Build.VERSION_CODES.KITKAT) nit: import android.os.Build and make this line just: @TargetApi(Build.VERSION_CODES.KITKAT) https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:165: /** This function takes sites by origin and host, and adds them all to one set. */ nit: remove the comma after host since these aren't two independent clauses
Alrighty, I still need tests, but PTAL and let me know what you think about the new init path. Based on my manual testing, the buttons definitely are visible and disabled for a second or two, so it correctly working Asyc. I imagine that it might be possible to use a lighter-weight method of initializing the browser process than extending that activity, but I couldn't get something simple working (see my email). Thanks! Daniel https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:328: android:configChanges="smallestScreenSize|screenSize|orientation|keyboardHidden|keyboard|screenSize|mcc|mnc" On 2016/05/03 at 00:04:27, Theresa Wellington wrote: > nit: move smallestScreenSize|screenSize to the end to match the rest of the configChanges in this file. Done. https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:35: android:paddingTop="-1dp"> On 2016/05/03 at 00:04:27, Theresa Wellington wrote: > I'm still not a fan of the negative top padding. How does this look with out it? Are the designers okay with leaving it out? Whoops, I thought I removed this. My bad. https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:43: android:src="@drawable/dotted_line_480px" On 2016/05/03 at 00:04:27, Theresa Wellington wrote: > This ImageView could use a declared style as well. Or a separate layout that can be used with <include> since it's the exact same ImageView is used 3 times. > > http://developer.android.com/training/improving-layouts/reusing-layouts.html Done. https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/re... File chrome/android/java/res/values/constants.xml (right): https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/constants.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2016/05/03 at 00:04:27, Theresa Wellington wrote: > This file can be deleted now that the string is in values.xml Done. https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/re... File chrome/android/java/res/values/values.xml (right): https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/values.xml:59: <!-- Our manage space activity. Default pre-KLP to be nothing. --> On 2016/05/03 at 00:04:27, Theresa Wellington wrote: > s/pre-KLP/pre-KitKat Done. https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:39: @TargetApi(value = android.os.Build.VERSION_CODES.KITKAT) On 2016/05/03 at 00:04:27, Theresa Wellington wrote: > nit: import android.os.Build and make this line just: > @TargetApi(Build.VERSION_CODES.KITKAT) Done. https://codereview.chromium.org/1465363002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:165: /** This function takes sites by origin and host, and adds them all to one set. */ On 2016/05/03 at 00:04:27, Theresa Wellington wrote: > nit: remove the comma after host since these aren't two independent clauses Thanks!
Async working. next up, tests.
Thank you for all of the changes! I nee to do another end-to-end pass but it's looking good overall. https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/re... File chrome/android/java/res/layout/dotted_line_span.xml (right): https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/re... chrome/android/java/res/layout/dotted_line_span.xml:8: android:src="@drawable/dotted_line_480px" We should drop the "480px" from the name. How big are all of the png's for the dotted_line pngs combined? https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:79: // the chromium process to boot up. nit: capitalize Chromium https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:95: } Probably not necessary with this CL, but in the future we could also implement onStartupFailure and change something in the UI to indicate that we tried to get storage #'s and failed. https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:118: if (mIsNativeInitialized) refreshStorageNumbers(); I'd switch to call super.onResume() first unless there's a reason for it being called second.
I believe I have addressed everything. I still need to make a test for ManageSpaceActivity, but other than that I think I'm good. I also need to email the manual testing team, what was their email? chrome-ee? Daniel https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:97: viewHolder.textView.setChecked(true); On 2016/05/03 at 00:04:27, Theresa Wellington wrote: > On 2016/04/29 23:53:48, dmurph wrote: > > On 2016/04/27 at 21:10:58, Theresa Wellington wrote: > > > Using a CheckedTextView for this seems weird. I think what you really want is > > a layout with a checkbox, text, and image. You can set drawables on TextView's > > (see android:drawableLeft). Or you may be able to do this with just the Android > > CheckBox class by adding a drawableRight and drawableLeft. > > > > So this acts weird because it's part of a dialog in a list that supports > > checking. I'm not sure why, but playing with these values did weird things, like > > having two checkboxes on the left and other strange things. This ended up > > working nicely, is it alright if we keep it this way? > > Instead of using select_dialog_multichoice_material, we could use custom xml to define a horizontal LinearLayout with a CheckBox and an ImageView if drawableRight and drawableLeft don't work. That feels a lot more natural to me than using a CheckedTextView. I explored this for a bit, and I'd personally prefer keeping the CheckedTextView, as it's less code and does exactly what I want. I get automatic touch-anything-toggle-checkbox behavior as well. If you feel strongly I'll change it, but I'd like to keep it this way. https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/re... File chrome/android/java/res/layout/dotted_line_span.xml (right): https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/re... chrome/android/java/res/layout/dotted_line_span.xml:8: android:src="@drawable/dotted_line_480px" On 2016/05/04 at 22:24:00, Theresa Wellington wrote: > We should drop the "480px" from the name. How big are all of the png's for the dotted_line pngs combined? 2kb https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:79: // the chromium process to boot up. On 2016/05/04 at 22:24:00, Theresa Wellington wrote: > nit: capitalize Chromium Done. https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:95: } On 2016/05/04 at 22:24:00, Theresa Wellington wrote: > Probably not necessary with this CL, but in the future we could also implement onStartupFailure and change something in the UI to indicate that we tried to get storage #'s and failed. Done. https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:118: if (mIsNativeInitialized) refreshStorageNumbers(); On 2016/05/04 at 22:24:00, Theresa Wellington wrote: > I'd switch to call super.onResume() first unless there's a reason for it being called second. Done.
https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/re... File chrome/android/java/res/layout/dotted_line_span.xml (right): https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/re... chrome/android/java/res/layout/dotted_line_span.xml:8: android:src="@drawable/dotted_line_480px" On 2016/05/05 21:53:29, dmurph wrote: > On 2016/05/04 at 22:24:00, Theresa Wellington wrote: > > We should drop the "480px" from the name. How big are all of the png's for the > dotted_line pngs combined? > > 2kb sg. Earlier I had suggested looking at using tileMode=repeat on a <bitmap> drawable, but I don't think 2kb is worth it. https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:340: showProgressDialog(); What happens now if the preference is clicked and we're still waiting on important sites? https://codereview.chromium.org/1465363002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:61: mCornerRadius = resources.getDimensionPixelSize(R.dimen.bookmark_item_corner_radius); I like trying to minimize the # of dimens we declare, but if bookmarks ever changes it's unlikely their dimensions, it's unlikely the bookmarks engineers will check to make sure it looks right in this dialog as well. I think it's best to rename them so it's really obvious they're being used for more than just bookmarks or just duplicate the dimens. https://codereview.chromium.org/1465363002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:123: viewHolder.textView.setCheckMarkDrawable(image); Can we use a custom layout that doesn't rely use a CheckMarkDrawable? https://codereview.chromium.org/1465363002/diff/260001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java (right): https://codereview.chromium.org/1465363002/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:476: ThreadUtils.runOnUiThreadBlocking(new Runnable() { nit: add a comment here, something like // Open the clear browsing data preference and click the clear button. https://codereview.chromium.org/1465363002/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:489: // Check that we are shown. nit: Check that the important sites dialog is shown. https://codereview.chromium.org/1465363002/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:526: // Check that it's in the set correctly. nit: Check that facebook.com is in the set of deselected domains. https://codereview.chromium.org/1465363002/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:561: }); Are there things we could check to see if browsing data actually got cleared? Or would that be difficult to do here?
https://codereview.chromium.org/1465363002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:123: viewHolder.textView.setCheckMarkDrawable(image); On 2016/05/06 17:05:19, Theresa Wellington wrote: > Can we use a custom layout that doesn't rely use a CheckMarkDrawable? * ... that doesn't use a CheckMarkDrawable?
Description was changed from
==========
[Storage] Android - ManageSpace UI, Important Origins, and CBD Dialog
See the screenshots in the bug.
This patch does the following:
* Adds an activity for the manageSpaceActivity target in the
AndroidManifest.xml which gives options for managing or clearing
storage.
NOTE: This is only hooked up in API >= KLP, as the API for
clearing an application's app data isn't available until then.
* We have a Dialog protecting the Reset App functionality.
* Adds a Clear All button to the bottom of the Storage site settings
view. This involves a layout that's almost an exact copy of the
default layout, except we have one button.
* Protected again by a dialog.
* Adds a dialog to the Clear Browsing Data screen, that only shows
up when the user has 'important' origins.
* The user is prompted about these sites, and can uncheck them.
STILL TODO:
* Write tests.
* Possibly remove the ClearAllStorageAction now that it's small, and
use the onActivityResult methodology.
BUG=560549,579763
==========
to
==========
[Storage] Android - ManageSpace UI, Important Origins, and CBD Dialog
See the screenshots in the bug.
This patch does the following:
* Adds an activity for the manageSpaceActivity target in the
AndroidManifest.xml which gives options for managing or clearing
storage.
NOTE: This is only hooked up in API >= KLP, as the API for
clearing an application's app data isn't available until then.
* We have a Dialog protecting the Reset App functionality.
* Adds a Clear All button to the bottom of the Storage site settings
view. This involves a layout that's almost an exact copy of the
default layout, except we have one button.
* Protected again by a dialog.
* Adds a dialog to the Clear Browsing Data screen, that only shows
up when the user has 'important' origins.
* The user is prompted about these sites, and can uncheck them.
* Test is included, which also double checks that cookies are
not deleted.
STILL TODO:
* Write ManageStoreActivity test.
BUG=560549,579763
==========
dmurph@chromium.org changed reviewers: - finnur@chromium.org
Hey all! I added the row layout as asked, let me know if I should change it at all. The test now checks that cookies are deleted/not deleted! Yay! Hooray! Last thing I need to do is create a test for the manage space activity. I'm aiming for the 52 branch point, so PTAL with that in mid (I think mid-next-week). Thanks! Daniel https://codereview.chromium.org/1465363002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:61: mCornerRadius = resources.getDimensionPixelSize(R.dimen.bookmark_item_corner_radius); On 2016/05/06 at 17:05:19, Theresa Wellington wrote: > I like trying to minimize the # of dimens we declare, but if bookmarks ever changes it's unlikely their dimensions, it's unlikely the bookmarks engineers will check to make sure it looks right in this dialog as well. I think it's best to rename them so it's really obvious they're being used for more than just bookmarks or just duplicate the dimens. Done. https://codereview.chromium.org/1465363002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:123: viewHolder.textView.setCheckMarkDrawable(image); On 2016/05/06 at 17:05:55, Theresa Wellington wrote: > On 2016/05/06 17:05:19, Theresa Wellington wrote: > > Can we use a custom layout that doesn't rely use a CheckMarkDrawable? > > * ... that doesn't use a CheckMarkDrawable? Done. https://codereview.chromium.org/1465363002/diff/260001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java (right): https://codereview.chromium.org/1465363002/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:476: ThreadUtils.runOnUiThreadBlocking(new Runnable() { On 2016/05/06 at 17:05:19, Theresa Wellington wrote: > nit: add a comment here, something like > // Open the clear browsing data preference and click the clear button. Done. https://codereview.chromium.org/1465363002/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:489: // Check that we are shown. On 2016/05/06 at 17:05:19, Theresa Wellington wrote: > nit: Check that the important sites dialog is shown. Done. https://codereview.chromium.org/1465363002/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:526: // Check that it's in the set correctly. On 2016/05/06 at 17:05:19, Theresa Wellington wrote: > nit: Check that facebook.com is in the set of deselected domains. Done. https://codereview.chromium.org/1465363002/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:561: }); On 2016/05/06 at 17:05:19, Theresa Wellington wrote: > Are there things we could check to see if browsing data actually got cleared? Or would that be difficult to do here? Did it! It took a little more work, but I'm way more comfortable now with this feature haha.
The last patchset message references a ManageSpaceActivityTest but I don't see one uploaded? Looking good overall! Thank you for all of the changes. I'm going to do a pass over the whole thing this afternoon. https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... File chrome/android/java/res/layout/list_row_checked_icon.xml (right): https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... chrome/android/java/res/layout/list_row_checked_icon.xml:1: <?xml version="1.0" encoding="utf-8"?> Instead of making this a generic list row, I think a layout for the important sites makes sense. It's unlikely this exact configuration will be needed somewhere else w/ the exact same padding, etc. This file could be named confirm_important_sites_list_row.xml. Rather than a RelativeLayout it can be a horizontal LinearLayout. The ImageView will get specific width & height matching the favicon size, and the Checkbox will get layout_width="0dp" and layout_weight="1". data_reduction_stats_layout.xml has some good examples. https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... chrome/android/java/res/layout/list_row_checked_icon.xml:15: android:layout_height="wrap_content" Should this height be match_parent so that it doesn't wrap to two lines? Or does this screen allow wrapping? https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... chrome/android/java/res/layout/list_row_checked_icon.xml:24: android:layout_height="wrap_content" I think this should be match_parent https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... chrome/android/java/res/layout/list_row_checked_icon.xml:27: android:padding="5dp" /> If the size of the icon doesn't exactly match the size of the ImageView, add android:scaleType="center" https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:30: <RelativeLayout LinearLayout's with weights are slightly more efficient than RelativeLayouts since RelativeLayouts have to measure things twice to know how to draw them on the screen. This layout is simple enough I think the RelativeLayout is okay, just something to be aware of for future changes :) https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:59: // We use the bookmark icon configuration. I think this comment can be removed. https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:114: loadLocalFavicon(viewHolder, domain); Why does this need to be loaded on click? https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:212: activityManager.getMemoryClass() / 32 * 1024 * 1024, FAVICON_MAX_CACHE_SIZE_BYTES); I think the cache will typically be between .5 - 1MB since the baseline Android memory class is 16 (according to documentation). Is that what you're shooting for? https://codereview.chromium.org/1465363002/diff/280001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java (right): https://codereview.chromium.org/1465363002/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:567: ThreadUtils.runOnUiThreadBlocking(new Runnable() { nit: extract this into a helper method. https://codereview.chromium.org/1465363002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:282: Log.i(TAG, "No websites!" + sitesByOrigin + sitesByHost); nit: extra debugging tag left here.
Description was changed from
==========
[Storage] Android - ManageSpace UI, Important Origins, and CBD Dialog
See the screenshots in the bug.
This patch does the following:
* Adds an activity for the manageSpaceActivity target in the
AndroidManifest.xml which gives options for managing or clearing
storage.
NOTE: This is only hooked up in API >= KLP, as the API for
clearing an application's app data isn't available until then.
* We have a Dialog protecting the Reset App functionality.
* Adds a Clear All button to the bottom of the Storage site settings
view. This involves a layout that's almost an exact copy of the
default layout, except we have one button.
* Protected again by a dialog.
* Adds a dialog to the Clear Browsing Data screen, that only shows
up when the user has 'important' origins.
* The user is prompted about these sites, and can uncheck them.
* Test is included, which also double checks that cookies are
not deleted.
STILL TODO:
* Write ManageStoreActivity test.
BUG=560549,579763
==========
to
==========
[Storage] Android - ManageSpace UI, Important Origins, and CBD Dialog
See the screenshots in the bug.
This patch does the following:
* Adds an activity for the manageSpaceActivity target in the
AndroidManifest.xml which gives options for managing or clearing
storage.
NOTE: This is only hooked up in API >= KLP, as the API for
clearing an application's app data isn't available until then.
* We have a Dialog protecting the Reset App functionality.
* Tested for clearing unimportant storage.
* Adds a Clear All button to the bottom of the Storage site settings
view. This involves a layout that's almost an exact copy of the
default layout, except we have one button.
* Protected again by a dialog.
* Adds a dialog to the Clear Browsing Data screen, that only shows
up when the user has 'important' origins.
* The user is prompted about these sites, and can uncheck them.
* Test is included, which also double checks that cookies are
not deleted.
BUG=560549,579763
==========
https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:326: android:configChanges="orientation|keyboardHidden|keyboard|screenSize|mcc|mnc|screenSize|smallestScreenSize" screenSize is included twice, screenLayout is missing https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... File chrome/android/java/res/layout/select_dialog_listview.xml (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... chrome/android/java/res/layout/select_dialog_listview.xml:13: android:overScrollMode="ifContentScrolls" ifContentScrolls is actually the default for android:overScrollMode despite what the documentation says. I think you can remove this line. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:29: <RelativeLayout android:id="@+id/button_bar" Can this be a FrameLayout instead of a RelativeLayout since it only contains one thing? https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... File chrome/android/java/res/values/values.xml (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... chrome/android/java/res/values/values.xml:60: <string name="manage_space_activity" translatable="false"></string> This doesn't need translatable=false since it's in values.xml rather than our normal string file. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:804: /** @return The maximum number of important sites that will be returend from the call above. */ s/returend/returned https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:811: public static void markOriginAsImportant(String origin) { nit: markOriginAsImportantForTesting() (so that it's extra obvious) https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:340: mWaitingForImportantSitesBeforeClear = true; What happens if the preference is clicked and we're still waiting for important sites? Is there any indication to the user that we're still waiting? I think you may have answered this question before, but I'm having trouble finding it in the comments. Sorry if it's repetitive! https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:76: return true; What does this override do? https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:116: private void loadLocalFavicon(final ViewAndFaviconHolder viewHolder, final String url) { nit: just loadFavicon()? https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:146: * the favicon image callback; so that we can make sure we load the correct favicon. nit: replace the semi-colon with a comma https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:41: * configured in AndroidManifest.xml by setting android:manageSpaceActivity for the application. Is there any way to finch this or are we rolling it out to 100% immediately? https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:42: * The browser process must be started here because this Activity may be started explicitly from nit: this line and the two below should be indented one less https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:113: Log.e(TAG, "Unable to load native library.", e); Should we set the data size text fields to "unknown" in this case too? Or something that indicates we're no longer trying to calculate. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:452: if (getActivity() == null) return; As an extra precaution, should we also check that the view clicked is indeed the clear button? https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2290: <message name="IDS_STORAGE_MANAGEMENT_SITE_DATA_DESCRIPTIVE_TEXT" desc="Text to describe the data stored on the phone by websites."> s/phone/device https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2315: Delete app data? Some languages will probably have a hard time translating this w/ only 24 characters (e.g. French from Google translate says: "Supprimer les données d'application?") Is 24 a strict limit here or could it be longer? This is just used in the dialog window, right? https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:521: public void testImportantSitesDialog() throws Exception { This test is really thorough, thank you! Does it make sense to split it into 3: 1. clear all data 2. clear no data (cancel) 3. data for unimportant site not cleared https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:584: // Uncheck the first item (our internal web server). nit: add a blank line above this to break up the code a bit https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:598: // Check that our server origin is in the set of deselected domains. nit: add a blank line above this too https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:26: * Tests for everything under Settings > Site Settings. Comment needs to be updated. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:29: @CommandLineFlags.Add({"enable-features=ImportantSitesInCBD", "enable-site-engagement"}) Do these flags actually need to be set for this test? https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:136: } I think it'd also be good to test the other buttons in the Activity. A TODO is okay for now. https://codereview.chromium.org/1465363002/diff/320001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:666: static void MarkOriginAsImportant(JNIEnv* env, nit: MarkOriginAsImportantForTesting for this method name too? https://codereview.chromium.org/1465363002/diff/320001/chrome/browser/android... File chrome/browser/android/preferences/website_preference_bridge.cc (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/browser/android... chrome/browser/android/preferences/website_preference_bridge.cc:52: // to the user, we're just using them for our 'clear unimportant' feature. s/feature/ feature in ManageSpaceActivity.java. https://codereview.chromium.org/1465363002/diff/320001/components/browsing_da... File components/browsing_data/storage_partition_http_cache_data_remover.cc (right): https://codereview.chromium.org/1465363002/diff/320001/components/browsing_da... components/browsing_data/storage_partition_http_cache_data_remover.cc:190: rv = net::ERR_IO_PENDING; Why is this an error now?
Last set of comments was mostly nits :) One other thought - it'd be nice to collet some UMA stats around this. Maybe add a TODO for that as well?
dmurph@chromium.org changed reviewers: + msramek@chromium.org
Thanks for the many comments! +msramek for the net::ERR_IO_PENDING change. https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... File chrome/android/java/res/layout/list_row_checked_icon.xml (right): https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... chrome/android/java/res/layout/list_row_checked_icon.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2016/05/12 at 20:22:41, Theresa Wellington wrote: > Instead of making this a generic list row, I think a layout for the important sites makes sense. It's unlikely this exact configuration will be needed somewhere else w/ the exact same padding, etc. > > This file could be named confirm_important_sites_list_row.xml. > > Rather than a RelativeLayout it can be a horizontal LinearLayout. The ImageView will get specific width & height matching the favicon size, and the Checkbox will get layout_width="0dp" and layout_weight="1". > > data_reduction_stats_layout.xml has some good examples. Done. https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... chrome/android/java/res/layout/list_row_checked_icon.xml:15: android:layout_height="wrap_content" On 2016/05/12 at 20:22:41, Theresa Wellington wrote: > Should this height be match_parent so that it doesn't wrap to two lines? Or does this screen allow wrapping? match_parent is fine. https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... chrome/android/java/res/layout/list_row_checked_icon.xml:24: android:layout_height="wrap_content" On 2016/05/12 at 20:22:41, Theresa Wellington wrote: > I think this should be match_parent done. https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... chrome/android/java/res/layout/list_row_checked_icon.xml:27: android:padding="5dp" /> On 2016/05/12 at 20:22:41, Theresa Wellington wrote: > If the size of the icon doesn't exactly match the size of the ImageView, add android:scaleType="center" done. https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:30: <RelativeLayout On 2016/05/12 at 20:22:41, Theresa Wellington wrote: > LinearLayout's with weights are slightly more efficient than RelativeLayouts since RelativeLayouts have to measure things twice to know how to draw them on the screen. This layout is simple enough I think the RelativeLayout is okay, just something to be aware of for future changes :) ok, thanks! https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:59: // We use the bookmark icon configuration. On 2016/05/12 at 20:22:41, Theresa Wellington wrote: > I think this comment can be removed. Done. https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:114: loadLocalFavicon(viewHolder, domain); On 2016/05/12 at 20:22:41, Theresa Wellington wrote: > Why does this need to be loaded on click? Good question! No idea why I had this here. https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:212: activityManager.getMemoryClass() / 32 * 1024 * 1024, FAVICON_MAX_CACHE_SIZE_BYTES); On 2016/05/12 at 20:22:41, Theresa Wellington wrote: > I think the cache will typically be between .5 - 1MB since the baseline Android memory class is 16 (according to documentation). Is that what you're shooting for? I'm not sure. Something that holds 5 icons. What would that be? I think maximum possible is 4kb, so 20kb? I guess I could be lower. Changing maximum to 100KB, and making 16 -> 25 KB. Does that make sense? https://codereview.chromium.org/1465363002/diff/280001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java (right): https://codereview.chromium.org/1465363002/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:567: ThreadUtils.runOnUiThreadBlocking(new Runnable() { On 2016/05/12 at 20:22:41, Theresa Wellington wrote: > nit: extract this into a helper method. Done. https://codereview.chromium.org/1465363002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:282: Log.i(TAG, "No websites!" + sitesByOrigin + sitesByHost); On 2016/05/12 at 20:22:42, Theresa Wellington wrote: > nit: extra debugging tag left here. removed. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:326: android:configChanges="orientation|keyboardHidden|keyboard|screenSize|mcc|mnc|screenSize|smallestScreenSize" On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > screenSize is included twice, screenLayout is missing Huh, I could have sworn I fixed this haha. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... File chrome/android/java/res/layout/select_dialog_listview.xml (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... chrome/android/java/res/layout/select_dialog_listview.xml:13: android:overScrollMode="ifContentScrolls" On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > ifContentScrolls is actually the default for android:overScrollMode despite what the documentation says. > > I think you can remove this line. Done. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:29: <RelativeLayout android:id="@+id/button_bar" On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > Can this be a FrameLayout instead of a RelativeLayout since it only contains one thing? Done. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... File chrome/android/java/res/values/values.xml (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... chrome/android/java/res/values/values.xml:60: <string name="manage_space_activity" translatable="false"></string> On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > This doesn't need translatable=false since it's in values.xml rather than our normal string file. Done. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:804: /** @return The maximum number of important sites that will be returend from the call above. */ On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > s/returend/returned Done. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:811: public static void markOriginAsImportant(String origin) { On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > nit: markOriginAsImportantForTesting() (so that it's extra obvious) Done. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:340: mWaitingForImportantSitesBeforeClear = true; On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > What happens if the preference is clicked and we're still waiting for important sites? Is there any indication to the user that we're still waiting? > > I think you may have answered this question before, but I'm having trouble finding it in the comments. Sorry if it's repetitive! No indication. Added another progress dialog. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:76: return true; On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > What does this override do? If this is true, then the list doesn't re-generate views for ids it already has a view for. Optimization. But I guess since we don't change our dataset it doesn't matter. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:146: * the favicon image callback; so that we can make sure we load the correct favicon. On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > nit: replace the semi-colon with a comma haha not sure why that was there. Removed. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:41: * configured in AndroidManifest.xml by setting android:manageSpaceActivity for the application. On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > Is there any way to finch this or are we rolling it out to 100% immediately? We're rolling this out 100% immediately. I confirmed this with Dru. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:42: * The browser process must be started here because this Activity may be started explicitly from On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > nit: this line and the two below should be indented one less Done. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:113: Log.e(TAG, "Unable to load native library.", e); On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > Should we set the data size text fields to "unknown" in this case too? Or something that indicates we're no longer trying to calculate. Done. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:452: if (getActivity() == null) return; On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > As an extra precaution, should we also check that the view clicked is indeed the clear button? Done. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2290: <message name="IDS_STORAGE_MANAGEMENT_SITE_DATA_DESCRIPTIVE_TEXT" desc="Text to describe the data stored on the phone by websites."> On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > s/phone/device Done. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2315: Delete app data? On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > Some languages will probably have a hard time translating this w/ only 24 characters (e.g. French from Google translate says: "Supprimer les données d'application?") > > Is 24 a strict limit here or could it be longer? This is just used in the dialog window, right? Yeah, as the title. Removed limit as per other titles. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:521: public void testImportantSitesDialog() throws Exception { On 2016/05/13 at 06:02:31, Theresa Wellington wrote: > This test is really thorough, thank you! > > Does it make sense to split it into 3: 1. clear all data 2. clear no data (cancel) 3. data for unimportant site not cleared Done. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:584: // Uncheck the first item (our internal web server). On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > nit: add a blank line above this to break up the code a bit Done. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:598: // Check that our server origin is in the set of deselected domains. On 2016/05/13 at 06:02:31, Theresa Wellington wrote: > nit: add a blank line above this too Done. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:26: * Tests for everything under Settings > Site Settings. On 2016/05/13 at 06:02:31, Theresa Wellington wrote: > Comment needs to be updated. Done. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:29: @CommandLineFlags.Add({"enable-features=ImportantSitesInCBD", "enable-site-engagement"}) On 2016/05/13 at 06:02:31, Theresa Wellington wrote: > Do these flags actually need to be set for this test? Good catch, no just the second one. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:136: } On 2016/05/13 at 06:02:31, Theresa Wellington wrote: > I think it'd also be good to test the other buttons in the Activity. A TODO is okay for now. Not sure how I'll test the clear all app data button... adding todo. https://codereview.chromium.org/1465363002/diff/320001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:666: static void MarkOriginAsImportant(JNIEnv* env, On 2016/05/13 at 06:02:31, Theresa Wellington wrote: > nit: MarkOriginAsImportantForTesting for this method name too? Done. https://codereview.chromium.org/1465363002/diff/320001/chrome/browser/android... File chrome/browser/android/preferences/website_preference_bridge.cc (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/browser/android... chrome/browser/android/preferences/website_preference_bridge.cc:52: // to the user, we're just using them for our 'clear unimportant' feature. On 2016/05/13 at 06:02:31, Theresa Wellington wrote: > s/feature/ feature in ManageSpaceActivity.java. Done. https://codereview.chromium.org/1465363002/diff/320001/components/browsing_da... File components/browsing_data/storage_partition_http_cache_data_remover.cc (right): https://codereview.chromium.org/1465363002/diff/320001/components/browsing_da... components/browsing_data/storage_partition_http_cache_data_remover.cc:190: rv = net::ERR_IO_PENDING; On 2016/05/13 at 06:02:31, Theresa Wellington wrote: > Why is this an error now? Not really an error. This was missing from Martin's CL, just fixing here. I'll add him to confirm.
note: Rebecca said she's leaning towards using the dialog that's already there for handing the async case of fetchImportantSites in the ClearBrowsingDataPreference. In that case, I would just use the current showProgressDialog and dismissProgressDialog methods, and remove the strings.
https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/280001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:30: <RelativeLayout On 2016/05/13 23:46:22, dmurph wrote: > On 2016/05/12 at 20:22:41, Theresa Wellington wrote: > > LinearLayout's with weights are slightly more efficient than RelativeLayouts > since RelativeLayouts have to measure things twice to know how to draw them on > the screen. This layout is simple enough I think the RelativeLayout is okay, > just something to be aware of for future changes :) > > ok, thanks! I was taking an overly simple view on this; I think RelativeLayout makes sense here. Nested RelativeLayouts are bad, but this is fine. More background on view efficiency: http://stackoverflow.com/questions/4069037/android-is-a-relativelayout-more-e... http://developer.android.com/intl/in/training/improving-layouts/optimizing-la... https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:29: <RelativeLayout android:id="@+id/button_bar" On 2016/05/13 23:46:23, dmurph wrote: > On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > > Can this be a FrameLayout instead of a RelativeLayout since it only contains > one thing? > > Done. On second thought - or just the child. Does it need a viewgroup parent?
https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:76: return true; On 2016/05/13 23:46:23, dmurph wrote: > On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > > What does this override do? > > If this is true, then the list doesn't re-generate views for ids it already has > a view for. Optimization. But I guess since we don't change our dataset it > doesn't matter. You can leave it in, I was just curious. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:41: * configured in AndroidManifest.xml by setting android:manageSpaceActivity for the application. On 2016/05/13 23:46:24, dmurph wrote: > On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > > Is there any way to finch this or are we rolling it out to 100% immediately? > > We're rolling this out 100% immediately. I confirmed this with Dru. It'd be nice to have a kill switch in case things go terribly wrong, but I can't think of a way to integrate finch w/ the AndroidManifest flag :-/. Oh well. https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:29: @CommandLineFlags.Add({"enable-features=ImportantSitesInCBD", "enable-site-engagement"}) On 2016/05/13 23:46:24, dmurph wrote: > On 2016/05/13 at 06:02:31, Theresa Wellington wrote: > > Do these flags actually need to be set for this test? > > Good catch, no just the second one. If site engagement isn't enabled, does the ManageSpaceActivity's clear unimportant button just clear all site data? https://codereview.chromium.org/1465363002/diff/340001/chrome/android/java/re... File chrome/android/java/res/layout/confirm_important_sites_list_row.xml (right): https://codereview.chromium.org/1465363002/diff/340001/chrome/android/java/re... chrome/android/java/res/layout/confirm_important_sites_list_row.xml:26: android:layout_width="wrap_content" We know the final width for the ImageView right? If so, let's use a specific width instead of wrap_content. https://codereview.chromium.org/1465363002/diff/340001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/340001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:29: <FrameLayout android:id="@+id/button_bar" Can this parent be dropped entirely? https://codereview.chromium.org/1465363002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:453: if (v != mClearButton) return; nit: combine with the check above. https://codereview.chromium.org/1465363002/diff/340001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java (right): https://codereview.chromium.org/1465363002/diff/340001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:600: getPressButtonInImportantDialogRunnable(preferences, AlertDialog.BUTTON_NEGATIVE)); Check that data hasn't been cleared with this code:? assertEquals("true", runJavaScriptCodeInCurrentTab("hasAllStorage()")); https://codereview.chromium.org/1465363002/diff/340001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1465363002/diff/340001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:635: LOG(ERROR) << "Clearing excluding domains: "; Why was this error log added? https://codereview.chromium.org/1465363002/diff/340001/chrome/browser/browsin... File chrome/browser/browsing_data/registrable_domain_filter_builder.cc (right): https://codereview.chromium.org/1465363002/diff/340001/chrome/browser/browsin... chrome/browser/browsing_data/registrable_domain_filter_builder.cc:102: for (const auto& domain : *registerable_domains) { Why is it an error to have registerable_domains?
Note: I'm waiting for Rebecca to get back to me about how the button should look. (in storage_preference_fragement.xml). https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/320001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:29: <RelativeLayout android:id="@+id/button_bar" On 2016/05/14 at 00:05:38, Theresa Wellington wrote: > On 2016/05/13 23:46:23, dmurph wrote: > > On 2016/05/13 at 06:02:30, Theresa Wellington wrote: > > > Can this be a FrameLayout instead of a RelativeLayout since it only contains > > one thing? > > > > Done. > > On second thought - or just the child. Does it need a viewgroup parent? Done.
https://codereview.chromium.org/1465363002/diff/340001/chrome/android/java/re... File chrome/android/java/res/layout/confirm_important_sites_list_row.xml (right): https://codereview.chromium.org/1465363002/diff/340001/chrome/android/java/re... chrome/android/java/res/layout/confirm_important_sites_list_row.xml:26: android:layout_width="wrap_content" On 2016/05/16 at 22:37:08, Theresa Wellington wrote: > We know the final width for the ImageView right? If so, let's use a specific width instead of wrap_content. Done. https://codereview.chromium.org/1465363002/diff/340001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/340001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:29: <FrameLayout android:id="@+id/button_bar" On 2016/05/16 at 22:37:08, Theresa Wellington wrote: > Can this parent be dropped entirely? Done. https://codereview.chromium.org/1465363002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:453: if (v != mClearButton) return; On 2016/05/16 at 22:37:08, Theresa Wellington wrote: > nit: combine with the check above. Done. https://codereview.chromium.org/1465363002/diff/340001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java (right): https://codereview.chromium.org/1465363002/diff/340001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:600: getPressButtonInImportantDialogRunnable(preferences, AlertDialog.BUTTON_NEGATIVE)); On 2016/05/16 at 22:37:08, Theresa Wellington wrote: > Check that data hasn't been cleared with this code:? > > assertEquals("true", runJavaScriptCodeInCurrentTab("hasAllStorage()")); Whoops, thanks. https://codereview.chromium.org/1465363002/diff/340001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1465363002/diff/340001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:635: LOG(ERROR) << "Clearing excluding domains: "; On 2016/05/16 at 22:37:08, Theresa Wellington wrote: > Why was this error log added? Whoops, forgot to remove. This was for debugging. https://codereview.chromium.org/1465363002/diff/340001/chrome/browser/browsin... File chrome/browser/browsing_data/registrable_domain_filter_builder.cc (right): https://codereview.chromium.org/1465363002/diff/340001/chrome/browser/browsin... chrome/browser/browsing_data/registrable_domain_filter_builder.cc:102: for (const auto& domain : *registerable_domains) { On 2016/05/16 at 22:37:08, Theresa Wellington wrote: > Why is it an error to have registerable_domains? Whoops, forgot to remove.
https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:43: android:layout_alignParentEnd="true" In the event this text is long, do these two views attempt to overlap (test by just changing the string to superreallyfrigingtruly100percenteverywherecompletelyunimportantstuffthatyoushoulddefintielydelete)? Wondering if you also should add something like the following to force it to align nicely: android:android:layout_toEndOf="@id/unimportant_site_data_size_prefix" https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:79: style="@style/ManageSpaceActivityButton" /> -4 indent for consistency https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... File chrome/android/java/res/layout/select_dialog_listview.xml (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... chrome/android/java/res/layout/select_dialog_listview.xml:5: trailing ws https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:18: android:clipToPadding="false" the padding is 0, why do we need this? https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:19: android:cacheColorHint="@android:color/transparent"/> what is this accomplishing? in particular, it states that it will be used to help android since it is drawing on top of a solid opaque color, which transparent isn't. https://developer.android.com/reference/android/widget/AbsListView.html#attr_... https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:32: android:layout_weight="0" I think we typically don't specify 0 as it is the default value. Not technically wrong though I guess. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:219: private boolean mWaitingForImportantSitesBeforeClear = false; false and null are default values, so we don't specify them in java (same for 0 for ints) https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:51: String[] mDomains; private final as well? https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:91: public void configureChildView(int position, ViewAndFaviconHolder viewHolder) { private? https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:181: return "http://www." + domain; what happens if it https? Does that not matter or could we potentially be losing access to some favicons? https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:201: mDeselectedDomains = new HashSet<>(); If you go into android's dev options and select don't keep activities, what happens if you leave chrome while this screen is open and then reselect it from android recents? Assuming it is possible, it looks like this will crash as the native libraries will not have been loaded so much of these calls won't work. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:202: mProfile = Profile.getLastUsedProfile(); If we don't want to handle these cases, I think you can move this code into onCreateDialog but add a check like: if (savedInstanceState != null) { dismiss(); } (Of course, your onDismiss then needs to handle a null mLargeIconBridge) https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:256: R.layout.select_dialog_listview, null); I would give this a more specific name if it is currently not shared with anywhere else. clear_browsing_select_dialog_listview https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:261: mAdapter.onClick(position, v); I would reverse the ordering of the params to be consistent with the onItemClick signature https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:119: public void finishNativeInitialization() { I would add: /** @see BrowserParts#finishNativeInitialization */ https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:189: AlertDialog.Builder builder = new AlertDialog.Builder(this); it is a bit odd to have a reference to the mUnimportantDialog but not follow the same pattern here. I guess for testing? https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:199: builder.create().show(); If you try really quickly, can you get multiple dialogs/fragments to appear if you really hammer on any of these buttons? If so, you might want to add some special casing to not allow any of these to happen if any dialogs were previously triggered and not yet dismissed. But if any of these can be called multiple times w/o issue, then I guess it is not as important (it will still probably end up being a bug, but not a blocker) https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:92: private List<WebsitePreference> mWebsites = null; drop the = null https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:329: for (WebsitePreference preference : mWebsites) { for lists, prefer the for (int i = 0; i < mWebsites.size(); i++) { form. The foreach allocates an unnecessary iterator that typically is unneeded as we almost exclusively use arraylists in java
I still need to do another pass over ConfirmImportantSitesDialogFragment, ClearBrowsingDataPreferences, and ManageSpaceActivity. Find bugs has a complaint: FindBugs reported the following issues: EI_EXPOSE_REP2: May expose internal representation by incorporating reference to mutable object In class org.chromium.chrome.browser.preferences.privacy.ClearBrowsingDataPreferences In method org.chromium.chrome.browser.preferences.privacy.ClearBrowsingDataPreferences.onImportantRegisterableDomainsReady(String[]) Field org.chromium.chrome.browser.preferences.privacy.ClearBrowsingDataPreferences.mSortedImportantDomains At ClearBrowsingDataPreferences.java:[line 580] https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/re... File chrome/android/java/res/layout/confirm_important_sites_list_row.xml (right): https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/re... chrome/android/java/res/layout/confirm_important_sites_list_row.xml:26: android:layout_width="16dip" nit: 16dp. Let's set the height to 16dp too. https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:14: android:layout_height="0px" nit: 0dp https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:32: android:layout_weight="0" nit: the default weight is 0, so this can be dropped. https://codereview.chromium.org/1465363002/diff/380001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java (right): https://codereview.chromium.org/1465363002/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:89: private Criteria getWaitForProgressCompleteCriteria(final Preferences preferences) { nit: Pass in ClearBrowsingDataPreferences & cast below to avoid people accidentally calling this with other Preferences. https://codereview.chromium.org/1465363002/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:498: final Preferences preferences, final int numImportantSites) { nit: same comment here w/ casting. https://codereview.chromium.org/1465363002/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:555: Preferences preferences = startPreferences(ClearBrowsingDataPreferences.class.getName()); You could do the cast here. https://codereview.chromium.org/1465363002/diff/380001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java (right): https://codereview.chromium.org/1465363002/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:26: * Tests that we only clear unimporant sites. nit: "Tests for ManageSpaceActivity" or something more general that allows for adding more test later w/out changing this comment. https://codereview.chromium.org/1465363002/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:125: CriteriaHelper.pollUiThread(getIsClearButtonEnabledCriteria(manageSpaceActivity)); nit: split up this big code block with some blank lines.
https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:541: || !ChromeFeatureList.isEnabled(ChromeFeatureList.IMPORTANT_SITES_IN_CBD)) { If I understand correctly, important sites are only protected from cookies and cache deletion. Shouldn't we therefore only show the dialog if cookies or cache are selected? We probably don't want to pop up additional dialogs if not necessary. https://codereview.chromium.org/1465363002/diff/380001/components/browsing_da... File components/browsing_data/storage_partition_http_cache_data_remover.cc (right): https://codereview.chromium.org/1465363002/diff/380001/components/browsing_da... components/browsing_data/storage_partition_http_cache_data_remover.cc:190: rv = net::ERR_IO_PENDING; Thanks for fixing this! But it should be: rv = (new ConditionalCache...)->DeleteAndDestroySelfWhenFinished(...). It is true that |DeleteAndDestroySelfWhenFinished| currently only returns net::ERR_IO_PENDING, but it could conceivably return any other net error code.
thanks for the revies! Three changes: 1. I updated the way the progress dialog is shown, so it updates the text if it's already there. This makes is so the dialog doesn't flash twice if we're waiting for the important sites, and then decide to just clear everything (if they're empty). Another option here is to just give up and clear all if the person touches 'clear' before we know the sites. 2. I changed the style of the row to be 'wrap_content' again for the favicon, because for some reason it was clipping the left side. Maybe this is because it includes the padding? Let me know if you have insight here. 3. I styled the button in the storage list to match the mocks. it looks pretty great! https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:43: android:layout_alignParentEnd="true" On 2016/05/16 at 23:32:07, Ted C wrote: > In the event this text is long, do these two views attempt to overlap (test by just changing the string to superreallyfrigingtruly100percenteverywherecompletelyunimportantstuffthatyoushoulddefintielydelete)? > > Wondering if you also should add something like the following to force it to align nicely: > > android:android:layout_toEndOf="@id/unimportant_site_data_size_prefix" That makes it move all the way to the left. I think using formatFileSize should keep the size small. They overlap if it's too big. What would you like me to do here? I can't right-justify text, because android. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:79: style="@style/ManageSpaceActivityButton" /> On 2016/05/16 at 23:32:06, Ted C wrote: > -4 indent for consistency Done. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... File chrome/android/java/res/layout/select_dialog_listview.xml (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... chrome/android/java/res/layout/select_dialog_listview.xml:5: On 2016/05/16 at 23:32:07, Ted C wrote: > trailing ws Done. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:18: android:clipToPadding="false" On 2016/05/16 at 23:32:07, Ted C wrote: > the padding is 0, why do we need this? Dunno, copied this file. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:19: android:cacheColorHint="@android:color/transparent"/> On 2016/05/16 at 23:32:07, Ted C wrote: > what is this accomplishing? > > in particular, it states that it will be used to help android since it is drawing on top of a solid opaque color, which transparent isn't. > > https://developer.android.com/reference/android/widget/AbsListView.html#attr_... Dunno, I copied this from the default style. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:32: android:layout_weight="0" On 2016/05/16 at 23:32:07, Ted C wrote: > I think we typically don't specify 0 as it is the default value. Not technically wrong though I guess. Done. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:219: private boolean mWaitingForImportantSitesBeforeClear = false; On 2016/05/16 at 23:32:07, Ted C wrote: > false and null are default values, so we don't specify them in java (same for 0 for ints) Done. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:51: String[] mDomains; On 2016/05/16 at 23:32:07, Ted C wrote: > private final as well? Done. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:91: public void configureChildView(int position, ViewAndFaviconHolder viewHolder) { On 2016/05/16 at 23:32:07, Ted C wrote: > private? Done. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:181: return "http://www." + domain; On 2016/05/16 at 23:32:07, Ted C wrote: > what happens if it https? Does that not matter or could we potentially be losing access to some favicons? This is just for displaying favicons. We just want to transform the domain into a real URL so it resolves to an actual favicon. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:202: mProfile = Profile.getLastUsedProfile(); On 2016/05/16 at 23:32:07, Ted C wrote: > If we don't want to handle these cases, I think you can move this code into onCreateDialog but add a check like: > > if (savedInstanceState != null) { > dismiss(); > } > > (Of course, your onDismiss then needs to handle a null mLargeIconBridge) Done. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:256: R.layout.select_dialog_listview, null); On 2016/05/16 at 23:32:07, Ted C wrote: > I would give this a more specific name if it is currently not shared with anywhere else. > > clear_browsing_select_dialog_listview Done. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:261: mAdapter.onClick(position, v); On 2016/05/16 at 23:32:07, Ted C wrote: > I would reverse the ordering of the params to be consistent with the onItemClick signature Done. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:119: public void finishNativeInitialization() { On 2016/05/16 at 23:32:07, Ted C wrote: > I would add: > > /** @see BrowserParts#finishNativeInitialization */ Done. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:189: AlertDialog.Builder builder = new AlertDialog.Builder(this); On 2016/05/16 at 23:32:07, Ted C wrote: > it is a bit odd to have a reference to the mUnimportantDialog but not follow the same pattern here. I guess for testing? Yes, we only store that dialog for testing. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:199: builder.create().show(); On 2016/05/16 at 23:32:07, Ted C wrote: > If you try really quickly, can you get multiple dialogs/fragments to appear if you really hammer on any of these buttons? > > If so, you might want to add some special casing to not allow any of these to happen if any dialogs were previously triggered and not yet dismissed. But if any of these can be called multiple times w/o issue, then I guess it is not as important (it will still probably end up being a bug, but not a blocker) I can't get that to work. It behaves correctly. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:92: private List<WebsitePreference> mWebsites = null; On 2016/05/16 at 23:32:07, Ted C wrote: > drop the = null Done. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:329: for (WebsitePreference preference : mWebsites) { On 2016/05/16 at 23:32:07, Ted C wrote: > for lists, prefer the for (int i = 0; i < mWebsites.size(); i++) { form. > > The foreach allocates an unnecessary iterator that typically is unneeded as we almost exclusively use arraylists in java Done. https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/re... File chrome/android/java/res/layout/confirm_important_sites_list_row.xml (right): https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/re... chrome/android/java/res/layout/confirm_important_sites_list_row.xml:26: android:layout_width="16dip" On 2016/05/16 at 23:45:09, Theresa Wellington wrote: > nit: 16dp. > > Let's set the height to 16dp too. This makes our icon appear in the top, instead of centered. Not sure why. https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:14: android:layout_height="0px" On 2016/05/16 at 23:45:09, Theresa Wellington wrote: > nit: 0dp Done. https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:32: android:layout_weight="0" On 2016/05/16 at 23:45:09, Theresa Wellington wrote: > nit: the default weight is 0, so this can be dropped. Done. https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:541: || !ChromeFeatureList.isEnabled(ChromeFeatureList.IMPORTANT_SITES_IN_CBD)) { On 2016/05/17 at 14:02:24, msramek wrote: > If I understand correctly, important sites are only protected from cookies and cache deletion. Shouldn't we therefore only show the dialog if cookies or cache are selected? We probably don't want to pop up additional dialogs if not necessary. sgtm https://codereview.chromium.org/1465363002/diff/380001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java (right): https://codereview.chromium.org/1465363002/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:89: private Criteria getWaitForProgressCompleteCriteria(final Preferences preferences) { On 2016/05/16 at 23:45:09, Theresa Wellington wrote: > nit: Pass in ClearBrowsingDataPreferences & cast below to avoid people accidentally calling this with other Preferences. Done. https://codereview.chromium.org/1465363002/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:498: final Preferences preferences, final int numImportantSites) { On 2016/05/16 at 23:45:09, Theresa Wellington wrote: > nit: same comment here w/ casting. Done. https://codereview.chromium.org/1465363002/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:555: Preferences preferences = startPreferences(ClearBrowsingDataPreferences.class.getName()); On 2016/05/16 at 23:45:09, Theresa Wellington wrote: > You could do the cast here. Done. https://codereview.chromium.org/1465363002/diff/380001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java (right): https://codereview.chromium.org/1465363002/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:26: * Tests that we only clear unimporant sites. On 2016/05/16 at 23:45:09, Theresa Wellington wrote: > nit: "Tests for ManageSpaceActivity" or something more general that allows for adding more test later w/out changing this comment. Done. https://codereview.chromium.org/1465363002/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:125: CriteriaHelper.pollUiThread(getIsClearButtonEnabledCriteria(manageSpaceActivity)); On 2016/05/16 at 23:45:09, Theresa Wellington wrote: > nit: split up this big code block with some blank lines. Done. https://codereview.chromium.org/1465363002/diff/380001/components/browsing_da... File components/browsing_data/storage_partition_http_cache_data_remover.cc (right): https://codereview.chromium.org/1465363002/diff/380001/components/browsing_da... components/browsing_data/storage_partition_http_cache_data_remover.cc:190: rv = net::ERR_IO_PENDING; On 2016/05/17 at 14:02:24, msramek wrote: > Thanks for fixing this! But it should be: > > rv = (new ConditionalCache...)->DeleteAndDestroySelfWhenFinished(...). > > It is true that |DeleteAndDestroySelfWhenFinished| currently only returns net::ERR_IO_PENDING, but it could conceivably return any other net error code. Done.
Now would be a good time to rebase and upload before more changes are made :) https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:43: android:layout_alignParentEnd="true" On 2016/05/18 23:27:58, dmurph wrote: > On 2016/05/16 at 23:32:07, Ted C wrote: > > In the event this text is long, do these two views attempt to overlap (test by > just changing the string to > superreallyfrigingtruly100percenteverywherecompletelyunimportantstuffthatyoushoulddefintielydelete)? > > > > Wondering if you also should add something like the following to force it to > align nicely: > > > > android:android:layout_toEndOf="@id/unimportant_site_data_size_prefix" > > That makes it move all the way to the left. > > I think using formatFileSize should keep the size small. > > They overlap if it's too big. What would you like me to do here? I can't > right-justify text, because android. In RelativeLayout, layout_toEndOf takes precendence over alignParentEnd and they're mutually exclusive: https://cs.corp.google.com/clankium/src/third_party/android_tools/sdk/sources... Does setting the layout_width of unimportant_site_data_size_prefix help? This might need to be a LinearLayout w/ weights to avoid the overlap. Does formatFileSize change between kb, mb, etc? Even if the file size is small, it may be possible that the label, when translated, ends up taking up a significant amount of space on small phones causing overlap. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:181: return "http://www." + domain; On 2016/05/18 23:27:59, dmurph wrote: > On 2016/05/16 at 23:32:07, Ted C wrote: > > what happens if it https? Does that not matter or could we potentially be > losing access to some favicons? > > This is just for displaying favicons. We just want to transform the domain into > a real URL so it resolves to an actual favicon. I think that's a good question - there's nothing in the history backend that indicates we coalesce https & http urls from the same domain, but I wouldn't be surprised if that were the case. It would be good test https vs http if you haven't already. https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:201: mDeselectedDomains = new HashSet<>(); On 2016/05/16 23:32:07, Ted C wrote: > If you go into android's dev options and select don't keep activities, what > happens if you leave chrome while this screen is open and then reselect it from > android recents? > > Assuming it is possible, it looks like this will crash as the native libraries > will not have been loaded so much of these calls won't work. I agree that this should be tested. We call getLastUsedProfile in other DialogFragments (e.g. PassphraseCreationDialogFragment), but we do it in onCreateDialog() so that may be an option if there are crashes. https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/re... File chrome/android/java/res/layout/confirm_important_sites_list_row.xml (right): https://codereview.chromium.org/1465363002/diff/380001/chrome/android/java/re... chrome/android/java/res/layout/confirm_important_sites_list_row.xml:26: android:layout_width="16dip" On 2016/05/18 23:28:00, dmurph wrote: > On 2016/05/16 at 23:45:09, Theresa Wellington wrote: > > nit: 16dp. > > > > Let's set the height to 16dp too. > > This makes our icon appear in the top, instead of centered. Not sure why. Hm. I would have thought the scaleType and gravity positioned it correctly (maybe it needs to be gravity="center"?), but if not then it's fine as is. https://codereview.chromium.org/1465363002/diff/400001/chrome/android/java/re... File chrome/android/java/res/drawable/flush_footer_button.xml (right): https://codereview.chromium.org/1465363002/diff/400001/chrome/android/java/re... chrome/android/java/res/drawable/flush_footer_button.xml:8: <corners android:radius="0dp"/> Wouldn't corners with radius 0 just be a regular rectangle? What happens if you remove this line? https://codereview.chromium.org/1465363002/diff/400001/chrome/android/java/re... File chrome/android/java/res/layout/confirm_important_sites_list_row.xml (right): https://codereview.chromium.org/1465363002/diff/400001/chrome/android/java/re... chrome/android/java/res/layout/confirm_important_sites_list_row.xml:30: android:paddingEnd="10dp" /> Will you please take screenshots & upload them to the bug for UI to review? It doesn't need to block landing this CL, but they'll be able to confirm whether the padding looks correct everywhere, etc. https://codereview.chromium.org/1465363002/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:112: } catch (ProcessInitException e) { Is it possible other exceptions will be thrown here? Would it make sense to just make the catch for generic "Exception e" so that the activity doesn't crash if something goes wrong anywhere in initialization? https://codereview.chromium.org/1465363002/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:155: mClearUnimportantButton.setEnabled(false); I don't think it needs to block landing this CL - but something test is what happens if you click this button then click one of the other two buttons while data is still being cleared. It might make sense to disable all of the buttons while an operation is in progress.
https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/360001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:43: android:layout_alignParentEnd="true" On 2016/05/19 18:31:52, Theresa Wellington wrote: > On 2016/05/18 23:27:58, dmurph wrote: > > On 2016/05/16 at 23:32:07, Ted C wrote: > > > In the event this text is long, do these two views attempt to overlap (test > by > > just changing the string to > > > superreallyfrigingtruly100percenteverywherecompletelyunimportantstuffthatyoushoulddefintielydelete)? > > > > > > Wondering if you also should add something like the following to force it to > > align nicely: > > > > > > android:android:layout_toEndOf="@id/unimportant_site_data_size_prefix" > > > > That makes it move all the way to the left. > > > > I think using formatFileSize should keep the size small. > > > > They overlap if it's too big. What would you like me to do here? I can't > > right-justify text, because android. > > In RelativeLayout, layout_toEndOf takes precendence over alignParentEnd and > they're mutually exclusive: > https://cs.corp.google.com/clankium/src/third_party/android_tools/sdk/sources... I think that isn't quite right. That is the loop for all relativelayout params, so they can be applied together. I was slightly off in my initial suggestion. But here is a working version (android studio ftw): <?xml version="1.0" encoding="utf-8"?> <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android" xmlns:tools="http://schemas.android.com/tools" android:layout_width="match_parent" android:layout_height="match_parent"> <TextView android:layout_width="wrap_content" android:layout_height="wrap_content" android:textAppearance="?android:attr/textAppearanceMedium" android:text="TESTING" android:id="@+id/textView" android:layout_alignParentTop="true" android:layout_alignParentEnd="true" android:textColor="#000000" /> <TextView android:layout_width="wrap_content" android:layout_height="wrap_content" android:text="Hello World! asdfjsldfkjas dfslakdjfslakdjf sadlfkjsdf asldkfjsaldkfj asldkfj saldfkjsad flkjsad flkasdjf lkasjfd sldkjf lasdkjfsdlakj" android:id="@+id/textView2" android:layout_alignParentStart="true" android:layout_alignStart="@id/textView" android:layout_toStartOf="@+id/textView" /> </RelativeLayout> You can also use textalignment to right justify things, but it is only available in api 17+. Also in the above example, the second listed view (textView2) is actually the first visible view in the relative layout. Looking at the RTL version in android studio, the above works there as well. > > Does setting the layout_width of unimportant_site_data_size_prefix help? This > might need to be a LinearLayout w/ weights to avoid the overlap. I actually think this is easier to manage. While my example technically works above, the first view is the one that will wrap as it states to be to the start of the second text view. I suspect in this use case that isn't what you want. While you might be able to achieve the right sizing and alignment using textAlignment=viewEnd, but I suspect you are going to fight this more than you need to. <I couldn't get it to work in the 2 min that I tried> Granted, a linear layout doesn't work perfectly either. In the event the first and second text are long, the resizing behavior will be weird. Honestly, I don't think you could get the ideal case to work without an entirely custom view. I think a LinearLayout with weight 0 and the first textview and wrap_content on the second will get what you want (and should be exactly the same as the relativelayout config I pasted above). Let's just use a LinearLayout here because it is so confusing to too many of us already. > > Does formatFileSize change between kb, mb, etc? Even if the file size is small, > it may be possible that the label, when translated, ends up taking up a > significant amount of space on small phones causing overlap.
Thanks for the reviews! I made that layout linear as suggested. Works great! Next patch will include better origin resolution of the important domains so that we can get a favicon. https://codereview.chromium.org/1465363002/diff/400001/chrome/android/java/re... File chrome/android/java/res/drawable/flush_footer_button.xml (right): https://codereview.chromium.org/1465363002/diff/400001/chrome/android/java/re... chrome/android/java/res/drawable/flush_footer_button.xml:8: <corners android:radius="0dp"/> On 2016/05/19 at 18:31:52, Theresa Wellington wrote: > Wouldn't corners with radius 0 just be a regular rectangle? What happens if you remove this line? ah, yeah, done. https://codereview.chromium.org/1465363002/diff/400001/chrome/android/java/re... File chrome/android/java/res/layout/confirm_important_sites_list_row.xml (right): https://codereview.chromium.org/1465363002/diff/400001/chrome/android/java/re... chrome/android/java/res/layout/confirm_important_sites_list_row.xml:30: android:paddingEnd="10dp" /> On 2016/05/19 at 18:31:52, Theresa Wellington wrote: > Will you please take screenshots & upload them to the bug for UI to review? It doesn't need to block landing this CL, but they'll be able to confirm whether the padding looks correct everywhere, etc. Done. https://codereview.chromium.org/1465363002/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:112: } catch (ProcessInitException e) { On 2016/05/19 at 18:31:52, Theresa Wellington wrote: > Is it possible other exceptions will be thrown here? Would it make sense to just make the catch for generic "Exception e" so that the activity doesn't crash if something goes wrong anywhere in initialization? Sounds good. https://codereview.chromium.org/1465363002/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:155: mClearUnimportantButton.setEnabled(false); On 2016/05/19 at 18:31:53, Theresa Wellington wrote: > I don't think it needs to block landing this CL - but something test is what happens if you click this button then click one of the other two buttons while data is still being cleared. It might make sense to disable all of the buttons while an operation is in progress. Hm... I would disable the 'Manage Storage' button, as it might have bad data, but the clear all data should always be available. I'll do that.
Also... I don't understand this: FindBugs reported the following issues: EI_EXPOSE_REP2: May expose internal representation by incorporating reference to mutable object In class org.chromium.chrome.browser.preferences.privacy.ClearBrowsingDataPreferences In method org.chromium.chrome.browser.preferences.privacy.ClearBrowsingDataPreferences.onImportantRegisterableDomainsReady(String[]) Field org.chromium.chrome.browser.preferences.privacy.ClearBrowsingDataPreferences.mSortedImportantDomains At ClearBrowsingDataPreferences.java:[line 594] Is it complaining that I'm using the domains argument instead of copying it? Should I declare something final?
On 2016/05/19 22:51:31, dmurph wrote: > Also... I don't understand this: > > FindBugs reported the following issues: > EI_EXPOSE_REP2: May expose internal representation by incorporating reference to > mutable object > In class > org.chromium.chrome.browser.preferences.privacy.ClearBrowsingDataPreferences > In method > org.chromium.chrome.browser.preferences.privacy.ClearBrowsingDataPreferences.onImportantRegisterableDomainsReady(String[]) > Field > org.chromium.chrome.browser.preferences.privacy.ClearBrowsingDataPreferences.mSortedImportantDomains > At ClearBrowsingDataPreferences.java:[line 594] > > Is it complaining that I'm using the domains argument instead of copying it? > Should I declare something final? I usually go to stack overflow to figure out what findbugs is complaining about: http://stackoverflow.com/questions/18954873/malicious-code-vulnerability-may-... I don't think declaring final will help, since the array objects could still be edited. It sounds like it wants you to make a copy.
Theresa & Ted: I finished the favicon hookup. Can you PTAL? Thanks, Daniel
Just a few nits on the new patch set https://chromiumcodereview.appspot.com/1465363002/diff/480001/chrome/android/... File chrome/android/java/res/layout/confirm_important_sites_list_row.xml (right): https://chromiumcodereview.appspot.com/1465363002/diff/480001/chrome/android/... chrome/android/java/res/layout/confirm_important_sites_list_row.xml:24: android:textAppearance="@style/ConfirmImportantSitesListTextAppearance" Does this need a whole style or can you just set the textSize on this element directly? https://chromiumcodereview.appspot.com/1465363002/diff/480001/chrome/browser/... File chrome/browser/android/preferences/important_sites_util.h (right): https://chromiumcodereview.appspot.com/1465363002/diff/480001/chrome/browser/... chrome/browser/android/preferences/important_sites_util.h:22: // example origins for each domain. This can be used for favicons. nit: "... to retrieve favicons." ? https://chromiumcodereview.appspot.com/1465363002/diff/480001/chrome/browser/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://chromiumcodereview.appspot.com/1465363002/diff/480001/chrome/browser/... chrome/browser/android/preferences/pref_service_bridge.cc:660: // We reuse the important domains vector to convert example origins to string. nit: s/string/strings
Done. Do you think this is close to an lgtm? I think we're either done with UI changes, or just need some padding tweaked. https://codereview.chromium.org/1465363002/diff/480001/chrome/android/java/re... File chrome/android/java/res/layout/confirm_important_sites_list_row.xml (right): https://codereview.chromium.org/1465363002/diff/480001/chrome/android/java/re... chrome/android/java/res/layout/confirm_important_sites_list_row.xml:24: android:textAppearance="@style/ConfirmImportantSitesListTextAppearance" On 2016/05/25 at 18:14:27, Theresa Wellington wrote: > Does this need a whole style or can you just set the textSize on this element directly? Yeah, that's better. https://codereview.chromium.org/1465363002/diff/480001/chrome/browser/android... File chrome/browser/android/preferences/important_sites_util.h (right): https://codereview.chromium.org/1465363002/diff/480001/chrome/browser/android... chrome/browser/android/preferences/important_sites_util.h:22: // example origins for each domain. This can be used for favicons. On 2016/05/25 at 18:14:27, Theresa Wellington wrote: > nit: "... to retrieve favicons." ? Done. https://codereview.chromium.org/1465363002/diff/480001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1465363002/diff/480001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:660: // We reuse the important domains vector to convert example origins to string. On 2016/05/25 at 18:14:27, Theresa Wellington wrote: > nit: s/string/strings done.
https://codereview.chromium.org/1465363002/diff/500001/chrome/android/java/re... File chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml (right): https://codereview.chromium.org/1465363002/diff/500001/chrome/android/java/re... chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml:13: android:paddingTop="7dip" dp https://codereview.chromium.org/1465363002/diff/500001/chrome/android/java/re... chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml:28: android:scrollIndicators="top|bottom" /> Does only the ListView scroll now? What happens if you're in multi-window mode and the available screen height is less than the space the message takes up (unlikely but might happen on some device)? We've had problems in the past with the list being scrollable but the top piece of the dialog not being scrollable. https://codereview.chromium.org/1465363002/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1465363002/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:91: * @param exampleOrigins Example origins for each domain. These can be used for favicons. nit: "... to retrieve favicons" here too. https://codereview.chromium.org/1465363002/diff/500001/chrome/browser/android... File chrome/browser/android/preferences/important_sites_util_unittest.cc (right): https://codereview.chromium.org/1465363002/diff/500001/chrome/browser/android... chrome/browser/android/preferences/important_sites_util_unittest.cc:74: profile(), kNumImportantSites, nullptr) Will you please add a simple test where there are important sites but favicons aren't requested to future-proof passing a nullptr?
https://codereview.chromium.org/1465363002/diff/500001/chrome/android/java/re... File chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml (right): https://codereview.chromium.org/1465363002/diff/500001/chrome/android/java/re... chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml:28: android:scrollIndicators="top|bottom" /> On 2016/05/26 18:08:45, Theresa Wellington wrote: > Does only the ListView scroll now? What happens if you're in multi-window mode > and the available screen height is less than the space the message takes up > (unlikely but might happen on some device)? > > We've had problems in the past with the list being scrollable but the top piece > of the dialog not being scrollable. Per offline-discussion, add a TODO note to handle this later. https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... File chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml (right): https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml:20: <ListView xmlns:android="http://schemas.android.com/apk/res/android" nit: remove the xmlns https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml:30: unnecessary blank line. I found this and some whitespace problems when applying your patch. It should be easy to see the trailing whitespace issues with git show. https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... File chrome/android/java/res/layout/confirm_important_sites_list_row.xml (right): https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/layout/confirm_important_sites_list_row.xml:11: android:minHeight="?attr/listPreferredItemHeightSmall" nit: trailing whitespace on this line https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:10: android:layout_height="match_parent" > nit: remove the space before the > for consistency with the rest of the file https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:86: android:layout_height="wrap_content" nit: trailing white space on this line https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... File chrome/android/java/res/values/values.xml (right): https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/values/values.xml:58: trailing whitespace
https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... File chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml (right): https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml:26: android:scrollbars="vertical" I think this is the default and can be removed. Also, when I was playing around with this scrollIndicators didn't seem to have an effect (does it for you?) but setting android:fadeScrollbars="false" kept the scrollbar always visible (I'm assuming that's the goal). https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml:27: android:textAlignment="viewStart" Also, I think this is the default and can be removed and you may be able to remove android:scrollbars="vertical" as well (it should show up automatically). When I was playing around with this scrollIndicators didn't seem to have an effect (does it for you?) but setting android:fadeScrollbars="false" kept the scrollbar always visible (I'm assuming that's the goal).
Thanks! https://codereview.chromium.org/1465363002/diff/500001/chrome/android/java/re... File chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml (right): https://codereview.chromium.org/1465363002/diff/500001/chrome/android/java/re... chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml:13: android:paddingTop="7dip" On 2016/05/26 at 18:08:45, Theresa Wellington wrote: > dp Done. https://codereview.chromium.org/1465363002/diff/500001/chrome/android/java/re... chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml:28: android:scrollIndicators="top|bottom" /> On 2016/05/26 at 23:21:56, Theresa Wellington wrote: > On 2016/05/26 18:08:45, Theresa Wellington wrote: > > Does only the ListView scroll now? What happens if you're in multi-window mode > > and the available screen height is less than the space the message takes up > > (unlikely but might happen on some device)? > > > > We've had problems in the past with the list being scrollable but the top piece > > of the dialog not being scrollable. > > Per offline-discussion, add a TODO note to handle this later. Done. https://codereview.chromium.org/1465363002/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1465363002/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:91: * @param exampleOrigins Example origins for each domain. These can be used for favicons. On 2016/05/26 at 18:08:45, Theresa Wellington wrote: > nit: "... to retrieve favicons" here too. done. https://codereview.chromium.org/1465363002/diff/500001/chrome/browser/android... File chrome/browser/android/preferences/important_sites_util_unittest.cc (right): https://codereview.chromium.org/1465363002/diff/500001/chrome/browser/android... chrome/browser/android/preferences/important_sites_util_unittest.cc:74: profile(), kNumImportantSites, nullptr) On 2016/05/26 at 18:08:45, Theresa Wellington wrote: > Will you please add a simple test where there are important sites but favicons aren't requested to future-proof passing a nullptr? Done. https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... File chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml (right): https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml:20: <ListView xmlns:android="http://schemas.android.com/apk/res/android" On 2016/05/26 at 23:21:56, Theresa Wellington wrote: > nit: remove the xmlns Done. https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml:26: android:scrollbars="vertical" On 2016/05/26 at 23:44:49, Theresa Wellington wrote: > I think this is the default and can be removed. Also, when I was playing around with this scrollIndicators didn't seem to have an effect (does it for you?) but setting android:fadeScrollbars="false" kept the scrollbar always visible (I'm assuming that's the goal). Done. https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml:27: android:textAlignment="viewStart" On 2016/05/26 at 23:44:49, Theresa Wellington wrote: > Also, I think this is the default and can be removed and you may be able to remove android:scrollbars="vertical" as well (it should show up automatically). > > When I was playing around with this scrollIndicators didn't seem to have an effect (does it for you?) but setting android:fadeScrollbars="false" kept the scrollbar always visible (I'm assuming that's the goal). Done. https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml:30: On 2016/05/26 at 23:21:56, Theresa Wellington wrote: > unnecessary blank line. > > I found this and some whitespace problems when applying your patch. It should be easy to see the trailing whitespace issues with git show. Done. https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... File chrome/android/java/res/layout/confirm_important_sites_list_row.xml (right): https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/layout/confirm_important_sites_list_row.xml:11: android:minHeight="?attr/listPreferredItemHeightSmall" On 2016/05/26 at 23:21:56, Theresa Wellington wrote: > nit: trailing whitespace on this line Done. https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:10: android:layout_height="match_parent" > On 2016/05/26 at 23:21:56, Theresa Wellington wrote: > nit: remove the space before the > for consistency with the rest of the file Done. https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:86: android:layout_height="wrap_content" On 2016/05/26 at 23:21:56, Theresa Wellington wrote: > nit: trailing white space on this line Done. https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... File chrome/android/java/res/values/values.xml (right): https://codereview.chromium.org/1465363002/diff/520001/chrome/android/java/re... chrome/android/java/res/values/values.xml:58: On 2016/05/26 at 23:21:56, Theresa Wellington wrote: > trailing whitespace Done.
https://codereview.chromium.org/1465363002/diff/540001/chrome/android/java/re... File chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml (right): https://codereview.chromium.org/1465363002/diff/540001/chrome/android/java/re... chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml:10: <!-- TODO(dmurph): Handle super small screens. --> Please file a crbug and and include a link to it from this TODO note (so that the TODO doesn't get lost & the fix can be prioritized correctly). Also, can this TODO say something more specific about what needs to be handled? e.g. Handle small screens where the title, message and buttons take up the full height causing the list view to be hidden https://codereview.chromium.org/1465363002/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:358: showImportantDialogThenClear(); Why is the show important sites progress dialog no longer needed?
https://codereview.chromium.org/1465363002/diff/540001/chrome/android/java/re... File chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml (right): https://codereview.chromium.org/1465363002/diff/540001/chrome/android/java/re... chrome/android/java/res/layout/clear_browsing_important_dialog_listview.xml:10: <!-- TODO(dmurph): Handle super small screens. --> On 2016/06/01 at 00:41:32, Theresa Wellington wrote: > Please file a crbug and and include a link to it from this TODO note (so that the TODO doesn't get lost & the fix can be prioritized correctly). > > Also, can this TODO say something more specific about what needs to be handled? > > e.g. Handle small screens where the title, message and buttons take up the full height causing the list view to be hidden Done. https://codereview.chromium.org/1465363002/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:358: showImportantDialogThenClear(); On 2016/06/01 at 00:41:32, Theresa Wellington wrote: > Why is the show important sites progress dialog no longer needed? If we haven't fetched the sites yet, then we just do a regular clear and don't worry about it. This currently has no effect as the fetch is synchronous anyways. Since this is going to be a finch experiment and we're only showing it to a fraction of users anyways, we'd rather just not show it if the sites don't load instead of dealing with UI and new dialogs that add more logic.
https://codereview.chromium.org/1465363002/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:358: showImportantDialogThenClear(); On 2016/06/01 18:16:25, dmurph wrote: > On 2016/06/01 at 00:41:32, Theresa Wellington wrote: > > Why is the show important sites progress dialog no longer needed? > If we haven't fetched the sites yet, then we just do a regular clear and don't > worry about it. This currently has no effect as the fetch is synchronous > anyways. Since this is going to be a finch experiment and we're only showing it > to a fraction of users anyways, we'd rather just not show it if the sites don't > load instead of dealing with UI and new dialogs that add more logic. Will you add a comment explaining that (the first sentence should work, e.g. "If sites haven't been fetched, just clear browsing data regularly rather than showing the important sites dialog.") https://codereview.chromium.org/1465363002/diff/580001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java (right): https://codereview.chromium.org/1465363002/diff/580001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:528: SigninTestUtil.get().addAndSignInTestAccount(); It looks like this class was made fully static & the get() method was removed: https://codereview.chromium.org/2025653002
lgtm % two previous comments
Done, thanks! https://codereview.chromium.org/1465363002/diff/580001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java (right): https://codereview.chromium.org/1465363002/diff/580001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:528: SigninTestUtil.get().addAndSignInTestAccount(); On 2016/06/01 at 20:46:24, Theresa Wellington wrote: > It looks like this class was made fully static & the get() method was removed: https://codereview.chromium.org/2025653002 Done.
https://codereview.chromium.org/1465363002/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:220: mFaviconURLs = args.getStringArray(FAVICON_URLS_TAG); mImportantDomains and mFaviconURLs need to be the same length right? If so, let's add an assert to that effect. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:27: android:text="@string/storage_management_unimportant_site_data_descriptive_text" this string name is a bit on the short side :-P https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:83: android:id="@+id/clear_all_data_section" forgive me for potentially asking a repeated question, but why do you have this LinearLayout? I tried to look for usages of clear_all_data_section and I didn't see anything. Other than applying a top padding, this doesn't seem to do anything as you're already inside of a vertical LinearLayout at this point. Why not add the padding to the first view? Or did I miss the usage somewhere? I was thinking you'd only need this if you wanted a quick way to hide all the views or something, but I couldn't find that. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:1: <?xml version="1.0" encoding="utf-8"?> storage_preference_fragment doesn't seem to be consistent naming. the other case this is used is just website_preferences.xml. maybe this should be storage_preferences.xml or something. Mainly the _fragment bit seems unnecessary. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:10: android:background="@android:color/transparent"> why do you need to specify a background? could you use @null if you must be clearing some default? https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:16: android:paddingTop="0dp" shouldn't 0 be the default padding? why adding these? https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:19: <TextView android:id="@android:id/empty" do you actually need this? I don't see it set anywhere else and the text here is empty, so I don't know if it adds anything. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:92: * favicons. should be aligned with "Example origins " above https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:357: // If sites haven't been fetched, just clear the browsing data regularly rather than This comment threw me off for a bit, but it's actually referring to the case after this conditional right? Maybe it should go afterwards? https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:568: mSortedImportantDomains = Arrays.copyOf(domains, domains.length); why make a copy? https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:79: return mDomains.length; isn't the the default implementation of an ArrayAdapter? https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:107: public void onClick(View view, int position) { no need to be public Should add a simple javadoc as well. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:131: private Drawable getFaviconDrawable(Bitmap icon, int fallbackColor, String url) { If we end up supporting a larger number of items, we should move this image post processing into the LargeIconBridge. We are ending up duplicating the storage with this approach, but it doesn't matter too much with 10 items. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:236: mLargeIconBridge.destroy(); I think this can be null in the case you call dimsiss() when "savedInstanceState != null" happens below https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:244: mProfile = Profile.getLastUsedProfile(); Should we be calling getOriginalProfile() here? If you open an incognito tab and then go to settings, do you get incognito here? https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:259: Intent data = getActivity().getIntent(); Do you need anything from the original intent? If not, I would create a new one instead of adding things to the one that triggered this dialog. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:276: mSitesListView.setOnItemClickListener(new AdapterView.OnItemClickListener() { Any reason you don't have your adapter implement this interface directly? Then you could just set your on click listener to mAdapter https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:174: mUnimportantDialog = null; should you be setting this to null in an onDismiss listener? What happens if you hit back? Or cancel? Can you not trigger this again? https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:260: private int mNumSitesClearing = 0; 0 is the default, so no need to specify that here https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:288: site.clearAllStoredData(this); is this blocking/doing any i/o? Or is this kicking off a background task? Making sure we're not doing anything we shouldn't on the UI thread. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:311: if (!mCategory.showStorageSites()) { :-/ This is sad to have to support a completely different view type for this one setting. I wonder if we should be splitting this out more somehow (subclass or something). I guess this is fine for now, we I would really like to avoid having this class be littered with special cases. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:531: final String test_url = Should use camel casing in java testUrl serverOrigin etc... below https://codereview.chromium.org/1465363002/diff/600001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:63: public Criteria getIsClearButtonEnabledCriteria(final ManageSpaceActivity activity) { I believe the more common practice is to have the polling in the methods as well. These would be: waitForClearButtonEnabled waitForDialogShowing etc... https://codereview.chromium.org/1465363002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:100: public void testLaunchActivity() { we should add a case where it doesn't start the browser process...it's kind of gross. But you need to special case startMainActivity to look for a particular test name and not start the activity in that case (looking to make that less gross somehow). https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... https://codereview.chromium.org/1465363002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:106: final String cookies_url = same naming thing as the other file
Thanks for taking a look Ted! I did everything BUT adding the test where the browser doesn't start yet. That will be in the next patch. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... File chrome/android/java/res/layout/manage_space_activity.xml (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:27: android:text="@string/storage_management_unimportant_site_data_descriptive_text" On 2016/06/06 at 23:53:54, Ted C wrote: > this string name is a bit on the short side :-P shortened. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/manage_space_activity.xml:83: android:id="@+id/clear_all_data_section" On 2016/06/06 at 23:53:54, Ted C wrote: > forgive me for potentially asking a repeated question, but why do you have this LinearLayout? I tried to look for usages of clear_all_data_section and I didn't see anything. > > Other than applying a top padding, this doesn't seem to do anything as you're already inside of a vertical LinearLayout at this point. Why not add the padding to the first view? > > Or did I miss the usage somewhere? I was thinking you'd only need this if you wanted a quick way to hide all the views or something, but I couldn't find that. Yes, since we no longer hide the view when the API is less than kitkat, we don't need this. Now we just assert. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... File chrome/android/java/res/layout/storage_preference_fragment.xml (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2016/06/06 at 23:53:55, Ted C wrote: > storage_preference_fragment doesn't seem to be consistent naming. > > the other case this is used is just website_preferences.xml. maybe this should be storage_preferences.xml or something. Mainly the _fragment bit seems unnecessary. Done. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:10: android:background="@android:color/transparent"> On 2016/06/06 at 23:53:54, Ted C wrote: > why do you need to specify a background? could you use @null if you must be clearing some default? This is from the default layout xml. I can remove. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:16: android:paddingTop="0dp" On 2016/06/06 at 23:53:54, Ted C wrote: > shouldn't 0 be the default padding? why adding these? Copied this from the system layout file. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/storage_preference_fragment.xml:19: <TextView android:id="@android:id/empty" On 2016/06/06 at 23:53:54, Ted C wrote: > do you actually need this? I don't see it set anywhere else and the text here is empty, so I don't know if it adds anything. Yes, when the list is empty we display this text. We store it here: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:92: * favicons. On 2016/06/06 at 23:53:55, Ted C wrote: > should be aligned with "Example origins " above Done. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:357: // If sites haven't been fetched, just clear the browsing data regularly rather than On 2016/06/06 at 23:53:55, Ted C wrote: > This comment threw me off for a bit, but it's actually referring to the case after this conditional right? Maybe it should go afterwards? Yes, done. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:568: mSortedImportantDomains = Arrays.copyOf(domains, domains.length); On 2016/06/06 at 23:53:55, Ted C wrote: > why make a copy? Yes, otherwise we get yelled at by FindErrors or FixErrors or something. It's a vulnerability. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:79: return mDomains.length; On 2016/06/06 at 23:53:55, Ted C wrote: > isn't the the default implementation of an ArrayAdapter? Ah, yeah. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:107: public void onClick(View view, int position) { On 2016/06/06 at 23:53:55, Ted C wrote: > no need to be public > > Should add a simple javadoc as well. Done. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:131: private Drawable getFaviconDrawable(Bitmap icon, int fallbackColor, String url) { On 2016/06/06 at 23:53:55, Ted C wrote: > If we end up supporting a larger number of items, we should move this image post processing into the LargeIconBridge. We are ending up duplicating the storage with this approach, but it doesn't matter too much with 10 items. ok. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:236: mLargeIconBridge.destroy(); On 2016/06/06 at 23:53:55, Ted C wrote: > I think this can be null in the case you call dimsiss() when "savedInstanceState != null" happens below Protected. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:244: mProfile = Profile.getLastUsedProfile(); On 2016/06/06 at 23:53:55, Ted C wrote: > Should we be calling getOriginalProfile() here? If you open an incognito tab and then go to settings, do you get incognito here? Done. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:259: Intent data = getActivity().getIntent(); On 2016/06/06 at 23:53:55, Ted C wrote: > Do you need anything from the original intent? If not, I would create a new one instead of adding things to the one that triggered this dialog. ok. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java:276: mSitesListView.setOnItemClickListener(new AdapterView.OnItemClickListener() { On 2016/06/06 at 23:53:55, Ted C wrote: > Any reason you don't have your adapter implement this interface directly? Then you could just set your on click listener to mAdapter done. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:174: mUnimportantDialog = null; On 2016/06/06 at 23:53:55, Ted C wrote: > should you be setting this to null in an onDismiss listener? > > What happens if you hit back? Or cancel? Can you not trigger this again? I guess it doesn't matter, I don't have to set it to null. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:260: private int mNumSitesClearing = 0; On 2016/06/06 at 23:53:55, Ted C wrote: > 0 is the default, so no need to specify that here Done. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java:288: site.clearAllStoredData(this); On 2016/06/06 at 23:53:55, Ted C wrote: > is this blocking/doing any i/o? Or is this kicking off a background task? Making sure we're not doing anything we shouldn't on the UI thread. Kicks off a background task. This is the same thing we do for per-site clearing. See this method for where we kick off the task: https://cs.chromium.org/chromium/src/chrome/browser/storage/storage_info_fetc... https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:311: if (!mCategory.showStorageSites()) { On 2016/06/06 at 23:53:56, Ted C wrote: > :-/ This is sad to have to support a completely different view type for this one setting. I wonder if we should be splitting this out more somehow (subclass or something). I guess this is fine for now, we I would really like to avoid having this class be littered with special cases. Yes, I believe we'd want to split this out, especially if we start getting more specialized with our info. I'd like to save that for later. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:531: final String test_url = On 2016/06/06 at 23:53:56, Ted C wrote: > Should use camel casing in java > > testUrl > > serverOrigin > > etc... below old habits. Done. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:100: public void testLaunchActivity() { On 2016/06/06 at 23:53:56, Ted C wrote: > we should add a case where it doesn't start the browser process...it's kind of gross. > > But you need to special case startMainActivity to look for a particular test name > and not start the activity in that case (looking to make that less gross somehow). > > https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... Doing in next patch. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivityTest.java:106: final String cookies_url = On 2016/06/06 at 23:53:56, Ted C wrote: > same naming thing as the other file Done.
lgtm Wooooot! This is awesome. But let's split up changes this big in the future :-P https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:568: mSortedImportantDomains = Arrays.copyOf(domains, domains.length); On 2016/06/07 17:57:01, dmurph wrote: > On 2016/06/06 at 23:53:55, Ted C wrote: > > why make a copy? > > Yes, otherwise we get yelled at by FindErrors or FixErrors or something. It's a > vulnerability. Acknowledged. https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java:311: if (!mCategory.showStorageSites()) { On 2016/06/07 17:57:02, dmurph wrote: > On 2016/06/06 at 23:53:56, Ted C wrote: > > :-/ This is sad to have to support a completely different view type for this > one setting. I wonder if we should be splitting this out more somehow (subclass > or something). I guess this is fine for now, we I would really like to avoid > having this class be littered with special cases. > > Yes, I believe we'd want to split this out, especially if we start getting more > specialized with our info. I'd like to save that for later. Agreed https://codereview.chromium.org/1465363002/diff/600001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java (right): https://codereview.chromium.org/1465363002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:531: final String test_url = On 2016/06/07 17:57:02, dmurph wrote: > On 2016/06/06 at 23:53:56, Ted C wrote: > > Should use camel casing in java > > > > testUrl > > > > serverOrigin > > > > etc... below > > old habits. Done. looks like you missed a few https://codereview.chromium.org/1465363002/diff/620001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1465363002/diff/620001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:329: android:icon="@mipmap/app_icon" you shouldn't need the icon as it is defined at the application level and is inherited
Ok, I modified BrowserStartupController to have allowThreadDiskReads for tests. https://codereview.chromium.org/1465363002/diff/620001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1465363002/diff/620001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:329: android:icon="@mipmap/app_icon" On 2016/06/09 at 23:43:19, Ted C wrote: > you shouldn't need the icon as it is defined at the application level and is inherited Done.
still lgtm
lgtm w/ one final comment nit https://codereview.chromium.org/1465363002/diff/640001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java (right): https://codereview.chromium.org/1465363002/diff/640001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:296: // This strict mode modification is needed for tests that test loading the native libraries This isn't actually accurate. This strictmode exception is to cover the case where the browser process is being started asynchronously but not in the main browser flow. The main browser flow will trigger library loading earlier and this will be a no-op, but in the other cases this will need to block on loading libraries. You should hit this strict mode violation if you were to kill chrome and then go to the manage chrome data screen yourself even without being in a test.
https://codereview.chromium.org/1465363002/diff/640001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java (right): https://codereview.chromium.org/1465363002/diff/640001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:296: // This strict mode modification is needed for tests that test loading the native libraries On 2016/06/11 at 00:06:30, Ted C wrote: > This isn't actually accurate. > > This strictmode exception is to cover the case where the browser process is being started asynchronously but not in the main browser flow. The main browser flow will trigger library loading earlier and this will be a no-op, but in the other cases this will need to block on loading libraries. > > You should hit this strict mode violation if you were to kill chrome and then go to the manage chrome data screen yourself even without being in a test. Done.
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, twellington@chromium.org Link to the patchset: https://codereview.chromium.org/1465363002/#ps680001 (title: "fixed comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1465363002/680001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, twellington@chromium.org Link to the patchset: https://codereview.chromium.org/1465363002/#ps700001 (title: "mistake in test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1465363002/700001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, twellington@chromium.org Link to the patchset: https://codereview.chromium.org/1465363002/#ps720001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1465363002/720001
Message was sent while issue was closed.
Committed patchset #37 (id:720001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from
==========
[Storage] Android - ManageSpace UI, Important Origins, and CBD Dialog
See the screenshots in the bug.
This patch does the following:
* Adds an activity for the manageSpaceActivity target in the
AndroidManifest.xml which gives options for managing or clearing
storage.
NOTE: This is only hooked up in API >= KLP, as the API for
clearing an application's app data isn't available until then.
* We have a Dialog protecting the Reset App functionality.
* Tested for clearing unimportant storage.
* Adds a Clear All button to the bottom of the Storage site settings
view. This involves a layout that's almost an exact copy of the
default layout, except we have one button.
* Protected again by a dialog.
* Adds a dialog to the Clear Browsing Data screen, that only shows
up when the user has 'important' origins.
* The user is prompted about these sites, and can uncheck them.
* Test is included, which also double checks that cookies are
not deleted.
BUG=560549,579763
==========
to
==========
[Storage] Android - ManageSpace UI, Important Origins, and CBD Dialog
See the screenshots in the bug.
This patch does the following:
* Adds an activity for the manageSpaceActivity target in the
AndroidManifest.xml which gives options for managing or clearing
storage.
NOTE: This is only hooked up in API >= KLP, as the API for
clearing an application's app data isn't available until then.
* We have a Dialog protecting the Reset App functionality.
* Tested for clearing unimportant storage.
* Adds a Clear All button to the bottom of the Storage site settings
view. This involves a layout that's almost an exact copy of the
default layout, except we have one button.
* Protected again by a dialog.
* Adds a dialog to the Clear Browsing Data screen, that only shows
up when the user has 'important' origins.
* The user is prompted about these sites, and can uncheck them.
* Test is included, which also double checks that cookies are
not deleted.
BUG=560549,579763
Committed: https://crrev.com/05781339eb7e4153776b66bd6353d4b7ba24d0a6
Cr-Commit-Position: refs/heads/master@{#399584}
==========
Message was sent while issue was closed.
Patchset 37 (id:??) landed as https://crrev.com/05781339eb7e4153776b66bd6353d4b7ba24d0a6 Cr-Commit-Position: refs/heads/master@{#399584}
Message was sent while issue was closed.
Patchset #38 (id:740001) has been deleted |
