Last Comment Bug 793459 - Update File.lastModifiedDate to latest spec
: Update File.lastModifiedDate to latest spec
Status: RESOLVED FIXED
[mentor=khuey][lang=C++]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla19
Assigned To: Tomas Dainovec
:
:
Mentors:
Depends on:
Blocks: 673586
  Show dependency treegraph
 
Reported: 2012-09-22 18:01 PDT by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2013-03-23 15:21 PDT (History)
6 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Possible fix (3.45 KB, patch)
2012-09-30 10:08 PDT, Tomas Dainovec
khuey: review+
dmandelin: superreview-
Details | Diff | Splinter Review
Possible fix No.2 (1.89 KB, patch)
2012-10-03 09:26 PDT, Tomas Dainovec
khuey: review+
Details | Diff | Splinter Review
Possible fix No.3 (1.93 KB, patch)
2012-10-04 07:42 PDT, Tomas Dainovec
khuey: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-09-22 18:01:22 PDT
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] 2012-09-29 04:47:19 PDT
Tomas is interested in working on this.
Comment 2 Tomas Dainovec 2012-09-30 10:08:25 PDT
Created attachment 666342 [details] [diff] [review]
Possible fix
Comment 3 Josh Matthews [:jdm] 2012-09-30 13:28:21 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-10-01 10:02:37 PDT
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:] (PTO Sep23-28) 2012-10-01 16:04:31 PDT
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] 2012-10-01 17:29:17 PDT
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 2012-10-03 09:26:54 PDT
Created attachment 667515 [details] [diff] [review]
Possible fix No.2

This one does not include new functions.
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-10-03 12:01:44 PDT
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 2012-10-03 14:02:12 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-10-03 14:30:48 PDT
No worries.  That's what I get for speaking math instead of code ;-)
Comment 11 Tomas Dainovec 2012-10-04 07:42:59 PDT
Created attachment 667970 [details] [diff] [review]
Possible fix No.3

Fixed the test AND found and fixed another small bug in previous fix.
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-10-04 07:52:36 PDT
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 2012-10-04 08:07:53 PDT
so can the bug be marked as resolved?
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-10-04 08:12:45 PDT
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 2012-10-04 08:41:37 PDT
I'm glad to help :)
Comment 16 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-10-09 10:22:41 PDT
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] 2012-10-09 18:34:48 PDT
https://hg.mozilla.org/mozilla-central/rev/1c00efc87bf9

Note You need to log in before you can comment on or make changes to this bug.