Closed Bug 1351193 Opened 7 years ago Closed 6 years ago

Implement the DataTransfer constructor

Categories

(Core :: DOM: Copy & Paste and Drag & Drop, enhancement, P3)

Product:
Core
Component:
DOM: Copy & Paste and Drag & Drop
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- ?
firefox62 --- fixed

People

(Reporter: d, Assigned: annyG, Mentored)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files)

Bug 1351193 - Part 1: Added new DataTransfer constructor,
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1351193 - Part 2: Prevent content sniffing of the response in original echo-content.py,
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1351193 - Part 3: Update status of affected Web-Platform tests,
59 bytes, text/x-review-board-request
nika
: review+
Details 
Spec change: https://github.com/whatwg/html/commit/688a102b52c7050a6808a05c5d8fe149c0937fd7
Tests: http://w3c-test.org/html/editing/dnd/datastore/datatransfer-constructor-001.html

This is part of a larger feature for async clipboard APIs; see discussion in https://github.com/w3c/clipboard-apis/issues/33. But it can be implemented independently.
Implementing this is going to be exciting, given the existing ChromeConstructor we have here.

Specifically, we have:

  [ChromeConstructor(DOMString eventType, boolean isExternal)]

and the spec has:

  [Constructor]

We can change our existing thing to be an additional consructor overload, but that will be observably different from the spec when arguments are passed to the constructor.

We could try changing our codegen to allow both Constructor and ChromeConstructor on the same interface and skipping all the ChromeConstructor-related bits if called from non-system code.

I can mentor someone interested in learning about bindings codegen if they want to try to take that on.  Of course there will also be the additional work of actually implementing the spec constructor.
Mentor: bzbarsky
status-firefox56: --- → wontfix
status-firefox57: --- → wontfix
status-firefox58: --- → fix-optional
Priority: -- → P3
This looks like a fun challenge, could you assign me it?

How would I go about starting?
Flags: needinfo?(bzbarsky)
Start by making sure you can build Firefox.  ;)

If you have that working, see comment 1 and ask me questions (private mail is fine) about anything that's not clear in there.
Assignee: nobody → me
Flags: needinfo?(bzbarsky)
So I guess my steps would be:

1. Add the constructor defined in https://html.spec.whatwg.org/multipage/dnd.html#the-datatransfer-interface to DataTransfer.webidl.

2. Add the C++ reflections to DataTransfer.cpp/.h as described by https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#C_reflections_of_WebIDL_constructs

3. Add tests based on http://w3c-test.org/html/editing/dnd/datastore/datatransfer-constructor-001.html
Flags: needinfo?(bzbarsky)
Step 3 doesn't really need to be done.  That test is already in-tree at testing/web-platform/tests/html/editing/dnd/datastore/datatransfer-constructor-001.html

You would probably need to edit, or remove, testing/web-platform/meta/html/editing/dnd/datastore/datatransfer-constructor-001.html.ini to indicate the passing status of the test.

That said, the resulting behavior would not be quite spec-compliant because of the issue comemnt 1 describes around [ChromeConstructor] already existing on the interface in question...
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7)
> That said, the resulting behavior would not be quite spec-compliant because
> of the issue comemnt 1 describes around [ChromeConstructor] already existing
> on the interface in question...

Right. So how would I go about doing that, skipping ChromeConstructor for non system calls?
Flags: needinfo?(bzbarsky)
That's where changing the codegen comes in.  

Right now, [ChromeConstructor] is implemented like [Constructor] but with the constructor operation marked "ChromeOnly", and CGClassConstructor in dom/bindings/Codegen.py notices that "ChromeOnly" annotation and spits out a security check.

What we would probably need to do is keep track of the chrome and non-chrome constructors separately, and have CGClassConstructor generate both halves of the branch when both are present.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> What we would probably need to do is keep track of the chrome and non-chrome
> constructors separately, and have CGClassConstructor generate both halves of
> the branch when both are present.

I'm trying to wrap my mind on how the codegen works. 

So if we have a case where both [ChromeConstructor] and [Constructor] are present, is the idea to run codegen for each and then put those two together in whatever calls CGClassConstructor (CGDescriptor, or something further back)?

What would happen if we didn't change the codegen? All of this assuming that I only add the spec constructor, how would it respond (i.e. it only looks at one, we get a weird mix of both, there's an error, etc.)?
Flags: needinfo?(bzbarsky)
> is the idea to run codegen for each and then put those two together in whatever calls CGClassConstructor

You want a single CGClassConstructor that generates a single function, because in JS there's just the one constructor object.

But that function would possibly have two codepaths depending on the "caller is chrome" value when we have both Constructor and ChromeConstructor.

> What would happen if we didn't change the codegen?

Well, if you change nothing about the parser or codegen and just add "Constructor" to the interface, then the parser would error out, because we would be trying to set up a constructor that's both chromeonly and not, and parsing doesn't allow that.  So at the very least you would need to change the parser to output separate operations for a constructor and chromeconstructor, and then update the codegen to handle that.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11)
> But that function would possibly have two codepaths depending on the "caller
> is chrome" value when we have both Constructor and ChromeConstructor.

So where does the logic of "if Chrome, ChromeConstructor, else Constructor" occur? In CGClassConstructor itself, or in it's output?

> So at the very least you would need to change
> the parser to output separate operations for a constructor and
> chromeconstructor, and then update the codegen to handle that.

So right now, the parser would tag DataTransfer as ChromeOnly or not based on the first constructor it reads, and then it would throw an error when it got to the next constructor not sharing said tag? 

Thank you for your answers, by the way. Trying to understand this bug has been tricky, but I'd like to think I'm learning something :).
Flags: needinfo?(bzbarsky)
> So where does the logic of "if Chrome, ChromeConstructor, else Constructor" occur?

In the output of CGClassConstructor -- it's a runtime decision based on who's calling the function.

> So right now, the parser would tag DataTransfer as ChromeOnly or not based on the first constructor it reads,
> and then it would throw an error when it got to the next constructor not sharing said tag? 

It takes the actual constructor method.  See https://searchfox.org/mozilla-central/rev/38bddf549db0f26d4d7575cf695958ff703a3ed7/dom/bindings/parser/WebIDL.py#1643-1645
Flags: needinfo?(bzbarsky)
Considering my upcoming personal responsibilities, I think I bit off more than I could chew with this, so I'm taking myself off. I might come back to it if nobody else takes it up within the next couple of weeks, it's a very interesting system.

For any future readers, I highly recommend reading the documentation on WebIDL bindings (https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings), it answers a lot of questions that I had, and I frankly should've read it before even asking for this bug. 

Thanks again for answering all of my questions, Boris, even the ones that seem kind of obvious in retrospect.
Assignee: me → nobody
Rofael, you're welcome, and thank you for giving this a shot!
Just to point out that it is actually already possible to get a new writable DataTransfer object in Firefox by exploiting a bug in the ClipBoardEvent constructor: 

var dT = new ClipboardEvent('').clipboardData;

Indeed, this DataTransfer should not be writable but currently is.

(related: https://bugzilla.mozilla.org/show_bug.cgi?id=1422655)
My intern will be taking this on. Assigning to myself in the interim.
Assignee: nobody → nika
Assignee: nika → agakhokidze
Mentor: bzbarsky → nika
Comment on attachment 8980702 [details]
Bug 1351193 - Part 1: Added new DataTransfer constructor,

https://reviewboard.mozilla.org/r/246868/#review253356

r=me with those changes

::: commit-message-11ee7:2
(Diff revision 1)
> +Bug 1351193 - Part 1: Added new DataTransfer constructor, r=nika
> +

Please add an explanation of why it was ok to delete the old ChromeOnly constructor here :)

e.g. The chrome only constructor was only used for tests which can be easily changed.

::: dom/events/DataTransfer.cpp:204
(Diff revision 1)
> -    return nullptr;
> -  }
>  
> -  EventMessage eventMessage = nsContentUtils::GetEventMessage(eventTypeAtom);
>    RefPtr<DataTransfer> transfer = new DataTransfer(aGlobal.GetAsSupports(),
> -                                                     eventMessage, aIsExternal,
> +                                                   eCopy, false, -1);

please add a comment next to the false and -1 with the parameter name
Attachment #8980702 - Flags: review?(nika) → review+
Comment on attachment 8980703 [details]
Bug 1351193 - Part 2: Prevent content sniffing of the response in original echo-content.py,

https://reviewboard.mozilla.org/r/246870/#review253358

::: testing/web-platform/meta/MANIFEST.json:517685
(Diff revision 1)
>    "css/css-shapes/spec-examples/shape-outside-008.html": [
>     "5a9f340648d85f2fdd3cd3fe74b2e145fbc1a54a",
>     "reftest"
>    ],
>    "css/css-shapes/spec-examples/shape-outside-010.html": [
> -   "7baf8e86ee451f08ab18e03000d64a529a2824d0",
> +   "929078a33a23f1d10ce9d0f89016725f233e133e",

I dont really understand why this change is needed?

::: testing/web-platform/tests/fetch/api/resources/echo-content.py:5
(Diff revision 1)
>  def main(request, response):
>  
>      headers = [("X-Request-Method", request.method),
>                 ("X-Request-Content-Length", request.headers.get("Content-Length", "NO")),
> -               ("X-Request-Content-Type", request.headers.get("Content-Type", "NO"))]
> +               # Avoid any kind of content sniffing on the response.

please don't delete this line from the tests
Attachment #8980703 - Flags: review?(nika) → review-
Comment on attachment 8980704 [details]
Bug 1351193 - Part 3: Update status of affected Web-Platform tests,

https://reviewboard.mozilla.org/r/246872/#review253374

::: toolkit/content/tests/chrome/window_browser_drop.xul:170
(Diff revision 1)
>    // Dropping multiple items should open multiple pages.
>    await expectLink(browser,
>                      [ { url: "http://www.example.com/",
>                          name: "Example.com" },
>                        { url: "http://www.mozilla.org/",
>                          name: "http://www.mozilla.org/" }],
>                      [ [ { type: "text/x-moz-url",
>                            data: "http://www.example.com/\nExample.com" } ],
>                        [ { type: "text/plain",
>                            data: "http://www.mozilla.org/" } ] ],
>                      "text/x-moz-url and text/plain drop on browser " + type);

Lets just delete this test caller so that we dont have to make this change?
Attachment #8980704 - Flags: review?(nika) → review-
Comment on attachment 8980703 [details]
Bug 1351193 - Part 2: Prevent content sniffing of the response in original echo-content.py,

https://reviewboard.mozilla.org/r/246870/#review254486
Attachment #8980703 - Flags: review?(nika) → review+
Comment on attachment 8980704 [details]
Bug 1351193 - Part 3: Update status of affected Web-Platform tests,

https://reviewboard.mozilla.org/r/246872/#review254488

::: commit-message-f6cbb:1
(Diff revision 2)
> +Bug 1351193 - Part 3: Changed the status of tests that were affected by

Let's use a shorter bug name:
Bug 1351193 - Part 3: Update status of affected Web-Platform tests, r=nika

Also, let's put the short description on one line.

::: commit-message-f6cbb:5
(Diff revision 2)
> +Bug 1351193 - Part 3: Changed the status of tests that were affected by
> +addition of a DataTransfer 0 params constructor and by removal of the
> +Chrome only constructor, r=nika
> +
> +Because of modifications to the DataTransfer constructors, the status of

Please also change the constructor call in dom/bindings/test/test_dom_xrays.html to remove the now-ignored parameters.
Attachment #8980704 - Flags: review?(nika) → review+
Comment on attachment 8980704 [details]
Bug 1351193 - Part 3: Update status of affected Web-Platform tests,

https://reviewboard.mozilla.org/r/246872/#review254488

> Please also change the constructor call in dom/bindings/test/test_dom_xrays.html to remove the now-ignored parameters.

Thank you for catching that!
Pushed by nlayzell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/94ffa7844ae7
Part 1: Added new DataTransfer constructor, r=Nika
https://hg.mozilla.org/integration/autoland/rev/b05e943f8d2c
Part 2: Prevent content sniffing of the response in original echo-content.py, r=Nika
https://hg.mozilla.org/integration/autoland/rev/cc8d55217398
Part 3: Update status of affected Web-Platform tests, r=Nika
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11307 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging
When trying to merge autoland to mozilla-central: this push 94ffa7844ae7 got denied because the reviewer name is wrong.

You wrote r=nika, the correct review name would have been r=mystor.

I had to backout your push so it can be edited with the right names.

HG knows Nika as :mystor

Please update names and request review again.
Flags: needinfo?(nika)
Flags: needinfo?(agakhokidze)
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/561dd8c84f85
Backed out 3 changesets for landing with the wrong reviewer name
> When trying to merge autoland to mozilla-central: this push 94ffa7844ae7 got denied because the reviewer name is wrong.

What was the actual error message?

The correct name is "nika", so whatever system thinks it's "mystor" just needs to be fixed.  But you didn't provide enough information to tell what that is....
Flags: needinfo?(dluca)
Depends on: 1466362
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #37)
> > When trying to merge autoland to mozilla-central: this push 94ffa7844ae7 got denied because the reviewer name is wrong.
> 
> What was the actual error message?
> 
> The correct name is "nika", so whatever system thinks it's "mystor" just
> needs to be fixed.  But you didn't provide enough information to tell what
> that is....

Actually, he did provide the info he had at that point. Please see bug 1466362.

Issue was/is https://irccloud.mozilla.com/file/9tlixUbH/image.png
Flags: needinfo?(dluca) → needinfo?(bzbarsky)
It's possible that https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hghooks/mozhghooks/check/prevent_webidl_changes.py#l30 just needs to have 'nika' added to the nick list.  Looks like the current entry was added in bug 1428785.
> Actually, he did provide the info he had at that point. Please see bug 1466362.

OK, that info wasn't exactly clear in this bug...

Given that output, comment 39 is what needs to happen.
Flags: needinfo?(bzbarsky)
From #vcs:

tomprince> Anybody around that can review and deploy https://phabricator.services.mozilla.com/D1518 ? This is currently blocking merging autoland -> mozilla-centra. (cc: apavel|sheriffduty)
That change doesn't look correct.  The webidl hook needs the things in the nick list to be all-lowercase, no?
:gps could you please help us out wit this issue?
Flags: needinfo?(gps)
From the discussion above, it seems that I don't need to resend the patches for review again, because 'nika' will be added to the nickname list?
(In reply to Anny Gakhokidze [:annyG] from comment #44)
> From the discussion above, it seems that I don't need to resend the patches
> for review again, because 'nika' will be added to the nickname list?

Yes, that is correct, you have the details here Bug 1466362.
D1518 has been deployed.
Flags: needinfo?(gps)
Has this been backed out & we need to re-land it, or are we all good?
Flags: needinfo?(nika)
This needs to be relanded.
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d697c464a22e
Part 1: Added new DataTransfer constructor, r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/74219e6cc8b0
Part 2: Prevent content sniffing of the response in original echo-content.py, r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6f8fbdfc84b
Part 3: Update status of affected Web-Platform tests, r=nika
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Flags: needinfo?(agakhokidze)
I have checked that the docs are in order for this. We already had documentation for the constructor, so I've updated the browser compat tables:
https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer
https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer/DataTransfer

And added a note to the Fx62 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/62#DOM

Let me know if this looks OK. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
Hi Chris, it looks good to me! Thanks!!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: