Bug 231267 - Pull modifier key extraction into TestRunnerShared
Summary: Pull modifier key extraction into TestRunnerShared
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-05 16:46 PDT by Beth Dakin
Modified: 2021-10-09 14:36 PDT (History)
4 users (show)

See Also:


Attachments
Patch (45.69 KB, patch)
2021-10-05 16:51 PDT, Beth Dakin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (45.72 KB, patch)
2021-10-05 17:04 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (45.41 KB, patch)
2021-10-06 11:53 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (45.44 KB, patch)
2021-10-06 11:59 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (45.44 KB, patch)
2021-10-06 12:03 PDT, Beth Dakin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (45.46 KB, patch)
2021-10-06 13:00 PDT, Beth Dakin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (46.96 KB, patch)
2021-10-06 16:31 PDT, Beth Dakin
simon.fraser: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (46.97 KB, patch)
2021-10-07 16:44 PDT, Beth Dakin
thorton: review+
Details | Formatted Diff | Diff
Patch (46.76 KB, patch)
2021-10-08 13:17 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2021-10-05 16:46:14 PDT
Pull modifier key extraction into TestRunnerShared to avoid duplicated code in WKTR and DRT
Comment 1 Beth Dakin 2021-10-05 16:51:19 PDT
Created attachment 440298 [details]
Patch
Comment 2 Beth Dakin 2021-10-05 17:04:15 PDT
Created attachment 440303 [details]
Patch
Comment 3 Simon Fraser (smfr) 2021-10-05 17:04:57 PDT
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.
Comment 4 Beth Dakin 2021-10-06 11:24:42 PDT
(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.
Comment 5 Beth Dakin 2021-10-06 11:53:33 PDT
Created attachment 440402 [details]
Patch
Comment 6 Beth Dakin 2021-10-06 11:59:09 PDT
Created attachment 440404 [details]
Patch
Comment 7 Beth Dakin 2021-10-06 12:03:39 PDT
Created attachment 440405 [details]
Patch
Comment 8 Beth Dakin 2021-10-06 13:00:19 PDT
Created attachment 440418 [details]
Patch
Comment 9 Simon Fraser (smfr) 2021-10-06 13:22:50 PDT
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];
Comment 10 Simon Fraser (smfr) 2021-10-06 13:24:02 PDT
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.
Comment 11 Beth Dakin 2021-10-06 16:31:47 PDT
Created attachment 440454 [details]
Patch
Comment 12 Beth Dakin 2021-10-07 16:44:24 PDT
Created attachment 440555 [details]
Patch
Comment 13 Tim Horton 2021-10-08 13:09:28 PDT
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!
Comment 14 Beth Dakin 2021-10-08 13:17:57 PDT
Created attachment 440662 [details]
Patch

This should address all of Tim's feedback.
Comment 15 EWS 2021-10-09 14:35:28 PDT
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].
Comment 16 Radar WebKit Bug Importer 2021-10-09 14:36:18 PDT
<rdar://problem/84065856>