Bug 231267 – Pull modifier key extraction into TestRunnerShared

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Beth Dakin]

Pull modifier key extraction into TestRunnerShared to avoid duplicated code in WKTR and DRT

[Beth Dakin]

Created attachment 440298 [details]
Patch

[Beth Dakin]

Created attachment 440303 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 440298 [details]
Patch

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

> Tools/TestRunnerShared/cocoa/ModifierKeyExtractor.h:33
> +struct KeyMappingEntry {
> +    int macKeyCode;
> +    int macNumpadKeyCode;
> +    unichar character;
> +    NSString* characterName;
> +};

Can this be internal to ModifierKeyExtractor?

> Tools/TestRunnerShared/cocoa/ModifierKeyExtractor.h:43
> +- (id)initWithKey:(NSString *)key modifiers:(int)modifiers keyLocation:(unsigned)keyLocation;

How about:

@interface ModifierKeys : NSObject {
@public
    NSString *eventCharacter;
    unsigned short keyCode;
    NSString *charactersIgnoringModifiers;
    int modifierFlags;
}

+ (ModifierKeys*)modifierKeysWithKey:(NSString *)key modifiers:(int)modifiers keyLocation:(unsigned)keyLocation;

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:633
> +        modifierFlags:modifierKeyExtractor->modifierFlags

modifierKeyExtractor.modifierFlags here and elsewhere.

[Beth Dakin]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 440298 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440298&action=review
> 
> > Tools/TestRunnerShared/cocoa/ModifierKeyExtractor.h:33
> > +struct KeyMappingEntry {
> > +    int macKeyCode;
> > +    int macNumpadKeyCode;
> > +    unichar character;
> > +    NSString* characterName;
> > +};
> 
> Can this be internal to ModifierKeyExtractor?

Oh yes, good catch.

> 
> > Tools/TestRunnerShared/cocoa/ModifierKeyExtractor.h:43
> > +- (id)initWithKey:(NSString *)key modifiers:(int)modifiers keyLocation:(unsigned)keyLocation;
> 
> How about:
> 
> @interface ModifierKeys : NSObject {
> @public
>     NSString *eventCharacter;
>     unsigned short keyCode;
>     NSString *charactersIgnoringModifiers;
>     int modifierFlags;
> }
> 
> + (ModifierKeys*)modifierKeysWithKey:(NSString *)key
> modifiers:(int)modifiers keyLocation:(unsigned)keyLocation;

Okay! Will work on this rename. I suppose I should rename the file too.

> 
> > Tools/WebKitTestRunner/mac/EventSenderProxy.mm:633
> > +        modifierFlags:modifierKeyExtractor->modifierFlags
> 
> modifierKeyExtractor.modifierFlags here and elsewhere.

Slightly confused by this last piece of feedback. First, modifierKeyExtractor should probably be renamed to modifierKeys, yes? Second are you suggesting that the instance variables should be properties instead? I don't think that the suggested interface you typed out above makes that change from ivar to property. I might be missing the point of this piece of feedback though. Please help.

[Beth Dakin]

Created attachment 440402 [details]
Patch

[Beth Dakin]

Created attachment 440404 [details]
Patch

[Beth Dakin]

Created attachment 440405 [details]
Patch

[Beth Dakin]

Created attachment 440418 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 440418 [details]
Patch

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

> Tools/TestRunnerShared/cocoa/ModifierKeys.h:38
> +- (ModifierKeys*)modifierKeysWithKey:(NSString *)key modifiers:(int)modifiers keyLocation:(unsigned)keyLocation;

This needs to be a class method :
+ (ModifierKeys*) ...

> Tools/TestRunnerShared/cocoa/ModifierKeys.mm:45
> +- (ModifierKeys*)modifierKeysWithKey:(NSString *)character modifiers:(int)modifiers keyLocation:(unsigned)keyLocation

This should be an init method, then you need the class method that alloc/inits the object then returns it.

> Tools/TestRunnerShared/cocoa/ModifierKeys.mm:240
> +    return self;

return [self autorelease];

[Simon Fraser (smfr)]

Comment on attachment 440418 [details]
Patch

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

>> Tools/TestRunnerShared/cocoa/ModifierKeys.mm:45
>> +- (ModifierKeys*)modifierKeysWithKey:(NSString *)character modifiers:(int)modifiers keyLocation:(unsigned)keyLocation
> 
> This should be an init method, then you need the class method that alloc/inits the object then returns it.

And you need a -dealloc to not leak!

>> Tools/TestRunnerShared/cocoa/ModifierKeys.mm:240
>> +    return self;
> 
> return [self autorelease];

For the init method this is fine. The class method will return an autoreleased object.

[Beth Dakin]

Created attachment 440454 [details]
Patch

[Beth Dakin]

Created attachment 440555 [details]
Patch

[Tim Horton]

Comment on attachment 440555 [details]
Patch

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

> Tools/ChangeLog:18309
> -== Rolled over to ChangeLog-2021-03-18 ==
> +== Rolled over to ChangeLog-2021-03-18 ==

What's going on here

> Tools/TestRunnerShared/cocoa/ModifierKeys.h:32
> +    RetainPtr<NSString> eventCharacter;

public ivars in objc is quite unusual these days (vs. properties), but this is also fine, especially since you're just using it to replace what was a struct.

> Tools/TestRunnerShared/cocoa/ModifierKeys.h:38
> ++ (ModifierKeys*)modifierKeysWithKey:(NSString *)key modifiers:(int)modifiers keyLocation:(unsigned)keyLocation;

`ModifierKeys *`

> Tools/TestRunnerShared/cocoa/ModifierKeys.mm:34
> +#if PLATFORM(IOS_FAMILY)
> +#import <WebKit/KeyEventCodesIOS.h>
> +#import <WebKit/WebEvent.h>
> +#endif
> +#import <WebKit/WebKitLegacy.h>
> +#include <wtf/Vector.h>

Chaotic ordering (#ifdeffed ones should come in separate paragraph unless they can't for some reason), and also mixed import/include.

> Tools/TestRunnerShared/cocoa/ModifierKeys.mm:40
> +    NSString* characterName;

"star's on the wrong side"

> Tools/TestRunnerShared/cocoa/ModifierKeys.mm:61
> +    ModifierKeys *modiferKeys = [[ModifierKeys alloc] init];

This LOOKS like a leak if you don't see the autorelease 500 miles away at the bottom of the function. Autorelease here instead!

[Beth Dakin]

Created attachment 440662 [details]
Patch

This should address all of Tim's feedback.

[EWS]

Committed r283870 (242747@main): <https://commits.webkit.org/242747@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440662 [details].

[Radar WebKit Bug Importer]

<rdar://problem/84065856>