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

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

This is broken by https://codereview.chromium.org/2593073002/ where the
behavior of converting a null base::Time to JS time (double) has been
changed. We are discussing how to fix the base::Time API more properly.
For now, this check should work.

BUG=679079
TEST=Manually tested. I'll add auto tests later.

Review-Url: https://codereview.chromium.org/2650033007
Cr-Commit-Position: refs/heads/master@{#446246}
Committed: https://chromium.googlesource.com/chromium/src/+/d81adccb0dd24dd7c47ad0d3400dd745150e76db

[xhwang, 2017-01-25 18:00:06]

xhwang@chromium.org changed reviewers:
+ jrummell@chromium.org, majidvp@chromium.org

[xhwang, 2017-01-25 18:00:07]

PTAL!

[xhwang, 2017-01-25 18:00:27]

Description was changed from

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

This is broken by https://codereview.chromium.org/2593073002/ where the
behavior of converting a null base::Time to JS time (double) has been
changed. We are discussing how to fix the base::Time API more properly.
For now, this check should work.

BUG=679079
TEST=Manually tested. I'll add auto tests later.
==========

to

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

This is broken by https://codereview.chromium.org/2593073002/ where the
behavior of converting a null base::Time to JS time (double) has been
changed. We are discussing how to fix the base::Time API more properly.
For now, this check should work.

BUG=679079
TEST=Manually tested. I'll add auto tests later.
==========

[xhwang, 2017-01-25 18:00:35]

miu: FYI

[xhwang, 2017-01-25 18:08:14]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 18:08:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[majidvp, 2017-01-25 18:53:28]

https://codereview.chromium.org/2650033007/diff/1/media/blink/webcontentdecry...
File media/blink/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/2650033007/diff/1/media/blink/webcontentdecry...
media/blink/webcontentdecryptionmodulesession_impl.cc:501: :
new_expiry_time.ToJsTime());
This seems to duplicate some of the functionality in [1].
Can you to just pass in base::Time to 
|expirationChanged| function and then check for null there.


I think eventually |is_null| check should be replace with opationl<Time> 
but this seems a good approach in the interim.


[1]
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/module...

[xhwang, 2017-01-25 19:24:10]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[xhwang, 2017-01-25 19:24:41]

PTAL again!

https://codereview.chromium.org/2650033007/diff/1/media/blink/webcontentdecry...
File media/blink/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/2650033007/diff/1/media/blink/webcontentdecry...
media/blink/webcontentdecryptionmodulesession_impl.cc:501: :
new_expiry_time.ToJsTime());
On 2017/01/25 18:53:28, majidvp wrote:
> This seems to duplicate some of the functionality in [1].
> Can you to just pass in base::Time to 
> |expirationChanged| function and then check for null there.

Previously base::Time wasn't allowed in Blink. Now I checked again, it seems
base/time is not allowed by DEPS
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/DEPS?...

Also, since I need to merge this into M57, I'd like to keep this CL small. That
said, I fixed the FIXME at [1].

> I think eventually |is_null| check should be replace with opationl<Time> 
> but this seems a good approach in the interim.

Actually, the Blink time also has the concept of isNull(). I still think a null
time (corresponding to NaN double) makes sense :)

> [1]
>
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/module...
>

[commit-bot: I haz the power, 2017-01-25 19:24:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 21:01:04]

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

[commit-bot: I haz the power, 2017-01-25 21:01:05]

Dry run: This issue passed the CQ dry run.

[majidvp, 2017-01-25 22:28:51]

lgtm with nits. :)


>
https://codereview.chromium.org/2650033007/diff/1/media/blink/webcontentdecry...
> media/blink/webcontentdecryptionmodulesession_impl.cc:501: :
> new_expiry_time.ToJsTime());
> On 2017/01/25 18:53:28, majidvp wrote:
> > This seems to duplicate some of the functionality in [1].
> > Can you to just pass in base::Time to 
> > |expirationChanged| function and then check for null there.
> 
> Previously base::Time wasn't allowed in Blink. Now I checked again, it seems
> base/time is not allowed by DEPS
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/DEPS?...

There is WTF::Time see wtf/Time.h (But that may not work given that I am
removing 
isNull from it)


> > I think eventually |is_null| check should be replace with opationl<Time> 
> > but this seems a good approach in the interim.
> 
> Actually, the Blink time also has the concept of isNull(). I still think a
null
> time (corresponding to NaN double) makes sense :)

I am removing that here. https://codereview.chromium.org/2656843003/

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

https://codereview.chromium.org/2650033007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp
(right):

https://codereview.chromium.org/2650033007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:946: void
MediaKeySession::expirationChanged(double updatedExpiryTimeInMS) {
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/)

https://codereview.chromium.org/2650033007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:954: double
expirationTime = std::numeric_limits<double>::quiet_NaN();
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]

https://codereview.chromium.org/2650033007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp
(right):

https://codereview.chromium.org/2650033007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:946: void
MediaKeySession::expirationChanged(double updatedExpiryTimeInMS) {
On 2017/01/25 22:29:02, majidvp wrote:
> 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/)  

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...

https://codereview.chromium.org/2650033007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:954: double
expirationTime = std::numeric_limits<double>::quiet_NaN();
On 2017/01/25 22:29:02, majidvp wrote:
> nit: how about turning this quient_NaN() value into a constant e.g.,
> |NeverExpire| or something like that?
> This should help with readability.

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"...

[jrummell, 2017-01-25 22:41:03]

lgtm

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

https://codereview.chromium.org/2650033007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp
(right):

https://codereview.chromium.org/2650033007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:946: void
MediaKeySession::expirationChanged(double updatedExpiryTimeInMS) {
On 2017/01/25 22:34:06, xhwang_slow wrote:
> On 2017/01/25 22:29:02, majidvp wrote:
> > 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/)  
> 
> 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...

fair enough.

https://codereview.chromium.org/2650033007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:954: double
expirationTime = std::numeric_limits<double>::quiet_NaN();
On 2017/01/25 22:34:06, xhwang_slow wrote:
> On 2017/01/25 22:29:02, majidvp wrote:
> > nit: how about turning this quient_NaN() value into a constant e.g.,
> > |NeverExpire| or something like that?
> > This should help with readability.
> 
> 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"...

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]

https://codereview.chromium.org/2650033007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp
(right):

https://codereview.chromium.org/2650033007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:954: double
expirationTime = std::numeric_limits<double>::quiet_NaN();
On 2017/01/25 22:49:02, majidvp wrote:
> On 2017/01/25 22:34:06, xhwang_slow wrote:
> > On 2017/01/25 22:29:02, majidvp wrote:
> > > nit: how about turning this quient_NaN() value into a constant e.g.,
> > > |NeverExpire| or something like that?
> > > This should help with readability.
> > 
> > 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"...
> 
> 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."

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

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

[xhwang, 2017-01-26 06:00:28]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2017-01-26 06:00:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 06:04:34]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485410428910760,
"parent_rev": "b71beb8843deac62f26c8a0753fa5e8b16e63248", "commit_rev":
"d81adccb0dd24dd7c47ad0d3400dd745150e76db"}

[commit-bot: I haz the power, 2017-01-26 06:06:18]

Description was changed from

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

This is broken by https://codereview.chromium.org/2593073002/ where the
behavior of converting a null base::Time to JS time (double) has been
changed. We are discussing how to fix the base::Time API more properly.
For now, this check should work.

BUG=679079
TEST=Manually tested. I'll add auto tests later.
==========

to

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

This is broken by https://codereview.chromium.org/2593073002/ where the
behavior of converting a null base::Time to JS time (double) has been
changed. We are discussing how to fix the base::Time API more properly.
For now, this check should work.

BUG=679079
TEST=Manually tested. I'll add auto tests later.

Review-Url: https://codereview.chromium.org/2650033007
Cr-Commit-Position: refs/heads/master@{#446246}
Committed:
https://chromium.googlesource.com/chromium/src/+/d81adccb0dd24dd7c47ad0d3400d...
==========

[commit-bot: I haz the power, 2017-01-26 06:06:20]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d81adccb0dd24dd7c47ad0d3400d...