Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
74213009:
File constructor understands lastModified. (Closed)
|
Created:
7 years, 1 month ago by pwnall-personal Modified:
7 years, 1 month ago CC:
blink-reviews, Nils Barth (inactive), kojih, arv+blink, jsbell+bindings_chromium.org, abarth-chromium, marja+watch_chromium.org, adamk+blink_chromium.org, haraken, Nate Chapin, Inactive, sof Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionFile constructor understands lastModified.
BUG=164933
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162362
Patch Set 1 #
Total comments: 10
Patch Set 2 : Removed m_hasLastModifiedDate in release builds. #
Total comments: 6
Patch Set 3 : Removed unnecessary branches for NaN. #
Total comments: 14
Patch Set 4 : Addressed feedback. #Patch Set 5 : Use toWebCoreDate. #
Total comments: 3
Patch Set 6 : Reduce sketchiness of comparison against Date.now() in test. #
Total comments: 4
Patch Set 7 : Removed loop from layout test. #
Total comments: 4
Patch Set 8 : lastModifiedDate returns Date.now() for unknown mtimes. #Patch Set 9 : Updated to latest File API ED. #
Total comments: 14
Patch Set 10 : Addressed feedback. #
Total comments: 2
Patch Set 11 : Addressed feedback. #Messages
Total messages: 42 (0 generated)
I left lastModifiedDate out of the File constructor CL, because I was worried that it is too big. http://dev.w3.org/2006/webapi/FileAPI/#file This CL implements that last bit of the specification. Once it lands, we're compliant. Can you please take a look?
https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8B... File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right): https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8B... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:101: if (!lastModifiedDate->IsDate() && !lastModifiedDate->IsNumber()) { 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? https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8B... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:106: } 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.)
Thank you very much for your feedback! Can you please take a look at my thoughts, while I go learn about WebIDL undefined/uncertain Date values? https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8B... File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right): https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8B... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:101: if (!lastModifiedDate->IsDate() && !lastModifiedDate->IsNumber()) { On 2013/11/17 07:42:37, sof wrote: > 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? 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. https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8B... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:106: } On 2013/11/17 07:42:37, sof wrote: > 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.) 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(....)"
+kinuko I'm happy to review the CL implementation-wise, after the spec-wise discussion settles down.
https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8B... File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right): https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8B... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:101: if (!lastModifiedDate->IsDate() && !lastModifiedDate->IsNumber()) { On 2013/11/17 09:01:23, pwnall wrote: > On 2013/11/17 07:42:37, sof wrote: > > 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? > > 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())" > 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 > 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? > 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. > 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. Others may have an opinion on what makes better sense "going forward". https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8B... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:102: throwError(v8SyntaxError, ExceptionMessages::failedToConstruct(blobClassName, "The \"lastModifiedDate\" property must be a Date instance."), isolate); If the value conversion fails, a TypeError should be thrown. https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8B... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:106: } On 2013/11/17 09:01:23, pwnall wrote: > On 2013/11/17 07:42:37, sof wrote: > > 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.) > > 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. > 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.)
Thank you very much for you feedback! Can you please take another look? https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8B... File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right): https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8B... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:101: if (!lastModifiedDate->IsDate() && !lastModifiedDate->IsNumber()) { On 2013/11/17 12:53:40, sof wrote: > On 2013/11/17 09:01:23, pwnall wrote: > > On 2013/11/17 07:42:37, sof wrote: > > > 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? > > > > 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())" > > > > 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 > > > 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? > > > > 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. > > > 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. > > Others may have an opinion on what makes better sense "going forward". 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. https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8B... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:102: throwError(v8SyntaxError, ExceptionMessages::failedToConstruct(blobClassName, "The \"lastModifiedDate\" property must be a Date instance."), isolate); On 2013/11/17 12:53:40, sof wrote: > If the value conversion fails, a TypeError should be thrown. Done. Thank you for catching this! https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8B... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:106: } On 2013/11/17 12:53:40, sof wrote: > On 2013/11/17 09:01:23, pwnall wrote: > > On 2013/11/17 07:42:37, sof wrote: > > > 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.) > > > > 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. > > > > 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.) 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?
Thanks, this takes care of the issues I came across. Just some details commented on below. Re: Date or timestamps; given how widely implemented the File API is by now, changing lastModifiedDate's type is not going to be trivial. https://codereview.chromium.org/74213009/diff/150001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right): https://codereview.chromium.org/74213009/diff/150001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:114: if (std::isnan(dateValue)) The properties of NaN make this work out without having to check. https://codereview.chromium.org/74213009/diff/150001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/150001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8FileCustom.cpp:90: v8SetReturnValue(info, v8::Date::New(file->lastModifiedDate())); Well spotted (that you need a custom getter.) This actually looks like a bug/limitation in the IDL code generator -- it generating code that assumes that a NaN returned from the object getter is to be given the interpretation that "HTMLInputElement.valueAsDate" has (where an invalid date string maps to null, not a Date with a NaN [[PrimitiveValue]].) Which is fine for Dates that are nullable (like valueAsDate is defined to be), but not without (like here.) Looks easy to fix, but that can be done as a slightly wider followup. I suppose... I'll leave that to the reviewers :-) https://codereview.chromium.org/74213009/diff/150001/Source/core/fileapi/File... File Source/core/fileapi/File.cpp (right): https://codereview.chromium.org/74213009/diff/150001/Source/core/fileapi/File... Source/core/fileapi/File.cpp:165: if (isValidFileTime(m_snapshotModificationTime)) This check looks redundant also, just return "m_snapshotModificationTime * msPerSecond".
Thank you for your feedback! https://codereview.chromium.org/74213009/diff/150001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right): https://codereview.chromium.org/74213009/diff/150001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:114: if (std::isnan(dateValue)) On 2013/11/17 17:16:34, sof wrote: > The properties of NaN make this work out without having to check. Done. Thank you! https://codereview.chromium.org/74213009/diff/150001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/150001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8FileCustom.cpp:90: v8SetReturnValue(info, v8::Date::New(file->lastModifiedDate())); On 2013/11/17 17:16:34, sof wrote: > Well spotted (that you need a custom getter.) > > This actually looks like a bug/limitation in the IDL code generator -- it > generating code that assumes that a NaN returned from the object getter is to be > given the interpretation that "HTMLInputElement.valueAsDate" has (where an > invalid date string maps to null, not a Date with a NaN [[PrimitiveValue]].) > Which is fine for Dates that are nullable (like valueAsDate is defined to be), > but not without (like here.) > > Looks easy to fix, but that can be done as a slightly wider followup. I > suppose... I'll leave that to the reviewers :-) I'd be happy to change the code generator to account for non-nullable Date instances, if that is desirable. I'm slightly biased towards making it happen in a different CL, so there's less to revert if something breaks, but I will do what the reviewers think is best. https://codereview.chromium.org/74213009/diff/150001/Source/core/fileapi/File... File Source/core/fileapi/File.cpp (right): https://codereview.chromium.org/74213009/diff/150001/Source/core/fileapi/File... Source/core/fileapi/File.cpp:165: if (isValidFileTime(m_snapshotModificationTime)) On 2013/11/17 17:16:34, sof wrote: > This check looks redundant also, just return "m_snapshotModificationTime * > msPerSecond". Done. Thank you!
On 2013/11/17 17:16:34, sof wrote: > Re: Date or timestamps; given how widely implemented the File API is by now, > changing lastModifiedDate's type is not going to be trivial. > Sorry, I forgot to answer to this earlier! I do not want to change the type of the lastModifiedDate property. Based on my understanding of the spec, lastModifiedDate uses the File object's construction time to represent an unknown date. This means that lastModifiedDate will never return "null", so we don't need the NaN -> null conversion that the default binding code for lastModifiedDate gives us. The File constructor on the other hand is relatively new. The specification is still an ED, and we're shipping the constructor under the experimental Web platform features flag. I think this means there's room to switch to accepting both Date instances and numbers, if the specification's editor chooses to do that. FWIW, Mozilla's Boris Zbarsky thinks that the right thing to do is to make the lastModifiedDate property of FilePropertyBag a long long, so that it would accept both Date instances and numbers. https://bugzilla.mozilla.org/show_bug.cgi?id=939509#c2 Last, I think the difference between the two choices is ~2 lines of code and ~3 lines of LayoutTests, so I can easily update the code to reflect changes in the spec.
On 2013/11/17 09:42:59, haraken wrote: > +kinuko > > I'm happy to review the CL implementation-wise, after the spec-wise discussion > settles down. haraken, kinuko: Given the ongoing discussion on the spec, I think we should be conservative and have lastModifiedDate only accept Date instances. It's easier to relax requirements than it is to tighten them later on.
LGTM for bindings/. File API experts should take another look. https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right): https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:113: double dateValue = static_cast<double>(v8::Local<v8::Date>::Cast(lastModifiedDate)->ValueOf()); You can use toWebCoreDate() in V8Binding.h. https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8BlobCustomHelpers.h (right): https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.h:44: struct ParsedProperties { We normally use class instead of struct for this kind of non-trivial structure. https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.h:63: ParsedProperties(bool hasFileProperties); Add 'explicit'. https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8FileCustom.cpp:90: v8SetReturnValue(info, v8::Date::New(file->lastModifiedDate())); I'm not sure how we should treat a case where file->lastModifiedDate() returns infinite. In many cases, we want to return v8::Null() when the date is infinite, but it depends on the spec. Please check the spec and consider using v8DateOrNull() in V8Binding.h.
Thank you very much for your feedback! I apologize, but I'm still not sure how to act on it in a couple of places. Can you please take another look? https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right): https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:113: double dateValue = static_cast<double>(v8::Local<v8::Date>::Cast(lastModifiedDate)->ValueOf()); On 2013/11/18 10:27:48, haraken wrote: > > You can use toWebCoreDate() in V8Binding.h. I avoided toWebCoreDate() because it does an unnecessary check for the value's type and then it uses NumberValue(), and the v8::Date class definition in src/v8/include/v8.h says that ValueOf() should be preferred instead of NumberValue(). Is there a general rule for navigating this kind of clarity/performance trade-offs in the bindings code? https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8BlobCustomHelpers.h (right): https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.h:44: struct ParsedProperties { On 2013/11/18 10:27:48, haraken wrote: > > We normally use class instead of struct for this kind of non-trivial structure. Done. I also turned the function that took a ParsedProperty reference into a method, and shortened the code by a tiny bit. https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.h:63: ParsedProperties(bool hasFileProperties); On 2013/11/18 10:27:48, haraken wrote: > > Add 'explicit'. Done. Thank you! https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8FileCustom.cpp:90: v8SetReturnValue(info, v8::Date::New(file->lastModifiedDate())); On 2013/11/18 10:27:48, haraken wrote: > > I'm not sure how we should treat a case where file->lastModifiedDate() returns > infinite. In many cases, we want to return v8::Null() when the date is infinite, > but it depends on the spec. Please check the spec and consider using > v8DateOrNull() in V8Binding.h. I think the specs are a bit messy and confusing here. I added references to the relevant specs explaining why we need to return Date instances with a value of NaN here. According to my understanding, Section 7.2 of the File API mandates returning the current date/time, not null, when the modification time is not available. Sections 7.1 and 7.2 together say we need to pass through any WebIDL Date that we get in the constructor, including the undefined Date. Personally, I think that the WebIDL concept of an undefined Date is confusing, at least in combination with the File API. At the same time, I think that being predictable (spec-compliant) overrides most other concerns.
https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right): https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:113: double dateValue = static_cast<double>(v8::Local<v8::Date>::Cast(lastModifiedDate)->ValueOf()); On 2013/11/18 11:23:45, pwnall wrote: > On 2013/11/18 10:27:48, haraken wrote: > > > > You can use toWebCoreDate() in V8Binding.h. > > I avoided toWebCoreDate() because it does an unnecessary check for the value's > type and then it uses NumberValue(), and the v8::Date class definition in > src/v8/include/v8.h says that ValueOf() should be preferred instead of > NumberValue(). > > Is there a general rule for navigating this kind of clarity/performance > trade-offs in the bindings code? We want to use utility methods in V8Binding.h as much as possible, since direct use of V8 APIs are error-prone. Would you update the implementation of toWebCoreDate() so that it uses ValueOf(), and use toWebCoreDate() here? I agree it's doing an extra check but this code wouldn't be performance-sensitive. https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8FileCustom.cpp:90: v8SetReturnValue(info, v8::Date::New(file->lastModifiedDate())); On 2013/11/18 11:23:45, pwnall wrote: > On 2013/11/18 10:27:48, haraken wrote: > > > > I'm not sure how we should treat a case where file->lastModifiedDate() returns > > infinite. In many cases, we want to return v8::Null() when the date is > infinite, > > but it depends on the spec. Please check the spec and consider using > > v8DateOrNull() in V8Binding.h. > > I think the specs are a bit messy and confusing here. I added references to the > relevant specs explaining why we need to return Date instances with a value of > NaN here. > > According to my understanding, Section 7.2 of the File API mandates returning > the current date/time, not null, when the modification time is not available. > > Sections 7.1 and 7.2 together say we need to pass through any WebIDL Date that > we get in the constructor, including the undefined Date. > > Personally, I think that the WebIDL concept of an undefined Date is confusing, > at least in combination with the File API. At the same time, I think that being > predictable (spec-compliant) overrides most other concerns. Yeah, I might want to defer the decision to File API experts.
Sorry for coming in late, https://codereview.chromium.org/74213009/diff/280001/LayoutTests/fast/files/f... File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/280001/LayoutTests/fast/files/f... LayoutTests/fast/files/file-constructor.html:101: shouldBeTrue("Math.abs(Date.now() - (new File([], 'world.html')).lastModifiedDate) <= 1000"); Feels slightly sketchy, can this be like: 1. Date.now() 2. new File 3. Date.now() again and verify 1 <= 2 <= 3? https://codereview.chromium.org/74213009/diff/340002/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/340002/Source/bindings/v8/custo... Source/bindings/v8/custom/V8FileCustom.cpp:94: // "null". Ah sorry, here we could be given NaN when last modified date for the backing file is not known, when the file is created not by constructor but for a real file by the embedder or client, i.e. by chromium. This was done purely for internal implementation convenience and not related to spec at all. When we implemented this part the spec was saying we should return null in such a case (http://www.w3.org/TR/2012/WD-FileAPI-20120712/#dfn-lastModifiedDate), but looks like the latest spec says we should set Date.now() in the case?
Thank you very much for your feedback! Can you please take another look? https://codereview.chromium.org/74213009/diff/280001/LayoutTests/fast/files/f... File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/280001/LayoutTests/fast/files/f... LayoutTests/fast/files/file-constructor.html:101: shouldBeTrue("Math.abs(Date.now() - (new File([], 'world.html')).lastModifiedDate) <= 1000"); On 2013/11/18 12:21:43, kinuko wrote: > Feels slightly sketchy, can this be like: 1. Date.now() 2. new File 3. > Date.now() again and verify 1 <= 2 <= 3? Done. Is the DST robustness overkill? https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right): https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:113: double dateValue = static_cast<double>(v8::Local<v8::Date>::Cast(lastModifiedDate)->ValueOf()); On 2013/11/18 11:29:07, haraken wrote: > On 2013/11/18 11:23:45, pwnall wrote: > > On 2013/11/18 10:27:48, haraken wrote: > > > > > > You can use toWebCoreDate() in V8Binding.h. > > > > I avoided toWebCoreDate() because it does an unnecessary check for the value's > > type and then it uses NumberValue(), and the v8::Date class definition in > > src/v8/include/v8.h says that ValueOf() should be preferred instead of > > NumberValue(). > > > > Is there a general rule for navigating this kind of clarity/performance > > trade-offs in the bindings code? > > We want to use utility methods in V8Binding.h as much as possible, since direct > use of V8 APIs are error-prone. > > Would you update the implementation of toWebCoreDate() so that it uses > ValueOf(), and use toWebCoreDate() here? I agree it's doing an extra check but > this code wouldn't be performance-sensitive. Done. https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8FileCustom.cpp:90: v8SetReturnValue(info, v8::Date::New(file->lastModifiedDate())); On 2013/11/18 11:29:07, haraken wrote: > On 2013/11/18 11:23:45, pwnall wrote: > > On 2013/11/18 10:27:48, haraken wrote: > > > > > > I'm not sure how we should treat a case where file->lastModifiedDate() > returns > > > infinite. In many cases, we want to return v8::Null() when the date is > > infinite, > > > but it depends on the spec. Please check the spec and consider using > > > v8DateOrNull() in V8Binding.h. > > > > I think the specs are a bit messy and confusing here. I added references to > the > > relevant specs explaining why we need to return Date instances with a value of > > NaN here. > > > > According to my understanding, Section 7.2 of the File API mandates returning > > the current date/time, not null, when the modification time is not available. > > > > Sections 7.1 and 7.2 together say we need to pass through any WebIDL Date that > > we get in the constructor, including the undefined Date. > > > > Personally, I think that the WebIDL concept of an undefined Date is confusing, > > at least in combination with the File API. At the same time, I think that > being > > predictable (spec-compliant) overrides most other concerns. > > Yeah, I might want to defer the decision to File API experts. I will wait for kinuko's feedback. Thank you for your guidance! https://codereview.chromium.org/74213009/diff/340002/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/340002/Source/bindings/v8/custo... Source/bindings/v8/custom/V8FileCustom.cpp:94: // "null". On 2013/11/18 12:21:43, kinuko wrote: > Ah sorry, here we could be given NaN when last modified date for the backing > file is not known, when the file is created not by constructor but for a real > file by the embedder or client, i.e. by chromium. This was done purely for > internal implementation convenience and not related to spec at all. When we > implemented this part the spec was saying we should return null in such a case > (http://www.w3.org/TR/2012/WD-FileAPI-20120712/#dfn-lastModifiedDate), but looks > like the latest spec says we should set Date.now() in the case? That is my understanding of the spec too. I think there is one source of confusion in our codebase, namely invalidFileTime() / isValidFileTime() in Source/platform/FileMetadata.h. The existence of this API makes me believe that the value used to represent an invalid file time can be changed simply by changing the numbers here. In fact, we have a few code paths that rely on this value being a NaN. At the same time, it really makes sense to have NaN represent an invalid time, given how JavaScript and WebIDL define invalid Dates. Should I add a comment?
https://codereview.chromium.org/74213009/diff/340002/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/340002/Source/bindings/v8/custo... Source/bindings/v8/custom/V8FileCustom.cpp:94: // "null". On 2013/11/18 12:48:57, pwnall wrote: > On 2013/11/18 12:21:43, kinuko wrote: > > Ah sorry, here we could be given NaN when last modified date for the backing > > file is not known, when the file is created not by constructor but for a real > > file by the embedder or client, i.e. by chromium. This was done purely for > > internal implementation convenience and not related to spec at all. When we > > implemented this part the spec was saying we should return null in such a case > > (http://www.w3.org/TR/2012/WD-FileAPI-20120712/#dfn-lastModifiedDate), but > looks > > like the latest spec says we should set Date.now() in the case? > > That is my understanding of the spec too. > > I think there is one source of confusion in our codebase, namely > invalidFileTime() / isValidFileTime() in Source/platform/FileMetadata.h. > > The existence of this API makes me believe that the value used to represent an > invalid file time can be changed simply by changing the numbers here. In fact, > we have a few code paths that rely on this value being a NaN. At the same time, > it really makes sense to have NaN represent an invalid time, given how > JavaScript and WebIDL define invalid Dates. Should I add a comment? Sounds good. https://codereview.chromium.org/74213009/diff/410002/LayoutTests/fast/files/f... File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/410002/LayoutTests/fast/files/f... LayoutTests/fast/files/file-constructor.html:107: continue; // Time went backwards. (DST) Ah oh. Right time could go backwards... maybe we can just skip the tests in that case?
https://codereview.chromium.org/74213009/diff/410002/LayoutTests/fast/files/f... File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/410002/LayoutTests/fast/files/f... LayoutTests/fast/files/file-constructor.html:107: continue; // Time went backwards. (DST) On 2013/11/18 13:27:09, kinuko wrote: > Ah oh. Right time could go backwards... maybe we can just skip the tests in that > case? I just started doing that, then realized that skipping the shouldBeTrue calls would mean the text output wouldn't match :( How about me submitting another CL that pulls this out to a shouldBeNow function in js-test.js?
https://codereview.chromium.org/74213009/diff/410002/LayoutTests/fast/files/f... File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/410002/LayoutTests/fast/files/f... LayoutTests/fast/files/file-constructor.html:107: continue; // Time went backwards. (DST) On 2013/11/18 13:47:34, pwnall wrote: > On 2013/11/18 13:27:09, kinuko wrote: > > Ah oh. Right time could go backwards... maybe we can just skip the tests in > that > > case? > > I just started doing that, then realized that skipping the shouldBeTrue calls > would mean the text output wouldn't match :( > > How about me submitting another CL that pulls this out to a shouldBeNow function > in js-test.js? I see, sounds good, could we then remove this DST looping for now? (Instead we could leave a FIXME comment that it could fail) This code should be fine but I don't want to introduce potential infinite loop in the test code.
https://codereview.chromium.org/74213009/diff/410002/LayoutTests/fast/files/f... File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/410002/LayoutTests/fast/files/f... LayoutTests/fast/files/file-constructor.html:107: continue; // Time went backwards. (DST) On 2013/11/18 13:57:58, kinuko wrote: > On 2013/11/18 13:47:34, pwnall wrote: > > On 2013/11/18 13:27:09, kinuko wrote: > > > Ah oh. Right time could go backwards... maybe we can just skip the tests in > > that > > > case? > > > > I just started doing that, then realized that skipping the shouldBeTrue calls > > would mean the text output wouldn't match :( > > > > How about me submitting another CL that pulls this out to a shouldBeNow > function > > in js-test.js? > > I see, sounds good, could we then remove this DST looping for now? (Instead we > could leave a FIXME comment that it could fail) This code should be fine but I > don't want to introduce potential infinite loop in the test code. Done. Good point, thank you!
Thanks, this patch almost lgtm now. https://codereview.chromium.org/74213009/diff/460002/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/460002/Source/bindings/v8/custo... Source/bindings/v8/custom/V8FileCustom.cpp:104: v8SetReturnValue(info, v8::Date::New(file->lastModifiedDate())); (Going back to this one) so for NaN case should we return Date.now() given that we use NaN for invalid case and if we basically try to follow the File API?
https://codereview.chromium.org/74213009/diff/460002/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/460002/Source/bindings/v8/custo... Source/bindings/v8/custom/V8FileCustom.cpp:104: v8SetReturnValue(info, v8::Date::New(file->lastModifiedDate())); On 2013/11/18 14:16:59, kinuko wrote: > (Going back to this one) so for NaN case should we return Date.now() given that > we use NaN for invalid case and if we basically try to follow the File API? Thank you for pointing this out again! I think I just realized the full extent of what you're trying to say. Sorry for being slow! If NaN means "return Date.now()", then we need to figure out how to handle the case "lastModifiedDate: new Date(NaN)". Right now, I think the most reasonable thing to do is to consider that Step 3.3 in Section 7.1 of the File API excludes invalid dates. Specifically, I would interpret it as: "If the lastModifiedDate member is provided, and it is not the invalid Date, let d be set to the lastModifiedDate dictionary member." In this case, I'd set lastModifiedDate to the time when the File object is constructed. Does this seem reasonable?
https://codereview.chromium.org/74213009/diff/460002/LayoutTests/fast/files/f... File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/460002/LayoutTests/fast/files/f... LayoutTests/fast/files/file-constructor.html:104: // FIXME: these checks will fail if time goes backwards, e.g. DST. We should have a generic bullet-proof method for comparing against Date.now(). Drive-by comment: Date.now() is ms since epoch in UTC, so there should be no DST issue. Things like the clock on the machine being reset by NTP or user action could affect the value, though.
On 2013/11/18 20:54:05, jsbell wrote: > https://codereview.chromium.org/74213009/diff/460002/LayoutTests/fast/files/f... > File LayoutTests/fast/files/file-constructor.html (right): > > https://codereview.chromium.org/74213009/diff/460002/LayoutTests/fast/files/f... > LayoutTests/fast/files/file-constructor.html:104: // FIXME: these checks will > fail if time goes backwards, e.g. DST. We should have a generic bullet-proof > method for comparing against Date.now(). > Drive-by comment: > > Date.now() is ms since epoch in UTC, so there should be no DST issue. > > Things like the clock on the machine being reset by NTP or user action could > affect the value, though. Ah oh, good catch!
On 2013/11/18 14:44:34, pwnall wrote: > https://codereview.chromium.org/74213009/diff/460002/Source/bindings/v8/custo... > File Source/bindings/v8/custom/V8FileCustom.cpp (right): > > https://codereview.chromium.org/74213009/diff/460002/Source/bindings/v8/custo... > Source/bindings/v8/custom/V8FileCustom.cpp:104: v8SetReturnValue(info, > v8::Date::New(file->lastModifiedDate())); > On 2013/11/18 14:16:59, kinuko wrote: > > (Going back to this one) so for NaN case should we return Date.now() given > that > > we use NaN for invalid case and if we basically try to follow the File API? > > Thank you for pointing this out again! I think I just realized the full extent > of what you're trying to say. Sorry for being slow! > > If NaN means "return Date.now()", then we need to figure out how to handle the > case "lastModifiedDate: new Date(NaN)". > > Right now, I think the most reasonable thing to do is to consider that Step 3.3 > in Section 7.1 of the File API excludes invalid dates. Specifically, I would > interpret it as: > "If the lastModifiedDate member is provided, and it is not the invalid Date, let > d be set to the lastModifiedDate dictionary member." > > In this case, I'd set lastModifiedDate to the time when the File object is > constructed. Does this seem reasonable? I'm split which way we should do for constructor case, but I think it's ok to assume so (as in most browsers Date(NaN) is used to indicate 'invalid date'), at least until the spec's updated to have more exact behavior for NaN case.
Thank you for your feedback and thoughts! Based on the conversation in the spec bug below, it seems like some changes are coming, and the NaN date case will go away one way or another. https://www.w3.org/Bugs/Public/show_bug.cgi?id=23853 I think what we have right now is useful, because it gives developers a way to set lastModifiedDate. Once the specification changes settle, I will submit a CL that implementing them. What do you think? https://codereview.chromium.org/74213009/diff/460002/LayoutTests/fast/files/f... File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/460002/LayoutTests/fast/files/f... LayoutTests/fast/files/file-constructor.html:104: // FIXME: these checks will fail if time goes backwards, e.g. DST. We should have a generic bullet-proof method for comparing against Date.now(). On 2013/11/18 20:54:05, jsbell wrote: > Drive-by comment: > > Date.now() is ms since epoch in UTC, so there should be no DST issue. > > Things like the clock on the machine being reset by NTP or user action could > affect the value, though. > > > Thank you very much for pointing this out! I updated the comment and the CL I put together for making this more robust. https://codereview.chromium.org/69813015
On 2013/11/19 09:10:11, pwnall wrote: > Thank you for your feedback and thoughts! > > Based on the conversation in the spec bug below, it seems like some changes are > coming, and the NaN date case will go away one way or another. > https://www.w3.org/Bugs/Public/show_bug.cgi?id=23853 > > I think what we have right now is useful, because it gives developers a way to > set lastModifiedDate. Once the specification changes settle, I will submit a CL > that implementing them. > > What do you think? Thanks for the link and also for filing a bug. I was re-reading the spec(s) and thread. I think you've been referring to the latest editor's draft: http://dev.w3.org/2006/webapi/FileAPI/#file-constructor rather than the one in LCWD status: http://www.w3.org/TR/FileAPI/#file-constructor And it looks like the ED seems to have been already revised? It now says that the FilePropertyBag takes long long 'lastModified' value rather than the Date value. At this time I'm not fully convinced about landing this patch, since the draft this CL is based on doesn't seem stable enough to implement. Feeling sorry to tell this but could we wait until the discussion settles down, or do you have a strong need for this feature?
On 2013/11/19 10:38:43, kinuko wrote: > On 2013/11/19 09:10:11, pwnall wrote: > > Thank you for your feedback and thoughts! > > > > Based on the conversation in the spec bug below, it seems like some changes > are > > coming, and the NaN date case will go away one way or another. > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=23853 > > > > I think what we have right now is useful, because it gives developers a way to > > set lastModifiedDate. Once the specification changes settle, I will submit a > CL > > that implementing them. > > > > What do you think? > > Thanks for the link and also for filing a bug. I was re-reading the spec(s) and > thread. > > I think you've been referring to the latest editor's draft: > http://dev.w3.org/2006/webapi/FileAPI/#file-constructor > > rather than the one in LCWD status: > http://www.w3.org/TR/FileAPI/#file-constructor > > And it looks like the ED seems to have been already revised? It now says that > the FilePropertyBag takes long long 'lastModified' value rather than the Date > value. At this time I'm not fully convinced about landing this patch, since the > draft this CL is based on doesn't seem stable enough to implement. Feeling > sorry to tell this but could we wait until the discussion settles down, or do > you have a strong need for this feature? I'm happy to do what makes the reviewers' life easier :) I will update the patch in the meantime. Should I remove "lastModifiedDate", or implement both "lastModified" and "lastModifiedDate", and have the latter output a deprecation warning?
On 2013/11/19 10:51:53, pwnall wrote: > On 2013/11/19 10:38:43, kinuko wrote: > > On 2013/11/19 09:10:11, pwnall wrote: > > > Thank you for your feedback and thoughts! > > > > > > Based on the conversation in the spec bug below, it seems like some changes > > are > > > coming, and the NaN date case will go away one way or another. > > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=23853 > > > > > > I think what we have right now is useful, because it gives developers a way > to > > > set lastModifiedDate. Once the specification changes settle, I will submit a > > CL > > > that implementing them. > > > > > > What do you think? > > > > Thanks for the link and also for filing a bug. I was re-reading the spec(s) > and > > thread. > > > > I think you've been referring to the latest editor's draft: > > http://dev.w3.org/2006/webapi/FileAPI/#file-constructor > > > > rather than the one in LCWD status: > > http://www.w3.org/TR/FileAPI/#file-constructor > > > > And it looks like the ED seems to have been already revised? It now says that > > the FilePropertyBag takes long long 'lastModified' value rather than the Date > > value. At this time I'm not fully convinced about landing this patch, since > the > > draft this CL is based on doesn't seem stable enough to implement. Feeling > > sorry to tell this but could we wait until the discussion settles down, or do > > you have a strong need for this feature? > > I'm happy to do what makes the reviewers' life easier :) > > I will update the patch in the meantime. > > Should I remove "lastModifiedDate", or implement both "lastModified" and > "lastModifiedDate", and have the latter output a deprecation warning? I don't think supporting lastModifiedDate makes a lot sense since the version of the spec doesn't even have its own link. Does Firefox support the version? Do you know if he's/they're going to update the implementation?
On 2013/11/19 11:16:09, kinuko wrote: > On 2013/11/19 10:51:53, pwnall wrote: > > On 2013/11/19 10:38:43, kinuko wrote: > > > On 2013/11/19 09:10:11, pwnall wrote: > > > > Thank you for your feedback and thoughts! > > > > > > > > Based on the conversation in the spec bug below, it seems like some > changes > > > are > > > > coming, and the NaN date case will go away one way or another. > > > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=23853 > > > > > > > > I think what we have right now is useful, because it gives developers a > way > > to > > > > set lastModifiedDate. Once the specification changes settle, I will submit > a > > > CL > > > > that implementing them. > > > > > > > > What do you think? > > > > > > Thanks for the link and also for filing a bug. I was re-reading the spec(s) > > and > > > thread. > > > > > > I think you've been referring to the latest editor's draft: > > > http://dev.w3.org/2006/webapi/FileAPI/#file-constructor > > > > > > rather than the one in LCWD status: > > > http://www.w3.org/TR/FileAPI/#file-constructor > > > > > > And it looks like the ED seems to have been already revised? It now says > that > > > the FilePropertyBag takes long long 'lastModified' value rather than the > Date > > > value. At this time I'm not fully convinced about landing this patch, since > > the > > > draft this CL is based on doesn't seem stable enough to implement. Feeling > > > sorry to tell this but could we wait until the discussion settles down, or > do > > > you have a strong need for this feature? > > > > I'm happy to do what makes the reviewers' life easier :) > > > > I will update the patch in the meantime. > > > > Should I remove "lastModifiedDate", or implement both "lastModified" and > > "lastModifiedDate", and have the latter output a deprecation warning? > > I don't think supporting lastModifiedDate makes a lot sense since the version of > the spec doesn't even have its own link. > Does Firefox support the version? Do you know if he's/they're going to update > the implementation? Boris Zbarsky seemed uninterested in backwards compatibility for this attribute. I figured we might care more, since we have the filesystem API, and maybe we have Chrome apps / extensions relying on it. Not having to worry about backwards compatibility will make the patch smaller, so I'm happy to go that route if we can afford to!
On 2013/11/19 11:16:09, kinuko wrote: > Does Firefox support the version? Do you know if he's/they're going to update > the implementation? This version has a change that they asked for (Date -> long long), so it would be surprising if they wouldn't update. Also, once my patch lands and I have tests that pass in Chromium, I intend to file bugs against Firefox and port all the File constructor code to WebKit. I don't want Chromium to be a snowflake :)
> Boris Zbarsky seemed uninterested in backwards compatibility for this attribute. > > I figured we might care more, since we have the filesystem API, and maybe we > have Chrome apps / extensions relying on it. Not having to worry about backwards > compatibility will make the patch smaller, so I'm happy to go that route if we > can afford to! Let me be clear, I meant to say I don't think we should support lastModifiedDate in the FilePropertyBag. lastModifiedDate attribute is surely being used, and I don't think we should remove it until the ED gets cut as WD.
> Let me be clear, I meant to say I don't think we should support lastModifiedDate > in the FilePropertyBag. > lastModifiedDate attribute is surely being used, and I don't think we should > remove it until the ED gets cut as WD. ...and if we do this we'll need to output deprecation warning :)
On 2013/11/19 12:27:39, kinuko wrote: > Let me be clear, I meant to say I don't think we should support lastModifiedDate > in the FilePropertyBag. > lastModifiedDate attribute is surely being used, and I don't think we should > remove it until the ED gets cut as WD. Thank you for clarifying, and I'm sorry I misunderstood!
On 2013/11/19 11:26:35, pwnall wrote: > On 2013/11/19 11:16:09, kinuko wrote: > > Does Firefox support the version? Do you know if he's/they're going to update > > the implementation? > > This version has a change that they asked for (Date -> long long), so it would > be surprising if they wouldn't update. Also, once my patch lands and I have > tests that pass in Chromium, I intend to file bugs against Firefox and port all > the File constructor code to WebKit. I don't want Chromium to be a snowflake :) Cool, thanks.
On 2013/11/19 10:38:43, kinuko wrote: > And it looks like the ED seems to have been already revised? It now says that > the FilePropertyBag takes long long 'lastModified' value rather than the Date > value. At this time I'm not fully convinced about landing this patch, since the > draft this CL is based on doesn't seem stable enough to implement. Feeling > sorry to tell this but could we wait until the discussion settles down, or do > you have a strong need for this feature? The bug was maked as resolved / fixed, so I think the debate on lastModifiedDate -> lastModified has concluded. In the few weeks that I've been looking at the ED, it only changed in response to feedback from Mozilla or us. The editor asked people to take one last look, which leads me to believe that he won't be making changes unless there are bug reports. I have an implementation that I'm happy with based on the current draft and I'm not planning to file more bugs. Based on the comments, I'm reasonably sure that the Firefox developers are also content and won't be filing bugs. I updated this patch to match the ED. I added a "lastModified" attribute to File, gated by the same RuntimeEnabledFlag as the File constructor. I would like to land this CL and prepare a CL deprecating lastModifiedDate, which would get landed around the time when we set the RuntimeEnabledFlag to "stable". What do you think? Does this seem reasonable?
On 2013/11/20 01:26:57, pwnall wrote: > On 2013/11/19 10:38:43, kinuko wrote: > > And it looks like the ED seems to have been already revised? It now says that > > the FilePropertyBag takes long long 'lastModified' value rather than the Date > > value. At this time I'm not fully convinced about landing this patch, since > the > > draft this CL is based on doesn't seem stable enough to implement. Feeling > > sorry to tell this but could we wait until the discussion settles down, or do > > you have a strong need for this feature? > > The bug was maked as resolved / fixed, so I think the debate on lastModifiedDate > -> lastModified has concluded. > > In the few weeks that I've been looking at the ED, it only changed in response > to feedback from Mozilla or us. The editor asked people to take one last look, > which leads me to believe that he won't be making changes unless there are bug > reports. I have an implementation that I'm happy with based on the current draft > and I'm not planning to file more bugs. Based on the comments, I'm reasonably > sure that the Firefox developers are also content and won't be filing bugs. > > I updated this patch to match the ED. I added a "lastModified" attribute to > File, gated by the same RuntimeEnabledFlag as the File constructor. I would like > to land this CL and prepare a CL deprecating lastModifiedDate, which would get > landed around the time when we set the RuntimeEnabledFlag to "stable". > > What do you think? Does this seem reasonable? Ok, the runtime flag name for attribute feels slightly skewed but as far as they're behind the flag we should be ok. I left some more comments for the current code.
https://codereview.chromium.org/74213009/diff/830001/LayoutTests/fast/files/f... File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/830001/LayoutTests/fast/files/f... LayoutTests/fast/files/file-constructor.html:93: // Unless specified, lastModifiedDate should default to now. nit: lastModifiedDate -> lastModified https://codereview.chromium.org/74213009/diff/830001/LayoutTests/fast/files/f... LayoutTests/fast/files/file-constructor.html:103: // Test the deprecated attribute lastModifiedTime. lastModifiedTime -> lastModifiedDate ? https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8BlobCustomHelpers.h (right): https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.h:47: // processBlobPropertyBag is only called when constructors receive a value for nit: comment looks stale https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.h:53: String endings; I think we prefer having accessors for C++ class (if it's not for structs) https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.h:62: explicit ParsedProperties(bool hasFileProperties); nit: usually we put ctor at the top https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8FileCustom.cpp:106: lastModified = currentTime(); Shouldn't this be currentTimeMS()?
Thank you very much for the feedback! Can you please take one more look? https://codereview.chromium.org/74213009/diff/830001/LayoutTests/fast/files/f... File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/830001/LayoutTests/fast/files/f... LayoutTests/fast/files/file-constructor.html:93: // Unless specified, lastModifiedDate should default to now. On 2013/11/20 08:01:22, kinuko wrote: > nit: lastModifiedDate -> lastModified Done. Thank you! https://codereview.chromium.org/74213009/diff/830001/LayoutTests/fast/files/f... LayoutTests/fast/files/file-constructor.html:103: // Test the deprecated attribute lastModifiedTime. On 2013/11/20 08:01:22, kinuko wrote: > lastModifiedTime -> lastModifiedDate ? Done. I'm so glad this name is going away :) https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8BlobCustomHelpers.h (right): https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.h:47: // processBlobPropertyBag is only called when constructors receive a value for On 2013/11/20 08:01:22, kinuko wrote: > nit: comment looks stale It's still true. lastModified is a bit of a snowflake, but all the other values come straight from the constructor if no property bag is passed. Should I add comments around the methods that deal with lastModified? https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.h:53: String endings; On 2013/11/20 08:01:22, kinuko wrote: > I think we prefer having accessors for C++ class (if it's not for structs) Done. Thank you! https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.h:62: explicit ParsedProperties(bool hasFileProperties); On 2013/11/20 08:01:22, kinuko wrote: > nit: usually we put ctor at the top Done. Thank you for your patience in teaching me! https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8FileCustom.cpp:106: lastModified = currentTime(); On 2013/11/20 08:01:22, kinuko wrote: > Shouldn't this be currentTimeMS()? Done. Yikes, thank you for catching this! I had the same mistake above on line 97.
Ok, lgtm. https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8BlobCustomHelpers.h (right): https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.h:47: // processBlobPropertyBag is only called when constructors receive a value for On 2013/11/20 09:51:07, pwnall wrote: > On 2013/11/20 08:01:22, kinuko wrote: > > nit: comment looks stale > > It's still true. I mean, we no longer seem to have the method processBlobPropertyBag. https://codereview.chromium.org/74213009/diff/1020001/Source/bindings/v8/cust... File Source/bindings/v8/custom/V8BlobCustomHelpers.h (right): https://codereview.chromium.org/74213009/diff/1020001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8BlobCustomHelpers.h:55: inline const String& endings() { return m_endings; } nit: no need of 'inline' Could these method be const? const String& contentType() const { return m_contentType; }
Thank you very much for the thorough feedback! Sorry for needing so much guidance! I hope I'll get to a point where my patches won't cause so much trouble :) https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8BlobCustomHelpers.h (right): https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8BlobCustomHelpers.h:47: // processBlobPropertyBag is only called when constructors receive a value for On 2013/11/20 10:13:39, kinuko wrote: > On 2013/11/20 09:51:07, pwnall wrote: > > On 2013/11/20 08:01:22, kinuko wrote: > > > nit: comment looks stale > > > > It's still true. > > I mean, we no longer seem to have the method processBlobPropertyBag. Done. Ah, that is a very good point! I updated the name. https://codereview.chromium.org/74213009/diff/1020001/Source/bindings/v8/cust... File Source/bindings/v8/custom/V8BlobCustomHelpers.h (right): https://codereview.chromium.org/74213009/diff/1020001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8BlobCustomHelpers.h:55: inline const String& endings() { return m_endings; } On 2013/11/20 10:13:40, kinuko wrote: > nit: no need of 'inline' > > Could these method be const? > > const String& contentType() const { return m_contentType; } Done. Thank you!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/costan@gmail.com/74213009/1080001
Message was sent while issue was closed.
Change committed as 162362 |
