793459 - Update File.lastModifiedDate to latest spec

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://www.w3.org/Bugs/Public/show_bug.cgi?id=17762 changed what we should return for Files that don't have a known lastModifiedDate.  We currently return null, but we should be returning the current date.

This means modifying the methods listed in http://mxr.mozilla.org/mozilla-central/search?string=::GetLastModifiedDate to return date objects with the current time and updating any tests (or maybe writing new ones).

Comment 1

[Saurabh Anand [:sawrubh]]

Tomas is interested in working on this.

Comment 2

[Tomas Dainovec]

Attachment: Possible fix

Comment 3

[Josh Matthews [:jdm]]

Comment on attachment 666342 [details] [diff] [review]
Possible fix

Thanks for the patch, Tomas! I've flagged it for a review by Kyle; you can do that yourself next time: https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_reviews

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 666342 [details] [diff] [review]
Possible fix

Review of attachment 666342 [details] [diff] [review]:
-----------------------------------------------------------------

A JS peer needs to approve the jsapi bits, so I've asked sfink to do that.  Other than the test comment it looks great!  Thanks for taking this on.

::: content/base/test/test_bug403852.html
@@ +41,5 @@
>  ok(d.getTime() == domFile.lastModifiedDate.getTime(), "lastModifiedDate should be the same.");
>  
> +var cf = document.getElementById("canvas").mozGetAsFile("canvFile");
> +d = new Date();
> +ok(d.getTime() == cf.lastModifiedDate.getTime(), "lastModifiedDate of file which does not have last modified date should be current time");

Testing this correctly is kind of tricky, because the clock can advance between calling 'new Date' and getting lastModifiedDate.  The way you should test this is:

var x = new Date();
var y = cf.lastModifiedDate;
var z = new Date();

and then check that x.getTime() <= y.getTime() <= z.getTime()

Comment 5

[Steve Fink [:sfink] [:s:]]

Comment on attachment 666342 [details] [diff] [review]
Possible fix

Review of attachment 666342 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, I think the JSAPI change requires super-review. I've requested it.

David - this adds a JS_NewNowDateObject() alongside the current JS_NewDateObject() and JS_NewDateObjectMsec(). Its behavior matches what the JS-level Date constructor does when passed no arguments -- namely, it creates a Date object for the current time.

::: js/src/jsdate.h
@@ +50,5 @@
>  /*
> + * Construct a new Date Object from the current time.
> + */
> +extern JS_FRIEND_API(JSObject*)
> +js_NewNowDateObjec(JSContext* cx);

Typo: left off the 't' at the end

Comment 6

[David Mandelin [:dmandelin]]

Comment on attachment 666342 [details] [diff] [review]
Possible fix

Review of attachment 666342 [details] [diff] [review]:
-----------------------------------------------------------------

Let's not add a new JSAPI function for this unless absolutely necessary. I think you can just use JS_Now() / PR_USEC_PER_MSEC as the input to JS_NewDateObjectMsec.

Comment 7

[Tomas Dainovec]

Attachment: Possible fix No.2

This one does not include new functions.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 667515 [details] [diff] [review]
Possible fix No.2

Review of attachment 667515 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  r=me with the following changes.

::: content/base/src/nsDOMFile.cpp
@@ +130,4 @@
>  NS_IMETHODIMP
>  nsDOMFileBase::GetLastModifiedDate(JSContext* cx, JS::Value *aLastModifiedDate)
>  {
> +  JSObject* date = JS_NewDateObjectMsec(cx, (JS_Now() / PR_USEC_PER_SEC));

The parentheses around "JS_Now() / PR_USEC_PER_SEC" are unnecessary.

::: content/base/test/test_bug403852.html
@@ +45,5 @@
> +var x = new Date();
> +var y = cf.lastModifiedDate;
> +var z = new Date();
> +
> +ok(x.getTime() <= y.getTime() <= z.getTime(), "lastModifiedDate of file which does not have last modified date should be current time");

This doesn't quite do what you expect.  It compares x.getTime() and y.getTime(), and then compares the boolean result of that comparison to z.getTime().

You need to write two separate statements

ok(x.getTime() <= y.getTime(), ...);
ok(y.getTime() <= z.getTime(), ...);

Comment 9

[Tomas Dainovec]

Wow, this is awkward. I have no idea why I did that :) I copied it from your previous example but didn't realize that it doesn't make any sense in JS.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

No worries.  That's what I get for speaking math instead of code ;-)

Comment 11

[Tomas Dainovec]

Attachment: Possible fix No.3

Fixed the test AND found and fixed another small bug in previous fix.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 667970 [details] [diff] [review]
Possible fix No.3

Review of attachment 667970 [details] [diff] [review]:
-----------------------------------------------------------------

Aha, nice catch.  Amazing what working tests can find ;-)

Comment 13

[Tomas Dainovec]

so can the bug be marked as resolved?

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Not yet, we need to check the patch in to the repository.  I'll do that later today.

Thanks for taking this on!

Comment 15

[Tomas Dainovec]

I'm glad to help :)

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c00efc87bf9

Sorry it took me so long to check this in, the tree was closed most of Friday.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1c00efc87bf9