Bug 144663 – New force-related DOM events should fire in WK1 views

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Beth Dakin]

NeNew force-related DOM events should fire in WK1 views

rdar://problem/20281886

[Beth Dakin]

Created attachment 252448 [details]
Patch

[WebKit Commit Bot]

Attachment 252448 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebImmediateActionController.mm:151:  The parameter name "element" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:53:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:78:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:109:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 4 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

Created attachment 252527 [details]
Patch

[WebKit Commit Bot]

Attachment 252527 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebImmediateActionController.mm:151:  The parameter name "element" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:53:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:78:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:109:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 4 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 252527 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252527&action=review

> Source/WebCore/page/mac/EventHandlerMac.mm:86
> +    static NeverDestroyed<RetainPtr<NSEvent>> event;

Is it really right for this to be stored between views? That seems weird.

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:428
> +        if ([event type] == NSEventTypePressure) {

You check this three times in this function. Maybe put it in a local?

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:439
> +    UNUSED_PARAM(lastPressureEvent);

Indentation is wrong I think.

> Source/WebKit/mac/WebView/WebHTMLView.mm:-3844
> -    [[[self _webView] _immediateActionController] webView:[self _webView] willHandleMouseDown:event];

Whaaaaaa?

> Source/WebKit/mac/WebView/WebViewData.h:178
> +    NSEvent *pressureEvent;

Maybe lastPressureEvent?

Could you not keep this in EventHandler instead and avoid keeping it here?

[Beth Dakin]

(In reply to comment #5)
> Comment on attachment 252527 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252527&action=review
> 
> > Source/WebCore/page/mac/EventHandlerMac.mm:86
> > +    static NeverDestroyed<RetainPtr<NSEvent>> event;
> 
> Is it really right for this to be stored between views? That seems weird.
> 

Hmm, maybe not. I was copying what's done for currentNSEventSlot(). I think it makes as much sense in this new case as it did in the old, but I'm a little unsure here.

> > Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:428
> > +        if ([event type] == NSEventTypePressure) {
> 
> You check this three times in this function. Maybe put it in a local?
> 

Sure, I can fix this up.

> > Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:439
> > +    UNUSED_PARAM(lastPressureEvent);
> 
> Indentation is wrong I think.
> 

Will look.

> > Source/WebKit/mac/WebView/WebHTMLView.mm:-3844
> > -    [[[self _webView] _immediateActionController] webView:[self _webView] willHandleMouseDown:event];
> 
> Whaaaaaa?
> 

This is because mouseDown used to come after the immediate action! Remember? We used to have shenanigans like this in WK2 as well, and we were able to remove them when we switched to delaysPrimaryMouseEvents:NO. This code was there purely because we were getting mouse down at a weird time.

> > Source/WebKit/mac/WebView/WebViewData.h:178
> > +    NSEvent *pressureEvent;
> 
> Maybe lastPressureEvent?
> 

Sure.

> Could you not keep this in EventHandler instead and avoid keeping it here?

Hmmmmmmm. I'll have to think this over. This is a closer mirror to what we do in WK2. If we moved it to EventHandler, we would have WK1 only member variables there, which we actually manage to avoid at this point.

[Tim Horton]

(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 252527 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=252527&action=review
> > 
> > > Source/WebCore/page/mac/EventHandlerMac.mm:86
> > > +    static NeverDestroyed<RetainPtr<NSEvent>> event;
> > 
> > Is it really right for this to be stored between views? That seems weird.
> > 
> 
> Hmm, maybe not. I was copying what's done for currentNSEventSlot(). I think
> it makes as much sense in this new case as it did in the old, but I'm a
> little unsure here.

It's OK to keep a "current" event globally, because the relevant parts of WebKit are single-threaded, and a "current" event will get cleared when it's done (I assume). But keeping a "last" event, that lives beyond its own handling, seems wrong in the multi-view case.

[Tim Horton]

(In reply to comment #6)
> (In reply to comment #5)
> > > Source/WebKit/mac/WebView/WebHTMLView.mm:-3844
> > > -    [[[self _webView] _immediateActionController] webView:[self _webView] willHandleMouseDown:event];
> > 
> > Whaaaaaa?
> > 
> 
> This is because mouseDown used to come after the immediate action! Remember?
> We used to have shenanigans like this in WK2 as well, and we were able to
> remove them when we switched to delaysPrimaryMouseEvents:NO. This code was
> there purely because we were getting mouse down at a weird time.

Righto.

> > Could you not keep this in EventHandler instead and avoid keeping it here?
> 
> Hmmmmmmm. I'll have to think this over. This is a closer mirror to what we
> do in WK2. If we moved it to EventHandler, we would have WK1 only member
> variables there, which we actually manage to avoid at this point.

Could you not then perhaps use them for WK2 as well, and share some code that you currently have duplicated between WebView and WKView?

[Beth Dakin]

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > > Source/WebKit/mac/WebView/WebHTMLView.mm:-3844
> > > > -    [[[self _webView] _immediateActionController] webView:[self _webView] willHandleMouseDown:event];
> > > 
> > > Whaaaaaa?
> > > 
> > 
> > This is because mouseDown used to come after the immediate action! Remember?
> > We used to have shenanigans like this in WK2 as well, and we were able to
> > remove them when we switched to delaysPrimaryMouseEvents:NO. This code was
> > there purely because we were getting mouse down at a weird time.
> 
> Righto.
> 
> > > Could you not keep this in EventHandler instead and avoid keeping it here?
> > 
> > Hmmmmmmm. I'll have to think this over. This is a closer mirror to what we
> > do in WK2. If we moved it to EventHandler, we would have WK1 only member
> > variables there, which we actually manage to avoid at this point.
> 
> Could you not then perhaps use them for WK2 as well, and share some code
> that you currently have duplicated between WebView and WKView?

Ugh, maybe some day? But I  find the idea of that exhausting because I had originally implemented a version of the WK2 code that was more like this, but Simon and Sam very much wanted every layer of the WebKit events to have force in them, and to squish the square peg (pressure events in AppKit are not mouse events) into the round hole (we want pressure events to be mouse events) at the moment NSEvents are converted to anything else in WK2. That is done in the UIProcess, way far away from EventHandler. In WK1, that conversion happens at a surprisingly low level, hence all of these new parameters.

[Tim Horton]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Comment on attachment 252527 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=252527&action=review
> > > 
> > > > Source/WebCore/page/mac/EventHandlerMac.mm:86
> > > > +    static NeverDestroyed<RetainPtr<NSEvent>> event;
> > > 
> > > Is it really right for this to be stored between views? That seems weird.
> > > 
> > 
> > Hmm, maybe not. I was copying what's done for currentNSEventSlot(). I think
> > it makes as much sense in this new case as it did in the old, but I'm a
> > little unsure here.
> 
> It's OK to keep a "current" event globally, because the relevant parts of
> WebKit are single-threaded, and a "current" event will get cleared when it's
> done (I assume). But keeping a "last" event, that lives beyond its own
> handling, seems wrong in the multi-view case.

OK, Sam explained that this *is* actually the force event matching the currently processing event, not actually the last event, and it doesn't persist between handling events, so I think it just needs a different name, not a big change.

[Beth Dakin]

Created attachment 252535 [details]
Patch

Okay, here's a new patch that uses a new name, assuming we do feel okay about this approach. I can try something else if there is another idea too!

[WebKit Commit Bot]

Attachment 252535 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebImmediateActionController.mm:151:  The parameter name "element" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:53:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:78:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:109:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 4 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

Created attachment 252540 [details]
Patch

[WebKit Commit Bot]

Attachment 252540 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebImmediateActionController.mm:151:  The parameter name "element" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:53:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:78:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:109:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 4 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 252540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252540&action=review

> Source/WebCore/page/mac/EventHandlerMac.mm:124
>      currentNSEventSlot() = m_savedCurrentEvent;
> +    correspondingPressureEventSlot() = nil;

I would follow the pattern we have here and keep a stack of corresponding events.

> Source/WebKit/mac/WebView/WebHTMLView.mm:4093
> +    NSEvent *lastPressureEvent = [[self _webView] _pressureEvent];
> +    if (event == lastPressureEvent)
> +        return;

Is this necessary? It seems like it can't happen.

> Source/WebKit/mac/WebView/WebViewData.h:178
> +    NSEvent *pressureEvent;

This should be a RetainPtr. You can probably get rid of the helpers when you make this change.

[Beth Dakin]

http://trac.webkit.org/changeset/183954

Thanks Sam and Tim!

[Said Abou-Hallawa]

http://trac.webkit.org/changeset/183968

Fixing iPhone 6 iOS simulator Build.

[Beth Dakin]

Another follow-up: https://bugs.webkit.org/show_bug.cgi?id=144805