Add fullscreen permission controls to settings

Add fullscreen permission controls to settings

See http://crbug/378412 for details
This change adds fullscreen permission control to site settings.
For fullscreen, the settings has to be either ASK or ALLOW.
The settings behavior should match that of the desktop:
1. when a site enters fullscreen, an infobar will be shown to user if permission is not ALLOW.
2. The infobar will be dismissed when user click any button on it, or site leaves fullscreen.

TBR=grt@chromium.org
BUG=378412
Committed: https://crrev.com/95ccf94d5e5f47b4b4fca37cddd4cdca51c01e51
Cr-Commit-Position: refs/heads/master@{#321669}

[qinmin, 2015-03-06 23:12:12]

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

[qinmin, 2015-03-06 23:12:13]

PTAL

[Ted C, 2015-03-12 21:21:34]

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

[Ted C, 2015-03-12 21:21:35]

+finnur for the website settings logic

You should also figure out if this should show up on the site settings popup
that you can see if you click on the lock icon (or long press on an http URL).

That is configured here:
WebsiteSettingsPopupAndroid::SetPermissionInfo

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java
(right):

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java:344:
String origin = UrlUtilities.getOriginForDisplay(new URI(tab.getUrl()), true);
any reason this isn't done inside of the infobar delegate?

Or potentially even down in native to keep it's logic consistent with how other
content settings are handled.

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenInfoBarDelegate.java
(right):

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenInfoBarDelegate.java:20:
FullscreenInfoBarDelegate(FullscreenHtmlApiHandler handler, Tab tab, String
origin) {
Looking at the native side of things, I think maybe this should be a static
method that returns
a delegate to make it clearer that building one of this will show a fullscreen
infobar.

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenInfoBarDelegate.java:41:
public void onFullscreenAllowed(String origin) {
are all of these just called by native?  If so, they can be private.

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

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentPreferences.java:87:
websitePrefs.add(FULLSCREEN_KEY);
why guard this to only api 19+?

https://codereview.chromium.org/986653004/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/986653004/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:13515: +        <message
name="IDS_FULLSCREEN_INFOBAR_TEXT" desc="Text to display on the fullscreen
infobar when a page enters fullscreen.">
Why are there two sets of strings?  Are these used in native code?  And there is
the ability to share strings across native and java as well, so there shouldn't
ever be a need to define them twice.

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/fulls...
File chrome/browser/android/fullscreen/fullscreen_infobar_delegate.cc (right):

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/fulls...
chrome/browser/android/fullscreen/fullscreen_infobar_delegate.cc:24: jlong
LaunchFullscreenInfoBar(
I think we usually put
// static
above functions like this.

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/fulls...
chrome/browser/android/fullscreen/fullscreen_infobar_delegate.cc:27: if
(!tab_android)
We shouldn't allow this on a null tab.  We should check that in java land.

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/fulls...
chrome/browser/android/fullscreen/fullscreen_infobar_delegate.cc:63:
InfoBarService::FromWebContents(tab_android->web_contents())->RemoveInfoBar(
what happens if this isn't called?

Should this class actually hold the web_contents ref itself (and add itself as
an observer for it's destruction) to ensure this will always happen.

That also goes along with my other comment about a consistent way for getting
the URL from the webcontents.

[Finnur, 2015-03-13 10:42:14]

https://codereview.chromium.org/986653004/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/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:56:
public static final String PREF_FULLSCREEN_PERMISSION =
"fullscreen_permission_list";
This list (Website permissions) is in alphabetical order, so please move this up
one line.

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

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:215:
} else if (mFilter.showFullscreenSites(mCategoryFilter)) {
This list is also in alphabetical order (see show*Sites function order), so this
should be on line 207.

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:367:
} else if (mFilter.showFullscreenSites(mCategoryFilter)) {
Same here. Move to line 354.

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:415:
if (mFilter.showCameraMicSites(mCategoryFilter)) return
!prefs.isCameraMicUserModifiable();
This list needs a fullscreen check for managed mode. 
While you are at it, can you move line 415 up to 411 to achieve alphabetical
order here too?

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/prefe...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/prefe...
chrome/browser/android/preferences/website_preference_bridge.cc:150: case 3:
setting = CONTENT_SETTING_ASK; break;
Hmmm, this does not seem right to me.

Category defaults can be set to ASK, but for individual origins the UI should
only support ALLOW and BLOCK. Why is this needed?

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/prefe...
chrome/browser/android/preferences/website_preference_bridge.cc:690: }
There's a host of similar functions on line 170 and down. Can you move this next
to those?

[qinmin, 2015-03-17 23:49:30]

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java
(right):

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java:344:
String origin = UrlUtilities.getOriginForDisplay(new URI(tab.getUrl()), true);
Done. Moved the Origin logic into the native FullscreenInfobarDelegate class

On 2015/03/12 21:21:34, Ted C wrote:
> any reason this isn't done inside of the infobar delegate?
> 
> Or potentially even down in native to keep it's logic consistent with how
other
> content settings are handled.

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenInfoBarDelegate.java
(right):

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenInfoBarDelegate.java:20:
FullscreenInfoBarDelegate(FullscreenHtmlApiHandler handler, Tab tab, String
origin) {
On 2015/03/12 21:21:34, Ted C wrote:
> Looking at the native side of things, I think maybe this should be a static
> method that returns
> a delegate to make it clearer that building one of this will show a fullscreen
> infobar.

Done.

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenInfoBarDelegate.java:41:
public void onFullscreenAllowed(String origin) {
On 2015/03/12 21:21:34, Ted C wrote:
> are all of these just called by native?  If so, they can be private.

Done.

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

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentPreferences.java:87:
websitePrefs.add(FULLSCREEN_KEY);
On 2015/03/12 21:21:35, Ted C wrote:
> why guard this to only api 19+?

Immersive fullscreen is only available for api 19+. 
But since the settings can exist for non-immersive mode, moved it outside the if
statement.

https://codereview.chromium.org/986653004/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/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:56:
public static final String PREF_FULLSCREEN_PERMISSION =
"fullscreen_permission_list";
On 2015/03/13 10:42:13, Finnur wrote:
> This list (Website permissions) is in alphabetical order, so please move this
up
> one line. 

Done.

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

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:215:
} else if (mFilter.showFullscreenSites(mCategoryFilter)) {
On 2015/03/13 10:42:14, Finnur wrote:
> This list is also in alphabetical order (see show*Sites function order), so
this
> should be on line 207.

Done.

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:367:
} else if (mFilter.showFullscreenSites(mCategoryFilter)) {
On 2015/03/13 10:42:14, Finnur wrote:
> Same here. Move to line 354.

Done.

https://codereview.chromium.org/986653004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:415:
if (mFilter.showCameraMicSites(mCategoryFilter)) return
!prefs.isCameraMicUserModifiable();
On 2015/03/13 10:42:14, Finnur wrote:
> This list needs a fullscreen check for managed mode. 
> While you are at it, can you move line 415 up to 411 to achieve alphabetical
> order here too?

Done.

https://codereview.chromium.org/986653004/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/986653004/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:13515: +        <message
name="IDS_FULLSCREEN_INFOBAR_TEXT" desc="Text to display on the fullscreen
infobar when a page enters fullscreen.">
Removed the strings in android_chrome_strings.grd, it is not being used here.

On 2015/03/12 21:21:35, Ted C wrote:
> Why are there two sets of strings?  Are these used in native code?  And there
is
> the ability to share strings across native and java as well, so there
shouldn't
> ever be a need to define them twice.

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/fulls...
File chrome/browser/android/fullscreen/fullscreen_infobar_delegate.cc (right):

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/fulls...
chrome/browser/android/fullscreen/fullscreen_infobar_delegate.cc:24: jlong
LaunchFullscreenInfoBar(
On 2015/03/12 21:21:35, Ted C wrote:
> I think we usually put
> // static
> above functions like this.

Done.

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/fulls...
chrome/browser/android/fullscreen/fullscreen_infobar_delegate.cc:27: if
(!tab_android)
On 2015/03/12 21:21:35, Ted C wrote:
> We shouldn't allow this on a null tab.  We should check that in java land.

Done.

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/fulls...
chrome/browser/android/fullscreen/fullscreen_infobar_delegate.cc:63:
InfoBarService::FromWebContents(tab_android->web_contents())->RemoveInfoBar(
Not necessary. 
InfoBarService is already a WebContentsObserver, and it will remove the infobar
for us if WebContents is gone.

On 2015/03/12 21:21:35, Ted C wrote:
> what happens if this isn't called?
> 
> Should this class actually hold the web_contents ref itself (and add itself as
> an observer for it's destruction) to ensure this will always happen.
> 
> That also goes along with my other comment about a consistent way for getting
> the URL from the webcontents.

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/prefe...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/prefe...
chrome/browser/android/preferences/website_preference_bridge.cc:150: case 3:
setting = CONTENT_SETTING_ASK; break;
BLOCK is not allowed for fullscreen. Check
HostContentSettingsMap::IsSettingAllowedForType(). So for fullscreen, the UI
only supports toggling between ALLOW and ASK.

On 2015/03/13 10:42:14, Finnur wrote:
> Hmmm, this does not seem right to me.
> 
> Category defaults can be set to ASK, but for individual origins the UI should
> only support ALLOW and BLOCK. Why is this needed?

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/prefe...
chrome/browser/android/preferences/website_preference_bridge.cc:690: }
On 2015/03/13 10:42:14, Finnur wrote:
> There's a host of similar functions on line 170 and down. Can you move this
next
> to those?

Done.

[Finnur, 2015-03-18 15:36:26]

... Upload your changes? :)

[qinmin, 2015-03-18 16:18:03]

On 2015/03/18 15:36:26, Finnur wrote:
> ... Upload your changes? :)

Done. Sorry, "git cl upload" stalled at missing OWNERS and I didn't notice that

[Finnur, 2015-03-19 10:35:26]

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/prefe...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/prefe...
chrome/browser/android/preferences/website_preference_bridge.cc:150: case 3:
setting = CONTENT_SETTING_ASK; break;
I suspect we're conflating two things here.

The function you refer to (IsSettingAllowedForType) is used by
SetDefaultContentSetting when setting a default value for a category. In that
specific case, many different combinations are valid, including ASK/ALLOW. And
that's fine... for default values.

This function, however, (SetSettingForOrigin) is not about setting default
values for a whole category, but overrides (of the default value) for individual
origins. It is therefore not governed by IsSettingAllowedForType. If the user is
configuring this value, then the only values that make sense are Allow and Deny.
There's no point for the user in setting it to "Ask me later" when they can make
a decision for this specific origin now: Allow or Block.

If we need to add Ask, then we should at least consult Rebecca Rolfe, who
oversaw the UI design of this to see if that's OK. But lets take a step back...
I don't see any code to support Ask in SingleWebsitePreferences.java, which
makes me think you are showing Allow/Block in the UI for an origin today and it
is simply working. What happens if you comment out this line?

If it JustWorks(tm) then I'd like a comment over the NOTREACHED on line 152, as
in:

  default:
    // Note: CONTENT_SETTINGS_ASK is not and should not be supported.
    NOTREACHED();

But even if it doesn't work, I think a better alternative would be to look into
what it takes to support BLOCK in the FullScreen API.

https://codereview.chromium.org/986653004/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java
(right):

https://codereview.chromium.org/986653004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:541:
PrefServiceBridge.getInstance().isPushNotificationsEnabled());
Thanks! :)

https://codereview.chromium.org/986653004/diff/20001/chrome/browser/android/p...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/986653004/diff/20001/chrome/browser/android/p...
chrome/browser/android/preferences/website_preference_bridge.cc:164: jclass
clazz,
nit: indentation seems off.

[Ted C, 2015-03-19 15:13:56]

On 2015/03/19 10:35:26, Finnur wrote:
>
https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/prefe...
> File chrome/browser/android/preferences/website_preference_bridge.cc (right):
> 
>
https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/prefe...
> chrome/browser/android/preferences/website_preference_bridge.cc:150: case 3:
> setting = CONTENT_SETTING_ASK; break;
> I suspect we're conflating two things here.
> 
> The function you refer to (IsSettingAllowedForType) is used by
> SetDefaultContentSetting when setting a default value for a category. In that
> specific case, many different combinations are valid, including ASK/ALLOW. And
> that's fine... for default values.
> 
> This function, however, (SetSettingForOrigin) is not about setting default
> values for a whole category, but overrides (of the default value) for
individual
> origins. It is therefore not governed by IsSettingAllowedForType. If the user
is
> configuring this value, then the only values that make sense are Allow and
Deny.
> There's no point for the user in setting it to "Ask me later" when they can
make
> a decision for this specific origin now: Allow or Block.
> 
> If we need to add Ask, then we should at least consult Rebecca Rolfe, who
> oversaw the UI design of this to see if that's OK. But lets take a step
back...
> I don't see any code to support Ask in SingleWebsitePreferences.java, which
> makes me think you are showing Allow/Block in the UI for an origin today and
it
> is simply working. What happens if you comment out this line?
> 
> If it JustWorks(tm) then I'd like a comment over the NOTREACHED on line 152,
as
> in:
> 
>   default:
>     // Note: CONTENT_SETTINGS_ASK is not and should not be supported.
>     NOTREACHED();
> 
> But even if it doesn't work, I think a better alternative would be to look
into
> what it takes to support BLOCK in the FullScreen API.
> 
>
https://codereview.chromium.org/986653004/diff/20001/chrome/android/java/src/...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java
> (right):
> 
>
https://codereview.chromium.org/986653004/diff/20001/chrome/android/java/src/...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:541:
> PrefServiceBridge.getInstance().isPushNotificationsEnabled());
> Thanks! :)
> 
>
https://codereview.chromium.org/986653004/diff/20001/chrome/browser/android/p...
> File chrome/browser/android/preferences/website_preference_bridge.cc (right):
> 
>
https://codereview.chromium.org/986653004/diff/20001/chrome/browser/android/p...
> chrome/browser/android/preferences/website_preference_bridge.cc:164: jclass
> clazz,
> nit: indentation seems off.

infobar/fullscreen code - lgtm

Will let finnur sign off on the preferences

[qinmin, 2015-03-19 18:15:00]

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/prefe...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/986653004/diff/1/chrome/browser/android/prefe...
chrome/browser/android/preferences/website_preference_bridge.cc:150: case 3:
setting = CONTENT_SETTING_ASK; break;
I see. I commented out the line and everything still works fine. For single
origin settings on android, we just let user to reset the settings to the
current default value. So there is no need to support ASK in
SingleWebsitePreferences as user can only set the setting to ALLOW or clear it
to DEFAULT later. 
Added the comments to note that CONTENT_SETTING_ASK is not supported.


On 2015/03/19 10:35:26, Finnur wrote:
> I suspect we're conflating two things here.
> 
> The function you refer to (IsSettingAllowedForType) is used by
> SetDefaultContentSetting when setting a default value for a category. In that
> specific case, many different combinations are valid, including ASK/ALLOW. And
> that's fine... for default values.
> 
> This function, however, (SetSettingForOrigin) is not about setting default
> values for a whole category, but overrides (of the default value) for
individual
> origins. It is therefore not governed by IsSettingAllowedForType. If the user
is
> configuring this value, then the only values that make sense are Allow and
Deny.
> There's no point for the user in setting it to "Ask me later" when they can
make
> a decision for this specific origin now: Allow or Block.
> 
> If we need to add Ask, then we should at least consult Rebecca Rolfe, who
> oversaw the UI design of this to see if that's OK. But lets take a step
back...
> I don't see any code to support Ask in SingleWebsitePreferences.java, which
> makes me think you are showing Allow/Block in the UI for an origin today and
it
> is simply working. What happens if you comment out this line?
> 
> If it JustWorks(tm) then I'd like a comment over the NOTREACHED on line 152,
as
> in:
> 
>   default:
>     // Note: CONTENT_SETTINGS_ASK is not and should not be supported.
>     NOTREACHED();
> 
> But even if it doesn't work, I think a better alternative would be to look
into
> what it takes to support BLOCK in the FullScreen API.

https://codereview.chromium.org/986653004/diff/20001/chrome/browser/android/p...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/986653004/diff/20001/chrome/browser/android/p...
chrome/browser/android/preferences/website_preference_bridge.cc:164: jclass
clazz,
On 2015/03/19 10:35:26, Finnur wrote:
> nit: indentation seems off.

Done.

[Finnur, 2015-03-20 15:14:10]

LGTM

[qinmin, 2015-03-20 21:07:59]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-03-20 21:08:00]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/986653004/#ps40001 (title:
"removing ASK from SetSettingForOrigin")

[commit-bot: I haz the power, 2015-03-20 21:08:40]

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

[commit-bot: I haz the power, 2015-03-20 21:14:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-20 21:14:05]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[qinmin, 2015-03-20 22:29:16]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-03-20 22:29:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/986653004/#ps60001 (title:
"rebase")

[commit-bot: I haz the power, 2015-03-20 22:29:47]

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

[commit-bot: I haz the power, 2015-03-20 22:37:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-20 22:37:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[qinmin, 2015-03-20 22:55:37]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2015-03-20 22:56:39]

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

[commit-bot: I haz the power, 2015-03-21 00:34:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-21 00:35:10]

Patchset 4 (id:??) landed as
https://crrev.com/95ccf94d5e5f47b4b4fca37cddd4cdca51c01e51
Cr-Commit-Position: refs/heads/master@{#321669}

[Finnur, 2015-03-30 13:42:23]

There are a couple of problems here. As checked in, this CL will cause a DCHECK
if you open the FullScreen category.

Fortunately, it was easy to track down so I've spun a CL to fix:
https://codereview.chromium.org/1044883002/

However, there are further problems. The FullScreen UI element was not showing
up on the SingleWebsitePreferences page due to the missing entry in its layout
XML (which I also fixed). 

But that gets us to a point where if you can now flip the permission for
FullScreen from Allow to Block, but it DCHECKs in doing so because of
IsSettingAllowedForType. This is what I asked you to test earlier, and since I
didn't hear back I thought it was working fine.

This is a problem you need to figure out in a follow-up CL. My take on it is
that we should support CONTENT_SETTING_BLOCK for FullScreen, but I don't know
that implementation well.

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

https://codereview.chromium.org/986653004/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:56:
public static final String PREF_FULLSCREEN_PERMISSION =
"fullscreen_permission_list";
I missed the fact that you did not update the xml layout file for this class
(single_website_preference.xml). I took care of that in my other CL.

https://codereview.chromium.org/986653004/diff/60001/chrome/browser/android/p...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/986653004/diff/60001/chrome/browser/android/p...
chrome/browser/android/preferences/website_preference_bridge.cc:116: env, list,
jorigin.obj(), jembedder.obj());
This is missing a break statement which results in a DCHECK on line 118. Working
on a fix (that also fixes the positioning of the menu item under Site Settings).