Don't enable HDCP again if it is active

ui/display/chromeos/display_configurator.cc

Index: ui/display/chromeos/display_configurator.cc
diff --git a/ui/display/chromeos/display_configurator.cc b/ui/display/chromeos/display_configurator.cc
index 19e122566e8412af5a1c2d3a5670a1acaeecf42e..a7443d26be80da504455a68c7d10f58cbb965dff 100644
--- a/ui/display/chromeos/display_configurator.cc
+++ b/ui/display/chromeos/display_configurator.cc
@@ -251,12 +251,18 @@ bool DisplayConfigurator::ApplyProtections(const ContentProtections& requests) {
       case DISPLAY_CONNECTION_TYPE_DISPLAYPORT:
       case DISPLAY_CONNECTION_TYPE_DVI:
       case DISPLAY_CONNECTION_TYPE_HDMI: {
-        HDCPState new_desired_state =
-            (all_desired & CONTENT_PROTECTION_METHOD_HDCP) ?
-                HDCP_STATE_DESIRED : HDCP_STATE_UNDESIRED;
-        if (!native_display_delegate_->SetHDCPState(*it->display,
-                                                    new_desired_state))
+        HDCPState now_state;

[Daniel Erat, 2014/09/18 14:45:30]

nit: current_state

[kcwu, 2014/09/19 04:39:14]

Done.

+        if (!native_display_delegate_->GetHDCPState(*it->display, &now_state))

[Daniel Erat, 2014/09/18 14:45:30]

does this trigger a slow probe of the display?

[dnicoara, 2014/09/18 15:39:16]

crbug.com/374465 tracked an issue where GetHDCP was slow. Though it should be
fixed.

A couple of follow-up questions:
1) Shouldn't the driver handle this correctly? I'm not particularly familiar
with the API contract and documentation is sparse, but it seems that only
certain drivers disable HDCP before setting the new state.
2) Reading the bug suggests that SetHDCP is called multiple times. Is it done in
a polling manner? It has been suggested that the HDCP state shouldn't be polled.
3) Would it make sense to cache the HDCP state?

It would be nice to expand the interface comments to be more explicit what the
expected behavior of these functions should be.

[kcwu, 2014/09/19 04:39:14]

The driver is fixed. See comment #2 in crbug/412841.

I originally assume the driver can handle this, so I could bypass multiple
enable operation. According the bug, maybe we need to prevent another driver's
fault in the future.
Currently, there is no way to get notification of HDCP state change, so HDCP
state need to be polled in case HDCP is disabled by other program for unknown
reason.
As the reason in 2), we may cache the HDCP state for few seconds for performance
but should not cache longer in user side.

           return false;
+        bool now_desired = (now_state != HDCP_STATE_UNDESIRED);

[Daniel Erat, 2014/09/18 14:45:30]

nit: s/now/current/; "new" and "now" look too similar

[kcwu, 2014/09/19 04:39:15]

Done.

+        bool new_desired = (all_desired & CONTENT_PROTECTION_METHOD_HDCP);
+        // Don't enable again if HDCP is already active.
+        if (now_desired != new_desired) {
+          HDCPState new_state =
+              new_desired ? HDCP_STATE_DESIRED : HDCP_STATE_UNDESIRED;
+          if (!native_display_delegate_->SetHDCPState(*it->display, new_state))
+            return false;
+        }
         break;
       }
       case DISPLAY_CONNECTION_TYPE_INTERNAL: