Closed Bug 793459 Opened 12 years ago Closed 12 years ago

Update File.lastModifiedDate to latest spec

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: khuey, Assigned: tomas.dainovec)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [mentor=khuey][lang=C++])

Attachments

(1 file, 2 obsolete files)

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).
Tomas is interested in working on this.
Assignee: nobody → tomas.dainovec
Attached patch Possible fix (obsolete) — Splinter Review
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
Attachment #666342 - Flags: review?(khuey)
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()
Attachment #666342 - Flags: review?(sphink)
Attachment #666342 - Flags: review?(khuey)
Attachment #666342 - Flags: review+
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
Attachment #666342 - Flags: review?(sphink) → superreview?(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.
Attachment #666342 - Flags: superreview?(dmandelin) → superreview-
Attached patch Possible fix No.2 (obsolete) — Splinter Review
This one does not include new functions.
Attachment #666342 - Attachment is obsolete: true
Attachment #667515 - Flags: review?(khuey)
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(), ...);
Attachment #667515 - Flags: review?(khuey) → review+
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.
No worries.  That's what I get for speaking math instead of code ;-)
Fixed the test AND found and fixed another small bug in previous fix.
Attachment #667515 - Attachment is obsolete: true
Attachment #667970 - Flags: review?(khuey)
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 ;-)
Attachment #667970 - Flags: review?(khuey) → review+
so can the bug be marked as resolved?
Not yet, we need to check the patch in to the repository.  I'll do that later today.

Thanks for taking this on!
I'm glad to help :)
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.
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/1c00efc87bf9
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: