673586 - Implement lastModifiedDate property for the File interface

Status: RESOLVED FIXED

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

Description

[Sonny Piers [:sonny]]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110619063910




Expected results:

According to http://www.w3.org/TR/FileAPI/#dfn-file the file interface should have a lastModifiedDate property.

Comment 1

[David E. Weekly]

FWIW this crashes sample file upload code that works just fine w/Chrome; would be nice to have this working! :)

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 3

[Mounir Lamouri (:mounir)]

Comment on attachment 625670 [details] [diff] [review]
patch v.1

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

r=me with the comments applied.

::: content/base/src/nsDOMFile.cpp
@@ +130,5 @@
>  
>  NS_IMETHODIMP
> +nsDOMFileBase::GetLastModifiedDate(JSContext* cx, JS::Value *aLastModifiedDate)
> +{
> +  *aLastModifiedDate = JSVAL_NULL;

Please do this instead:
aLastModifiedDate->setNull();

@@ +131,5 @@
>  NS_IMETHODIMP
> +nsDOMFileBase::GetLastModifiedDate(JSContext* cx, JS::Value *aLastModifiedDate)
> +{
> +  *aLastModifiedDate = JSVAL_NULL;
> +  return NS_OK;

I would whether throw NS_ERROR_NOT_IMPLEMENTED or call NS_ASSERTION. Or both even ;)

@@ +427,5 @@
>  NS_IMETHODIMP
> +nsDOMFileFile::GetLastModifiedDate(JSContext* cx, JS::Value *aLastModifiedDate)
> +{
> +  PRTime msecs;
> +  mFile->GetLastModifiedTime(&msecs);

AFAICT, this is returning a PRInt64 not PRTime. I know PRTime is typedef to PRInt64 but still...

@@ +430,5 @@
> +  PRTime msecs;
> +  mFile->GetLastModifiedTime(&msecs);
> +  JSObject* date = JS_NewDateObjectMsec(cx, msecs);
> +  if (date) {
> +    *aLastModifiedDate = OBJECT_TO_JSVAL(date);

aLastModifiedDate->setObject(date);

@@ +433,5 @@
> +  if (date) {
> +    *aLastModifiedDate = OBJECT_TO_JSVAL(date);
> +  }
> +  else {
> +    *aLastModifiedDate = JSVAL_NULL;

aLastModifieddate->setNull();

::: content/base/test/test_bug403852.html
@@ +33,5 @@
>  var domFile = fileList.files[0];
>  
>  is(domFile.name, "prefs.js", "fileName should be prefs.js");
>  
> +ok(domFile.lastModifiedDate, "lastModifiedDate must be present");

ok("lastModifiedDate" in domFile, "...") would be better.

@@ +34,5 @@
>  
>  is(domFile.name, "prefs.js", "fileName should be prefs.js");
>  
> +ok(domFile.lastModifiedDate, "lastModifiedDate must be present");
> +var d = new Date(testFile.lastModifiedTime);

nit: can you move that line with the block below.

Comment 4

[Doug Turner (:dougt)]

>  this is returning a PRInt64 not PRTime. I know PRTime is typedef to PRInt64 but still...

They are typed the same per docs.


> I would whether throw NS_ERROR_NOT_IMPLEMENTED or call NS_ASSERTION. Or both even ;)

I am not sure what we should do here.  Maybe ask Jonas (the editor?) and followup.


https://hg.mozilla.org/integration/mozilla-inbound/rev/877e541590c2

Comment 5

[Doug Turner (:dougt)]

PRTime followup:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/09b1b0fe568a

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/877e541590c2
https://hg.mozilla.org/mozilla-central/rev/09b1b0fe568a

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 625670 [details] [diff] [review]
patch v.1

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

::: content/base/src/nsDOMFile.cpp
@@ +434,5 @@
> +    *aLastModifiedDate = OBJECT_TO_JSVAL(date);
> +  }
> +  else {
> +    *aLastModifiedDate = JSVAL_NULL;
> +  }

Actually, aLastModifieddate->setObjectOrNull(date);

Comment 9

[Trevor]

https://developer.mozilla.org/en-US/docs/Firefox_15_for_developers
https://developer.mozilla.org/en-US/docs/DOM/File
https://developer.mozilla.org/en-US/docs/DOM/File.lastModifiedDate