Closed Bug 1016232 Opened 10 years ago Closed 9 years ago

Firefox has no detect when pen over and leave hover of digitizer

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

x86_64
Windows 8
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: alessarik, Assigned: alessarik)

References

Details

Attachments

(1 file, 8 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

Use pen digitizer.
Put pen over FireFox page (but don't touch surface of digitizer. Only hover)
After move up pen.


Actual results:

FireFox did not detect when pen came and pen leave hover of digitizer. Only move events.


Expected results:

FireFox should detect over event and leave event.

According specification http://technet.microsoft.com/en-us/exchange/ms695579%28v=vs.90%29.aspx
There is InkSystemGesture, which contains ISG_HoverEnter and ISG_HoverLeave items. Maybe FireFox should use it.
typedef enum InkSystemGesture { 
...
  ISG_HoverEnter   = 0x17, 
  ISG_HoverLeave   = 0x18, 
...
} InkSystemGesture;
Component: Untriaged → Panning and Zooming
Product: Firefox → Core
Component: Panning and Zooming → Event Handling
Blocks: 960316
Attached patch pen_out_detection_ver1.diff (obsolete) — Splinter Review
The main idea is using COM object InkCollector from InkObj.dll
This class have CursorOutOfRange event, which we can use.

Feadback and comments and suggestions and objections are very welcome.
Attachment #8586166 - Flags: feedback?(mbrubeck)
Attachment #8586166 - Flags: feedback?(jmathies)
Attachment #8586166 - Flags: feedback?(bugs)
Attachment #8586166 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8586166 [details] [diff] [review]
pen_out_detection_ver1.diff

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

I don't have much of an opinion on this, I'll let the windows widget peers comment.
Attachment #8586166 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8586166 [details] [diff] [review]
pen_out_detection_ver1.diff

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

::: widget/windows/InkCollector.cpp
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "InkCollector.h"
> +#include "nsDebug.h"
> +#include <msinkaut_i.c>

what's this?

@@ +21,5 @@
> +}
> +
> +void InkCollector::CleanUp()
> +{
> +  if (nullptr != mConnectionPoint)

nit - !mConnectionPoint

rinse, wash, and repeat with the rest of this patch.

@@ +22,5 @@
> +
> +void InkCollector::CleanUp()
> +{
> +  if (nullptr != mConnectionPoint)
> +  {

nit - we put the brace on the end of the first line for conditional blocks.

@@ +92,5 @@
> +}
> +
> +void InkCollector::Enable(VARIANT_BOOL newState)
> +{
> +  mInkCollector->put_Enabled(newState);

Do we need to be safe here and check for a valid mInkCollector? If unlikely lets add debug asserts just to be safe.

@@ +115,5 @@
> +
> +    // Note: we do not AddRef here because the lifetime
> +    //  of this object does not depend on reference counting
> +    //  but on the duration of the connection set up by
> +    //  the user of this class.

Can you explain this a bit? I'm confused why we are not ref counting this object.

@@ +148,5 @@
> +
> +void InkCollector::CursorOutOfRange(IInkCursor* Cursor)
> +{
> +  HWND targetWindow;
> +  mInkCollector->get_hWnd((LONG_PTR*)&targetWindow);

check for success here, if we fail, we should assert in some way and not call PostMessage.

::: widget/windows/InkCollector.h
@@ +7,5 @@
> +#define InkCollector_h__
> +
> +#include <msinkaut.h>
> +
> +#define WM_CURSOR_OUTOFRANGE  WM_USER + 0x83

MOZ_WM_CURSOR_OUTOFRANGE or similar so we know this is custom.

::: widget/windows/nsWindow.cpp
@@ +390,5 @@
>      if (SUCCEEDED(::OleInitialize(nullptr))) {
>        sIsOleInitialized = TRUE;
>      }
>      NS_ASSERTION(sIsOleInitialized, "***** OLE is not initialized!\n");
> +    if (SUCCEEDED(::CoInitialize(nullptr))) {

this should already be initialized. I would suggest adding this to the class constructor and destructor, these calls are mostly no-ops after the first call.

@@ +6465,5 @@
>    // Prevent the widget from sending additional events.
>    mWidgetListener = nullptr;
>    mAttachedWidgetListener = nullptr;
> +  delete mInkCollector;
> +  mInkCollector = nullptr;

ick - can you store this in a nsRefPtr or nsAutoPtr instead so destruction is handled automatically?
Attachment #8586166 - Flags: feedback?(jmathies) → feedback+
Attachment #8586166 - Flags: feedback?(mbrubeck)
Attached patch pen_out_detection_ver2.diff (obsolete) — Splinter Review
+ CoInitialize and CoUninitialize moved into c-tor and d-tor of InkCollector.
+ nsRefPtr were added.
+ Added check for externals graphic tablets.
+ InkCollector became static variable.
+ Added postponed initialization in case if user have device with PEN input.

Feadback and comments and suggestions and objections are very welcome.
Attachment #8586166 - Attachment is obsolete: true
Attachment #8586166 - Flags: feedback?(bugs)
Attachment #8586848 - Flags: feedback?(mbrubeck)
Attachment #8586848 - Flags: feedback?(jmathies)
Comment on attachment 8586848 [details] [diff] [review]
pen_out_detection_ver2.diff

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

::: widget/windows/InkCollector.cpp
@@ +14,5 @@
> +#include <msinkaut_i.c>
> +
> +InkCollector::InkCollector()
> +{
> +  HRESULT mComInitialized = ::CoInitialize(nullptr);

is this a member variable or a local variable? I think you wanted it to be a member var.. :) Also lets make this a bool.

@@ +88,5 @@
> +  mMarshaller = nullptr;
> +  Enable(false);
> +  mInkCollector = nullptr;
> +
> +  CoUninitialize();

You have two of these calls for some reason, one in the destructor, and one here?

::: widget/windows/InkCollector.h
@@ +46,5 @@
> +
> +  void CleanUp();
> +  bool IsTabletExternal(IInkTablet* tablet) const;
> +  bool IsEnabled() const { return mEnabled; }
> +  

nit - whitespace

::: widget/windows/nsWindow.cpp
@@ +5287,5 @@
>  
> +  case MOZ_WM_CURSOR_OUTOFRANGE:
> +    {
> +      LPARAM pos = lParamToClient(::GetMessagePos());
> +      DispatchMouseEvent(NS_MOUSE_EXIT, wParam, pos, false, WidgetMouseEvent::eLeftButton, nsIDOMMouseEvent::MOZ_SOURCE_PEN);

nit - wrap me


AFAICT this is the only gecko event this code triggers. Is this event all we wanted from all this code? If so, I'm wondering if we can find a simpler way to detect mouse exits.
Attachment #8586848 - Flags: feedback?(jmathies) → feedback+
Attached patch pen_out_detection_ver3.diff (obsolete) — Splinter Review
+ Source code moved from CleanUp() into ~InkCollector()
- Removed InkCollector::CleanUp()
+ Changed InkCollector::mEnabled
+ Changed InkCollector::Enable()
+ Changed InkCollector::IsEnabled()

Feadback and comments and suggestions and objections are very welcome.
Attachment #8586848 - Attachment is obsolete: true
Attachment #8586848 - Flags: feedback?(mbrubeck)
Attachment #8587475 - Flags: feedback?(mbrubeck)
Attachment #8587475 - Flags: feedback?(jmathies)
Attachment #8587475 - Flags: feedback?(bugs)
(In reply to Jim Mathies [:jimm] from comment #3)
> > +    // Note: we do not AddRef here because the lifetime
> > +    //  of this object does not depend on reference counting
> > +    //  but on the duration of the connection set up by
> > +    //  the user of this class.
> Can you explain this a bit? I'm confused why we are not ref counting this object.
This comment was got from another project.
(In reply to Jim Mathies [:jimm] from comment #5)
> > +
> > +  void CleanUp();
> > +  bool IsTabletExternal(IInkTablet* tablet) const;
> > +  bool IsEnabled() const { return mEnabled; }
> > +  
> nit - whitespace
Sorry. Could you please explain it more clearer?
(In reply to Jim Mathies [:jimm] from comment #5)
> AFAICT this is the only gecko event this code triggers. Is this event all we wanted from
> all this code? If so, I'm wondering if we can find a simpler way to detect mouse exits.
This code helps to detect leaving of PEN device (Graphics tablets) - not mouse.
Unfortunately, we could not find other ways to detect this pen leave event.
(In reply to Maksim Lebedev from comment #9)
> (In reply to Jim Mathies [:jimm] from comment #5)
> > AFAICT this is the only gecko event this code triggers. Is this event all we wanted from
> > all this code? If so, I'm wondering if we can find a simpler way to detect mouse exits.
> This code helps to detect leaving of PEN device (Graphics tablets) - not
> mouse.
> Unfortunately, we could not find other ways to detect this pen leave event.

Ok, so what do we need this event for? For example is there some sort of input bug I can reproduce without this patch applied?
(In reply to Maksim Lebedev from comment #8)
> (In reply to Jim Mathies [:jimm] from comment #5)
> > > +
> > > +  void CleanUp();
> > > +  bool IsTabletExternal(IInkTablet* tablet) const;
> > > +  bool IsEnabled() const { return mEnabled; }
> > > +  
> > nit - whitespace
> Sorry. Could you please explain it more clearer?

If you look in Splinter Review, You'll see some bright red whitespace right around here.
Comment on attachment 8587475 [details] [diff] [review]
pen_out_detection_ver3.diff

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

nits mostly, need to take this for a spin locally as well. Will report back, I want to understand that ref counting issue.

::: widget/windows/InkCollector.cpp
@@ +10,5 @@
> +
> +// According to requirements section of
> +// https://msdn.microsoft.com/en-us/library/windows/desktop/ms695519(v=vs.85).aspx
> +// "Msinkaut.h(also requires Msinkaut_i.c)" should both be included
> +#include <msinkaut_i.c>

Thanks for the added comment. Looks like we're ok here these headers have been around since XP Tablet Edition.

@@ +63,5 @@
> +  mInkCollector->put_MousePointer(InkMousePointer::IMP_Custom);
> +
> +  mInkCollector->SetAllTabletsMode(VARIANT_FALSE);
> +  mInkCollector->SetEventInterest(InkCollectorEventInterest::ICEI_AllEvents, VARIANT_FALSE);
> +  // On-screen marks get drawn if set to true

Is this useful? I wonder if we should expose something like this via a pref.

@@ +91,5 @@
> +    mResponds.push_back(aHwnd);
> +  }
> +}
> +
> +bool InkCollector::IsTabletExternal(IInkTablet* tablet) const

nit - aTablet

@@ +158,5 @@
> +HRESULT InkCollector::Invoke(DISPID dispidMember, REFIID riid, LCID lcid, WORD /*wFlags*/, DISPPARAMS* pdispparams, VARIANT* pvarResult,  EXCEPINFO* /*pexcepinfo*/,   UINT* /*puArgErr*/)
> +{
> +  switch (dispidMember) {
> +  case DISPID_ICECursorOutOfRange:
> +    CursorOutOfRange((IInkCursor*)pdispparams->rgvarg[0].pdispVal);

nit - please use c++ casting here if you can.

@@ +165,5 @@
> +
> +  return S_OK;
> +}
> +
> +void InkCollector::CursorOutOfRange(IInkCursor* Cursor)

nit - aCursor

::: widget/windows/InkCollector.h
@@ +17,5 @@
> +
> +class InkCollector : public _IInkCollectorEvents
> +{
> +  typedef HWND                Respond;
> +  typedef std::list<Respond>  RespondList;

can we convert this to nsArray or similar? we generally don't use std::list except in 3rd party libs.

::: widget/windows/nsWindow.cpp
@@ +5284,5 @@
>  
> +  case MOZ_WM_CURSOR_OUTOFRANGE:
> +    {
> +      LPARAM pos = lParamToClient(::GetMessagePos());
> +      DispatchMouseEvent(NS_MOUSE_EXIT, wParam, pos, false, WidgetMouseEvent::eLeftButton, nsIDOMMouseEvent::MOZ_SOURCE_PEN);

nit - wrap line
Attachment #8587475 - Flags: feedback?(mbrubeck)
(In reply to Jim Mathies [:jimm] from comment #10)
> Ok, so what do we need this event for? For example is there some sort of
> input bug I can reproduce without this patch applied?
Yes there is. Could You please see at:
http://w3c-test.org/pointerevents/pointerevent_pointerleave_pen-manual.html
Attached patch pen_out_detection_ver4.diff (obsolete) — Splinter Review
+ Moved sInkCollector from nsWindow into own class.
+ Added check for NS_MOUSE_BUTTON_DOWN to turn on InkCollector
+ Added NS_ADDREF. (But we have redefined AddRef for our class(?)).
+ Added using nsTArray.
+ Changes a lot of variable names.
+ Change mEnable from VARIANT_BOOL into bool.
+ MOZ_WM_CURSOR_OUTOFRANGE -> MOZ_WM_PEN_LEAVES_HOVER_OF_DIGITIZER.

Feadback and comments and suggestions and objections are very welcome.
Attachment #8587475 - Attachment is obsolete: true
Attachment #8587475 - Flags: feedback?(jmathies)
Attachment #8587475 - Flags: feedback?(bugs)
Attachment #8588048 - Flags: feedback?(jmathies)
Attachment #8588048 - Flags: feedback?(bugs)
Comment on attachment 8588048 [details] [diff] [review]
pen_out_detection_ver4.diff

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

::: widget/windows/InkCollector.cpp
@@ +36,5 @@
> +    CoUninitialize();
> +  }
> +}
> +
> +void InkCollector::Init(HWND aTargetWindow)

What is the significance of the HWND we use here? Looks like this HWND value is whatever window fires the first nsWindow::DispatchMouseEvent. Is that true? Is this expected?

@@ +89,5 @@
> +
> +void InkCollector::AddListener(HWND aHwnd)
> +{
> +  if (aHwnd) {
> +    if (!mListeners.Contains(aHwnd)) {

nit - if (aHwnd && !mListeners.Contains(aHwnd)) {

::: widget/windows/InkCollector.h
@@ +29,5 @@
> +
> +protected:
> +  // IUnknown
> +  virtual ULONG STDMETHODCALLTYPE AddRef() { return 1; }
> +  virtual ULONG STDMETHODCALLTYPE Release() { return 1; }

I'm still confused as to why we don't have normal ref counting going here?

::: widget/windows/nsWindow.cpp
@@ +652,5 @@
>         !nsUXThemeData::sTitlebarInfoPopulatedAero)) {
>      nsUXThemeData::UpdateTitlebarInfo(mWnd);
>    }
> +
> +  InkCollector::sInkCollector.AddListener(mWnd);

AFAICT you never remove these from the mListener list, which means we'll just keep accumulating hwnds in here, and firing events to them, even after they are destroyed.
Attachment #8588048 - Flags: feedback?(jmathies) → feedback-
Attached patch pen_out_detection_ver5.diff (obsolete) — Splinter Review
+ Changed working logic of InkCollector class
+ Allocated InkCollector::Initialize()
+ Allocated InkCollector::OnInit()
+ Added InkCollector::RemoveListener()
+ Added check for eWindowType_toplevel before AddListener
+ Changed implementation of AddRef and Release

Feadback and comments and suggestions and objections are very welcome.
Attachment #8588048 - Attachment is obsolete: true
Attachment #8588048 - Flags: feedback?(bugs)
Attachment #8589156 - Flags: feedback?(jmathies)
Attachment #8589156 - Flags: feedback?(bugs)
(In reply to Jim Mathies [:jimm] from comment #15)
> > +void InkCollector::Init(HWND aTargetWindow)
> What is the significance of the HWND we use here?
> Looks like this HWND value is whatever window fires the first nsWindow::DispatchMouseEvent.
> Is that true? Is this expected?
aTargetWindow we should use to put Wnd preferencies to IInkCollector.
That preferencies determines window on which ink will be drawn.
Summary: Firefox have no detect when pen over and leave hover of digitizer → Firefox has no detect when pen over and leave hover of digitizer
Attached patch pen_out_detection_ver6.diff (obsolete) — Splinter Review
+ Added own references counter and checking it in destructor.
+ Changes AddRefs and Release for using own refCounter.
+ Changed some nsRefPtr's into raw pointers for eliminate some failures.
- Removed InkCollector::IsEnabled()

Feadback and comments and suggestions and objections are very welcome.
Attachment #8589156 - Attachment is obsolete: true
Attachment #8589156 - Flags: feedback?(jmathies)
Attachment #8589156 - Flags: feedback?(bugs)
Attachment #8590957 - Flags: review?(jmathies)
Attachment #8590957 - Flags: review?(bugs)
The current questions:
  What will happend, if COM object InkCollector is unavailable on system.
  According to current patch we will try and try to get it.
  Maybe the good approach is try to get it only one time?

  According to current patch we try to get COM object every time at each PEN "UP" events.
  But really initialization happens, if user has real "PEN" device.
  Maybe the good approach is try to enable this COM object one time at startup?
  But in this case all users get working COM object, although does not have any PEN devices.

Comments and suggestions are very welcome.
Assignee: nobody → alessarik
nit - I'm finding Windows line endings throughout this work. Please clean that up on the next posting.
Comment on attachment 8590957 [details] [diff] [review]
pen_out_detection_ver6.diff

I don't understand the refcounting of InkCollector.
We create a static instance of it, so, why any refcounting? And Release() doesn't actually delete the object ever.
Why do we want the static object? 
In general using refcounted objects as static or stack objects is very error prone. 

If InkCollector needs to be refcounted, why not then make the static variable
static InkCollector* sInkCollector;
which is set to some value when InkCollector object is created and
cleared when the object sInkCollector is pointing to is deleted. And whoever
actually uses the InkCollector instance should hold a strong reference to it.

Or, if we don't want InkCollector to be refcounted, then either remove AddRef/Release, or
at least make those methods do nothing.


(But I don't quite understand the lifetime management here. How long should the stuff InkCollector has pointers to be kept alive?)
Attachment #8590957 - Flags: review?(bugs) → review-
So I was testing on a surface pro and I found that IsTabletExternal() is always false, sunch that after we set this all up and we're receiving callbacks we never send any events. Is that expected? 

Specifically, why do we set the rule "All events should be suppressed except from external tablets"?
Flags: needinfo?(alessarik)
(In reply to Jim Mathies [:jimm] from comment #22)
> So I was testing on a surface pro and I found that IsTabletExternal() is always false
I expect that surface pro returns combination of TabletHardwareCapabilities flags according to article which pointed in comments before method InkCollector::IsTabletExternal(). I have chance to use real external devices which returns expected THWC_HardProximity flag only.
> Specifically, why do we set the rule "All events should be suppressed except
> from external tablets"?
The main purpose of using this stuff is get only DISPID_ICECursorOutOfRange event now. In future it can be changed. In default mode several events can be call our InkCollector::Invoke method, but all of them is unneeded for us except DISPID_ICECursorOutOfRange. That's why I suppress another events to don't allow decreasing of FireFox performance.
(In reply to Maksim Lebedev from comment #23)
> (In reply to Jim Mathies [:jimm] from comment #22)
> > So I was testing on a surface pro and I found that IsTabletExternal() is always false
> I expect that surface pro returns combination of TabletHardwareCapabilities
> flags according to article which pointed in comments before method
> InkCollector::IsTabletExternal(). I have chance to use real external devices
> which returns expected THWC_HardProximity flag only.

So are you saying that I've found a bug in your patches with devices like the surface pro? Or is this expected behavior?
(In reply to Jim Mathies [:jimm] from comment #24)
> (In reply to Maksim Lebedev from comment #23)
> > (In reply to Jim Mathies [:jimm] from comment #22)
> > > So I was testing on a surface pro and I found that IsTabletExternal() is always false
> > I expect that surface pro returns combination of TabletHardwareCapabilities
> > flags according to article which pointed in comments before method
> > InkCollector::IsTabletExternal(). I have chance to use real external devices
> > which returns expected THWC_HardProximity flag only.
> 
> So are you saying that I've found a bug in your patches with devices like
> the surface pro? Or is this expected behavior?

These are bit flags, so this is a bug. The surface pro does return THWC_HardProximity, and THWC_Integrated.

typedef enum TabletHardwareCapabilities { 
  THWC_Integrated               = 1, 
  THWC_CursorMustTouch          = 2, 
  THWC_HardProximity            = 4, 
  THWC_CursorsHavePhysicalIds   = 8  
} TabletHardwareCapabilities;
Using this code on the surfacepro I get better results and the event gets fired:

bool InkCollector::IsTabletExternal(IInkTablet* aTablet) const
{
  if (aTablet) {
    TabletHardwareCapabilities caps;
    if (SUCCEEDED(aTablet->get_HardwareCapabilities(&caps))) {
      return !!(TabletHardwareCapabilities::THWC_HardProximity & caps);
    }
  }
  return false;
}
(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8590957 [details] [diff] [review]
> pen_out_detection_ver6.diff
> 
> I don't understand the refcounting of InkCollector.
> We create a static instance of it, so, why any refcounting? And Release()
> doesn't actually delete the object ever.
> Why do we want the static object? 
> In general using refcounted objects as static or stack objects is very error
> prone. 

I've been going back and forth with Maksim on this. We should be careful about mixed signals here since I've been asking why he's *not* properly refcounting. Now that I have this running locally I can take a closer look.. in general I think he wants to just keep this thing around for the entire run, which is fine and is working with the current patch as it is.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch review comments as patch (obsolete) — Splinter Review
This is a diff on top of your work (not including blowing away all the windows line endings in your patches). Please take a look, we're getting closer here but we still have at least one issue with a ref count that's off by one.
Comment on attachment 8590957 [details] [diff] [review]
pen_out_detection_ver6.diff

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

::: widget/windows/InkCollector.h
@@ +32,5 @@
> +
> +protected:
> +  // IUnknown
> +  virtual ULONG STDMETHODCALLTYPE AddRef() { return ++mRefCount; }
> +  virtual ULONG STDMETHODCALLTYPE Release() { return --mRefCount; }

I kinda like the way you have this set up currently with the asssert in the destructor.
Attachment #8590957 - Flags: review?(jmathies)
(In reply to Jim Mathies [:jimm] from comment #29)
> Comment on attachment 8590957 [details] [diff] [review]
> pen_out_detection_ver6.diff
> 
> Review of attachment 8590957 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/InkCollector.h
> @@ +32,5 @@
> > +
> > +protected:
> > +  // IUnknown
> > +  virtual ULONG STDMETHODCALLTYPE AddRef() { return ++mRefCount; }
> > +  virtual ULONG STDMETHODCALLTYPE Release() { return --mRefCount; }
> 
> I kinda like the way you have this set up currently with the asssert in the
> destructor.

Ignore this ^^^^^^
Comment on attachment 8591839 [details] [diff] [review]
review comments as patch

>   // Global shutdown
>   if (sInstanceCount == 0) {
>     IMEHandler::Terminate();
>+    InkCollector::sInkCollector.Shutdown();
>     NS_IF_RELEASE(sCursorImgContainer);
This is awesome place. With this proposal, refCounter became null in my patch. Thanks.
(In reply to Olli Pettay [:smaug] from comment #21)
> We create a static instance of it, so, why any refcounting?
> ... then either remove AddRef/Release ...
We should implement AddRef and Release, because our class inherits from interface with empty functions.
> And Release() doesn't actually delete the object ever.
Because it is static object :-)
> Why do we want the static object?
Static instance has the simpliest implementation. Why not?
> How long should the stuff InkCollector has pointers to be kept alive?
I think during all time, when FireFox can show pages.
(In reply to Jim Mathies [:jimm] from comment #24)
> So are you saying that I've found a bug in your patches with devices like
> the surface pro? Or is this expected behavior?
(In reply to Jim Mathies [:jimm] from comment #25)
> These are bit flags, so this is a bug. The surface pro does return
> THWC_HardProximity, and THWC_Integrated.
I cannot say what is it. I have no chance to test surface pro, that's why I cannot say about proper behavior on surface pro. Has surface pro some detecteable area under the screen? Can surface pro detect PEN, and can detect when PEN moved without pressing of screen? I can do changes proposing in your patch, but I need more information about this surface pro.
Attached patch pen_out_detection_ver7.diff (obsolete) — Splinter Review
+ Returned to using nsRefPtr<*>.
+ Resolved all issues related with reference counting.
+ Reallocated IncCollector::Shutdown().
+ Added Shutdown call into global shutdown place.

+ Changed check for InkCollector initialization to prevent waterfall of initializations.
+ Changed check for only once internal initialization.

+ Changed scheme for notify only one current window listener.
- Removed AddListener() and RemoveListener()

+ Init -> SetTarget()
+ IsTabletExternal() -> IsHardProximityTablet()

Feadback and comments and suggestions and objections are very welcome.
Attachment #8590957 - Attachment is obsolete: true
Flags: needinfo?(alessarik)
Attachment #8592911 - Flags: review?(jmathies)
Attachment #8592911 - Flags: review?(bugs)
(In reply to Maksim Lebedev from comment #33)
> (In reply to Jim Mathies [:jimm] from comment #24)
> > So are you saying that I've found a bug in your patches with devices like
> > the surface pro? Or is this expected behavior?
> (In reply to Jim Mathies [:jimm] from comment #25)
> > These are bit flags, so this is a bug. The surface pro does return
> > THWC_HardProximity, and THWC_Integrated.
> I cannot say what is it. I have no chance to test surface pro, that's why I
> cannot say about proper behavior on surface pro. Has surface pro some
> detecteable area under the screen? Can surface pro detect PEN, and can
> detect when PEN moved without pressing of screen? I can do changes proposing
> in your patch, but I need more information about this surface pro.

See comment 25, these values are bit flags  - 0x01, 0x02, 0x04, 0x08, .. so we can test for the presence of individual values. My patch on top of your does this.
Comment on attachment 8592911 [details] [diff] [review]
pen_out_detection_ver7.diff

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

This is getting close! :)

::: widget/windows/InkCollector.cpp
@@ +47,5 @@
> +  if (FAILED(::CoCreateInstance(CLSID_InkCollector, NULL, CLSCTX_INPROC_SERVER,
> +                                IID_IInkCollector, getter_AddRefs(mInkCollector)))) {
> +    return;
> +  }
> +  NS_ADDREF(mInkCollector);

why do we need this addref here... see below..

@@ +64,5 @@
> +      }
> +    }
> +  }
> +  // Don't need the connection point container any more.
> +  // NS_IF_RELEASE(connPointContainer);

nit - remove this dead code

@@ +76,5 @@
> +    mConnectionPoint->Unadvise(mCookie);
> +    NS_RELEASE(mConnectionPoint);
> +  }
> +  NS_IF_RELEASE(mMarshaller);
> +  NS_IF_RELEASE(mInkCollector);

CoCreateInstance AddRefs so I'm not sure why we need this. I think you can remove this and the NS_ADDREF(mInkCollector) above? Or would that throw things off?

@@ +155,5 @@
> +{
> +  if (aTablet) {
> +    TabletHardwareCapabilities caps;
> +    if (SUCCEEDED(aTablet->get_HardwareCapabilities(&caps))) {
> +      return (TabletHardwareCapabilities::THWC_HardProximity & caps);

perfect, thanks.

@@ +217,5 @@
> +    return;
> +  }
> +  // Notify current target window.
> +  if (mTargetWindow) {
> +    ::PostMessage(mTargetWindow, MOZ_WM_PEN_LEAVES_HOVER_OF_DIGITIZER, 0, 0);

Ok, great. We've gotten rid of the AddListener stuff. Good to see.

::: widget/windows/nsWindow.cpp
@@ +3788,5 @@
> +      // Currently this scheme is used only when pointer events is enabled.
> +      && gfxPref::PointerEventEnabled()
> +      // NS_MOUSE_EXIT is received, when InkCollector has been already initialized.
> +      && NS_MOUSE_EXIT != aEventType) {
> +    InkCollector::sInkCollector.SetTarget(mWnd);

One thing I noticed while testing the last patch - due to the late init here the first time you bring a pen to the surface and then remove it, we init the ink class but never fire MOZ_WM_PEN_LEAVES_HOVER_OF_DIGITIZER. This is probably not a huge deal, but it does mean that the very first time a user interacts with the pen, mouse leave events won't fire.
(In reply to Jim Mathies [:jimm] from comment #36)
> > +  if (FAILED(::CoCreateInstance(CLSID_InkCollector, NULL, CLSCTX_INPROC_SERVER,
> > +                                IID_IInkCollector, getter_AddRefs(mInkCollector)))) {
> > +    return;
> > +  }
> > +  NS_ADDREF(mInkCollector);
> why do we need this addref here... see below..

> > +    mConnectionPoint->Unadvise(mCookie);
> > +    NS_RELEASE(mConnectionPoint);
> > +  }
> > +  NS_IF_RELEASE(mMarshaller);
> > +  NS_IF_RELEASE(mInkCollector);
> 
> CoCreateInstance AddRefs so I'm not sure why we need this. I think you can remove
> this and the NS_ADDREF(mInkCollector) above? Or would that throw things off?

I investigated Initialization() and I found that during "mConnectionPoint->Advise()" our mRefCount will be increased from 0 to 2. I investigated Shutdown() and I found that during "mConnectionPoint->Unadvise()" our mRefCount decrease from 2 to 0. As result we will have mConnectionPoint and mInkCollector which point on deallocated memory but still have no nullptr value. That's why during NS_RELEASE or destructor of nsRefPtr we will get exception.
To prevent that exceptions, I use ADDREF for mInkCollector and mConnectionPoint and I get good results.
(In reply to Jim Mathies [:jimm] from comment #36)
> > +      // Currently this scheme is used only when pointer events is enabled.
> > +      && gfxPref::PointerEventEnabled()
> > +      // NS_MOUSE_EXIT is received, when InkCollector has been already initialized.
> > +      && NS_MOUSE_EXIT != aEventType) {
> > +    InkCollector::sInkCollector.SetTarget(mWnd);
> One thing I noticed while testing the last patch - due to the late init here
> the first time you bring a pen to the surface and then remove it, we init
> the ink class but never fire MOZ_WM_PEN_LEAVES_HOVER_OF_DIGITIZER. This is
> probably not a huge deal, but it does mean that the very first time a user
> interacts with the pen, mouse leave events won't fire.
I think the a lot of initialization during NS_MOUSE_MOVE will decrease perfomance of FireFox.
That's why I suppress NS_MOUSE_MOVE event for initialization.
During my investigation we can receive at this point such events:
  NS_MOUSE_BUTTON_DOWN/NS_MOUSE_BUTTON_UP - it is normal behavior for initialization;
  NS_MOOUSE_EXIT - FF sends this event himself when initialization already happened;
  NS_MOUSE_ENTER before first NS_MOUSE_MOVE when window become active.
You had got behavior (I knew about such behavior), which can put shadows of doubt
on this postponed initialization, but in any case I don't want to use NS_MOUSE_MOVE
for better initialization but with decreasing FF performance.

Possibly, the good and more obvious approach is use more hard comparisons with MOUSE_DOWN/MOUSE_ENTER ?
(In reply to Maksim Lebedev from comment #38)
> (In reply to Jim Mathies [:jimm] from comment #36)
> > > +      // Currently this scheme is used only when pointer events is enabled.
> > > +      && gfxPref::PointerEventEnabled()
> > > +      // NS_MOUSE_EXIT is received, when InkCollector has been already initialized.
> > > +      && NS_MOUSE_EXIT != aEventType) {
> > > +    InkCollector::sInkCollector.SetTarget(mWnd);
> > One thing I noticed while testing the last patch - due to the late init here
> > the first time you bring a pen to the surface and then remove it, we init
> > the ink class but never fire MOZ_WM_PEN_LEAVES_HOVER_OF_DIGITIZER. This is
> > probably not a huge deal, but it does mean that the very first time a user
> > interacts with the pen, mouse leave events won't fire.
> I think the a lot of initialization during NS_MOUSE_MOVE will decrease
> perfomance of FireFox.
> That's why I suppress NS_MOUSE_MOVE event for initialization.
> During my investigation we can receive at this point such events:
>   NS_MOUSE_BUTTON_DOWN/NS_MOUSE_BUTTON_UP - it is normal behavior for
> initialization;
>   NS_MOOUSE_EXIT - FF sends this event himself when initialization already
> happened;
>   NS_MOUSE_ENTER before first NS_MOUSE_MOVE when window become active.
> You had got behavior (I knew about such behavior), which can put shadows of
> doubt
> on this postponed initialization, but in any case I don't want to use
> NS_MOUSE_MOVE
> for better initialization but with decreasing FF performance.
> 
> Possibly, the good and more obvious approach is use more hard comparisons
> with MOUSE_DOWN/MOUSE_ENTER ?

I'm not seeing the same thing with the latest now, so maybe it was fixed.
Comment on attachment 8592911 [details] [diff] [review]
pen_out_detection_ver7.diff

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

Ok, I think this is looking safe to land.
Attachment #8592911 - Flags: review?(jmathies) → review+
Comment on attachment 8592911 [details] [diff] [review]
pen_out_detection_ver7.diff

So in principle I don't like using refcounted object on stack or as a static object.
What if something in Windows libraries keeps a reference to the object _really_ late? Then we'd have a crash.

So just make it a normal refcounted object (where --refcnt == 0 causes the object to be deleted) and then store instance of it
using  StaticRefPtr<InkCollector> and clear that reference in
nsWindow::~nsWindow()
if (sInstanceCount == 0) { ..
or something like that.
Attachment #8592911 - Flags: review?(bugs) → review-
+ Added using of template StaticRefPtr<*>
Attachment #8592911 - Attachment is obsolete: true
Attachment #8594042 - Flags: review?(jmathies)
Attachment #8594042 - Flags: review?(bugs)
(In reply to Jim Mathies [:jimm] from comment #39)
> I'm not seeing the same thing with the latest now, so maybe it was fixed.
Unfortunately, I assume that it cannot be fixed while we have postponed initialization.
(In reply to Olli Pettay [:smaug] from comment #41)
> So just make it a normal refcounted object (where --refcnt == 0 causes the
> object to be deleted) and then store instance of it
> using  StaticRefPtr<InkCollector> and clear that reference in


Ah nice catch, I've not worked with that. I think we added a little while ago for this type of situation though.
Comment on attachment 8594042 [details] [diff] [review]
pen_out_detection_ver8.diff

I'm already r+'d here so you can feed future reviews to smaug.
Attachment #8594042 - Flags: review?(jmathies) → review+
Attachment #8594042 - Flags: review?(bugs) → review+
Blocks: 1122211
If there are no objections, I ready to push latest changes into m-c.
Keywords: checkin-needed
(In reply to Maksim Lebedev from comment #46)
> If there are no objections, I ready to push latest changes into m-c.

could you attach a try run for the changes, thanks!
Flags: needinfo?(alessarik)
(In reply to Carsten Book [:Tomcat] from comment #47)
> could you attach a try run for the changes, thanks!
Sorry, I forgot attach it.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=04e401c802d0
Flags: needinfo?(alessarik)
Attachment #8591839 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f8342a0786ca
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Blocks: 1163056
Blocks: 1164473
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: