Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2508773005:
DataTransferItemList#add() file argument should not be nullable (Closed)
DescriptionDataTransferItemList#add() file argument should not be nullable
Compatibility:
dataTransfer.items.add(null)
Edge: does not throw
Firefox 49: dataTransfer.items is undefined
Firefox 50: throw as expected
Safari: dataTransfer.items is undefined
No use counter is added as we will likely get informed if any problems come up https://bugzilla.mozilla.org/show_bug.cgi?id=906420
BUG=662005
Committed: https://crrev.com/11a9aad4ec1d0ae6099a3721813f9a20cfc6fe6b
Cr-Commit-Position: refs/heads/master@{#434137}
Patch Set 1 : Initial checkin #Patch Set 2 : Added layout tests for dataTransferItemList.add() #
Messages
Total messages: 26 (17 generated)
The CQ bit was checked by lunalu@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_...)
lunalu@chromium.org changed reviewers: + foolip@chromium.org
Hi Philip, I feel like I should add a test there but I haven't really figure out how to construct a DataTransferItemList. Or do you know a trivial change like this doesn't require a layout test? Thanks
On 2016/11/21 21:59:07, loonybear wrote: > Hi Philip, I feel like I should add a test there but I haven't really figure out > how to construct a DataTransferItemList. Or do you know a trivial change like > this doesn't require a layout test? > > Thanks Easiest way is to dispatch a drag or clipboard event: event.dataTransfer.items will be a DataTransferItemList. (Not sure if this interface should be constructible or not, someone who's more familiar with specs should be able to say)
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
On 2016/11/21 21:59:07, loonybear wrote: > Hi Philip, I feel like I should add a test there but I haven't really figure out > how to construct a DataTransferItemList. Or do you know a trivial change like > this doesn't require a layout test? > > Thanks What I usually do is, to take this case, `git grep -w DataTransferItemList -- '*.idl'`. So, that finds, DataTransfer, and that in turn is on DragEvent and InputEvent. Unfortunately, the relevant attribute are nullable, so there's actually no way to create one of these attributes using scripts. Grep for dataTransfer to find some tests using eventSender to test this. Maybe adding an shouldThrow(...) to fast/events/clipboard-dataTransferItemList.html would work. But, the most important thing is checking what other browsers do, and for that you'll need a manual test, at least for Edge. For Gecko and WebKit, it can be quicker to just look at their IDL files: https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/DataTransferItemL... https://github.com/WebKit/webkit/blob/master/Source/WebCore/dom/DataTransferI... Although, unless you're very sure, it's still best to test the final product, there may be bindings issues that make the IDL not reflect the actual behavior.
Description was changed from ========== DataTransferItemList#add() file argument should not be nullable BUG=662005 ========== to ========== DataTransferItemList#add() file argument should not be nullable Compatibility: dataTransfer.items.add(null) Edge: does not throw Firefox 49: dataTransfer.items is undefined Firefox 50: throw as expected Safari: dataTransfer.items is undefined No use counter is added as we will likely get informed if any problems come up https://bugzilla.mozilla.org/show_bug.cgi?id=906420 BUG=662005 ==========
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I added the layout tests in third_party/WebKit/LayoutTests/fast/events/drag-dataTransferItemList.html instead, as there was already a test for the case items.add(null)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== DataTransferItemList#add() file argument should not be nullable Compatibility: dataTransfer.items.add(null) Edge: does not throw Firefox 49: dataTransfer.items is undefined Firefox 50: throw as expected Safari: dataTransfer.items is undefined No use counter is added as we will likely get informed if any problems come up https://bugzilla.mozilla.org/show_bug.cgi?id=906420 BUG=662005 ========== to ========== DataTransferItemList#add() file argument should not be nullable Compatibility: dataTransfer.items.add(null) Edge: does not throw Firefox 49: dataTransfer.items is undefined Firefox 50: throw as expected Safari: dataTransfer.items is undefined No use counter is added as we will likely get informed if any problems come up https://bugzilla.mozilla.org/show_bug.cgi?id=906420 BUG=662005 ==========
The CQ bit was checked by foolip@chromium.org
lgtm
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": 40001, "attempt_start_ts": 1479892214398710,
"parent_rev": "151cd2911c388845a4d273c6671ec7a0bcbbc05c", "commit_rev":
"a1c6ecb5a625811bb18481208a6b6a36c9be7824"}
Message was sent while issue was closed.
Description was changed from ========== DataTransferItemList#add() file argument should not be nullable Compatibility: dataTransfer.items.add(null) Edge: does not throw Firefox 49: dataTransfer.items is undefined Firefox 50: throw as expected Safari: dataTransfer.items is undefined No use counter is added as we will likely get informed if any problems come up https://bugzilla.mozilla.org/show_bug.cgi?id=906420 BUG=662005 ========== to ========== DataTransferItemList#add() file argument should not be nullable Compatibility: dataTransfer.items.add(null) Edge: does not throw Firefox 49: dataTransfer.items is undefined Firefox 50: throw as expected Safari: dataTransfer.items is undefined No use counter is added as we will likely get informed if any problems come up https://bugzilla.mozilla.org/show_bug.cgi?id=906420 BUG=662005 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== DataTransferItemList#add() file argument should not be nullable Compatibility: dataTransfer.items.add(null) Edge: does not throw Firefox 49: dataTransfer.items is undefined Firefox 50: throw as expected Safari: dataTransfer.items is undefined No use counter is added as we will likely get informed if any problems come up https://bugzilla.mozilla.org/show_bug.cgi?id=906420 BUG=662005 ========== to ========== DataTransferItemList#add() file argument should not be nullable Compatibility: dataTransfer.items.add(null) Edge: does not throw Firefox 49: dataTransfer.items is undefined Firefox 50: throw as expected Safari: dataTransfer.items is undefined No use counter is added as we will likely get informed if any problems come up https://bugzilla.mozilla.org/show_bug.cgi?id=906420 BUG=662005 Committed: https://crrev.com/11a9aad4ec1d0ae6099a3721813f9a20cfc6fe6b Cr-Commit-Position: refs/heads/master@{#434137} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/11a9aad4ec1d0ae6099a3721813f9a20cfc6fe6b Cr-Commit-Position: refs/heads/master@{#434137} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
