File constructor understands lastModified.

Source/bindings/v8/custom/V8BlobCustomHelpers.cpp

Index: Source/bindings/v8/custom/V8BlobCustomHelpers.cpp
diff --git a/Source/bindings/v8/custom/V8BlobCustomHelpers.cpp b/Source/bindings/v8/custom/V8BlobCustomHelpers.cpp
index ee359371b85b659787f80360eee7cc35104bd2fd..19cea842d643fc8050ea922423c7bbd13ae7af32 100644
--- a/Source/bindings/v8/custom/V8BlobCustomHelpers.cpp
+++ b/Source/bindings/v8/custom/V8BlobCustomHelpers.cpp
@@ -43,9 +43,34 @@ namespace WebCore {
 
 namespace V8BlobCustomHelpers {
 
-bool processBlobPropertyBag(v8::Local<v8::Value> propertyBag, const char* blobClassName, String& contentType, String& endings, v8::Isolate* isolate)
+ParsedProperties::ParsedProperties(bool hasFileProperties)
+    : endings("transparent")
+    , hasFileProperties(hasFileProperties)
+    , m_hasLastModifiedDate(false)
 {
-    ASSERT(endings == "transparent");
+}
+
+double ParsedProperties::lastModifiedDate()
+{
+    ASSERT(this->hasFileProperties);
+    if (!m_hasLastModifiedDate) {
+        m_lastModifiedDate = currentTime();
+        m_hasLastModifiedDate = true;
+    }
+    return m_lastModifiedDate;
+}
+
+void ParsedProperties::setLastModifiedDate(double lastModifiedDate)
+{
+    ASSERT(this->hasFileProperties);
+    m_lastModifiedDate = lastModifiedDate;
+    m_hasLastModifiedDate = true;
+}
+
+bool processBlobPropertyBag(v8::Local<v8::Value> propertyBag, const char* blobClassName, ParsedProperties& parsedProperties, v8::Isolate* isolate)
+{
+    ASSERT(parsedProperties.endings == "transparent");
+    String& endings = parsedProperties.endings;
 
     V8TRYCATCH_RETURN(Dictionary, dictionary, Dictionary(propertyBag, isolate), false);
 
@@ -57,6 +82,7 @@ bool processBlobPropertyBag(v8::Local<v8::Value> propertyBag, const char* blobCl
         }
     }
 
+    String& contentType = parsedProperties.contentType;
     V8TRYCATCH_RETURN(bool, containsType, dictionary.get("type", contentType), false);
     if (containsType) {
         if (!contentType.containsOnlyASCII()) {
@@ -65,6 +91,20 @@ bool processBlobPropertyBag(v8::Local<v8::Value> propertyBag, const char* blobCl
         }
         contentType = contentType.lower();
     }
+
+    if (!parsedProperties.hasFileProperties)
+        return true;
+
+    v8::Local<v8::Value> lastModifiedDate;
+    V8TRYCATCH_RETURN(bool, containsLastModifiedDate, dictionary.get("lastModifiedDate", lastModifiedDate), false);
+    if (containsLastModifiedDate) {
+        if (!lastModifiedDate->IsDate() && !lastModifiedDate->IsNumber()) {

[sof, 2013/11/17 07:42:37]

From spec standpoint, Number appears an extension; is there a compat need for
it? And how do you handle the WebIDL notion of undefined/uncertain Date values?

[pwnall-personal, 2013/11/17 09:01:23]

I was confused by step 3 sub-step 3 in the File constructor steps at
http://dev.w3.org/2006/webapi/FileAPI/#file-constructor

"If the lastModifiedDate member is provided, let d be set to the
lastModifiedDate dictionary member. If it is not provided, set d to the current
date and time (which is the equivalent of Date.now())"

Date.now() returns a number. "new Date()" returns the current date and time in a
Date object.

So I thought this means that means not having a lastModifiedDate is equivalent
to passing {lastModifiedDate: Date.now()}, which means we should handle numbers.
Should I file a bug against the spec for this lack of clarity?

I can remove the support for numbers. I put it in out of fear that, given the
current spec, app developers will try to use this with numbers. I'm really on
the fence though, and will do what you think is best.

[sof, 2013/11/17 12:53:40]

By the time you come to those steps, value conversion will already have
happened. And it is required to fail with a TypeError if the value isn't a Date

  http://heycam.github.io/webidl/#es-Date
There's been some discussion on limiting/removing the use of Date in WebIDL,

  https://www.w3.org/Bugs/Public/show_bug.cgi?id=22824
  https://mail.mozilla.org/pipermail/es-discuss/2013-July/032019.html  (a loong
thread)

I think the spec language you quote sort of reflects that..but as long as the
spec says Date, and there's no upcoming switch to a numeric timestamp instead,
then I think Date is what needs to be supported. 
Others may have an opinion on what makes better sense "going forward".

[pwnall-personal, 2013/11/17 16:02:08]

Thank you for explaining!
I removed the support for numbers.

Based on (cursory) my reading of the current WebIDL, it seems that undefined
dates in WebIDL are represented in JavaScript as Date objects with a NaN value. 
The File API spec says that any Date object must be passed through, so I changed
the code to support passing through dates with a NaN value.

+            throwError(v8SyntaxError, ExceptionMessages::failedToConstruct(blobClassName, "The \"lastModifiedDate\" property must be a Date instance."), isolate);

[sof, 2013/11/17 12:53:40]

If the value conversion fails, a TypeError should be thrown.

[pwnall-personal, 2013/11/17 16:02:08]

Done.
Thank you for catching this!

+            return false;
+        }
+        parsedProperties.setLastModifiedDate(static_cast<double>(lastModifiedDate->NumberValue()) / 1000.0); // ms to seconds
+    }

[sof, 2013/11/17 07:42:37]

If you call setLastModifiedDate(currentTime()) here, I _believe_ you can forgo
the m_hasLast* flag (and you will avoid the uncertainty that currently exist if
the currentTime() sampling that happens in the getter, is done at the right
'current' time.)

[pwnall-personal, 2013/11/17 09:01:23]

Sadly, that is not the case. This method is only called if a dictionary is
provided to the constructor. Otherwise, only the initialization in the struct
constructor takes place.

The only other solution that I thought about is to have an explicit initializer
that I call on an "else" branch on:
* line 67 in
https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8B...
* line 74 in
https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8F...

I thought the 2nd solution is a bit worse, because it requires expressing the
same knowledge about ParsedProperties initialization in two files, instead of
keeping everything localized.

I'll be grateful for any advice on taming this complexity.

Re: the sampling issue. If the current time is sampled, it is always sampled
when the WebCore::File is created. I believe that is at most a few milliseconds
after the JavaScript call to "new File(....)"

[sof, 2013/11/17 12:53:40]

Hmm, you're right - not quite as easy as it first looked :) However, when you do
construct it you will (or can) know if there's a property bag argument to
process or not.

Not sure that leaves you coming out ahead overall, though. (Not a big deal,
overall.)

[pwnall-personal, 2013/11/17 16:02:08]

I got rid of the boolean in release builds, which might make the code slightly
faster.

I looked at adding a constructor flag, and it seemed like I still need to keep
the flag in debug builds, to make sure that the flag passed to the constructor
matches the usage.

Does this look reasonable?

+
     return true;
 }