Bug 190141 – Override +[UIKeyboard isInHardwareKeyboardMode] in WebKitTestRunner and DumpRenderTree

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

[Daniel Bates]

Following up on a suggestion by Dan Bernstein and Darin Adler in bug 190017, comment 11 and bug 190017, comment 12, respectively, we should swizzle +[UIKeyboard isInHardwareKeyboardMode] in WebKitTestRunner and DumpRenderTree so as to remove the need to have WebKit/WebCore code check if we are executing in these apps before invoking +[UIKeyboard isInHardwareKeyboardMode].

[Daniel Bates]

Created attachment 351256 [details]
Patch

I shared the swizzling logic between DumpRenderTree and WebKitTestRunner by adding new files SwizzleUIKit.{h, mm} to TestRunnerShared. I am open to naming suggestions for these files as well as the function swizzleUIKit(), which does the actual swizzling. Let me know if it is preferred to make SwizzleUIKit a pure Objective-C file (.m) as opposed to an Objective-C++ file.

[Daniel Bates]

Assuming we like the approach of sharing the swizzling logic in TestRunnerShared I will move IOSLayoutTestCommunication.{h, cpp} into the Tools/TestRunnerShared/ios in a follow up bug.

[EWS Watchlist]

Comment on attachment 351256 [details]
Patch

Attachment 351256 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9412396

New failing tests:
media/range-extract-contents-crash.html

[EWS Watchlist]

Created attachment 351278 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Darin Adler]

Comment on attachment 351256 [details]
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.

[Daniel Bates]

(In reply to Darin Adler from comment #5)
> Comment on attachment 351256 [details]
> 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.

Fair enough. Based on this change I no longer see the value of sharing the "swizzle" code between test runners and I will inline the call to method_setImplementation() into TestController::platformInitialize() and prepareConsistentTestingEnvironment() (defined in DumpRenderTree.mm).


> 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?

Will remove this as it is not needed. For some reason when I wrote this patch I ran into some issue that led me to make this change. I am able to compile without this change.

> 
> > 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?

As aforementioned above, I am no longer going to share the "swizzle" code between test runners and hence will not add a new source file.

[Daniel Bates]

Created attachment 353906 [details]
To Land

[Daniel Bates]

(In reply to Daniel Bates from comment #6)
> > 
> > > Tools/TestRunnerShared/spi/UIKitTestSPI.h:66
> > > ++ (BOOL)isInHardwareKeyboardMode;
> > 
> > Why did we need to add this?
> 
> Will remove this as it is not needed. For some reason when I wrote this
> patch I ran into some issue that led me to make this change. I am able to
> compile without this change.
> 

iOS EWS reminds me why I did this:

/Volumes/Data/EWS/WebKit/Tools/WebKitTestRunner/ios/TestControllerIOS.mm:85:72: error: undeclared selector 'isInHardwareKeyboardMode' [-Werror,-Wundeclared-selector]
    method_setImplementation(class_getClassMethod([UIKeyboard class], @selector(isInHardwareKeyboardMode)), reinterpret_cast<IMP>(overrideIsInHardwareKeyboardMode));

[Daniel Bates]

Created attachment 353998 [details]
To Land

[Daniel Bates]

Comment on attachment 353998 [details]
To Land

Clearing flags on attachment: 353998

Committed r237935: <https://trac.webkit.org/changeset/237935>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45884871>