Bug 190141 - Override +[UIKeyboard isInHardwareKeyboardMode] in WebKitTestRunner and DumpRenderTree
Summary: Override +[UIKeyboard isInHardwareKeyboardMode] in WebKitTestRunner and DumpR...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad iOS 12
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks: 190571
  Show dependency treegraph
 
Reported: 2018-10-01 09:47 PDT by Daniel Bates
Modified: 2018-11-07 12:14 PST (History)
6 users (show)

See Also:


Attachments
Patch (25.09 KB, patch)
2018-10-01 10:06 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.14 MB, application/zip)
2018-10-01 11:16 PDT, EWS Watchlist
no flags Details
To Land (10.87 KB, patch)
2018-11-05 13:57 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land (11.40 KB, patch)
2018-11-06 13:48 PST, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2018-10-01 09:47:45 PDT
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].
Comment 1 Daniel Bates 2018-10-01 10:06:17 PDT
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.
Comment 2 Daniel Bates 2018-10-01 10:08:56 PDT
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.
Comment 3 EWS Watchlist 2018-10-01 11:16:38 PDT
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
Comment 4 EWS Watchlist 2018-10-01 11:16:39 PDT
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
Comment 5 Darin Adler 2018-10-02 18:58:04 PDT
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.
Comment 6 Daniel Bates 2018-11-05 13:49:00 PST
(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.
Comment 7 Daniel Bates 2018-11-05 13:57:40 PST
Created attachment 353906 [details]
To Land
Comment 8 Daniel Bates 2018-11-06 13:42:53 PST
(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));
Comment 9 Daniel Bates 2018-11-06 13:48:30 PST
Created attachment 353998 [details]
To Land
Comment 10 Daniel Bates 2018-11-07 12:12:53 PST
Comment on attachment 353998 [details]
To Land

Clearing flags on attachment: 353998

Committed r237935: <https://trac.webkit.org/changeset/237935>
Comment 11 Daniel Bates 2018-11-07 12:12:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2018-11-07 12:14:11 PST
<rdar://problem/45884871>