Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(37)

Issue 235373005: Handle JS-created files in <input type="file">. (Closed)

Created:
6 years, 8 months ago by pwnall-personal
Modified:
6 years, 5 months ago
Reviewers:
kinuko, tkent, haraken
CC:
blink-reviews, tzik, eae+blinkwatch, adamk+blink_chromium.org, rune+blink, jamesr, arv+blink, bemjb+rendering_chromium.org, dsinclair, abarth-chromium, marja+watch_chromium.org, dglazkov+blink, jchaffraix+rendering, pdr., zoltan1, nhiroki, Nate Chapin, jsbell+bindings_chromium.org, kouhei+bindings_chromium.org, leviw+renderwatch, Inactive, blink-reviews-bindings_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Handle JS-created files in <input type="file">. The first associated bug shows a (contorted) series of steps which can result in a non-user-selected file (created using the Filesystem API) ending up in a file input element's FileList. The second associated bug follows the pattern shown by the first bug's reporter, but uses a file created by the File constructor. This causes a renderer crash. Also, one day we might have a direct way to set an input's FileList, and it should handle files created by JavaScript (specifically, by the the Filesystem API and by the File constructor). This change adds a flag to File objects that distinguishes between files that come from a location visible to the user, and files that were created programatically and are not on user-visible filesystems. The latter category of files should not be browsed to when used to populate an <input>. The flag is used to fix the name displayed by a file upload input element on Mac. BUG=360308, 367334 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178752

Patch Set 1 : Rebased. #

Total comments: 15

Patch Set 2 : Rebased #

Patch Set 3 : Addressed feedback #

Patch Set 4 : Fixed visibility bug. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+286 lines, -34 lines) Patch
M LayoutTests/TestExpectations View 1 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/files/file-in-input-display.html View 1 2 1 chunk +84 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/filesystem/input-display.html View 1 2 1 chunk +99 lines, -0 lines 0 comments Download
A LayoutTests/platform/mac/fast/files/file-in-input-display-expected.png View Binary file 0 comments Download
A LayoutTests/platform/mac/fast/files/file-in-input-display-expected.txt View 1 chunk +15 lines, -0 lines 0 comments Download
A LayoutTests/platform/mac/http/tests/filesystem/input-display-expected.png View Binary file 0 comments Download
A LayoutTests/platform/mac/http/tests/filesystem/input-display-expected.txt View 1 chunk +14 lines, -0 lines 0 comments Download
M Source/bindings/core/v8/SerializedScriptValue.cpp View 1 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/fileapi/File.h View 1 2 3 6 chunks +21 lines, -7 lines 0 comments Download
M Source/core/fileapi/File.cpp View 1 6 chunks +10 lines, -4 lines 0 comments Download
M Source/core/fileapi/FileList.h View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/core/fileapi/FileList.cpp View 1 1 chunk +5 lines, -3 lines 0 comments Download
M Source/core/html/forms/FileInputType.cpp View 1 2 chunks +6 lines, -3 lines 0 comments Download
M Source/core/rendering/RenderThemeChromiumMac.mm View 1 1 chunk +5 lines, -1 line 0 comments Download
M Source/modules/filesystem/DOMFileSystemSync.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/web/WebDragData.cpp View 1 1 chunk +20 lines, -12 lines 0 comments Download

Messages

Total messages: 20 (0 generated)
pwnall-personal
This shows all the places we'd need to touch to get crbug.com/360308 fixed. What do ...
6 years, 8 months ago (2014-04-24 17:55:00 UTC) #1
pwnall-personal
I changed the crypto LayoutTests so they don't need an update every time SerializedScriptValue changes. ...
6 years, 8 months ago (2014-04-24 23:41:11 UTC) #2
sof
Making existing APIs work better together is always a good thing, which is one way ...
6 years, 8 months ago (2014-04-25 07:04:46 UTC) #3
kinuko
On 2014/04/25 07:04:46, sof wrote: > Making existing APIs work better together is always a ...
6 years, 8 months ago (2014-04-25 08:03:40 UTC) #4
pwnall-personal
On 2014/04/25 08:03:40, kinuko wrote: > On 2014/04/25 07:04:46, sof wrote: > > Making existing ...
6 years, 7 months ago (2014-04-29 01:59:14 UTC) #5
sof
On 2014/04/29 01:59:14, pwnall wrote: ... > > Instances produced by the File constructor crash ...
6 years, 7 months ago (2014-04-29 09:15:13 UTC) #6
sof
https://codereview.chromium.org/235373005/diff/70001/LayoutTests/http/tests/filesystem/input-display.html File LayoutTests/http/tests/filesystem/input-display.html (right): https://codereview.chromium.org/235373005/diff/70001/LayoutTests/http/tests/filesystem/input-display.html#newcode2 LayoutTests/http/tests/filesystem/input-display.html:2: Possible to rework this test as a fast/files/ (or ...
6 years, 7 months ago (2014-05-08 09:31:22 UTC) #7
pwnall-personal
I finally had some time to read and understand all the pieces at work in ...
6 years, 5 months ago (2014-07-18 05:42:09 UTC) #8
tkent
lgtm. - What is shown in a mouse-hover tooltip for input[type=file] with a filesystem file? ...
6 years, 5 months ago (2014-07-23 01:19:59 UTC) #9
pwnall-personal
https://codereview.chromium.org/235373005/diff/150001/LayoutTests/fast/files/file-in-input-display.html File LayoutTests/fast/files/file-in-input-display.html (right): https://codereview.chromium.org/235373005/diff/150001/LayoutTests/fast/files/file-in-input-display.html#newcode6 LayoutTests/fast/files/file-in-input-display.html:6: --> On 2014/07/23 01:19:58, tkent wrote: > Please mention ...
6 years, 5 months ago (2014-07-23 06:58:21 UTC) #10
pwnall-personal
On 2014/07/23 01:19:59, tkent wrote: > lgtm. Thank you for your thorough feedback! > - ...
6 years, 5 months ago (2014-07-23 07:29:15 UTC) #11
tkent
On 2014/07/23 07:29:15, pwnall wrote: > > - What is shown in a mouse-hover tooltip ...
6 years, 5 months ago (2014-07-23 07:37:26 UTC) #12
pwnall-personal
On 2014/07/23 07:37:26, tkent wrote: > On 2014/07/23 07:29:15, pwnall wrote: > > > - ...
6 years, 5 months ago (2014-07-23 07:43:09 UTC) #13
tkent
On 2014/07/23 07:43:09, pwnall wrote: > In each case (real files, Filesystem API, File constructor), ...
6 years, 5 months ago (2014-07-23 07:46:53 UTC) #14
pwnall-personal
On 2014/07/23 07:46:53, tkent wrote: > On 2014/07/23 07:43:09, pwnall wrote: > > In each ...
6 years, 5 months ago (2014-07-23 08:01:12 UTC) #15
haraken
bindings/ LGTM.
6 years, 5 months ago (2014-07-23 08:10:21 UTC) #16
pwnall-personal
The CQ bit was checked by costan@gmail.com
6 years, 5 months ago (2014-07-23 08:12:06 UTC) #17
pwnall-personal
On 2014/07/23 08:10:21, haraken wrote: > bindings/ LGTM. Thank you very much!
6 years, 5 months ago (2014-07-23 08:12:13 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/costan@gmail.com/235373005/200001
6 years, 5 months ago (2014-07-23 08:12:30 UTC) #19
commit-bot: I haz the power
6 years, 5 months ago (2014-07-23 12:21:49 UTC) #20
Message was sent while issue was closed.
Change committed as 178752

Powered by Google App Engine
This is Rietveld 408576698