[webkit-reviews] review granted: [Bug 231267] Pull modifier key extraction into TestRunnerShared : [Attachment 440555] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 8 13:09:28 PDT 2021


Tim Horton <thorton at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 231267: Pull modifier key extraction into TestRunnerShared
https://bugs.webkit.org/show_bug.cgi?id=231267

Attachment 440555: Patch

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




--- Comment #13 from Tim Horton <thorton at apple.com> ---
Comment on attachment 440555
  --> https://bugs.webkit.org/attachment.cgi?id=440555
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!


More information about the webkit-reviews mailing list