|
|
DescriptionFix 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}
Patch Set 1 : Fix tests. #
Total comments: 3
Patch Set 2 : jsbell feedback #
Dependent Patchsets: Messages
Total messages: 30 (22 generated)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pwnall@chromium.org changed reviewers: + dcheng@chromium.org
PTAL?
pwnall@chromium.org changed reviewers: + jsbell@chromium.org
jsbell: Can you please review the layout tests?
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.
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
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.
The CQ bit was checked by pwnall@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481923908120600, "parent_rev": "cf1e665f082d8ed7d48e129a7babf8ffea446d93", "commit_rev": "b82f0f7e14f2a985f9eea4932db6ac9413b4a084"}
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ce3b8eda8ca0eb98813fb256e069cff3f569a8aa Cr-Commit-Position: refs/heads/master@{#439280} |