Fix webkitdropzone to accept file drops on non-file: page.

Fix webkitdropzone to accept file drops on non-file: page.

Most of webkitdropzone's implementation is in findDropZone() in
core/input/EventHandler.cpp. The implementation relies on two functions
in DataTransfer that only work for DataTransfer instances that allow all
the drop data to be read. According to the drag-and-drop specification,
this should only be the case in the event handlers for dragstart (when
the DataTransfer instance is writable) and for drop. In all other cases,
the DataTransfer instance only allows limited metadata to be read.

For some reason we let DataTransfer instances be readable most of the
time for pages served from file: origins. This will be fixed in a
follow-up CL. Out LayoutTest covering file drops into a webkitdropzone
element passes due to this glitch, combined with the fact that the test
is served from a file: origin.

This CL fixes the functions in webkitdropzone's implementation, mostly
to clear the way for fixing DataTransfer's permissions.

BUG=104681
Committed: https://crrev.com/ce3b8eda8ca0eb98813fb256e069cff3f569a8aa
Cr-Commit-Position: refs/heads/master@{#439280}

[pwnall, 2016-12-15 01:57:31]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 01:57:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 02:05:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 02:05:27]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[pwnall, 2016-12-16 02:19:57]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 02:21:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 04:17:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 04:17:14]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pwnall, 2016-12-16 07:10:27]

Description was changed from

==========
Fix old FIXME in DataTransfer::setDropEffect.

BUG=104681
==========

to

==========
Fix webkitdropzone to accept file drops on non-file: page.

Most of webkitdropzone's implementation is in findDropZone() in
core/input/EventHandler.cpp. The implementation relies on two functions
in DataTransfer that only work for DataTransfer instances that allow all
the drop data to be read. According to the drag-and-drop specification,
this should only be the case in the event handlers for dragstart (when
the DataTransfer instance is writable) and for drop. In all other cases,
the DataTransfer instance only allows limited metadata to be read.

For some reason we let DataTransfer instances be readable most of the
time for pages served from file: origins. This will be fixed in a
follow-up CL. Out LayoutTest covering file drops into a webkitdropzone
element passes due to this glitch, combined with the fact that the test
is served from a file: origin.

This CL fixes the functions in webkitdropzone's implementation, mostly
to clear the way for fixing DataTransfer's permissions.

BUG=104681
==========

[pwnall, 2016-12-16 07:11:34]

Patchset #1 (id:1) has been deleted

[pwnall, 2016-12-16 07:11:44]

Patchset #1 (id:20001) has been deleted

[pwnall, 2016-12-16 07:11:55]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 07:12:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 08:27:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 08:27:36]

Dry run: This issue passed the CQ dry run.

[pwnall, 2016-12-16 08:32:33]

pwnall@chromium.org changed reviewers:
+ dcheng@chromium.org

[pwnall, 2016-12-16 08:32:34]

PTAL?

[pwnall, 2016-12-16 19:58:55]

pwnall@chromium.org changed reviewers:
+ jsbell@chromium.org

[pwnall, 2016-12-16 19:58:56]

jsbell: Can you please review the layout tests?

[dcheng, 2016-12-16 20:50:52]

Code changes LGTM. Perhaps let's try moving the helper up a level in a followup.

https://codereview.chromium.org/2575303002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/DataTransfer.cpp (right):

https://codereview.chromium.org/2575303002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/DataTransfer.cpp:469: for (size_t i =
0; i < m_dataObject->length(); ++i) {
The duplication is a bit unfortunate, but this seems unavoidable. One thing that
we could try is to push this helper up into DataObject itself.

[jsbell, 2016-12-16 21:18:52]

test lgtm

https://codereview.chromium.org/2575303002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/dnd/drop-file-on-webkitdropzone.html
(right):

https://codereview.chromium.org/2575303002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/dnd/drop-file-on-webkitdropzone.html:27:
dropZone.ondrop = t.step_func_done((event) => {
nit: no () needed around single argument

[pwnall, 2016-12-16 21:31:42]

Thanks for the quick turnaround!

https://codereview.chromium.org/2575303002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/dnd/drop-file-on-webkitdropzone.html
(right):

https://codereview.chromium.org/2575303002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/dnd/drop-file-on-webkitdropzone.html:27:
dropZone.ondrop = t.step_func_done((event) => {
On 2016/12/16 21:18:52, jsbell wrote:
> nit: no () needed around single argument

Done.

[pwnall, 2016-12-16 21:31:48]

The CQ bit was checked by pwnall@chromium.org

[pwnall, 2016-12-16 21:31:48]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2575303002/#ps60001
(title: "jsbell feedback")

[commit-bot: I haz the power, 2016-12-16 21:33:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-17 01:35:02]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481923908120600,
"parent_rev": "cf1e665f082d8ed7d48e129a7babf8ffea446d93", "commit_rev":
"b82f0f7e14f2a985f9eea4932db6ac9413b4a084"}

[commit-bot: I haz the power, 2016-12-17 01:35:43]

Description was changed from

==========
Fix webkitdropzone to accept file drops on non-file: page.

Most of webkitdropzone's implementation is in findDropZone() in
core/input/EventHandler.cpp. The implementation relies on two functions
in DataTransfer that only work for DataTransfer instances that allow all
the drop data to be read. According to the drag-and-drop specification,
this should only be the case in the event handlers for dragstart (when
the DataTransfer instance is writable) and for drop. In all other cases,
the DataTransfer instance only allows limited metadata to be read.

For some reason we let DataTransfer instances be readable most of the
time for pages served from file: origins. This will be fixed in a
follow-up CL. Out LayoutTest covering file drops into a webkitdropzone
element passes due to this glitch, combined with the fact that the test
is served from a file: origin.

This CL fixes the functions in webkitdropzone's implementation, mostly
to clear the way for fixing DataTransfer's permissions.

BUG=104681
==========

to

==========
Fix webkitdropzone to accept file drops on non-file: page.

Most of webkitdropzone's implementation is in findDropZone() in
core/input/EventHandler.cpp. The implementation relies on two functions
in DataTransfer that only work for DataTransfer instances that allow all
the drop data to be read. According to the drag-and-drop specification,
this should only be the case in the event handlers for dragstart (when
the DataTransfer instance is writable) and for drop. In all other cases,
the DataTransfer instance only allows limited metadata to be read.

For some reason we let DataTransfer instances be readable most of the
time for pages served from file: origins. This will be fixed in a
follow-up CL. Out LayoutTest covering file drops into a webkitdropzone
element passes due to this glitch, combined with the fact that the test
is served from a file: origin.

This CL fixes the functions in webkitdropzone's implementation, mostly
to clear the way for fixing DataTransfer's permissions.

BUG=104681

Review-Url: https://codereview.chromium.org/2575303002
==========

[commit-bot: I haz the power, 2016-12-17 01:35:44]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-12-17 01:40:10]

Description was changed from

==========
Fix webkitdropzone to accept file drops on non-file: page.

Most of webkitdropzone's implementation is in findDropZone() in
core/input/EventHandler.cpp. The implementation relies on two functions
in DataTransfer that only work for DataTransfer instances that allow all
the drop data to be read. According to the drag-and-drop specification,
this should only be the case in the event handlers for dragstart (when
the DataTransfer instance is writable) and for drop. In all other cases,
the DataTransfer instance only allows limited metadata to be read.

For some reason we let DataTransfer instances be readable most of the
time for pages served from file: origins. This will be fixed in a
follow-up CL. Out LayoutTest covering file drops into a webkitdropzone
element passes due to this glitch, combined with the fact that the test
is served from a file: origin.

This CL fixes the functions in webkitdropzone's implementation, mostly
to clear the way for fixing DataTransfer's permissions.

BUG=104681

Review-Url: https://codereview.chromium.org/2575303002
==========

to

==========
Fix webkitdropzone to accept file drops on non-file: page.

Most of webkitdropzone's implementation is in findDropZone() in
core/input/EventHandler.cpp. The implementation relies on two functions
in DataTransfer that only work for DataTransfer instances that allow all
the drop data to be read. According to the drag-and-drop specification,
this should only be the case in the event handlers for dragstart (when
the DataTransfer instance is writable) and for drop. In all other cases,
the DataTransfer instance only allows limited metadata to be read.

For some reason we let DataTransfer instances be readable most of the
time for pages served from file: origins. This will be fixed in a
follow-up CL. Out LayoutTest covering file drops into a webkitdropzone
element passes due to this glitch, combined with the fact that the test
is served from a file: origin.

This CL fixes the functions in webkitdropzone's implementation, mostly
to clear the way for fixing DataTransfer's permissions.

BUG=104681

Committed: https://crrev.com/ce3b8eda8ca0eb98813fb256e069cff3f569a8aa
Cr-Commit-Position: refs/heads/master@{#439280}
==========

[commit-bot: I haz the power, 2016-12-17 01:40:12]

Patchset 2 (id:??) landed as
https://crrev.com/ce3b8eda8ca0eb98813fb256e069cff3f569a8aa
Cr-Commit-Position: refs/heads/master@{#439280}