[webkit-reviews] review granted: [Bug 190141] Swizzle +[UIKeyboard isInHardwareKeyboardMode] in WebKitTestRunner and DumpRenderTree : [Attachment 351256] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 190141: Swizzle +[UIKeyboard isInHardwareKeyboardMode] in WebKitTestRunner
and DumpRenderTree
https://bugs.webkit.org/show_bug.cgi?id=190141

Attachment 351256: Patch

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




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 351256
  --> https://bugs.webkit.org/attachment.cgi?id=351256
Patch

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

> Tools/TestRunnerShared/ios/SwizzleUIKit.h:30
> +void swizzleUIKit();

I think this is a too-generic function name. Perhaps you are imagining that
later this single function will contain changes for many different UIKit
methods.

But I don’t think that’s right. This one method that we are patching might
later be patched other ways, for example, we might want to run tests in
hardware keyboard mode at some point and might add an internals function to
switch it.

I’d call this function something more like this:

    forceHardwareKeyboardModeOff();

I wouldn't personally use the word "swizzle". The way this is written, it’s
safe to call it over and over again, and I don’t think the emphasis should be
put on the implementation technique of the function.

I don’t care as much about the name of the source file. SwizzleUIKit.h might be
OK.

> Tools/TestRunnerShared/spi/UIKitTestSPI.h:66
> ++ (BOOL)isInHardwareKeyboardMode;

Why did we need to add this?

> Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:375
> +		CE2270912161DAA200DB260C /* SwizzleUIKit.h */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name =
SwizzleUIKit.h; path = ../TestRunnerShared/ios/SwizzleUIKit.h; sourceTree =
"<group>"; };
> +		CE2270922161DAA200DB260C /* SwizzleUIKit.mm */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp;
name = SwizzleUIKit.mm; path = ../TestRunnerShared/ios/SwizzleUIKit.mm;
sourceTree = "<group>"; };

Path doesn’t look quite right here. Is this how the other TestRunnerShared
source files are set up?

> Tools/WebKitTestRunner/ios/TestControllerIOS.mm:46
> +#import <objc/runtime.h>

This should not be here. Left over from before you moved this into another file
I assume.