Make sure Usage category heading goes away when we clear local data.

Make sure Usage category heading goes away when we clear local data.

BUG=461476
Committed: https://crrev.com/a34a2ff461d6bbe53e79fbf3fce83a0c88041481
Cr-Commit-Position: refs/heads/master@{#318672}

[Finnur, 2015-02-27 14:38:35]

finnur@chromium.org changed reviewers:
+ newt@chromium.org

[newt (away), 2015-02-27 19:26:44]

lgtm after comment

https://codereview.chromium.org/962213002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/962213002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:390:
if (!hasUsagePreferences()) {
Instead of hasUsagePreferences(), I'd directly check if the usage pref is
present:

    Preference usage = preferenceScreen.findPreference(PREF_USAGE)
    if (usage != null) preferenceScreen.removePreference(usage);

That seems safer in case the implementation of hasUsagePreferences() were to
change (e.g. to check whether it *should* have a usage pref, which at this point
would return false).

[Finnur, 2015-03-02 09:18:47]

https://codereview.chromium.org/962213002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/962213002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:390:
if (!hasUsagePreferences()) {
I think there's a bit of a misunderstanding here...

The code is doing the equivalent of:

RemoveClearDataPref();
if (NoMorePrefsUnderUsageCategory())
  RemoveUsageCategory();

What you are proposing is:

RemoveClearDataPref();
if (UsageCategoryVisible())
  RemoveUsageCategory();

That code would be correct today (since the Usage category currently only
contains one pref), but would break if we add a new ones...

Since I'm pretty sure this is correct as-is (and I'm on a different timezone),
I'll check in but am more than willing to do a followup CL if the conclusion of
this discussion is different than I expect.

[Finnur, 2015-03-02 09:19:09]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2015-03-02 09:19:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/962213002/1

[commit-bot: I haz the power, 2015-03-02 09:54:27]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-03-02 09:55:13]

Patchset 1 (id:??) landed as
https://crrev.com/a34a2ff461d6bbe53e79fbf3fce83a0c88041481
Cr-Commit-Position: refs/heads/master@{#318672}

[newt (away), 2015-03-02 19:14:04]

https://codereview.chromium.org/962213002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/962213002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:390:
if (!hasUsagePreferences()) {
On 2015/03/02 09:18:47, Finnur wrote:
> I think there's a bit of a misunderstanding here...
> 
> The code is doing the equivalent of:
> 
> RemoveClearDataPref();
> if (NoMorePrefsUnderUsageCategory())
>   RemoveUsageCategory();
> 
> What you are proposing is:
> 
> RemoveClearDataPref();
> if (UsageCategoryVisible())
>   RemoveUsageCategory();
> 
> That code would be correct today (since the Usage category currently only
> contains one pref), but would break if we add a new ones...
> 
> Since I'm pretty sure this is correct as-is (and I'm on a different timezone),
> I'll check in but am more than willing to do a followup CL if the conclusion
of
> this discussion is different than I expect.

Ah, my bad. I wasn't making the correct distinction between the usage category
title and the single item in that category ("clear data").