Last Comment Bug 673586 - Implement lastModifiedDate property for the File interface
: Implement lastModifiedDate property for the File interface
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla15
Assigned To: Doug Turner (:dougt)
:
Mentors:
http://dev.w3.org/2006/webapi/FileAPI...
: 390776 (view as bug list)
Depends on: 793459 793460
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-22 15:28 PDT by Sonny Piers [:sonny]
Modified: 2012-12-02 19:10 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v.1 (4.43 KB, patch)
2012-05-21 09:46 PDT, Doug Turner (:dougt)
mounir: review+
Details | Diff | Splinter Review

Description Sonny Piers [:sonny] 2011-07-22 15:28:04 PDT
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 2011-12-09 15:21:16 PST
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) 2012-05-21 09:46:33 PDT
Created attachment 625670 [details] [diff] [review]
patch v.1
Comment 3 Mounir Lamouri (:mounir) 2012-05-21 10:08:57 PDT
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) 2012-05-21 15:13:54 PDT
>  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) 2012-05-21 15:28:01 PDT
PRTime followup:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/09b1b0fe568a
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-05-22 09:23:15 PDT
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 8 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-26 11:35:57 PDT
*** Bug 390776 has been marked as a duplicate of this bug. ***

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