Add fullscreen permission controls to settings

chrome/browser/android/preferences/website_preference_bridge.cc

Index: chrome/browser/android/preferences/website_preference_bridge.cc
diff --git a/chrome/browser/android/preferences/website_preference_bridge.cc b/chrome/browser/android/preferences/website_preference_bridge.cc
index f5429b1581ad863d1a0e0c5ab35cf72e94b59d56..66b599c9abaaeb31c14870205ef51474072308c2 100644
--- a/chrome/browser/android/preferences/website_preference_bridge.cc
+++ b/chrome/browser/android/preferences/website_preference_bridge.cc
@@ -111,6 +111,9 @@ static void GetOrigins(JNIEnv* env,
         Java_WebsitePreferenceBridge_insertPushNotificationIntoList(
             env, list, jorigin.obj(), jembedder.obj());
         break;
+      case CONTENT_SETTINGS_TYPE_FULLSCREEN:
+        Java_WebsitePreferenceBridge_insertFullscreenInfoIntoList(
+            env, list, jorigin.obj(), jembedder.obj());
       default:
         DCHECK(false);
         break;
@@ -144,6 +147,7 @@ static void SetSettingForOrigin(JNIEnv* env,
     case 0: setting = CONTENT_SETTING_DEFAULT; break;
     case 1: setting = CONTENT_SETTING_ALLOW; break;
     case 2: setting = CONTENT_SETTING_BLOCK; break;
+    case 3: setting = CONTENT_SETTING_ASK; break;

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

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?

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

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:

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

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.

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

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:

     default:
       NOTREACHED();
   }
@@ -665,6 +669,26 @@ static void ClearCookieData(JNIEnv* env, jclass clazz, jstring jorigin) {
   site_data_deleter->Run();
 }
 
+static void GetFullscreenOrigins(JNIEnv* env,
+                                  jclass clazz,
+                                  jobject list,
+                                  jboolean managedOnly) {
+  GetOrigins(env, CONTENT_SETTINGS_TYPE_FULLSCREEN, list, managedOnly);
+}
+
+static jint GetFullscreenSettingForOrigin(JNIEnv* env, jclass clazz,
+    jstring origin, jstring embedder) {
+  return GetSettingForOrigin(env, CONTENT_SETTINGS_TYPE_FULLSCREEN,
+                             origin, embedder);
+}
+
+static void SetFullscreenSettingForOrigin(JNIEnv* env, jclass clazz,
+    jstring origin, jstring embedder, jint value) {
+  GURL embedder_url(ConvertJavaStringToUTF8(env, embedder));
+  SetSettingForOrigin(env, CONTENT_SETTINGS_TYPE_FULLSCREEN,
+      origin, ContentSettingsPattern::FromURLNoWildcard(embedder_url), value);
+}

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

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]

Done.

+
 // Register native methods
 bool RegisterWebsitePreferenceBridge(JNIEnv* env) {
   return RegisterNativesImpl(env);