Closed Bug 673586 Opened 8 years ago Closed 8 years ago

Implement lastModifiedDate property for the File interface

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: sonny, Assigned: dougt)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
FWIW this crashes sample file upload code that works just fine w/Chrome; would be nice to have this working! :)
Assignee: nobody → doug.turner
Attached patch patch v.1Splinter Review
Attachment #625670 - Flags: review?(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.
Attachment #625670 - Flags: review?(mounir) → review+
>  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
https://hg.mozilla.org/mozilla-central/rev/877e541590c2
https://hg.mozilla.org/mozilla-central/rev/09b1b0fe568a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
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);
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.