1137555 - [e10s] Navigator.maxTouchPoints doesn't work on e10s with dom.w3c_pointerevents.enabled = true and dom.w3c_touch_events.enabled=1

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Makoto Kato [:m_kato]]

Since we don't turn on dom.w3c_touch_events.enabled and dom.w3c_pointer_events.enabled, this isn't important now.

Navigator.maxTouchPoints uses widget->GetMaxTouchPoints on content process, so we should use it on parent process via IPC.

Comment 1

[Maksim Lebedev]

Could You provide any test for this?
And more information about specific preferencies, which should be on/off?

Comment 2

[Makoto Kato [:m_kato]]

(In reply to Maksim Lebedev from comment #1)
> Could You provide any test for this?

This issue depends on hardware and we cannot create mock of nsIWidget for test.

> And more information about specific preferencies, which should be on/off?

As default, dom.w3c_pointer_events.enabled is false.  Now, Google already ships this property on stable version, maybe, we should add new preferences for this like touch-action.

Also, if this property is implemented on gonk/B2G, e10s support is also added.

Comment 3

[Maksim Lebedev]

Some investigation:
[e10s] uses PuppetWidget class.
Windows-specific code is situated in nsWindow class.
Solution:
Make decision in accordance with platform, not in accordance with widgets.

Comment 4

[Maksim Lebedev]

Attachment: widget_utils_ver1.diff

+ Add base PlatformUtility and WindowsUtility
+ BaseWidget::getMaxTouchPoints uses Platform specific code

Feadback and comments and suggestions and objections are very welcome.

Comment 5

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

Comment on attachment 8597373 [details] [diff] [review]
widget_utils_ver1.diff

Review of attachment 8597373 [details] [diff] [review]:
-----------------------------------------------------------------

If I'm understanding the problem I'd rather see PuppetWidget implement this function and delegate to the TabChild/TabParent to get the info from the parent process widget, and then cache the result.

Comment 6

[Makoto Kato [:m_kato]]

Comment on attachment 8597373 [details] [diff] [review]
widget_utils_ver1.diff

I think content process should not get hardware data directly by API.  PuppetWidget or dom's navigator impl should use IPC to get it.

Also, when this is implemented for B2G, we will use ioctl to get whether single-touch or multi-touch.

Comment 7

[Maksim Lebedev]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> If I'm understanding the problem I'd rather see PuppetWidget implement this
> function and delegate to the TabChild/TabParent to get the info from the
> parent process widget, and then cache the result.
In this case I see sequence something like that:
content->widget->TabParent->...->getHardwareProperty.
Why we cannot simplify sequence to something like that:
content->getHardwareProperty.

Comment 8

[Maksim Lebedev]

Attachment: widget_utils_ver2.diff

+ Windows specifity function IsTouchDeviceSupportPresent() -> unplatformed WidgetUtils::IsTouchDeviceSupportPresent()
+ WinUtils have become inherit of WidgetUtils

Ideas of approach are to suppress some of "#ifdef XP_WIN" code and provide support more structured code (platform independent).

Feadback and comments and suggestions and objections are very welcome.

Comment 9

[Makoto Kato [:m_kato]]

(In reply to Maksim Lebedev from comment #7)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> > If I'm understanding the problem I'd rather see PuppetWidget implement this
> > function and delegate to the TabChild/TabParent to get the info from the
> > parent process widget, and then cache the result.
> In this case I see sequence something like that:
> content->widget->TabParent->...->getHardwareProperty.
> Why we cannot simplify sequence to something like that:
> content->getHardwareProperty.

We have a sandbox plan (See https://wiki.mozilla.org/Security/Sandbox) for content process etc.  So we want to reduce calling OS API directly on content process.  Also, to implement getting max touch points for B2G/gonk, I think that we need call ioctl() to use MT system call.  So ioctl() shouldn't be call on content process for security reason.

Comment 10

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> Comment on attachment 8597373 [details] [diff] [review]
> widget_utils_ver1.diff
> 
> Review of attachment 8597373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If I'm understanding the problem I'd rather see PuppetWidget implement this
> function and delegate to the TabChild/TabParent to get the info from the
> parent process widget, and then cache the result.

Yep, agree here. See PuppetWidget's GetNativeData for an example.

Comment 11

[Jim Mathies [:jimm]]

We should look at:

1) retrieving values from chrome via ContentChild or TabChild
2) if possible, figure out how we can cache these values on the child side and notify from the parent when they change.

item 2 is want vs. must have. Lets start with item # 1.

Comment 12

[Maksim Lebedev]

Attachment: widget_utils_ver3.diff

- Return to use current widget in Navigator::GetMaxTouchPoints()
- Return simple BaseWidget::GetMaxTouchPoints()
+ Add GetMaxTouchPoints into PBrowser.ipdl protocol
+ Add Send/RecvGetMaxTouchPoints into TabChild/TabParent
+ PuppetWidget::GetMaxTouchPoints use TabChild/TabParent behavior
+ nsWindow::GetMaxTouchPoints use platform independent WidgetUtils

Feadback and comments and suggestions and objections are very welcome.

Comment 13

[Makoto Kato [:m_kato]]

Comment on attachment 8599418 [details] [diff] [review]
widget_utils_ver3.diff

Review of attachment 8599418 [details] [diff] [review]:
-----------------------------------------------------------------

I think we need some changes and you should explain why new platform utility class is required in widget.  nsIWidget is already abstract class per platform.  If this platform utility class is under HAL, I make sense.

::: dom/events/TouchEvent.cpp
@@ +196,1 @@
>        }

Although this becomes another bug, maybe, we shouldn't use IsTouchDeviceSupportPresent on child process.  (This method will be called on child process too).

Could you change this for IPC support too?

::: widget/windows/nsWindow.cpp
@@ +3540,5 @@
>  
>  uint32_t
>  nsWindow::GetMaxTouchPoints() const
>  {
> +  return WidgetUtils::GetMaxTouchPoints();

I think you should modify nsBaseWidget::GetMaxTouchPoints() instead of this platform widget if you use adding PlatformUtility class.

But I don't think this change is required to add new PlatformUtility class.  Because widget is per platform code.  So PlatformUtility is unnecessary.

Comment 14

[Makoto Kato [:m_kato]]

first review comment is for sIsTouchDeviceSupportPresent = WidgetUtils::IsTouchDeviceSupportPresent();

Comment 15

[Maksim Lebedev]

(In reply to Makoto Kato (:m_kato) from comment #13)
> I think we need some changes and you should explain why new platform utility
> class is required in widget.  nsIWidget is already abstract class per
> platform.  If this platform utility class is under HAL, I make sense.
As I can see in some cases on one platform we can use several different widgets
(For instance: nsWindow, MetroWidget and PuppetWidget under Windows platform)
But I think that some of properties is related with devices, but not with widgets
(For instance: maxTouchPoints or touchDeviceSupportPresent).
In some places like dom/events/TouchEvents.cpp we can see very specific code,
which use preprocessors definitions #ifdef XP_WIN which looks very strange and unwanted.
We can suppress such definitions in most part of existing code,
that's why I tried to describe platform specific code in PlatformUtility class.
Looks like, WidgetUtils and WinUtils are already existing helper classes,
that's why we can use their as starting point to implement new PlatformUtility class.
Looks like, there are nsCocoaUtils and HwcUtils also on another plarforms.

Comment 16

[Maksim Lebedev]

(In reply to Makoto Kato (:m_kato) from comment #13)
> I think you should modify nsBaseWidget::GetMaxTouchPoints() instead of this
> platform widget if you use adding PlatformUtility class.
> But I don't think this change is required to add new PlatformUtility class. 
> Because widget is per platform code.  So PlatformUtility is unnecessary.
Only PlatformUtility class can give me uniform interface to functionality.

Comment 17

[Maksim Lebedev]

(In reply to Makoto Kato (:m_kato) from comment #13)
> ::: dom/events/TouchEvent.cpp
> sIsTouchDeviceSupportPresent = WidgetUtils::IsTouchDeviceSupportPresent();
> Although this becomes another bug, maybe, we shouldn't use
> IsTouchDeviceSupportPresent on child process.  (This method will be called
> on child process too).
> Could you change this for IPC support too?
IMHO that code exists only for experimental using and I would like to remove them,
but if anybody thinks that it is needed and should be converted in safety scheme,
I would prefere make such changes in another bug
(to suppress increasing a lot of unrelated changes in current bug).

Comment 18

[Maksim Lebedev]

Attachment: widget_utils_ver4.diff

- Remove nsWindow::GetMaxTouchPoints()
+ nsBaseWidget::GetMaxTouchPoints() use WidgetUtils::GetMaxTouchPoints()

Feadback and comments and suggestions and objections are very welcome.

Comment 19

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

Comment on attachment 8599877 [details] [diff] [review]
widget_utils_ver4.diff

I think we really must cache the maxTouchPoints value on child side.
Otherwise any code accessing navigator.maxTouchPoints may need to wait for
a long time if parent process is doing something heavy.
Also, implementing caching should be rather easy.

So, perhaps store the cached value in int32_t, and -1 means cache isn't valid.
And set the value to -1 initially and probably also in RecvUpdateDimensions
(that should get called when window is moved to another screen)

Comment 20

[Jim Mathies [:jimm]]

Comment on attachment 8599877 [details] [diff] [review]
widget_utils_ver4.diff

Review of attachment 8599877 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like everything that access the

::: widget/WidgetUtils.h
@@ +39,5 @@
>                       ScreenRotation aRotation);
>  
>  namespace widget {
>  
> +class PlatformUtility

What is the purpose of this class? I don't understand the registration stuff your doing here. It looks like every access to the new apis is static too? Can you give me a basic overview here of what your trying to do and why?

Comment 21

[Jim Mathies [:jimm]]

(In reply to Maksim Lebedev from comment #15)
> (In reply to Makoto Kato (:m_kato) from comment #13)
> > I think we need some changes and you should explain why new platform utility
> > class is required in widget.  nsIWidget is already abstract class per
> > platform.  If this platform utility class is under HAL, I make sense.
> As I can see in some cases on one platform we can use several different
> widgets
> (For instance: nsWindow, MetroWidget and PuppetWidget under Windows platform)
> But I think that some of properties is related with devices, but not with
> widgets
> (For instance: maxTouchPoints or touchDeviceSupportPresent).
> In some places like dom/events/TouchEvents.cpp we can see very specific code,
> which use preprocessors definitions #ifdef XP_WIN which looks very strange
> and unwanted.
> We can suppress such definitions in most part of existing code,
> that's why I tried to describe platform specific code in PlatformUtility
> class.
> Looks like, WidgetUtils and WinUtils are already existing helper classes,
> that's why we can use their as starting point to implement new
> PlatformUtility class.
> Looks like, there are nsCocoaUtils and HwcUtils also on another plarforms.

Sorry, missed this comment. So we don't have all the consumers of PlatformUtility here yet. Let me look at this some more..

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 8599877 [details] [diff] [review]
widget_utils_ver4.diff

Review of attachment 8599877 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/WidgetUtils.cpp
@@ +8,5 @@
>  #include "mozilla/WidgetUtils.h"
>  
>  namespace mozilla {
>  
> +PlatformUtility* WidgetUtils::sPlatformUtility = nullptr;

remove

@@ +91,5 @@
>        return aRect;
>    }
>  }
> +
> +PlatformUtility::PlatformUtility()

remove

@@ +96,5 @@
> +{
> +  WidgetUtils::RegisterPlatformUtility(this);
> +}
> +
> +PlatformUtility::~PlatformUtility()

remove

::: widget/WidgetUtils.h
@@ +39,5 @@
>                       ScreenRotation aRotation);
>  
>  namespace widget {
>  
> +class PlatformUtility

please remove.

@@ +95,5 @@
>                                           uint32_t* aShiftedCharCode);
> +
> +  /**************************************************************
> +  *
> +  * SECTION: PlatformUtility interface

these can just be static methods on WinUtils. I don't see any need for PlatformUtility.

@@ +119,5 @@
> +  static void RegisterPlatformUtility(PlatformUtility* aUtility);
> +
> +private:
> +
> +  static PlatformUtility* sPlatformUtility;

I see no use for this, please remove it.

::: widget/windows/WinUtils.cpp
@@ +406,5 @@
>  
>  const char FaviconHelper::kJumpListCacheDir[] = "jumpListCache";
>  const char FaviconHelper::kShortcutCacheDir[] = "shortcutCache";
>  
> +WinUtils gWinUtils;

This isn't in use anywhere in your work here afaict, please remove it.

Comment 23

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 8599877 [details] [diff] [review]
widget_utils_ver4.diff

Deferring to :jimm

Comment 24

[Maksim Lebedev]

Attachment: widget_utils_ver5.diff

- Unfortunately, removed PlatformUtility.
+ Added cashing of maxTouchPoints at PuppetWidget.

Feadback and comments and suggestions and objections are very welcome.

Comment 25

[Makoto Kato [:m_kato]]

Comment on attachment 8601461 [details] [diff] [review]
widget_utils_ver5.diff

Review of attachment 8601461 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/TouchEvent.cpp
@@ +195,5 @@
>        static bool sIsTouchDeviceSupportPresent = false;
>        // On Windows we auto-detect based on device support.
>        if (!sDidCheckTouchDeviceSupport) {
>          sDidCheckTouchDeviceSupport = true;
> +        sIsTouchDeviceSupportPresent = WinUtils::IsTouchDeviceSupportPresent();

By bug 978679, roc will implement IsTouchDeviceSupportPresent code for GTK3.

By your changes, this become for Windows only code, so I don't think that this modification of nsTouchEvent.cpp (and nsLookAndFeel.cpp) is necessary.  

If review agrees this modification, please ignore my comment.

Comment 26

[Maksim Lebedev]

(In reply to Makoto Kato (:m_kato) from comment #25)
> > +        sIsTouchDeviceSupportPresent = WinUtils::IsTouchDeviceSupportPresent();
> By your changes, this become for Windows only code, so I don't think that
> this modification of nsTouchEvent.cpp (and nsLookAndFeel.cpp) is necessary.  
All that code is situated at block #ifdef XP_WIN  - #endif,
so that code is compiled only at windows.
> By bug 978679, roc will implement IsTouchDeviceSupportPresent code for GTK3.
If any platforms will be added into that approach of touch support autodetection,
I think we can use better approach to get that property from Widget without #ifdef blocks.
That's why at the first patches, PlatformUtility class was existing and could support all platforms.

Comment 27

[Jim Mathies [:jimm]]

Comment on attachment 8601461 [details] [diff] [review]
widget_utils_ver5.diff

Review of attachment 8601461 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/TouchEvent.cpp
@@ +9,5 @@
>  #include "mozilla/dom/TouchListBinding.h"
>  #include "mozilla/Preferences.h"
>  #include "mozilla/TouchEvents.h"
>  #include "nsContentUtils.h"
> +#include "mozilla/WidgetUtils.h"

nit - don't think you need this extra include, please confirm.

@@ +195,5 @@
>        static bool sIsTouchDeviceSupportPresent = false;
>        // On Windows we auto-detect based on device support.
>        if (!sDidCheckTouchDeviceSupport) {
>          sDidCheckTouchDeviceSupport = true;
> +        sIsTouchDeviceSupportPresent = WinUtils::IsTouchDeviceSupportPresent();

If we need this to be cross platform we can add it to widget utils right?

::: widget/PuppetWidget.cpp
@@ +1139,5 @@
>  
> +uint32_t PuppetWidget::GetMaxTouchPoints() const
> +{
> +  static uint32_t touchPoints = 0;
> +  static bool IsInitialized = false;

nit - sIsInitialized is typical for mozilla code

::: widget/windows/WinUtils.h
@@ +34,5 @@
>  #include "nsIWidget.h"
>  #include "nsIThread.h"
>  
>  #include "mozilla/Attributes.h"
> +#include "mozilla/WidgetUtils.h"

nit - don't think you need this extra include, please confirm.

@@ +124,5 @@
>  };
>  #endif
>  
> +class WinUtils
> +{

nit - whitespace change

Comment 28

[Maksim Lebedev]

(In reply to Jim Mathies [:jimm] from comment #27)
> > +        sIsTouchDeviceSupportPresent = WinUtils::IsTouchDeviceSupportPresent();
> If we need this to be cross platform we can add it to widget utils right?
Sounds good to me. In some of previous versions of patches it was made.
PlatformUtility helps to determine platforms, but that class was removed recently.
How we can resolve compilation issues on different platforms in this case?

Comment 29

[Jim Mathies [:jimm]]

(In reply to Maksim Lebedev from comment #28)
> (In reply to Jim Mathies [:jimm] from comment #27)
> > > +        sIsTouchDeviceSupportPresent = WinUtils::IsTouchDeviceSupportPresent();
> > If we need this to be cross platform we can add it to widget utils right?
> Sounds good to me. In some of previous versions of patches it was made.
> PlatformUtility helps to determine platforms, but that class was removed
> recently.
> How we can resolve compilation issues on different platforms in this case?

ifdef'd widget utils function with native widget calls I would guess.

Comment 30

[Maksim Lebedev]

(In reply to Jim Mathies [:jimm] from comment #27)
> ::: dom/events/TouchEvent.cpp
> >  #include "nsContentUtils.h"
> > +#include "mozilla/WidgetUtils.h"
> nit - don't think you need this extra include, please confirm.
My confirmation.

Comment 31

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

Comment on attachment 8601461 [details] [diff] [review]
widget_utils_ver5.diff

>+uint32_t PuppetWidget::GetMaxTouchPoints() const
>+{
>+  static uint32_t touchPoints = 0;
>+  static bool IsInitialized = false;
Static variables should have s-prefix.



>+  if (IsInitialized) {
>+    return touchPoints;
>+  }
>+  if (mTabChild) {
>+    mTabChild->GetMaxTouchPoints(&touchPoints);
>+    IsInitialized = true;
>+  }
>+  return touchPoints;
>+}
So we don't ever update this data. Perhaps that is fine for now.

Comment 32

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #31)
> So we don't ever update this data. Perhaps that is fine for now.
Very hard to me to suggest such case when updating will be needed,
but if it happens, it could be made in another bug later.

Comment 33

[Maksim Lebedev]

Attachment: widget_utils_ver6.diff

+ Returned universal interface WidgetUtils::IsTouchDeviceSupportPresent()
(Implementation through unwanted block #ifdef)

Comment 34

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

(In reply to Maksim Lebedev from comment #32)
> (In reply to Olli Pettay [:smaug] from comment #31)
> > So we don't ever update this data.
You plug in a touch enabled screen and move browser window there?

Comment 35

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

Comment on attachment 8602824 [details] [diff] [review]
widget_utils_ver6.diff

(and can one even enable/disable touchscreen support on Windows dynamically?)

static uint32_t touchPoints = 0;
is also static. So, sTouchPoints = 0;
no need to re-ask review for that change.

Comment 36

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #35)
> (and can one even enable/disable touchscreen support on Windows dynamically?)
I doubt that Windows informs apps about changes related with touchscreens and touchpoints.
Maybe in this case better solution is removing caching?

Comment 37

[Maksim Lebedev]

Attachment: widget_utils_ver7.diff

+ touchPoints -> sTouchPoints

Comment 38

[Maksim Lebedev]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=daad1bf5e40f

Comment 39

[Maksim Lebedev]

If there are no objections, I put checkin-needed flag.

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d85ee61074c4

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d85ee61074c4

Comment 42

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d85ee61074c4