Closed
Bug 793459
Opened 12 years ago
Closed 12 years ago
Update File.lastModifiedDate to latest spec
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
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)
1.93 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
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).
Assignee | ||
Comment 2•12 years ago
|
||
Comment 3•12 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)
Reporter | ||
Comment 4•12 years ago
|
||
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 5•12 years ago
|
||
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 6•12 years ago
|
||
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•12 years ago
|
||
This one does not include new functions.
Attachment #666342 -
Attachment is obsolete: true
Attachment #667515 -
Flags: review?(khuey)
Reporter | ||
Comment 8•12 years ago
|
||
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•12 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.
Reporter | ||
Comment 10•12 years ago
|
||
No worries. That's what I get for speaking math instead of code ;-)
Assignee | ||
Comment 11•12 years ago
|
||
Fixed the test AND found and fixed another small bug in previous fix.
Attachment #667515 -
Attachment is obsolete: true
Attachment #667970 -
Flags: review?(khuey)
Reporter | ||
Comment 12•12 years ago
|
||
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•12 years ago
|
||
so can the bug be marked as resolved?
Reporter | ||
Comment 14•12 years ago
|
||
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•12 years ago
|
||
I'm glad to help :)
Reporter | ||
Comment 16•12 years ago
|
||
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+
Comment 17•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Updated•12 years ago
|
Keywords: dev-doc-needed
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•