Implement DragEvent and move MouseEvent.dataTransfer

Implement DragEvent and move MouseEvent.dataTransfer

This CL is to add a new interface and we are discussing as follows.

Intent to Implement and Ship : 
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/SbQzldr3aOM

R=rbyers@chromium.org
BUG=498504
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200649

[Jimmy Jo, 2015-07-16 13:38:24]

jincheol.jo@navercorp.com changed reviewers:
+ philipj@opera.com, rbyers@chromium.org

[Jimmy Jo, 2015-07-17 11:31:47]

Patchset #1 (id:1) has been deleted

[Jimmy Jo, 2015-07-17 12:53:42]

PTAL!!

[philipj_slow, 2015-07-20 14:40:18]

I'm going to defer this to rbyers@ since I have many reviews right now, but will
note that this needs a test.

[philipj_slow, 2015-07-20 14:40:35]

philipj@opera.com changed reviewers:
- philipj@opera.com

[Jimmy Jo, 2015-07-22 12:22:21]

On 2015/07/20 14:40:18, philipj wrote:
> I'm going to defer this to rbyers@ since I have many reviews right now, but
will
> note that this needs a test.

There are so many tests about DragEvent in LayoutTest.
Could you explain more in detail what we should do about test? or some review
url?

[philipj_slow, 2015-07-22 15:10:42]

On 2015/07/22 12:22:21, Jimmy Jo wrote:
> On 2015/07/20 14:40:18, philipj wrote:
> > I'm going to defer this to rbyers@ since I have many reviews right now, but
> will
> > note that this needs a test.
> 
> There are so many tests about DragEvent in LayoutTest.
> Could you explain more in detail what we should do about test? or some review
> url?

This CL adds new interfaces DragEvent and DragEventInit, so in some existing
test you could add an `event instanceof DragEvent` check.

You'll also have to add explicit tests for the DragEvent constructor, and you'll
probably find that's difficult because it isn't actually possible to create
DataTransfer objects. This is a problem for ClipboardEvent as well, mentioned as
part of <https://github.com/w3c/clipboard-apis/issues/10>:

"If DataTransfer doesn't have a constructor, maybe it should? If it shouldn't,
then perhaps a ClipboardEvent constructor doesn't make sense either."

I've also just filed an HTML bug:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28984

I think it makes sense to split dataTransfer into a DragEvent interface even if
DragEvent doesn't have a constructor, so see perhaps you need not spend too much
time on that problem, just see if everything else adds up.

[Jimmy Jo, 2015-07-28 09:13:40]

On 2015/07/22 15:10:42, philipj wrote:
> On 2015/07/22 12:22:21, Jimmy Jo wrote:
> > On 2015/07/20 14:40:18, philipj wrote:
> > > I'm going to defer this to rbyers@ since I have many reviews right now,
but
> > will
> > > note that this needs a test.
> > 
> > There are so many tests about DragEvent in LayoutTest.
> > Could you explain more in detail what we should do about test? or some
review
> > url?
> 
> This CL adds new interfaces DragEvent and DragEventInit, so in some existing
> test you could add an `event instanceof DragEvent` check.
> 
> You'll also have to add explicit tests for the DragEvent constructor, and
you'll
> probably find that's difficult because it isn't actually possible to create
> DataTransfer objects. This is a problem for ClipboardEvent as well, mentioned
as
> part of <https://github.com/w3c/clipboard-apis/issues/10>:
> 
> "If DataTransfer doesn't have a constructor, maybe it should? If it shouldn't,
> then perhaps a ClipboardEvent constructor doesn't make sense either."
> 
> I've also just filed an HTML bug:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=28984
> 
> I think it makes sense to split dataTransfer into a DragEvent interface even
if
> DragEvent doesn't have a constructor, so see perhaps you need not spend too
much
> time on that problem, just see if everything else adds up.

@Philipj
Sorry for delay. Thanks for the detailed explanation.
I will give a test soon.

[Rick Byers, 2015-07-28 13:33:38]

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

[commit-bot: I haz the power, 2015-07-28 13:33:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232003009/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232003009/20001

[Rick Byers, 2015-07-28 13:34:27]

Nice cleanup, thanks!
/cc dcheng the DnD expert

I know you can't usefully create a DragEvent without some way to create a
DataTransfer, but are you at least able to create a DragEvent with a null
dataTransfer object?  Ideally you'd add a
fast/events/drag-event-constructor.html test (like the other
*-event-constructor.html tests there), even if it didn't do a whole lot.

Also I believe you'll need to update the expectations for at least some tests in
webexposed.  I've kicked off try jobs to see what fails.

Please also add a link to the intent-to-ship thread in the CL description.

[commit-bot: I haz the power, 2015-07-28 13:35:43]

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

[commit-bot: I haz the power, 2015-07-28 13:35:44]

Dry run: Try jobs failed on following builders:
  android_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)

[Jimmy Jo, 2015-07-29 03:52:07]

On 2015/07/28 13:35:44, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   android_blink_compile_dbg on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)

@Rick: Thanks for the kind and detailed reply. I was encouraged by you.
I will give test soon.
Also it seems to need rebase for commit-bot.

[Jimmy Jo, 2015-07-30 13:24:13]

Patchset #2 (id:40001) has been deleted

[Jimmy Jo, 2015-07-30 13:24:55]

Patchset #2 (id:60001) has been deleted

[Jimmy Jo, 2015-07-30 13:27:03]

The CQ bit was checked by jincheol.jo@navercorp.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-30 13:27:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232003009/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232003009/80001

[commit-bot: I haz the power, 2015-07-30 13:27:19]

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

[commit-bot: I haz the power, 2015-07-30 13:27:19]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Jimmy Jo, 2015-07-30 13:39:51]

I uploaded some tests.
I tried to test my code through trybot but it couldn't do well.

[Rick Byers, 2015-07-30 17:24:18]

Test looks good.  Did you try "CQ dry run"?  I'll do that now.  You may not have
access to create try jobs yourself (but we can get that for you).

https://codereview.chromium.org/1232003009/diff/80001/LayoutTests/fast/events...
File LayoutTests/fast/events/dragevent-constructor.html (right):

https://codereview.chromium.org/1232003009/diff/80001/LayoutTests/fast/events...
LayoutTests/fast/events/dragevent-constructor.html:3: <head>
nit: you can ommit html, head and body tags:
https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-...

[Rick Byers, 2015-07-30 17:24:25]

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

[commit-bot: I haz the power, 2015-07-30 17:24:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232003009/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232003009/80001

[Rick Byers, 2015-07-30 17:24:56]

Patch LGTM assuming tests pass

[commit-bot: I haz the power, 2015-07-30 18:56:07]

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

[commit-bot: I haz the power, 2015-07-30 18:56:09]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)

[Jimmy Jo, 2015-07-31 03:43:40]

I modifed  a patch to fix failure of some tests.

https://codereview.chromium.org/1232003009/diff/80001/LayoutTests/fast/events...
File LayoutTests/fast/events/dragevent-constructor.html (right):

https://codereview.chromium.org/1232003009/diff/80001/LayoutTests/fast/events...
LayoutTests/fast/events/dragevent-constructor.html:3: <head>
On 2015/07/30 17:24:18, Rick Byers (Out until Aug 4th) wrote:
> nit: you can ommit html, head and body tags:
>
https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-...

Done.

[Jimmy Jo, 2015-07-31 03:43:59]

The CQ bit was checked by jincheol.jo@navercorp.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-31 03:44:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232003009/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232003009/100001

[Jimmy Jo, 2015-07-31 03:47:59]

On 2015/07/31 03:44:05, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1232003009/100001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1232003009/100001

I tried "CQ dry run" to pass tests.

[commit-bot: I haz the power, 2015-07-31 05:12:10]

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

[commit-bot: I haz the power, 2015-07-31 05:12:11]

Dry run: Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/64549)

[Jimmy Jo, 2015-07-31 09:55:02]

On 2015/07/31 05:12:11, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   mac_blink_rel on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/64549)

I verified the reason of failure.
When it needs creating MouseEvent, we should check to need DragEvent not
MouseEvent.
Maybe we should consider all things about creating MouseEvent.

[Jimmy Jo, 2015-08-01 11:18:24]

The CQ bit was checked by jincheol.jo@navercorp.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-01 11:18:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232003009/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232003009/120001

[commit-bot: I haz the power, 2015-08-01 12:24:05]

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

[commit-bot: I haz the power, 2015-08-01 12:24:06]

Dry run: This issue passed the CQ dry run.

[Jimmy Jo, 2015-08-04 06:27:18]

On 2015/08/01 12:24:06, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

We got the LGTM3 on blink-dev thread. 
Please take a look again this patch.
It passed "CQ dry run".

[Jimmy Jo, 2015-08-06 00:55:13]

On 2015/08/04 06:27:18, Jimmy Jo wrote:
> On 2015/08/01 12:24:06, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> We got the LGTM3 on blink-dev thread. 
> Please take a look again this patch.
> It passed "CQ dry run".

ping...

[Jimmy Jo, 2015-08-06 08:11:35]

On 2015/07/30 17:24:18, Rick Byers wrote:
> Test looks good.  Did you try "CQ dry run"?  I'll do that now.  You may not
have
> access to create try jobs yourself (but we can get that for you).
> 
>
https://codereview.chromium.org/1232003009/diff/80001/LayoutTests/fast/events...
> File LayoutTests/fast/events/dragevent-constructor.html (right):
> 
>
https://codereview.chromium.org/1232003009/diff/80001/LayoutTests/fast/events...
> LayoutTests/fast/events/dragevent-constructor.html:3: <head>
> nit: you can ommit html, head and body tags:
>
https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-...

Rick!!
Could you give me "try bot permission"?

[Rick Byers, 2015-08-06 14:22:32]

Sorry for the delay, I was on vacation - finally getting caught up now.  This is
great overall, just a couple minor issues.

https://codereview.chromium.org/1232003009/diff/80001/Source/core/events/Drag...
File Source/core/events/DragEvent.cpp (right):

https://codereview.chromium.org/1232003009/diff/80001/Source/core/events/Drag...
Source/core/events/DragEvent.cpp:46: bool DragEvent::isMouseEvent() const
Why do you want isMouseEvent to be true for DragEvents?  That's inconsistent
with how WheelEvent behaves (which is also a subclass of MouseEvent).  I think
we want to use the same pattern for all Event subclasses.  Can we update the
necessary callers instead of removing this?

https://codereview.chromium.org/1232003009/diff/120001/LayoutTests/imported/w...
File LayoutTests/imported/web-platform-tests/html/editing/dnd/synthetic/001.html
(left):

https://codereview.chromium.org/1232003009/diff/120001/LayoutTests/imported/w...
LayoutTests/imported/web-platform-tests/html/editing/dnd/synthetic/001.html:14:
test(function() {
These "imported/web-platform-tests" are special, see
https://www.chromium.org/blink/importing-the-w3c-tests.  I don't think we want
to make edits to them here.  The official home is here:
https://github.com/w3c/web-platform-tests/commits/master/html/editing/dnd/syn....

I think it's fine to just update our expectations.  Obviously we didn't exactly
expect this test to pass before, and it still doesn't pass completely.

Of course if you'd also like to contribute improvements to the test back to the
w3c that's great too.  But I wouldn't block this CL on it.

[Rick Byers, 2015-08-06 14:23:40]

On 2015/08/06 08:11:35, Jimmy Jo wrote:
> On 2015/07/30 17:24:18, Rick Byers wrote:
> > Test looks good.  Did you try "CQ dry run"?  I'll do that now.  You may not
> have
> > access to create try jobs yourself (but we can get that for you).
> > 
> >
>
https://codereview.chromium.org/1232003009/diff/80001/LayoutTests/fast/events...
> > File LayoutTests/fast/events/dragevent-constructor.html (right):
> > 
> >
>
https://codereview.chromium.org/1232003009/diff/80001/LayoutTests/fast/events...
> > LayoutTests/fast/events/dragevent-constructor.html:3: <head>
> > nit: you can ommit html, head and body tags:
> >
>
https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-...
> 
> Rick!!
> Could you give me "try bot permission"?

Since the CQ dry run worked, I think you have everything you're likely to need. 
Or is there something that's not working for you?

[Jimmy Jo, 2015-08-07 02:17:52]

Patchset #5 (id:140001) has been deleted

[Jimmy Jo, 2015-08-07 02:18:03]

Patchset #5 (id:160001) has been deleted

[Jimmy Jo, 2015-08-07 02:35:07]

Patchset #5 (id:180001) has been deleted

[Jimmy Jo, 2015-08-07 02:42:03]

I updated things you mentioned.

https://codereview.chromium.org/1232003009/diff/80001/Source/core/events/Drag...
File Source/core/events/DragEvent.cpp (right):

https://codereview.chromium.org/1232003009/diff/80001/Source/core/events/Drag...
Source/core/events/DragEvent.cpp:46: bool DragEvent::isMouseEvent() const
On 2015/08/06 14:22:31, Rick Byers wrote:
> Why do you want isMouseEvent to be true for DragEvents?  That's inconsistent
> with how WheelEvent behaves (which is also a subclass of MouseEvent).  I think
> we want to use the same pattern for all Event subclasses.  Can we update the
> necessary callers instead of removing this?

Done.

https://codereview.chromium.org/1232003009/diff/120001/LayoutTests/imported/w...
File LayoutTests/imported/web-platform-tests/html/editing/dnd/synthetic/001.html
(left):

https://codereview.chromium.org/1232003009/diff/120001/LayoutTests/imported/w...
LayoutTests/imported/web-platform-tests/html/editing/dnd/synthetic/001.html:14:
test(function() {
On 2015/08/06 14:22:32, Rick Byers wrote:
> These "imported/web-platform-tests" are special, see
> https://www.chromium.org/blink/importing-the-w3c-tests.  I don't think we want
> to make edits to them here.  The official home is here:
>
https://github.com/w3c/web-platform-tests/commits/master/html/editing/dnd/syn....
> 
> I think it's fine to just update our expectations.  Obviously we didn't
exactly
> expect this test to pass before, and it still doesn't pass completely.
> 
> Of course if you'd also like to contribute improvements to the test back to
the
> w3c that's great too.  But I wouldn't block this CL on it.

Done.

[Jimmy Jo, 2015-08-07 05:08:03]

The CQ bit was checked by jincheol.jo@navercorp.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-07 05:08:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232003009/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232003009/200001

[commit-bot: I haz the power, 2015-08-07 06:57:20]

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

[commit-bot: I haz the power, 2015-08-07 06:57:21]

Dry run: Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/65404)

[Rick Byers, 2015-08-07 18:23:23]

On 2015/08/07 06:57:21, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   mac_blink_rel on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/65404)

It looks like you need to update WebPluginContainerImpl::handleEvent to handle
drag events explicitly instead of treating them the same as mouse events.  It
looks like that will probably clean up the code there a little.

I wonder if there might be similar bugs elsewhere?  Maybe worth a quick scan of
other callers of isDragEvent to see if any expect isMouseEvent to be true too
(or would otherwise be better using DragEvent instead of MouseEvent)?

[Jimmy Jo, 2015-08-13 12:20:51]

The CQ bit was checked by jincheol.jo@navercorp.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-13 12:21:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232003009/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232003009/220001

[Jimmy Jo, 2015-08-13 12:23:38]

On 2015/08/07 18:23:23, Rick Byers wrote:
> On 2015/08/07 06:57:21, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   mac_blink_rel on tryserver.blink (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/65404)
> 
> It looks like you need to update WebPluginContainerImpl::handleEvent to handle
> drag events explicitly instead of treating them the same as mouse events.  It
> looks like that will probably clean up the code there a little.
> 
> I wonder if there might be similar bugs elsewhere?  Maybe worth a quick scan
of
> other callers of isDragEvent to see if any expect isMouseEvent to be true too
> (or would otherwise be better using DragEvent instead of MouseEvent)?

Thanks for your good tip.
I agree with you. I will check callers of isDragEvent and isMouseEvent.

[commit-bot: I haz the power, 2015-08-13 13:36:57]

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

[commit-bot: I haz the power, 2015-08-13 13:36:58]

Dry run: This issue passed the CQ dry run.

[Jimmy Jo, 2015-08-14 03:59:12]

On 2015/08/07 18:23:23, Rick Byers wrote:
> On 2015/08/07 06:57:21, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   mac_blink_rel on tryserver.blink (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/65404)
> 
> It looks like you need to update WebPluginContainerImpl::handleEvent to handle
> drag events explicitly instead of treating them the same as mouse events.  It
> looks like that will probably clean up the code there a little.
> 
> I wonder if there might be similar bugs elsewhere?  Maybe worth a quick scan
of
> other callers of isDragEvent to see if any expect isMouseEvent to be true too
> (or would otherwise be better using DragEvent instead of MouseEvent)?

I checked callers of isDragEvent and isMouseEvent elsewhere.
They seemed unlikely to be problematic.

[Jimmy Jo, 2015-08-17 13:01:23]

ping...

[Rick Byers, 2015-08-17 14:26:34]

On 2015/08/17 13:01:23, Jimmy Jo wrote:
> ping...

Sorry I missed your last update.  LGTM.

[Jimmy Jo, 2015-08-17 14:37:16]

The CQ bit was checked by jincheol.jo@navercorp.com

[commit-bot: I haz the power, 2015-08-17 14:37:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232003009/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232003009/220001

[commit-bot: I haz the power, 2015-08-17 16:16:43]

Committed patchset #6 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200649

[Jimmy Jo, 2015-08-18 00:34:20]

On 2015/08/17 14:26:34, Rick Byers wrote:
> On 2015/08/17 13:01:23, Jimmy Jo wrote:
> > ping...
> 
> Sorry I missed your last update.  LGTM.

No problem.
Thanks for your detailed guidance.