Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(24)

Issue 74213009: File constructor understands lastModified. (Closed)

Created:
4 years ago by pwnall-personal
Modified:
4 years ago
Reviewers:
haraken, kinuko, sof, Inactive, jsbell
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.

Description

File 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)
pwnall-personal
I left lastModifiedDate out of the File constructor CL, because I was worried that it ...
4 years ago (2013-11-16 11:47:05 UTC) #1
sof
https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8BlobCustomHelpers.cpp File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right): https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8BlobCustomHelpers.cpp#newcode101 Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:101: if (!lastModifiedDate->IsDate() && !lastModifiedDate->IsNumber()) { From spec standpoint, Number ...
4 years ago (2013-11-17 07:42:36 UTC) #2
pwnall-personal
Thank you very much for your feedback! Can you please take a look at my ...
4 years ago (2013-11-17 09:01:23 UTC) #3
haraken
+kinuko I'm happy to review the CL implementation-wise, after the spec-wise discussion settles down.
4 years ago (2013-11-17 09:42:59 UTC) #4
sof
https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8BlobCustomHelpers.cpp File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right): https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8BlobCustomHelpers.cpp#newcode101 Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:101: if (!lastModifiedDate->IsDate() && !lastModifiedDate->IsNumber()) { On 2013/11/17 09:01:23, pwnall ...
4 years ago (2013-11-17 12:53:40 UTC) #5
pwnall-personal
Thank you very much for you feedback! Can you please take another look? https://codereview.chromium.org/74213009/diff/1/Source/bindings/v8/custom/V8BlobCustomHelpers.cpp File ...
4 years ago (2013-11-17 16:02:08 UTC) #6
sof
Thanks, this takes care of the issues I came across. Just some details commented on ...
4 years ago (2013-11-17 17:16:34 UTC) #7
pwnall-personal
Thank you for your feedback! https://codereview.chromium.org/74213009/diff/150001/Source/bindings/v8/custom/V8BlobCustomHelpers.cpp File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right): https://codereview.chromium.org/74213009/diff/150001/Source/bindings/v8/custom/V8BlobCustomHelpers.cpp#newcode114 Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:114: if (std::isnan(dateValue)) On 2013/11/17 ...
4 years ago (2013-11-17 17:50:46 UTC) #8
pwnall-personal
On 2013/11/17 17:16:34, sof wrote: > Re: Date or timestamps; given how widely implemented the ...
4 years ago (2013-11-17 20:39:34 UTC) #9
pwnall-personal
On 2013/11/17 09:42:59, haraken wrote: > +kinuko > > I'm happy to review the CL ...
4 years ago (2013-11-18 10:13:54 UTC) #10
haraken
LGTM for bindings/. File API experts should take another look. https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custom/V8BlobCustomHelpers.cpp File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right): https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custom/V8BlobCustomHelpers.cpp#newcode113 ...
4 years ago (2013-11-18 10:27:47 UTC) #11
pwnall-personal
Thank you very much for your feedback! I apologize, but I'm still not sure how ...
4 years ago (2013-11-18 11:23:44 UTC) #12
haraken
https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custom/V8BlobCustomHelpers.cpp File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right): https://codereview.chromium.org/74213009/diff/280001/Source/bindings/v8/custom/V8BlobCustomHelpers.cpp#newcode113 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: ...
4 years ago (2013-11-18 11:29:07 UTC) #13
kinuko
Sorry for coming in late, https://codereview.chromium.org/74213009/diff/280001/LayoutTests/fast/files/file-constructor.html File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/280001/LayoutTests/fast/files/file-constructor.html#newcode101 LayoutTests/fast/files/file-constructor.html:101: shouldBeTrue("Math.abs(Date.now() - (new File([], ...
4 years ago (2013-11-18 12:21:42 UTC) #14
pwnall-personal
Thank you very much for your feedback! Can you please take another look? https://codereview.chromium.org/74213009/diff/280001/LayoutTests/fast/files/file-constructor.html File ...
4 years ago (2013-11-18 12:48:56 UTC) #15
kinuko
https://codereview.chromium.org/74213009/diff/340002/Source/bindings/v8/custom/V8FileCustom.cpp File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/340002/Source/bindings/v8/custom/V8FileCustom.cpp#newcode94 Source/bindings/v8/custom/V8FileCustom.cpp:94: // "null". On 2013/11/18 12:48:57, pwnall wrote: > On ...
4 years ago (2013-11-18 13:27:09 UTC) #16
pwnall-personal
https://codereview.chromium.org/74213009/diff/410002/LayoutTests/fast/files/file-constructor.html File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/410002/LayoutTests/fast/files/file-constructor.html#newcode107 LayoutTests/fast/files/file-constructor.html:107: continue; // Time went backwards. (DST) On 2013/11/18 13:27:09, ...
4 years ago (2013-11-18 13:47:33 UTC) #17
kinuko
https://codereview.chromium.org/74213009/diff/410002/LayoutTests/fast/files/file-constructor.html File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/410002/LayoutTests/fast/files/file-constructor.html#newcode107 LayoutTests/fast/files/file-constructor.html:107: continue; // Time went backwards. (DST) On 2013/11/18 13:47:34, ...
4 years ago (2013-11-18 13:57:57 UTC) #18
pwnall-personal
https://codereview.chromium.org/74213009/diff/410002/LayoutTests/fast/files/file-constructor.html File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/410002/LayoutTests/fast/files/file-constructor.html#newcode107 LayoutTests/fast/files/file-constructor.html:107: continue; // Time went backwards. (DST) On 2013/11/18 13:57:58, ...
4 years ago (2013-11-18 14:04:54 UTC) #19
kinuko
Thanks, this patch almost lgtm now. https://codereview.chromium.org/74213009/diff/460002/Source/bindings/v8/custom/V8FileCustom.cpp File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/460002/Source/bindings/v8/custom/V8FileCustom.cpp#newcode104 Source/bindings/v8/custom/V8FileCustom.cpp:104: v8SetReturnValue(info, v8::Date::New(file->lastModifiedDate())); (Going ...
4 years ago (2013-11-18 14:16:58 UTC) #20
pwnall-personal
https://codereview.chromium.org/74213009/diff/460002/Source/bindings/v8/custom/V8FileCustom.cpp File Source/bindings/v8/custom/V8FileCustom.cpp (right): https://codereview.chromium.org/74213009/diff/460002/Source/bindings/v8/custom/V8FileCustom.cpp#newcode104 Source/bindings/v8/custom/V8FileCustom.cpp:104: v8SetReturnValue(info, v8::Date::New(file->lastModifiedDate())); On 2013/11/18 14:16:59, kinuko wrote: > (Going ...
4 years ago (2013-11-18 14:44:34 UTC) #21
jsbell
https://codereview.chromium.org/74213009/diff/460002/LayoutTests/fast/files/file-constructor.html File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/460002/LayoutTests/fast/files/file-constructor.html#newcode104 LayoutTests/fast/files/file-constructor.html:104: // FIXME: these checks will fail if time goes ...
4 years ago (2013-11-18 20:54:05 UTC) #22
kinuko
On 2013/11/18 20:54:05, jsbell wrote: > https://codereview.chromium.org/74213009/diff/460002/LayoutTests/fast/files/file-constructor.html > File LayoutTests/fast/files/file-constructor.html (right): > > https://codereview.chromium.org/74213009/diff/460002/LayoutTests/fast/files/file-constructor.html#newcode104 > ...
4 years ago (2013-11-19 02:19:28 UTC) #23
kinuko
On 2013/11/18 14:44:34, pwnall wrote: > https://codereview.chromium.org/74213009/diff/460002/Source/bindings/v8/custom/V8FileCustom.cpp > File Source/bindings/v8/custom/V8FileCustom.cpp (right): > > https://codereview.chromium.org/74213009/diff/460002/Source/bindings/v8/custom/V8FileCustom.cpp#newcode104 > ...
4 years ago (2013-11-19 02:57:46 UTC) #24
pwnall-personal
Thank you for your feedback and thoughts! Based on the conversation in the spec bug ...
4 years ago (2013-11-19 09:10:11 UTC) #25
kinuko
On 2013/11/19 09:10:11, pwnall wrote: > Thank you for your feedback and thoughts! > > ...
4 years ago (2013-11-19 10:38:43 UTC) #26
pwnall-personal
On 2013/11/19 10:38:43, kinuko wrote: > On 2013/11/19 09:10:11, pwnall wrote: > > Thank you ...
4 years ago (2013-11-19 10:51:53 UTC) #27
kinuko
On 2013/11/19 10:51:53, pwnall wrote: > On 2013/11/19 10:38:43, kinuko wrote: > > On 2013/11/19 ...
4 years ago (2013-11-19 11:16:09 UTC) #28
pwnall-personal
On 2013/11/19 11:16:09, kinuko wrote: > On 2013/11/19 10:51:53, pwnall wrote: > > On 2013/11/19 ...
4 years ago (2013-11-19 11:21:28 UTC) #29
pwnall-personal
On 2013/11/19 11:16:09, kinuko wrote: > Does Firefox support the version? Do you know if ...
4 years ago (2013-11-19 11:26:35 UTC) #30
kinuko
> Boris Zbarsky seemed uninterested in backwards compatibility for this attribute. > > I figured ...
4 years ago (2013-11-19 12:27:39 UTC) #31
kinuko
> Let me be clear, I meant to say I don't think we should support ...
4 years ago (2013-11-19 12:28:45 UTC) #32
pwnall-personal
On 2013/11/19 12:27:39, kinuko wrote: > Let me be clear, I meant to say I ...
4 years ago (2013-11-19 12:29:16 UTC) #33
kinuko
On 2013/11/19 11:26:35, pwnall wrote: > On 2013/11/19 11:16:09, kinuko wrote: > > Does Firefox ...
4 years ago (2013-11-19 12:29:38 UTC) #34
pwnall-personal
On 2013/11/19 10:38:43, kinuko wrote: > And it looks like the ED seems to have ...
4 years ago (2013-11-20 01:26:57 UTC) #35
kinuko
On 2013/11/20 01:26:57, pwnall wrote: > On 2013/11/19 10:38:43, kinuko wrote: > > And it ...
4 years ago (2013-11-20 08:01:03 UTC) #36
kinuko
https://codereview.chromium.org/74213009/diff/830001/LayoutTests/fast/files/file-constructor.html File LayoutTests/fast/files/file-constructor.html (right): https://codereview.chromium.org/74213009/diff/830001/LayoutTests/fast/files/file-constructor.html#newcode93 LayoutTests/fast/files/file-constructor.html:93: // Unless specified, lastModifiedDate should default to now. nit: ...
4 years ago (2013-11-20 08:01:21 UTC) #37
pwnall-personal
Thank you very much for the feedback! Can you please take one more look? https://codereview.chromium.org/74213009/diff/830001/LayoutTests/fast/files/file-constructor.html ...
4 years ago (2013-11-20 09:51:06 UTC) #38
kinuko
Ok, lgtm. https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custom/V8BlobCustomHelpers.h File Source/bindings/v8/custom/V8BlobCustomHelpers.h (right): https://codereview.chromium.org/74213009/diff/830001/Source/bindings/v8/custom/V8BlobCustomHelpers.h#newcode47 Source/bindings/v8/custom/V8BlobCustomHelpers.h:47: // processBlobPropertyBag is only called when constructors ...
4 years ago (2013-11-20 10:13:38 UTC) #39
pwnall-personal
Thank you very much for the thorough feedback! Sorry for needing so much guidance! I ...
4 years ago (2013-11-20 10:40:29 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/costan@gmail.com/74213009/1080001
4 years ago (2013-11-20 12:50:44 UTC) #41
commit-bot: I haz the power
4 years ago (2013-11-20 13:28:55 UTC) #42
Message was sent while issue was closed.
Change committed as 162362

Powered by Google App Engine
This is Rietveld efc10ee0f