User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 Build ID: 20141105223254 Expected results: I suggest to allocate special class related with TouchEvents working. It helps to keep code in one place.

I propose version of TouchManager class. I allocated some stuff for my needs. Let me know if another stuff can be allocated in this class too.

Comment on attachment 8565050 [details] [diff] [review] allocate_touch_manager_ver1.diff >+ * Microsoft OpenTech >+ * Akvelon (Yaroslavl) >+ * Maksim Lebedev aka Alessar This kind of stuff isn't usually added anymore. And it doesn't even tell much. Most of the code was written by someone else and just moved to this new file. So, please drop it. >+ PresShell* mPresShell; // [STRONG] just use nsRefPtr >+ nsIDocument* mDocument; // [STRONG] and nsCOMPtr for this and drop // [STRONG] Why you need TouchManager to be explicitly allocated? Would be safer if it was just a member variable TouchManager mTouchManager; and then add Init() and Destroy() methods to it.

Comment on attachment 8565050 [details] [diff] [review] allocate_touch_manager_ver1.diff Review of attachment 8565050 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good for the most part. In addition to what smaug said above I have a few more comments: ::: layout/base/TouchManager.cpp @@ +8,5 @@ > + * Akvelon (Yaroslavl) > + * Maksim Lebedev aka Alessar > + */ > + > +/* implementation of touch manager */ This comment is unnecessary @@ +14,5 @@ > +#include "TouchManager.h" > + > +TouchManager::TouchManager(PresShell* aPresShell, nsIDocument* aDocument) : > + mPresShell(aPresShell), > + mDocument(aDocument) The only place mPresShell is used in this class is to to assign mCurrentEventContent in one place. If you make that an out-param to the PreHandleEvent function then you can get rid of this reference to the presShell entirely. ::: layout/base/TouchManager.h @@ +8,5 @@ > + * Akvelon (Yaroslavl) > + * Maksim Lebedev aka Alessar > + */ > + > +/* description of touch manager */ Put in an actual description here @@ +30,5 @@ > + PresShell* mPresShell; // [STRONG] > + nsIDocument* mDocument; // [STRONG] > +}; > + > +#endif /* !defined(TouchManager_h_) */ fix comment

+ Added some codes according with comments - Removed definition "friend TouchManager" in PresShell - Removed unwanted comments (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4) > The only place mPresShell is used in this class is to to assign > mCurrentEventContent in one place. If you make that an out-param to the > PreHandleEvent function then you can get rid of this reference to the > presShell entirely. Reference to PresShell needs for me in future. But it helped me to remove friend definition.

// XXX How about IME events and input events for plugins? comment doesn't really make sense in TouchManager::PreHandleEvent

Comment on attachment 8565889 [details] [diff] [review] allocate_touch_manager_ver2.diff Quite some failures on tryserver.

+ Added set null to resources (it resolves issues on try servers) - Removed unwanted check and comment according with comments

Comment on attachment 8566429 [details] [diff] [review] allocate_touch_manager_ver3.diff Please add back 'if (aEvent->mFlags.mIsTrusted) {...}' which you removed in this version of the patch.

+ Added check (In reply to Olli Pettay [:smaug] from comment #11) > Please add back 'if (aEvent->mFlags.mIsTrusted) {...}' which you removed in > this version of the patch. I assume that it will more better if this check will stay in PresShell. I moved PreHandleEvent calling into that check.

+ Some additional stuff were moved from nsIPresShell into TouchManager

Comment on attachment 8567132 [details] [diff] [review] touch_manager_stuff_ver1.diff GetTouchById seems to be some unrelated change.

- Removed GetTouchById

(In reply to Olli Pettay [:smaug] from comment #15) > GetTouchById seems to be some unrelated change. By the way, there is GetTouchForIdentifier function in .\dom\ipc\TabChild.cpp This function is very similar but has translation from const into non-const object.

Very strange for me, that I see all good builds with OPTIMIZE mode and all failed builds with DEBUG mode. Should I resolve this issue in my patch or not?

yes. always test your patches with debug builds.

Are you possibly missing an #include in LayoutUtils?

Comment on attachment 8568402 [details] [diff] [review] touch_manager_stuff_ver2.diff Better to review a patch which compiles everywhere.

+ Added #include for resolving compilation issues

If nobody have objections, I put checkin flag.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e99a6c34aa50 https://hg.mozilla.org/integration/mozilla-inbound/rev/bd17cfb379b2

https://hg.mozilla.org/mozilla-central/rev/e99a6c34aa50 https://hg.mozilla.org/mozilla-central/rev/bd17cfb379b2