Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Update File.lastModifiedDate to latest spec

RESOLVED FIXED in mozilla19

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: khuey, Assigned: Tomas Dainovec)

Tracking

({dev-doc-complete})

unspecified
mozilla19
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=khuey][lang=C++])

Attachments

(1 attachment, 2 obsolete attachments)

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).
Blocks: 673586
Tomas is interested in working on this.
Assignee: nobody → tomas.dainovec
(Assignee)

Comment 2

5 years ago
Created attachment 666342 [details] [diff] [review]
Possible fix

Comment 3

5 years ago
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-
(Assignee)

Comment 7

5 years ago
Created attachment 667515 [details] [diff] [review]
Possible fix No.2

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+
(Assignee)

Comment 9

5 years ago
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 ;-)
(Assignee)

Comment 11

5 years ago
Created attachment 667970 [details] [diff] [review]
Possible fix No.3

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+
(Assignee)

Comment 13

5 years ago
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!
(Assignee)

Comment 15

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

5 years ago
Keywords: dev-doc-needed
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.