Disable fullscreen global permission setting in Android.

Disable fullscreen global permission setting in Android.

Fixes a bug where fullscreen *looks* disableable but it's actually
always permitted by default. Forces the global slider to always be
turned on (the user cannot switch it to the off position), and changes
the string from "Allowed" to "Always allowed" to communicate that it
cannot be disabled.

This is a stop-gap measure until we can delete the data (and then this
setting UI will be removed entirely). See bug for details.

Only applies when the simplified-fullscreen-ui flag is enabled (which it
is by default).

BUG=611327
Committed: https://crrev.com/827ed475ee038454ee7dfb2cadba17568fc4a2b5
Cr-Commit-Position: refs/heads/master@{#395513}

[Matt Giuca, 2016-05-19 07:17:33]

mgiuca@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Matt Giuca, 2016-05-19 07:17:33]

Hi Ted,

Sorry to do this at the last minute. This should be landed before tomorrow
(branch point) because it represents a regression from M51, and it can't be
merged (due to string).

If you don't want to land the whole thing just yet but think the string is OK,
let me know and I can just land the string now, and merge the rest later.

Screenshots on the bug.

[Matt Giuca, 2016-05-19 07:18:38]

On 2016/05/19 07:17:33, Matt Giuca wrote:
> Hi Ted,
> 
> Sorry to do this at the last minute. This should be landed before tomorrow
> (branch point) because it represents a regression from M51, and it can't be
> merged (due to string).
> 
> If you don't want to land the whole thing just yet but think the string is OK,
> let me know and I can just land the string now, and merge the rest later.
> 
> Screenshots on the bug.

Alternative CL to l-g-t-m if you think it's OK to land the string but not OK to
land this in its current form:
https://codereview.chromium.org/1990153002

[Finnur, 2016-05-19 11:07:21]

Description was changed from

==========
Disable fullscreen global permission setting in Android.

Fixes a bug where fullscreen *looks* disableable but it's actually
always permitted by default. Forces the global slider to always be
turned on (the user cannot switch it to the off position), and changes
the string from "Allowed" to "Always allowed" to communicate that it
cannot be disabled.

This is a stop-gap measure until we can delete the data (and then this
setting UI will be removed entirely). See bug for details.

Only applies when the simplified-fullscreen-ui flag is enabled (which it
is by default).

BUG=611327
==========

to

==========
Disable fullscreen global permission setting in Android.

Fixes a bug where fullscreen *looks* disableable but it's actually
always permitted by default. Forces the global slider to always be
turned on (the user cannot switch it to the off position), and changes
the string from "Allowed" to "Always allowed" to communicate that it
cannot be disabled.

This is a stop-gap measure until we can delete the data (and then this
setting UI will be removed entirely). See bug for details.

Only applies when the simplified-fullscreen-ui flag is enabled (which it
is by default).

BUG=611327
==========

[Finnur, 2016-05-19 11:07:21]

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

[Finnur, 2016-05-19 11:09:16]

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

[Finnur, 2016-05-19 11:09:17]

Adding Rebecca, the UX person who oversaw the design of this.

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

https://codereview.chromium.org/1989303005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:112:
}
The label decision here looks weird...

If the experiment is off, the text does not alter as you toggle but if the
experiment is on, this toggles between:
Allowed and Always Allowed.

That's... strange.

But, leaving that aside, I would question the statement made in the bug:
"We can't easily remove the global setting checkbox while keeping the exceptions
list."

Those are separate UI entities; the former defined in website_preferences.xml
with key 'read_write_toggle' and that class (SingleWebsitePreferences.java)
already removes some UI elements on demand, so at least there is precedent.

But, it seems to me that the least invasive option, and one that makes most
sense UX-wise is to set the text as 'Always allowed'/'Ask' (depending on the
state of the experiment) and just disable the UI toggle. Right? That's the
general way of communicating that something can't be changed.

[Matt Giuca, 2016-05-19 11:53:59]

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

https://codereview.chromium.org/1989303005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:112:
}
On 2016/05/19 11:09:17, Finnur wrote:
> The label decision here looks weird...
> 
> If the experiment is off, the text does not alter as you toggle but if the
> experiment is on, this toggles between:
> Allowed and Always Allowed.
> 
> That's... strange.

No, if the experiment (which by the way is now the default) is on, it does:
fullscreenEnabledSummary = "Always allowed"
fullscreenDisabledSummary = 0  // Defaults to "Ask", but note that this isn't
visible since the user isn't allowed to disable it when the experiment is on.

If the experiment is off, it does:
fullscreenEnabledSummary = 0 // Defaults to "Allowed"
fullscreenDisabledSummary = "Ask first before allowing sites to enter fullscreen
(recommended)"

> 
> But, leaving that aside, I would question the statement made in the bug:
> "We can't easily remove the global setting checkbox while keeping the
exceptions
> list."
> 
> Those are separate UI entities; the former defined in website_preferences.xml
> with key 'read_write_toggle' and that class (SingleWebsitePreferences.java)
> already removes some UI elements on demand, so at least there is precedent.

Hmm OK I'll have to investigate that. I didn't find a way to selectively disable
the global checkbox without removing the exceptions list (or even what that
would look like).

> 
> But, it seems to me that the least invasive option, and one that makes most
> sense UX-wise is to set the text as 'Always allowed'/'Ask' (depending on the
> state of the experiment) and just disable the UI toggle. Right? That's the
> general way of communicating that something can't be changed.

Yes that's what this code does. (Except I couldn't find a way to disable the UI
toggle either ... as in grey it out. There is literally no method called
anything like setDisabled on the Android permission class for this checkbox; I
checked all the way up the hierarchy.) The best I could find was to make it so
if you click it, it sets itself back to Allowed again. Sorry this is my first
time doing Android UI.

[Matt Giuca, 2016-05-19 11:55:24]

Note what I said in my first comment: if this isn't landed by tomorrow (i.e., in
~12 hours from now), we can't land the new string. So if you think we want that
"Always allowed" string here, then I'd like to at least land
https://codereview.chromium.org/1988553002/.

[Matt Giuca, 2016-05-19 11:55:49]

On 2016/05/19 11:55:24, Matt Giuca wrote:
> Note what I said in my first comment: if this isn't landed by tomorrow (i.e.,
in
> ~12 hours from now), we can't land the new string. So if you think we want
that
> "Always allowed" string here, then I'd like to at least land
> https://codereview.chromium.org/1988553002/.

#9 I mean https://codereview.chromium.org/1990153002/

[Finnur, 2016-05-19 16:13:06]

Looks better in light of your explanation. Would still like to see the toggle
disabled along with these changes. But I approved the other CL (string change),
just in case.

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

https://codereview.chromium.org/1989303005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:112:
}
Ah, I see. Sorry, I misread this. That labeling makes more sense. 

Thinking about this some more, I think removing the toggle altogether would
probably not make sense, because the verbiage of the list below it depends on
the context (when the default is set to Blocked, then the verbiage changes from
Allowed to Exceptions, which doesn't make sense without the toggle).

tedchoc can probably help you better with the disabling. It is EOD here and I
need to run (I would have to investigate it anyway, which takes time, and Ted
might know off-hand).

[Matt Giuca, 2016-05-20 00:44:34]

On 2016/05/19 16:13:06, Finnur wrote:
> Looks better in light of your explanation. Would still like to see the toggle
> disabled along with these changes. But I approved the other CL (string
change),
> just in case.
> 
>
https://codereview.chromium.org/1989303005/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java
> (right):
> 
>
https://codereview.chromium.org/1989303005/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:112:
> }
> Ah, I see. Sorry, I misread this. That labeling makes more sense. 
> 
> Thinking about this some more, I think removing the toggle altogether would
> probably not make sense, because the verbiage of the list below it depends on
> the context (when the default is set to Blocked, then the verbiage changes
from
> Allowed to Exceptions, which doesn't make sense without the toggle).
> 
> tedchoc can probably help you better with the disabling. It is EOD here and I
> need to run (I would have to investigate it anyway, which takes time, and Ted
> might know off-hand).

FYI: The strings have landed. This fix can go in next week.

[Finnur, 2016-05-20 10:04:40]

I just checked and disabling the toggle at the top is easy. As an experiment, I
added:

globalToggle.setEnabled(false);

... towards the top of configureGlobalToggles() in
SingleCategoryPreferences.java, and it worked as expected.

[Matt Giuca, 2016-05-23 05:39:06]

mgiuca@chromium.org changed reviewers:
- rolfe@chromium.org

[Matt Giuca, 2016-05-23 05:39:07]

Hi Finnur,

Thanks for that tip. That did the trick and now the button is greyed out when
the flag is enabled (which is the default). I will attach a screenshot on the
bug momentarily.

PTAL.

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

https://codereview.chromium.org/1989303005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:112:
}
On 2016/05/19 16:13:06, Finnur wrote:
> Ah, I see. Sorry, I misread this. That labeling makes more sense. 
> 
> Thinking about this some more, I think removing the toggle altogether would
> probably not make sense, because the verbiage of the list below it depends on
> the context (when the default is set to Blocked, then the verbiage changes
from
> Allowed to Exceptions, which doesn't make sense without the toggle).
> 
> tedchoc can probably help you better with the disabling. It is EOD here and I
> need to run (I would have to investigate it anyway, which takes time, and Ted
> might know off-hand).

I've simplified the logic now so it shouldn't be so confusing. The disabled
label doesn't need to change (since now that the checkbox is disabled in
ViewsSimplifiedFullscreenUI, it won't be seen at all).

[Matt Giuca, 2016-05-23 05:39:44]

Oh and another thing: now that it's disabled, it isn't clear that we need
"Always allowed" or if we just keeping "Allowed" (while greyed out) is
sufficient. WDYT?

[Finnur, 2016-05-23 10:24:41]

LGTM.

> or if we just keeping "Allowed" (while greyed out) is
> sufficient. WDYT?

I'm fine either way.

[Ted C, 2016-05-23 20:28:54]

On 2016/05/23 10:24:41, Finnur wrote:
> LGTM.
> 
> > or if we just keeping "Allowed" (while greyed out) is
> > sufficient. WDYT?
> 
> I'm fine either way.

lgtm <if you need it>

[Matt Giuca, 2016-05-24 00:41:20]

On 2016/05/23 10:24:41, Finnur wrote:
> LGTM.
> 
> > or if we just keeping "Allowed" (while greyed out) is
> > sufficient. WDYT?
> 
> I'm fine either way.

So is rolfe@, so we'll just keep "Always allowed".

[Matt Giuca, 2016-05-24 00:42:06]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2016-05-24 00:42:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1989303005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1989303005/80001

[commit-bot: I haz the power, 2016-05-24 02:19:09]

Description was changed from

==========
Disable fullscreen global permission setting in Android.

Fixes a bug where fullscreen *looks* disableable but it's actually
always permitted by default. Forces the global slider to always be
turned on (the user cannot switch it to the off position), and changes
the string from "Allowed" to "Always allowed" to communicate that it
cannot be disabled.

This is a stop-gap measure until we can delete the data (and then this
setting UI will be removed entirely). See bug for details.

Only applies when the simplified-fullscreen-ui flag is enabled (which it
is by default).

BUG=611327
==========

to

==========
Disable fullscreen global permission setting in Android.

Fixes a bug where fullscreen *looks* disableable but it's actually
always permitted by default. Forces the global slider to always be
turned on (the user cannot switch it to the off position), and changes
the string from "Allowed" to "Always allowed" to communicate that it
cannot be disabled.

This is a stop-gap measure until we can delete the data (and then this
setting UI will be removed entirely). See bug for details.

Only applies when the simplified-fullscreen-ui flag is enabled (which it
is by default).

BUG=611327
==========

[commit-bot: I haz the power, 2016-05-24 02:19:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-24 02:20:14]

Description was changed from

==========
Disable fullscreen global permission setting in Android.

Fixes a bug where fullscreen *looks* disableable but it's actually
always permitted by default. Forces the global slider to always be
turned on (the user cannot switch it to the off position), and changes
the string from "Allowed" to "Always allowed" to communicate that it
cannot be disabled.

This is a stop-gap measure until we can delete the data (and then this
setting UI will be removed entirely). See bug for details.

Only applies when the simplified-fullscreen-ui flag is enabled (which it
is by default).

BUG=611327
==========

to

==========
Disable fullscreen global permission setting in Android.

Fixes a bug where fullscreen *looks* disableable but it's actually
always permitted by default. Forces the global slider to always be
turned on (the user cannot switch it to the off position), and changes
the string from "Allowed" to "Always allowed" to communicate that it
cannot be disabled.

This is a stop-gap measure until we can delete the data (and then this
setting UI will be removed entirely). See bug for details.

Only applies when the simplified-fullscreen-ui flag is enabled (which it
is by default).

BUG=611327

Committed: https://crrev.com/827ed475ee038454ee7dfb2cadba17568fc4a2b5
Cr-Commit-Position: refs/heads/master@{#395513}
==========

[commit-bot: I haz the power, 2016-05-24 02:20:15]

Patchset 5 (id:??) landed as
https://crrev.com/827ed475ee038454ee7dfb2cadba17568fc4a2b5
Cr-Commit-Position: refs/heads/master@{#395513}