Fix DCHECK and positioning for FullScreen within the Site Settings menu.

Fix DCHECK and positioning for FullScreen within the Site Settings menu.

BUG=378412
Committed: https://crrev.com/1fa4e6a25218b4c73a1618d69bbf8244ec378765
Cr-Commit-Position: refs/heads/master@{#323231}

[Finnur, 2015-03-30 12:00:46]

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

[Finnur, 2015-03-30 12:01:36]

finnur@chromium.org changed reviewers:
- qinmin@chromium.org

[Finnur, 2015-03-30 12:01:36]

Newton, PTAL.

qinmin as FYI.

[Finnur, 2015-03-30 13:43:45]

Min, once this is checked in there is one more problem you need to take care of:
(see my comments on https://codereview.chromium.org/986653004/)

[qinmin, 2015-03-30 15:25:41]

On 2015/03/30 13:43:45, Finnur wrote:
> Min, once this is checked in there is one more problem you need to take care
of:
> (see my comments on https://codereview.chromium.org/986653004/)

what if we don't add Fullscreen settings to single site preference UI?
Currently user can only toggle the default settings, and list a website allowed
for fullscreen as an "exception" in the settings list.  And he can clear that
exception in the default settings.
If we add single site preference UI, user is able to toggle that to BLOCK.
However, Desktop always supports fullscreen api, but never supports BLOCK. So I
am wondering whether android should support BLOCK behavior

[qinmin, 2015-03-30 16:04:17]

On 2015/03/30 15:25:41, qinmin wrote:
> On 2015/03/30 13:43:45, Finnur wrote:
> > Min, once this is checked in there is one more problem you need to take care
> of:
> > (see my comments on https://codereview.chromium.org/986653004/)
> 
> what if we don't add Fullscreen settings to single site preference UI?
> Currently user can only toggle the default settings, and list a website
allowed
> for fullscreen as an "exception" in the settings list.  And he can clear that
> exception in the default settings.
> If we add single site preference UI, user is able to toggle that to BLOCK.
> However, Desktop always supports fullscreen api, but never supports BLOCK. So
I
> am wondering whether android should support BLOCK behavior

Hi, Finnur, here is the current behavior on desktop, you can test it with
http://davidwalsh.name/demo/fullscreen.php.
When clicking the fullscreen button, the desktop shows "allow" and "exit
fullscreen" button.
If user clicks "allow", it will show up s an exception in the fullscreen
settings page.
When user clicks the fullscreen setting to manage the exceptions,  the only
action he can perform is to remove the exception (he cannot change the setting
on that exception)
So the current android behavior matches that of the desktop. Only allow user to
toggle default settings, and single site can show up as an exception in the
default settings page. This is what I reported on #11 in the previous CL when
fixing the comments.
If we allow single site preference UI, user can change the setting to BLOCK. In
that case, android will have a different fullscreen behavior from that of the
desktop. Not sure whether that's desirable.

[Finnur, 2015-03-30 16:09:50]

It wouldn't be a good UX.

When testing this, I navigated to youtube.com and allowed Full Screen.
I then went into Settings \ Site Settings \ Full Screen and saw the Youtube site
listed (as expected -- after all, it has the Full Screen permission).
I clicked that, but then found Full Screen didn't appear as a permission in the
details and got confused. Especially since the Youtube page was empty because
Youtube had not requested any other permissions nor used up local data. So there
was nothing to show.

And that didn't look right.

> So I am wondering whether android should support BLOCK behavior

That's a question for the FullScreen people. If we cannot support that then we
need to do something different. Adding a Delete option for a given permission is
probably what we'd want here. Then it would work like Desktop does today.

[Finnur, 2015-03-30 16:12:22]

> So the current android behavior matches that of the desktop. 

Not sure what you mean by 'current' Android behavior (it has not reached
non-broken state).

[qinmin, 2015-03-30 19:09:33]

On 2015/03/30 16:12:22, Finnur wrote:
> > So the current android behavior matches that of the desktop. 
> 
> Not sure what you mean by 'current' Android behavior (it has not reached
> non-broken state).

This is the current android behavior, which matches your earlier repro:
1. navigate to http://davidwalsh.name/demo/fullscreen.php, hit the "launch
fullscreen" button
2. page enters fullscreen, and an infobar shows up with 2 buttons (exit
fullscreen and allow).
3. if user hit "exit fullscreen", page will leave fullscreen.
4. if user hit "allow", then in settings->site settings->fullscreen, 
davidwalsh.name will be listed as an exception (which is expected).
5. when clicking that site, we show a "clear & reset" button

for 5, it is not very desirable as fullscreen permission is not shown. But since
user gets here through the fullscreen settings page, so it is intuitive that
this will reset the fullscreen permission.
A better approach to match the desktop behavior will be to list the fullscreen
permission under the site, with the "clear & reset" button underneath it. And
the fullscreen permission is not clickable, or the setting is not togglable when
clicked. WDYT?

[qinmin, 2015-03-30 23:40:14]

On 2015/03/30 19:09:33, qinmin wrote:
> On 2015/03/30 16:12:22, Finnur wrote:
> > > So the current android behavior matches that of the desktop. 
> > 
> > Not sure what you mean by 'current' Android behavior (it has not reached
> > non-broken state).
> 
> This is the current android behavior, which matches your earlier repro:
> 1. navigate to http://davidwalsh.name/demo/fullscreen.php, hit the "launch
> fullscreen" button
> 2. page enters fullscreen, and an infobar shows up with 2 buttons (exit
> fullscreen and allow).
> 3. if user hit "exit fullscreen", page will leave fullscreen.
> 4. if user hit "allow", then in settings->site settings->fullscreen, 
> davidwalsh.name will be listed as an exception (which is expected).
> 5. when clicking that site, we show a "clear & reset" button
> 
> for 5, it is not very desirable as fullscreen permission is not shown. But
since
> user gets here through the fullscreen settings page, so it is intuitive that
> this will reset the fullscreen permission.
> A better approach to match the desktop behavior will be to list the fullscreen
> permission under the site, with the "clear & reset" button underneath it. And
> the fullscreen permission is not clickable, or the setting is not togglable
when
> clicked. WDYT?

On 2nd thought, non-clickable permission didn't solve the whole problem as the
only way for user to change the permission is to "clear & reset" all
permissions.
Finnur, I have create a new CL: https://codereview.chromium.org/1049083002/ to
add a delete icon next to the fullscreen permission, so user can reset the
fullscreen permission without resetting the others.

[Finnur, 2015-03-31 12:18:42]

Newton: Friendly ping?

qinmin: I'm fine with adding a delete button, but not the way it is added in
that CL you point to.

We should have a widget that shows that FullScreen is allowed -- and that widget
should look aesthetically like the others on the page. A single Reset Fullscreen
button (as implemented in the CL) does not satisfy that requirement, but see my
proposal on the other CL.

[newt (away), 2015-03-31 17:37:52]

I think the change to single_website_preferences.xml should happen along with
qinmin's CL to allow deleting the fullscreen permission for a site. Otherwise,
some users could end up with fullscreen in the BLOCK state, which could cause
problems.

The rest of this CL lgtm.

[Finnur, 2015-04-01 13:29:03]

OK, I have taken out the entry from the XML to avoid that situation.

But I think the rest of it should go in now, especially the .cc change as I was
seeing the missing break cause a DCHECK regardless of the UI being in.

[Finnur, 2015-04-01 13:29:08]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2015-04-01 13:29:09]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1044883002/#ps40001
(title: "Not show the FullScreen entry on Site Details page")

[commit-bot: I haz the power, 2015-04-01 13:29:19]

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

[commit-bot: I haz the power, 2015-04-01 14:00:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-01 14:01:39]

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