Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2875013002:
DataTransfer: Make |types| be a FrozenArray<DOMString>. (Closed)
|
Created:
3 years, 7 months ago by Raphael Kubo da Costa (rakuco) Modified:
3 years, 7 months ago CC:
chromium-reviews, blink-reviews, dcheng, Rick Byers Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDataTransfer: Make |types| be a FrozenArray<DOMString>.
Fix a TODO item: https://github.com/whatwg/html/issues/11 was solved several
months ago by
https://github.com/whatwg/html/commit/466379ade56b8ba18584057085443844f4df5669,
so we can finally use a FrozenArray here.
Due to the way |types| is spec'ed, we need to use the [CachedAttribute]
custom extended attribute so that we can tell the bindings layer when a new
FrozenArray must be returned.
Since the data store item list can change via calls to DataTransfer's
setData() and DataTransfer.clearData(), as well as DataTransfer.items'
add(), remove() clear(), DataTransfer itself is notified of changes via the
newly-added DataObject::Observer whenever DataObject's underlying
|item_list_| is modified.
A new test has been added to verify this behavior.
BUG=652815
R=foolip@chromium.org,jsbell@chromium.org
Review-Url: https://codereview.chromium.org/2875013002
Cr-Commit-Position: refs/heads/master@{#472072}
Committed: https://chromium.googlesource.com/chromium/src/+/fb8089d1d38e7573bbf99ea1828425bc25630c3d
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add new test, use CachedAttribute #
Total comments: 4
Patch Set 3 : Follow to jsbell's suggestions #Patch Set 4 : Fix interactive_ui_tests #
Total comments: 3
Patch Set 5 : Use an observer to be notified of changes #
Total comments: 10
Patch Set 6 : Adjust to comments, add new unit test #Patch Set 7 : Fix win_chromium_compile_dbg_ng #Messages
Total messages: 48 (32 generated)
PTAL There doesn't seem to be a clear set of owners for clipboard/, but this change seems to fall under your areas of interest.
The CQ bit was checked by raphael.kubo.da.costa@intel.com 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.
If you enjoy this kind, I also have a similar TODO in NavigatorLanguage.idl, for another CL. https://codereview.chromium.org/2875013002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/clipboard/DataTransfer.idl (right): https://codereview.chromium.org/2875013002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/clipboard/DataTransfer.idl:42: readonly attribute FrozenArray<DOMString> types; This will require some tests, can you update LayoutTests/external/wpt/html/editing/dnd/datastore/datatransfer-constructor-001.html? LayoutTests/external/wpt/mediasession/mediametadata.html is an example of how to use Object.isFrozen to test this. Although you are not changing it here, can you also test `assert_equals(transfer.types, transfer.types)`. I'm pretty sure that will fail, and if you want to fix it at the same time, that'd be great. I think you'll need to use [CachedAttribute=bla] here to fix it.
Description was changed from ========== DataTransfer: Make |types| be a FrozenArray<DOMString>. Fix a TODO item: https://github.com/whatwg/html/issues/11 was solved several months ago by https://github.com/whatwg/html/commit/466379ade56b8ba18584057085443844f4df5669, so we can finally use a FrozenArray here. R=foolip@chromium.org,rbyers@chromium.org ========== to ========== DataTransfer: Make |types| be a FrozenArray<DOMString>. Fix a TODO item: https://github.com/whatwg/html/issues/11 was solved several months ago by https://github.com/whatwg/html/commit/466379ade56b8ba18584057085443844f4df5669, so we can finally use a FrozenArray here. R=foolip@chromium.org,rbyers@chromium.org ==========
rbyers@chromium.org changed reviewers: - rbyers@chromium.org
I believe the storage team now owns the clipboard APIs so cc/ jsbell (but either foolip@ or jsbell@ can approve this change).
Description was changed from ========== DataTransfer: Make |types| be a FrozenArray<DOMString>. Fix a TODO item: https://github.com/whatwg/html/issues/11 was solved several months ago by https://github.com/whatwg/html/commit/466379ade56b8ba18584057085443844f4df5669, so we can finally use a FrozenArray here. R=foolip@chromium.org,rbyers@chromium.org ========== to ========== DataTransfer: Make |types| be a FrozenArray<DOMString>. Fix a TODO item: https://github.com/whatwg/html/issues/11 was solved several months ago by https://github.com/whatwg/html/commit/466379ade56b8ba18584057085443844f4df5669, so we can finally use a FrozenArray here. R=foolip@chromium.org ==========
Description was changed from ========== DataTransfer: Make |types| be a FrozenArray<DOMString>. Fix a TODO item: https://github.com/whatwg/html/issues/11 was solved several months ago by https://github.com/whatwg/html/commit/466379ade56b8ba18584057085443844f4df5669, so we can finally use a FrozenArray here. R=foolip@chromium.org ========== to ========== DataTransfer: Make |types| be a FrozenArray<DOMString>. Fix a TODO item: https://github.com/whatwg/html/issues/11 was solved several months ago by https://github.com/whatwg/html/commit/466379ade56b8ba18584057085443844f4df5669, so we can finally use a FrozenArray here. Due to the way |types| is spec'ed, we need to use the [CachedAttribute] custom extended attribute so that we can tell the bindings layer when a new FrozenArray must be returned: we can consider that the data store's item list has been changed whenever DataTransfer.setData() has been called or when calling clearData() produces an item list with a different length. A new test has been added to verify this behavior. BUG=652815 R=foolip@chromium.org,jsbell@chromium.org ==========
raphael.kubo.da.costa@intel.com changed reviewers: + jsbell@chromium.org
Patch v2 is up, please take another look: - There's actually a bug about this, so reference it in the CL description. - Add a new test for types and how it is supposed to work. The way we detect whether a new FrozenArray needs to be returned looks very simplistic, but matches what I could make of the prose describing DataTransfer's "types array".
The CQ bit was checked by raphael.kubo.da.costa@intel.com 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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
mostly lg, one meaningful suggestion for the dirty flag. Also, can you update LayoutTests/external/wpt/interfaces/html.idl -- alas, we're not importing html/dom/interfaces.html so it doesn't get us any coverage. https://codereview.chromium.org/2875013002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/datastore/datatransfer-types.html (right): https://codereview.chromium.org/2875013002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/datastore/datatransfer-types.html:5: <script src="/resources/testharnessreport.js"></script> A spec link is nice to have (but not strictly required), e.g. <link rel="help" href="https://html.spec.whatwg.org/multipage/interaction.html#the-datatransfer-interface"> (Less important here than elsewhere since the directory structure here is informative.) https://codereview.chromium.org/2875013002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/datastore/datatransfer-types.html:8: let dt = new DataTransfer(); nit: could be const (here and below) https://codereview.chromium.org/2875013002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/datastore/datatransfer-types.html:9: assert_true(Object.isFrozen(dt.types), "types must be a FrozenArray<>"); Do we want to assert_true(Array.isArray(dt.types)) too? https://codereview.chromium.org/2875013002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/clipboard/DataTransfer.cpp (right): https://codereview.chromium.org/2875013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/clipboard/DataTransfer.cpp:204: data_store_item_list_changed_ = false; Hrm... most places where we use [CachedAttribute] the hasXXXChanged()/isXXXDirty() bool method is const and the flag/state is reset in the xxx() accessor. NavigatorLanguage is the only one like this. History and the 5 in the modules are const methods. What do you think about changing this? i.e. make this method const, and move the '... = false' line into types()
The CQ bit was checked by raphael.kubo.da.costa@intel.com 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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
lgtm % question, but please wait for jsbell@ to confirm as well. https://codereview.chromium.org/2875013002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/datastore/datatransfer-types.html (right): https://codereview.chromium.org/2875013002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/datastore/datatransfer-types.html:62: }, "type's identity"); Good test! https://codereview.chromium.org/2875013002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/clipboard/DataTransfer.cpp (right): https://codereview.chromium.org/2875013002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/clipboard/DataTransfer.cpp:202: return data_store_item_list_changed_ || !CanReadTypes(); Is there a test that would fail if the CanReadTypes() bit of this condition were removed?
Thanks for the review; I'm actually making the patch a bit more complicated because the data store can change via DataTransfer.items, the new version should be ready soon. https://codereview.chromium.org/2875013002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/clipboard/DataTransfer.cpp (right): https://codereview.chromium.org/2875013002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/clipboard/DataTransfer.cpp:202: return data_store_item_list_changed_ || !CanReadTypes(); On 2017/05/15 15:11:02, foolip wrote: > Is there a test that would fail if the CanReadTypes() bit of this condition were > removed? Yes; the previous iterations of this CL didn't have it, and were causing SameSiteSubframe/DragAndDropBrowserTest.DragImageBetweenFrames/0 and CrossSiteSubframe/DragAndDropBrowserTest.DragImageBetweenFrames/0 (from interactive_ui_tests) to fail because they were expecting "types" to return an empty array on "dragend" events. To be honest, I can't find anything on the spec saying types' behavior should vary depending on the data store's mode but I found it safer to preserve that part of the current behavior (ie. just because types returned a filled-in array when the data store's mode was read-write doesn't mean the same array should be returned when the data store's mode is set to "protected").
Description was changed from ========== DataTransfer: Make |types| be a FrozenArray<DOMString>. Fix a TODO item: https://github.com/whatwg/html/issues/11 was solved several months ago by https://github.com/whatwg/html/commit/466379ade56b8ba18584057085443844f4df5669, so we can finally use a FrozenArray here. Due to the way |types| is spec'ed, we need to use the [CachedAttribute] custom extended attribute so that we can tell the bindings layer when a new FrozenArray must be returned: we can consider that the data store's item list has been changed whenever DataTransfer.setData() has been called or when calling clearData() produces an item list with a different length. A new test has been added to verify this behavior. BUG=652815 R=foolip@chromium.org,jsbell@chromium.org ========== to ========== DataTransfer: Make |types| be a FrozenArray<DOMString>. Fix a TODO item: https://github.com/whatwg/html/issues/11 was solved several months ago by https://github.com/whatwg/html/commit/466379ade56b8ba18584057085443844f4df5669, so we can finally use a FrozenArray here. Due to the way |types| is spec'ed, we need to use the [CachedAttribute] custom extended attribute so that we can tell the bindings layer when a new FrozenArray must be returned. Since the data store item list can change via calls to DataTransfer's setData() and DataTransfer.clearData(), as well as DataTransfer.items' add(), remove() clear(), DataTransfer itself is notified of changes via the newly-added DataObject::Observer whenever DataObject's underlying |item_list_| is modified. A new test has been added to verify this behavior. BUG=652815 R=foolip@chromium.org,jsbell@chromium.org ==========
The CQ bit was checked by raphael.kubo.da.costa@intel.com 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...
PTAL at patch v5: it introduces DataObject::Observer as a way to notify DataTransfer to invalidate its types cache. The rationale here is that DataTransfer.setData() and DataTransfer.clearData() are not the only ways to change a data store: it can be changed via DataTransferItemList as well. Since the code would look similar in both, I found it better to use an observer and leave DataTransferItemList untouched.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
lgtm Since we already have DataObjectTest.cpp it wouldn't be hard to add unit tests that verify that an observer is called (N times) when each of the mutation methods is called. https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/clipboard/DataObject.cpp (right): https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/clipboard/DataObject.cpp:97: const size_t old_length = item_list_.size(); We don't care about the size specifically, just that it was non-empty? How about making this an early return instead, i.e. `if (item_list_.isEmpty()) return;` or wrap the work in `if (!item_list_.isEmpty()) { ... }` ? https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/clipboard/DataObject.cpp:181: if (!Add(data, type)) We'll get two calls to NotifyItemListChanged() for each call to SetData(), which seems fine. Calling it out in case we maybe want to mention that in the header. Probably not worth it. https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/clipboard/DataObject.h (right): https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/clipboard/DataObject.h:59: virtual void OnItemListChanged() {} Make this pure virtual? i.e. replace `{}` with `= 0;` ? https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/clipboard/DataTransfer.cpp (right): https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/clipboard/DataTransfer.cpp:206: return types; Not new in this CL, but how about `return Vector<String>();` here and remove the `types` local? https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/clipboard/DataTransfer.h (right): https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/clipboard/DataTransfer.h:146: void OnItemListChanged(); Mark with `override`
Thanks; I think I've addressed everything in patch v6. https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/clipboard/DataObject.cpp (right): https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/clipboard/DataObject.cpp:97: const size_t old_length = item_list_.size(); On 2017/05/15 20:49:28, jsbell wrote: > We don't care about the size specifically, just that it was non-empty? > > How about making this an early return instead, i.e. `if (item_list_.isEmpty()) > return;` or wrap the work in `if (!item_list_.isEmpty()) { ... }` ? > Done. https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/clipboard/DataObject.cpp:181: if (!Add(data, type)) On 2017/05/15 20:49:28, jsbell wrote: > We'll get two calls to NotifyItemListChanged() for each call to SetData(), which > seems fine. Calling it out in case we maybe want to mention that in the header. > Probably not worth it. It doesn't hurt either, so I've added a small comment to OnItemListChanged() in the header. https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/clipboard/DataObject.h (right): https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/clipboard/DataObject.h:59: virtual void OnItemListChanged() {} On 2017/05/15 20:49:28, jsbell wrote: > Make this pure virtual? i.e. replace `{}` with `= 0;` ? Done. https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/clipboard/DataTransfer.cpp (right): https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/clipboard/DataTransfer.cpp:206: return types; On 2017/05/15 20:49:28, jsbell wrote: > Not new in this CL, but how about `return Vector<String>();` here and remove the > `types` local? > Done. https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/clipboard/DataTransfer.h (right): https://codereview.chromium.org/2875013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/clipboard/DataTransfer.h:146: void OnItemListChanged(); On 2017/05/15 20:49:29, jsbell wrote: > Mark with `override` Done.
The CQ bit was checked by raphael.kubo.da.costa@intel.com 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 checked by raphael.kubo.da.costa@intel.com 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.
The CQ bit was checked by raphael.kubo.da.costa@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org, jsbell@chromium.org Link to the patchset: https://codereview.chromium.org/2875013002/#ps120001 (title: "Fix win_chromium_compile_dbg_ng ")
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": 120001, "attempt_start_ts": 1494939733410890,
"parent_rev": "844305bdea48410a37ff8d6ebf120ee2167b91e0", "commit_rev":
"fb8089d1d38e7573bbf99ea1828425bc25630c3d"}
Message was sent while issue was closed.
Description was changed from ========== DataTransfer: Make |types| be a FrozenArray<DOMString>. Fix a TODO item: https://github.com/whatwg/html/issues/11 was solved several months ago by https://github.com/whatwg/html/commit/466379ade56b8ba18584057085443844f4df5669, so we can finally use a FrozenArray here. Due to the way |types| is spec'ed, we need to use the [CachedAttribute] custom extended attribute so that we can tell the bindings layer when a new FrozenArray must be returned. Since the data store item list can change via calls to DataTransfer's setData() and DataTransfer.clearData(), as well as DataTransfer.items' add(), remove() clear(), DataTransfer itself is notified of changes via the newly-added DataObject::Observer whenever DataObject's underlying |item_list_| is modified. A new test has been added to verify this behavior. BUG=652815 R=foolip@chromium.org,jsbell@chromium.org ========== to ========== DataTransfer: Make |types| be a FrozenArray<DOMString>. Fix a TODO item: https://github.com/whatwg/html/issues/11 was solved several months ago by https://github.com/whatwg/html/commit/466379ade56b8ba18584057085443844f4df5669, so we can finally use a FrozenArray here. Due to the way |types| is spec'ed, we need to use the [CachedAttribute] custom extended attribute so that we can tell the bindings layer when a new FrozenArray must be returned. Since the data store item list can change via calls to DataTransfer's setData() and DataTransfer.clearData(), as well as DataTransfer.items' add(), remove() clear(), DataTransfer itself is notified of changes via the newly-added DataObject::Observer whenever DataObject's underlying |item_list_| is modified. A new test has been added to verify this behavior. BUG=652815 R=foolip@chromium.org,jsbell@chromium.org Review-Url: https://codereview.chromium.org/2875013002 Cr-Commit-Position: refs/heads/master@{#472072} Committed: https://chromium.googlesource.com/chromium/src/+/fb8089d1d38e7573bbf99ea18284... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/fb8089d1d38e7573bbf99ea18284...
Message was sent while issue was closed.
On 2017/05/16 08:51:51, Raphael Kubo da Costa (rakuco) wrote: > Thanks; I think I've addressed everything in patch v6. > Awesome, thanks!
Message was sent while issue was closed.
On 2017/05/16 at 16:46:44, jsbell wrote: > On 2017/05/16 08:51:51, Raphael Kubo da Costa (rakuco) wrote: > > Thanks; I think I've addressed everything in patch v6. > > > > Awesome, thanks! An upstream export PR was created for this but the results were unstable on Edge: https://github.com/w3c/web-platform-tests/pull/5942 Any help in resolving this would be appreciated. I can update the PR with any new patches.
Message was sent while issue was closed.
On 2017/05/16 18:26:24, jeffcarp wrote: > An upstream export PR was created for this but the results were unstable on > Edge: > https://github.com/w3c/web-platform-tests/pull/5942 > > Any help in resolving this would be appreciated. I can update the PR with any > new patches. Interesting... the failure looks unrelated to this change -- specifically, it's validating an XMLDocument instance! https://github.com/w3c/web-platform-tests/blob/master/html/dom/interfaces.htm... The change to interfaces/html.idl may have tickled the bots. *sigh* The Edge (and Safari) bots are new. Do we know how stable the results are for a typical run?
Message was sent while issue was closed.
On 2017/05/16 18:52:02, jsbell wrote: > On 2017/05/16 18:26:24, jeffcarp wrote: > > An upstream export PR was created for this but the results were unstable on > > Edge: > > https://github.com/w3c/web-platform-tests/pull/5942 > > > > Any help in resolving this would be appreciated. I can update the PR with any > > new patches. > > Interesting... the failure looks unrelated to this change -- specifically, it's > validating an XMLDocument instance! > > https://github.com/w3c/web-platform-tests/blob/master/html/dom/interfaces.htm... > > The change to interfaces/html.idl may have tickled the bots. *sigh* > > The Edge (and Safari) bots are new. Do we know how stable the results are for a > typical run? As you're say they're brand new, activated on May 15. This is the first Edge flakiness that I've seen, but I don't know what the general state is. I'll ask @bobholt on https://github.com/w3c/web-platform-tests/pull/5942, which I've now merged manually after confirming that the flakiness is not new. |
