media: Invoke PlatformVerificationDialog from ProtectedMediaIdentifierPermissionContext.

chrome/browser/media/protected_media_identifier_permission_context.cc

Index: chrome/browser/media/protected_media_identifier_permission_context.cc
diff --git a/chrome/browser/media/protected_media_identifier_permission_context.cc b/chrome/browser/media/protected_media_identifier_permission_context.cc
index 990be4c913e70ad32e41955653b9c0b37cf8ebbb..b81837f20d01eec7e39670ce65750675291bfc24 100644
--- a/chrome/browser/media/protected_media_identifier_permission_context.cc
+++ b/chrome/browser/media/protected_media_identifier_permission_context.cc
@@ -13,14 +13,31 @@
 #include "content/public/browser/web_contents.h"
 
 #if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/attestation/platform_verification_dialog.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chromeos/settings/cros_settings_names.h"
+#include "ui/views/widget/widget.h"

[Jun Mukai, 2015/02/05 02:08:14]

blank line between #include and using.

[xhwang, 2015/02/05 02:49:34]

Done.

+using chromeos::attestation::PlatformVerificationDialog;
+using chromeos::attestation::PlatformVerificationFlow;
 #endif
 
+namespace {
+
+PermissionRequestID GetInvalidPendingId() {
+  return PermissionRequestID(-1, -1, -1, GURL());
+}
+
+}
+
 ProtectedMediaIdentifierPermissionContext::
     ProtectedMediaIdentifierPermissionContext(Profile* profile)
     : PermissionContextBase(profile,
-                            CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER) {
+                            CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER),
+#if defined(OS_CHROMEOS)
+      pending_id_(GetInvalidPendingId()),

[Jun Mukai, 2015/02/05 02:08:14]

It seems that pending_id_ and widget_ have really similar role. I think you can
remove pending_id_ and use (widget_ == nullptr) for the validation.

[xhwang, 2015/02/05 02:49:34]

There are two reasons I like pending_id_:
1, We don't want to cancel the wrong request. Even if the second request is
rejected, but if the caller didn't get the response immediately (e.g. a callback
is posted), it could happen.
2, We could have an reentrancy issue here. See below.

+      widget_(nullptr),
+#endif
+      weak_factory_(this) {
 }
 
 ProtectedMediaIdentifierPermissionContext::
@@ -30,21 +47,59 @@ ProtectedMediaIdentifierPermissionContext::
 void ProtectedMediaIdentifierPermissionContext::RequestPermission(
     content::WebContents* web_contents,
     const PermissionRequestID& id,
-    const GURL& requesting_frame_origin,
+    const GURL& requesting_origin,
     bool user_gesture,
     const BrowserPermissionCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  if (!IsProtectedMediaIdentifierEnabled()) {
-    NotifyPermissionSet(id,
-                        requesting_frame_origin,
-                        web_contents->GetLastCommittedURL().GetOrigin(),
-                        callback, false, false);
+  GURL embedding_origin = web_contents->GetLastCommittedURL().GetOrigin();
+
+  if (!requesting_origin.is_valid() || !embedding_origin.is_valid() ||
+      !IsProtectedMediaIdentifierEnabled()) {
+    NotifyPermissionSet(id, requesting_origin, embedding_origin,
+                        callback, false /* persist */, false /* granted */);
     return;
   }
 
+#if defined(OS_CHROMEOS)
+  // On ChromeOS, we don't use PermissionContextBase::RequestPermission() which
+  // uses the standard permission infobar/bubble UI. See http://crbug.com/454847
+  // Instead, we check the content setting and show existing platform

[ddorwin, 2015/02/05 02:23:24]

nit: show the...

[xhwang, 2015/02/05 02:49:35]

Done.

+  // verification UI.
+  // TODO(xhwang): Remove when http://crbug.com/454847 is fixed.
+  ContentSetting content_setting =
+      GetPermissionStatus(requesting_origin, embedding_origin);
+
+  switch (content_setting) {
+    case CONTENT_SETTING_BLOCK:
+      callback.Run(false);

[ddorwin, 2015/02/05 02:23:24]

I'm a little concerned about the differences between this and the base. I don't
have any better ideas, though.

[xhwang, 2015/02/05 02:49:35]

Thanks for checking that. Updated to match base.

+      return;
+    case CONTENT_SETTING_ALLOW:
+      callback.Run(true);
+      return;
+    default:
+      break;
+  }
+
+  // We only support one prompt and one pending permission request.

[ddorwin, 2015/02/05 02:23:24]

Reference 447005?

[xhwang, 2015/02/05 02:49:35]

Done.

+  // Reject the new one if there is already one pending.
+  if (!pending_id_.Equals(GetInvalidPendingId())) {
+    callback.Run(false);
+    return;
+  }
+
+  pending_id_ = id;
+  widget_ = PlatformVerificationDialog::ShowDialog(
+      web_contents, requesting_origin,
+      base::Bind(&ProtectedMediaIdentifierPermissionContext::
+                     OnPlatformVerificationResult,
+                 weak_factory_.GetWeakPtr(), id, requesting_origin,
+                 embedding_origin, callback));
+  return;

[ddorwin, 2015/02/05 02:23:24]

#else to avoid unreachable code

[xhwang, 2015/02/05 02:49:34]

Done.

+#endif
+
   PermissionContextBase::RequestPermission(web_contents, id,
-                                           requesting_frame_origin,
+                                           requesting_origin,
                                            user_gesture,
                                            callback);
 }
@@ -59,6 +114,26 @@ ContentSetting ProtectedMediaIdentifierPermissionContext::GetPermissionStatus(
                                                     embedding_origin);
 }
 
+void ProtectedMediaIdentifierPermissionContext::CancelPermissionRequest(
+    content::WebContents* web_contents,
+    const PermissionRequestID& id) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+#if defined(OS_CHROMEOS)
+  if (!widget_ || !pending_id_.Equals(id))

[Jun Mukai, 2015/02/05 02:08:14]

Do we have to care about pending_id_ here?

[ddorwin, 2015/02/05 02:23:24]

Should we DCHECK(pending_id_.Equals(id))?  This would be a code bug, right?

[xhwang, 2015/02/05 02:49:34]

See above.

[xhwang, 2015/02/05 02:49:34]

IN case there are multiple requests, this can be tricky. I didn't fully
inspected the full stack to make sure this should never happen. We will remove
all of these when we have the UI fixed, so I just want to make this code robust
for now.... WDYT?

+    return;
+
+  // Close the |widget_|. OnPlatformVerificationResult() will be fired
+  // during this process, but since |pending_id_| is cleared, the callback will
+  // be dropped.
+  pending_id_ = GetInvalidPendingId();
+  widget_->Close();

[Jun Mukai, 2015/02/05 02:08:14]

then, widget_ = nullptr;

[xhwang, 2015/02/05 02:49:34]

widget->Close() could cause OnPlatformVerificationResult to be called, which
will reenter this class and check |widget_|. So widget_ = nullptr will be too
late here. I hate firing callbacks synchronously but this seems to be the style
we have here.

#1 0x7f69fe495a79 chromeos::attestation::PlatformVerificationDialog::Close()
#2 0x7f69fa1a25d3 views::DialogClientView::CanClose()
#3 0x7f69fa1a64d0 views::NonClientView::CanClose()
#4 0x7f69fa18bb1f views::Widget::Close()

+  return;
+#endif

[ddorwin, 2015/02/05 02:23:24]

Ditto on #else. Or just ifdef the existence of this override.

[xhwang, 2015/02/05 02:49:34]

Done.

+
+  PermissionContextBase::CancelPermissionRequest(web_contents, id);
+}
+
 void ProtectedMediaIdentifierPermissionContext::UpdateTabContext(
     const PermissionRequestID& id,
     const GURL& requesting_frame,
@@ -73,7 +148,6 @@ void ProtectedMediaIdentifierPermissionContext::UpdateTabContext(
     content_settings->OnProtectedMediaIdentifierPermissionSet(
         requesting_frame.GetOrigin(), allowed);
   }
-
 }
 
 // TODO(xhwang): We should consolidate the "protected content" related pref
@@ -101,3 +175,34 @@ bool ProtectedMediaIdentifierPermissionContext::
       << "Protected media identifier disabled by the user or by device policy.";
   return enabled;
 }
+
+#if defined(OS_CHROMEOS)
+void ProtectedMediaIdentifierPermissionContext::OnPlatformVerificationResult(
+    const PermissionRequestID& id,
+    const GURL& requesting_origin,
+    const GURL& embedding_origin,
+    const BrowserPermissionCallback& callback,
+    chromeos::attestation::PlatformVerificationFlow::ConsentResponse response) {
+  DCHECK(widget_);
+  widget_ = nullptr;
+
+  // The request may have been canceled. Drop the callback here.
+  if (!pending_id_.Equals(id))

[ddorwin, 2015/02/05 02:23:24]

ditto on DCHECK

[xhwang, 2015/02/05 02:49:35]

This will happen if CancelPermissionRequest() is called. See l.129.

+    return;
+
+  pending_id_ = GetInvalidPendingId();
+
+  if (response == PlatformVerificationFlow::CONSENT_RESPONSE_NONE) {
+    // Deny request and do not save to content settings.
+    NotifyPermissionSet(id, requesting_origin, embedding_origin, callback,

[ddorwin, 2015/02/05 02:23:24]

Like the base code, this appears to store by requesting and top-level origin,
but that is not how things appear to be presented in
chrome://settings/contentExceptions#popups. Are these actually stored by
combination?

[xhwang, 2015/02/05 02:49:35]

This is what I have in the preference file:

 "content_settings": {
         "pattern_pairs": {
            "http://[*.]www.netflix.com,http://[*.]www.netflix.com": {
               "protected-media-identifier": 1
            }
         },
         "pref_version": 1
      },

Yeah, in chrome://settings/contentExceptions#popups we only have one domain,
hmm....

+                        false,   // Do not save to content settings.
+                        false);  // Do not allow the permission.
+    return;
+  }
+
+  NotifyPermissionSet(
+      id, requesting_origin, embedding_origin, callback,
+      true,  // Save to content settings.
+      response == PlatformVerificationFlow::CONSENT_RESPONSE_ALLOW);
+}
+#endif