media: Fix expiry time convertion from base::Time to JS time

third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp

 /*
  * Copyright (C) 2013 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 925 matching lines @@
   // 6. Resolve promise.
   m_closedPromise->resolve(ToV8UndefinedGenerator());
 
   // After this algorithm has run, event handlers for the events queued by
   // this algorithm will be executed, but no further events can be queued.
   // As a result, no messages can be sent by the CDM as a result of closing
   // the session.
   m_isClosed = true;
 }
 
 void MediaKeySession::expirationChanged(double updatedExpiryTimeInMS) {

[majidvp, 2017/01/25 22:29:02]

nit: Please update the function comment for 
WebContentDecryptionModuleSession::expirationChange to call out NaN as a 
valid input and its meaning. 

(Perhaps even s/updatedExpiryTimeInMS/updatedExpiryTimeInMSOrNaN/)  

[xhwang, 2017/01/25 22:34:06]

I looked at quiet_NaN() and isnan() call sites and I didn't see anywhere use the
pattern FooOrNan... I suppose NaN is just a part of double. Like you don't pass
in a number as NumOrZero...

[majidvp, 2017/01/25 22:49:01]

fair enough.

   DVLOG(MEDIA_KEY_SESSION_LOG_LEVEL) << __func__ << "(" << this << ") "
                                      << updatedExpiryTimeInMS;
 
   // From https://w3c.github.io/encrypted-media/#update-expiration:
   // The following steps are run:
   // 1. Let the session be the associated MediaKeySession object.
   // 2. Let expiration time be NaN.
   double expirationTime = std::numeric_limits<double>::quiet_NaN();

[majidvp, 2017/01/25 22:29:02]

nit: how about turning this quient_NaN() value into a constant e.g.,
|NeverExpire| or something like that?
This should help with readability.

[xhwang, 2017/01/25 22:34:06]

Well, if you read the spec language on line 953, it's explicit that this should
be NaN. Then it's in the spec text that NaN means "never expire"...

[majidvp, 2017/01/25 22:49:02]

I am not suggesting to change it from NaN. Rather have a named
constant with that value. In fact, I think this matches the style
guide

[1] https://google.github.io/styleguide/cppguide.html

"If the argument is a literal constant, and the same constant is used in
multiple function calls in a way that tacitly assumes they're the same, you
should use a named constant to make that constraint explicit, and to guarantee
that it holds."

[xhwang, 2017/01/25 23:22:39]

Well... quiet_Nan() is already such a constant name...

Do you mean I should have a const double kDoubleNaN = quiet_NaN()?

 
   // 3. If the new expiration time is not NaN, let expiration time be the
   //    new expiration time in milliseconds since 01 January 1970 UTC.
-  //    (Note that Chromium actually passes 0 to indicate no expiry.)
-  // FIXME: Get Chromium to pass NaN.
-  if (!std::isnan(updatedExpiryTimeInMS) && updatedExpiryTimeInMS != 0.0)
+  if (!std::isnan(updatedExpiryTimeInMS))
     expirationTime = updatedExpiryTimeInMS;
 
   // 4. Set the session's expiration attribute to expiration time.
   m_expiration = expirationTime;
 }
 
 void MediaKeySession::keysStatusesChange(
     const WebVector<WebEncryptedMediaKeyInformation>& keys,
     bool hasAdditionalUsableKey) {
   DVLOG(MEDIA_KEY_SESSION_LOG_LEVEL)
@@ skipping 72 matching lines @@
   visitor->trace(m_asyncEventQueue);
   visitor->trace(m_pendingActions);
   visitor->trace(m_mediaKeys);
   visitor->trace(m_keyStatusesMap);
   visitor->trace(m_closedPromise);
   EventTargetWithInlineData::trace(visitor);
   ContextLifecycleObserver::trace(visitor);
 }
 
 }  // namespace blink