Closed Bug 1133492 Opened 5 years ago Closed 5 years ago

Extract some of nsPresShell into a separate TouchManager class

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: alessarik, Assigned: alessarik)

References

Details

Attachments

(2 files, 5 obsolete files)

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.
Blocks: 736048
OS: Windows 8 → All
Hardware: x86_64 → All
Attached patch allocate_touch_manager_ver1.diff (obsolete) — Splinter Review
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.
Attachment #8565050 - Flags: review?(bugs)
Attachment #8565050 - Flags: feedback?(jmathies)
Attachment #8565050 - Flags: feedback?(bugmail.mozilla)
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.
Attachment #8565050 - Flags: review?(bugs) → review-
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
Attachment #8565050 - Flags: feedback?(bugmail.mozilla) → feedback+
Component: Untriaged → DOM: Events
Product: Firefox → Core
Summary: Allocate TouchManager class → Extract some of nsPresShell into a separate TouchManager class
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8565050 - Flags: feedback?(jmathies) → feedback+
Attached patch allocate_touch_manager_ver2.diff (obsolete) — Splinter Review
+ 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.
Attachment #8565050 - Attachment is obsolete: true
Attachment #8565889 - Flags: review?(jst)
Attachment #8565889 - Flags: review?(bugs)
// 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.
Attachment #8565889 - Flags: review?(jst)
Attachment #8565889 - Flags: review?(bugs)
Attachment #8565889 - Flags: review-
Attached patch allocate_touch_manager_ver3.diff (obsolete) — Splinter Review
+ Added set null to resources (it resolves issues on try servers)
- Removed unwanted check and comment according with comments
Attachment #8565889 - Attachment is obsolete: true
Attachment #8566429 - Flags: review?(bugs)
Attachment #8566429 - Flags: feedback?(mbrubeck)
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.
Attachment #8566429 - Flags: review?(bugs)
Attachment #8566429 - Flags: review+
Attachment #8566429 - Flags: feedback?(mbrubeck)
+ 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.
Attachment #8566429 - Attachment is obsolete: true
Attachment #8566995 - Flags: review?(bugs)
Attachment #8566995 - Flags: review?(bugs) → review+
Attached patch touch_manager_stuff_ver1.diff (obsolete) — Splinter Review
+ Some additional stuff were moved from nsIPresShell into TouchManager
Attachment #8567132 - Flags: review?(bugs)
Attachment #8567132 - Flags: feedback?(mbrubeck)
Attachment #8567132 - Flags: feedback?(jmathies)
Comment on attachment 8567132 [details] [diff] [review]
touch_manager_stuff_ver1.diff

GetTouchById seems to be some unrelated change.
Attachment #8567132 - Flags: review?(bugs)
Attachment #8567132 - Flags: review-
Attachment #8567132 - Flags: feedback?(mbrubeck)
Attachment #8567132 - Flags: feedback?(jmathies)
Attached patch touch_manager_stuff_ver2.diff (obsolete) — Splinter Review
- Removed GetTouchById
Attachment #8567132 - Attachment is obsolete: true
Attachment #8568402 - Flags: review?(bugs)
Attachment #8568402 - Flags: feedback?(jmathies)
(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.
Attachment #8568402 - Flags: review?(bugs)
Attachment #8568402 - Flags: feedback?(jmathies)
+ Added #include for resolving compilation issues
Attachment #8568402 - Attachment is obsolete: true
Attachment #8568978 - Flags: review?(bugs)
Attachment #8568978 - Flags: feedback?(jmathies)
Attachment #8568978 - Flags: review?(bugs)
Attachment #8568978 - Flags: review+
Attachment #8568978 - Flags: feedback?(jmathies)
If nobody have objections, I put checkin flag.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e99a6c34aa50
https://hg.mozilla.org/mozilla-central/rev/bd17cfb379b2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.