Site preferences: merge in storage data and clear screen on reset.

Site preferences: merge in storage data and clear screen on reset.

BUG=464030
Committed: https://crrev.com/94b9f34475d7fc3b94596ec3dc947b5d1c51482b
Cr-Commit-Position: refs/heads/master@{#319604}

[Michael van Ouwerkerk, 2015-03-06 16:09:31]

mvanouwerkerk@chromium.org changed reviewers:
+ miguelg@chromium.org, peter@chromium.org

[Michael van Ouwerkerk, 2015-03-06 16:09:31]

Peter, Miguel, could you take a look please?

[Miguel Garcia, 2015-03-06 16:29:04]

https://codereview.chromium.org/984073003/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/984073003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:514:
for (String key : PERMISSION_PREFERENCE_KEYS) {
I usually prefer to actually do stuff (clear permissions in this case) then
reflect it in the UX

https://codereview.chromium.org/984073003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:520:
mSite.setCookiePermission(null);
If you are going to have an array after all, perhaps it's time to remove all the
set***Permission and add a setPermission(key, value) ...

[Michael van Ouwerkerk, 2015-03-06 16:42:28]

https://codereview.chromium.org/984073003/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/984073003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:514:
for (String key : PERMISSION_PREFERENCE_KEYS) {
On 2015/03/06 16:29:04, Miguel Garcia wrote:
> I usually prefer to actually do stuff (clear permissions in this case) then
> reflect it in the UX

Normally, yes. But as the logic currently is, the screen needs to be cleared
first. Once the storage clearing routines (called below) are complete, they
check whether the screen is empty and if so they finish the activity. Some of
the logic of this class depends on the state of the UI in that way.

https://codereview.chromium.org/984073003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:520:
mSite.setCookiePermission(null);
On 2015/03/06 16:29:04, Miguel Garcia wrote:
> If you are going to have an array after all, perhaps it's time to remove all
the
> set***Permission and add a setPermission(key, value) ...

An appealing idea. However, that would require changes to the Website class. I'm
trying to keep this relatively small so that cherry picking is less likely to
have merge conflicts.

[Miguel Garcia, 2015-03-06 16:45:17]

LGTM

https://codereview.chromium.org/984073003/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/984073003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:514:
for (String key : PERMISSION_PREFERENCE_KEYS) {
OMG

OK, can you file a bug or something to make it more sensible altogether?

On 2015/03/06 16:42:28, Michael van Ouwerkerk wrote:
> On 2015/03/06 16:29:04, Miguel Garcia wrote:
> > I usually prefer to actually do stuff (clear permissions in this case) then
> > reflect it in the UX
> 
> Normally, yes. But as the logic currently is, the screen needs to be cleared
> first. Once the storage clearing routines (called below) are complete, they
> check whether the screen is empty and if so they finish the activity. Some of
> the logic of this class depends on the state of the UI in that way.

https://codereview.chromium.org/984073003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:520:
mSite.setCookiePermission(null);
Yes sorry, I didn't mean right now just add a TODO.

On 2015/03/06 16:42:28, Michael van Ouwerkerk wrote:
> On 2015/03/06 16:29:04, Miguel Garcia wrote:
> > If you are going to have an array after all, perhaps it's time to remove all
> the
> > set***Permission and add a setPermission(key, value) ...
> 
> An appealing idea. However, that would require changes to the Website class.
I'm
> trying to keep this relatively small so that cherry picking is less likely to
> have merge conflicts.

[Michael van Ouwerkerk, 2015-03-09 11:43:16]

Thanks Miguel.

https://codereview.chromium.org/984073003/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/984073003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:514:
for (String key : PERMISSION_PREFERENCE_KEYS) {
On 2015/03/06 16:45:17, Miguel Garcia wrote:
> OMG
> 
> OK, can you file a bug or something to make it more sensible altogether?
> 
> On 2015/03/06 16:42:28, Michael van Ouwerkerk wrote:
> > On 2015/03/06 16:29:04, Miguel Garcia wrote:
> > > I usually prefer to actually do stuff (clear permissions in this case)
then
> > > reflect it in the UX
> > 
> > Normally, yes. But as the logic currently is, the screen needs to be cleared
> > first. Once the storage clearing routines (called below) are complete, they
> > check whether the screen is empty and if so they finish the activity. Some
of
> > the logic of this class depends on the state of the UI in that way.
> 

Done.

https://codereview.chromium.org/984073003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:520:
mSite.setCookiePermission(null);
On 2015/03/06 16:45:17, Miguel Garcia wrote:
> Yes sorry, I didn't mean right now just add a TODO.
> 
> On 2015/03/06 16:42:28, Michael van Ouwerkerk wrote:
> > On 2015/03/06 16:29:04, Miguel Garcia wrote:
> > > If you are going to have an array after all, perhaps it's time to remove
all
> > the
> > > set***Permission and add a setPermission(key, value) ...
> > 
> > An appealing idea. However, that would require changes to the Website class.
> I'm
> > trying to keep this relatively small so that cherry picking is less likely
to
> > have merge conflicts.
> 

Done.

[Michael van Ouwerkerk, 2015-03-09 11:43:21]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2015-03-09 11:43:21]

The patchset sent to the CQ was uploaded after l-g-t-m from miguelg@chromium.org
Link to the patchset: https://codereview.chromium.org/984073003/#ps20001 (title:
"Add todos.")

[commit-bot: I haz the power, 2015-03-09 11:43:32]

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

[commit-bot: I haz the power, 2015-03-09 12:08:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-09 12:09:16]

Patchset 2 (id:??) landed as
https://crrev.com/94b9f34475d7fc3b94596ec3dc947b5d1c51482b
Cr-Commit-Position: refs/heads/master@{#319604}