[webkit-reviews] review granted: [Bug 57423] REGRESSION (r82320): Spacebar no longer pages down : [Attachment 87574] updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 57423: REGRESSION (r82320): Spacebar no longer pages down
https://bugs.webkit.org/show_bug.cgi?id=57423

Attachment 87574: updated patch
https://bugs.webkit.org/attachment.cgi?id=87574&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=87574&action=review

> Source/WebKit/mac/ChangeLog:15
> +	   Even if this specific command hasn't been handled, that doens't
nullify side effects from

Typo in the word doesn't

> Source/WebKit/mac/ChangeLog:20
> +	   bug, removed setting _private->interpretKeyEventsParameters to 0. I
don't see any way for
> +	   another WebHTMLView NSTextInput method to be called from within
insertText:, so no one is
> +	   going to look at it.

Seems OK, but risky.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5946
> -	   NSString *rangeString = [string
attribute:NSTextInputReplacementRangeAttributeName atIndex:0
longestEffectiveRange:NULL inRange:NSMakeRange(0, markedTextLength)];
> +	   NSString *rangeString = [string
attribute:NSTextInputReplacementRangeAttributeName atIndex:0
longestEffectiveRange:nil inRange:NSMakeRange(0, markedTextLength)];

This is a pointer to an NSRange, not a pointer to an Objective-C object, so the
use of nil is not appropriate.

> Source/WebKit/mac/WebView/WebHTMLView.mm:6033
> -	   // event in TSM.  This behaviour matches that of -[WebHTMLView
setMarkedText:selectedRange:] when it receives an
> +	   // event in TSM. This behaviour matches that of -[WebHTMLView
setMarkedText:selectedRange:] when it receives an

If you’re tweaking things, you could move to the U.S. spelling of behavior.

> Source/WebKit/mac/WebView/WebHTMLView.mm:6073
> +    bool eventHandled = false;
>      String eventText = text;
>      eventText.replace(NSBackTabCharacter, NSTabCharacter); // same thing is
done in KeyEventMac.mm in WebCore
>      if (coreFrame && coreFrame->editor()->canEdit()) {
> -	   if (!coreFrame->editor()->hasComposition())
> -	       coreFrame->editor()->insertText(eventText, event);
> -	   else
> +	   if (!coreFrame->editor()->hasComposition()) {
> +	       // An insertText: might be handled by other responders in the
chain if we don't handle it.
> +	       // One example is space bar that results in scrolling down the
page.
> +	       eventHandled = coreFrame->editor()->insertText(eventText,
event);
> +	   } else {
> +	       eventHandled = true;
>	       coreFrame->editor()->confirmComposition(eventText);
> +	   }
>      }
>      
>      if (parameters)
> -	   parameters->eventInterpretationHadSideEffects = true;
> -
> -    _private->interpretKeyEventsParameters = parameters;
> +	   parameters->eventInterpretationHadSideEffects |= eventHandled;

I don’t see why we are doing the work to make eventText and set up the
eventHandled boolean in cases where we don’t need them. I would put more of the
code inside the if, or use an early return instead of an if.