775746 - Make nsDOMTouch inherit from SingleTouchData

Status: RESOLVED WONTFIX

(Core :: DOM: Events, defect)

Description

[Doug Sherk (:drs) (inactive)]

They are nearly identical and we will have duplicate code if we don't do this. For now, nsDOMTouch is used in too many places that I don't understand. nsDOMTouch can only be used on the main thread because it has nsRefPtr<nsIWidget>, whereas SingleTouchData only has the data values like screen point, force, etc.

Comment 1

[Maksim Lebedev]

Attachment: dom_touch_inherit_ver1.diff

Comment 2

[Maksim Lebedev]

If it is still needed, who should review this?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Does the patch make the code any simpler? I'm somewhat worried that if Touch becomes
SingleTouchData, and SingleTouchData is supposed to be thread-agnostic, we'll get crashes
if Touch objects (which are main-thread only) are passed  to other threads.

Comment 4

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] (no new review requests before March 8, please) from comment #3)
This changes made according with existing bug and comment http://mxr.mozilla.org/mozilla-central/source/widget/InputData.h#113

Comment 5

[Maksim Lebedev]

Could you please, give your opinion about this bug?

Comment 6

[Kartikaya Gupta (email:[e-mail])]

I would rather close this bug as WONTFIX. I think there is a fairly clear distinction between SingleTouchData (and in general the stuff living in widget/InputData.h) and the DOM versions. The widget/InputData stuff is to be used in the widget code and when interacting with APZ, but once it goes to the main thread for gecko dispatch it should be converted to the DOM version.

The specific thing that makes me against having the DOM version extend the InputData version is that we have added like "mLocalScreenPoint" to SingleTouchData for APZ use, and it doesn't really make sense to import that into the DOM version. Also the coordinate systems of InputData and DOM are different (at least when APZ is around) and so if we were to combine the two we would have problems with ambiguity in that respect - should a point be in Screen space or LayoutDevice space? (That's one of the reasons we added mLocalScreenPoint in the first place, but I don't want to extend that to LayoutDevice space as well).

I'm not super concerned about the threading aspect that smaug mentioned in comment 3 although it is a valid concern as well. Overall I just don't think there's much win here, at least not to justify the effort.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Closing for now, but feel free to argue your case if you feel otherwise.