Bug 193683 - [iOS] Move calls to [UIKeyboard isInHardwareKeyboardMode] to the UI process.
Summary: [iOS] Move calls to [UIKeyboard isInHardwareKeyboardMode] to the UI process.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-22 14:07 PST by Per Arne Vollan
Modified: 2019-12-17 10:20 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.28 KB, patch)
2019-01-22 14:24 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.42 MB, application/zip)
2019-01-22 16:18 PST, EWS Watchlist
no flags Details
Patch (6.40 KB, patch)
2019-01-23 12:22 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (8.07 KB, patch)
2019-02-01 14:26 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (8.00 KB, patch)
2019-02-01 15:30 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (8.59 KB, patch)
2019-02-04 09:43 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (8.86 KB, patch)
2019-02-26 12:49 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (8.87 KB, patch)
2019-02-27 10:58 PST, Per Arne Vollan
bfulgham: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.88 MB, application/zip)
2019-02-27 12:59 PST, EWS Watchlist
no flags Details
Patch (8.87 KB, patch)
2019-02-28 11:53 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (8.87 KB, patch)
2019-02-28 12:28 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2019-01-22 14:07:20 PST
I think it would be preferable to have these calls in the UI process, not in the WebContent process.
Comment 1 Per Arne Vollan 2019-01-22 14:24:00 PST
Created attachment 359772 [details]
Patch
Comment 2 EWS Watchlist 2019-01-22 16:18:43 PST
Comment on attachment 359772 [details]
Patch

Attachment 359772 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10845868

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html
Comment 3 EWS Watchlist 2019-01-22 16:18:44 PST
Created attachment 359802 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 4 Per Arne Vollan 2019-01-23 11:12:21 PST
I think the ios-sim crash is unrelated to this patch. This is the crash log:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.CoreFoundation      	0x0000000110d0d140 CFBasicHashRemoveValue + 800
1   com.apple.CoreFoundation      	0x0000000110bd64f8 CFDictionaryRemoveValue + 152
2   com.apple.VideoToolbox        	0x000000069147efe7 VTPixelTransferSessionTransferImage + 5581
3   com.apple.VideoToolbox        	0x00000006914e7814 VTPixelBufferConformerCopyConformedPixelBuffer + 2068
4   com.apple.WebCore             	0x0000000117c30cc5 WebCore::PixelBufferConformerCV::convert(__CVBuffer*) + 117 (PixelBufferConformerCV.cpp:145)
5   com.apple.WebCore             	0x00000001169b7b79 WebCore::RealtimeOutgoingVideoSourceCocoa::convertToYUV(__CVBuffer*) + 233 (RealtimeOutgoingVideoSourceCocoa.mm:49)
6   com.apple.WebCore             	0x0000000117c98d7e WebCore::RealtimeOutgoingVideoSourceCocoa::sampleBufferUpdated(WebCore::MediaStreamTrackPrivate&, WebCore::MediaSample&) + 334 (utility:896)
7   com.apple.WebCore             	0x0000000117c78427 WebCore::MediaStreamTrackPrivate::forEachObserver(WTF::Function<void (WebCore::MediaStreamTrackPrivate::Observer&)> const&) const + 727 (Atomics.h:247)
8   com.apple.WebCore             	0x0000000117c78c56 WebCore::MediaStreamTrackPrivate::videoSampleAvailable(WebCore::MediaSample&) + 182 (memory:2285)
9   com.apple.WebCore             	0x0000000117c7a867 WebCore::RealtimeMediaSource::forEachObserver(WTF::Function<void (WebCore::RealtimeMediaSource::Observer&)> const&) const + 727 (Atomics.h:247)
10  com.apple.WebCore             	0x0000000117c7aabb WebCore::RealtimeMediaSource::videoSampleAvailable(WebCore::MediaSample&) + 59 (memory:2285)
11  com.apple.WebCore             	0x0000000117225066 WebCore::CanvasCaptureMediaStreamTrack::Source::captureCanvas() + 86 (utility:896)
12  com.apple.WebCore             	0x0000000117b623bd WebCore::ThreadTimers::sharedTimerFiredInternal() + 173 (ThreadTimers.cpp:132)
13  com.apple.WebCore             	0x0000000117b86cff WebCore::timerFired(__CFRunLoopTimer*, void*) + 31 (MainThreadSharedTimerCF.cpp:75)
14  com.apple.CoreFoundation      	0x0000000110c1a344 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
15  com.apple.CoreFoundation      	0x0000000110c19f42 __CFRunLoopDoTimer + 1026
16  com.apple.CoreFoundation      	0x0000000110c197aa __CFRunLoopDoTimers + 266
17  com.apple.CoreFoundation      	0x0000000110c13e2c __CFRunLoopRun + 2252
18  com.apple.CoreFoundation      	0x0000000110c13221 CFRunLoopRunSpecific + 625
19  com.apple.Foundation          	0x000000010ec0a522 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 277
20  com.apple.Foundation          	0x000000010ec0a692 -[NSRunLoop(NSRunLoop) run] + 76
21  libxpc.dylib                  	0x00000001128c6812 _xpc_objc_main + 460
22  libxpc.dylib                  	0x00000001128c8cbd xpc_main + 143
23  com.apple.WebKit.WebContent   	0x000000010eb4e248 WebKit::XPCServiceMain(int, char const**) + 403 (OSObjectPtr.h:69)
24  com.apple.WebKit.WebContent   	0x000000010eb4e3e9 main + 9 (XPCServiceMain.mm:165)
25  libdyld.dylib                 	0x00000001125be551 start + 1
Comment 5 Daniel Bates 2019-01-23 11:28:12 PST
Comment on attachment 359772 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.h:680
> +    void hardwareKeyboardAvailabilityChanged(bool keyboardAttached);

No change required, but something about this name feels weird. I could be recalling how we order adjectives wrong. Do we put the adjective at the beginning or end? Anything in style guide? Cocoa naming conventions?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3122
> +static bool sKeyboardAttached = false;

No need for the initialization. Is this named correctly? For some reason I seem to recall "s_"-prefix, but maybe this is for class variables. BOOL instead of bool for consistency with overrideIsInHardwareKeyboardMode.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3134
> +    sKeyboardAttached = keyboardAttached;

This should be updated before we call capsLockStateMayHaveChanged(). Otherwise, capsLockStateMayHaveChanged() and code it calls do not have up-to-date state. May not effect the caps lock logic now, but could in the future.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3135
> +    method_setImplementation(class_getClassMethod([UIKeyboard class], @selector(isInHardwareKeyboardMode)), reinterpret_cast<IMP>(overrideIsInHardwareKeyboardMode));

We replace the implementation on each invocation. Could we just put this somewhere to do this once replacement once?
Comment 6 Per Arne Vollan 2019-01-23 11:56:04 PST
(In reply to Daniel Bates from comment #5)
> Comment on attachment 359772 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359772&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.h:680
> > +    void hardwareKeyboardAvailabilityChanged(bool keyboardAttached);
> 
> No change required, but something about this name feels weird. I could be
> recalling how we order adjectives wrong. Do we put the adjective at the
> beginning or end? Anything in style guide? Cocoa naming conventions?
> 
> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3122
> > +static bool sKeyboardAttached = false;
> 
> No need for the initialization. Is this named correctly? For some reason I
> seem to recall "s_"-prefix, but maybe this is for class variables. BOOL
> instead of bool for consistency with overrideIsInHardwareKeyboardMode.
> 
> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3134
> > +    sKeyboardAttached = keyboardAttached;
> 
> This should be updated before we call capsLockStateMayHaveChanged().
> Otherwise, capsLockStateMayHaveChanged() and code it calls do not have
> up-to-date state. May not effect the caps lock logic now, but could in the
> future.
> 
> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3135
> > +    method_setImplementation(class_getClassMethod([UIKeyboard class], @selector(isInHardwareKeyboardMode)), reinterpret_cast<IMP>(overrideIsInHardwareKeyboardMode));
> 
> We replace the implementation on each invocation. Could we just put this
> somewhere to do this once replacement once?

Thanks for reviewing, Dan! I will address these issues in a new patch.
Comment 7 Per Arne Vollan 2019-01-23 12:22:11 PST
Created attachment 359920 [details]
Patch
Comment 8 Per Arne Vollan 2019-02-01 14:26:23 PST
Created attachment 360902 [details]
Patch
Comment 9 Per Arne Vollan 2019-02-01 15:30:15 PST
Created attachment 360912 [details]
Patch
Comment 10 Brent Fulgham 2019-02-01 15:54:00 PST
Comment on attachment 360912 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4613
> +extern bool keyboardIsAttached;

I don't think this is needed.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4658
> +            || keyboardIsAttached

Why do we need this change? This lives in UIProcess.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:65
> +bool keyboardIsAttached;

I don't think this is needed.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:677
> +    keyboardIsAttached = [UIKeyboard isInHardwareKeyboardMode];

Then this could be a local bool.
Comment 11 Per Arne Vollan 2019-02-04 09:43:56 PST
Created attachment 361064 [details]
Patch
Comment 12 Per Arne Vollan 2019-02-04 09:47:06 PST
(In reply to Brent Fulgham from comment #10)
> Comment on attachment 360912 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360912&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4613
> > +extern bool keyboardIsAttached;
> 
> I don't think this is needed.
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4658
> > +            || keyboardIsAttached
> 
> Why do we need this change? This lives in UIProcess.
> 

It seems the call to[UIKeyboard isInHardwareKeyboardMode] is expensive. Maybe it will be a performance improvement to cache the value in the UI process?

> > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:65
> > +bool keyboardIsAttached;
> 
> I don't think this is needed.
> 
> > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:677
> > +    keyboardIsAttached = [UIKeyboard isInHardwareKeyboardMode];
> 
> Then this could be a local bool.

Thanks for reviewing!
Comment 13 Per Arne Vollan 2019-02-26 12:49:48 PST
Created attachment 363007 [details]
Patch
Comment 14 Per Arne Vollan 2019-02-27 10:55:11 PST
rdar://problem/47634345
Comment 15 Per Arne Vollan 2019-02-27 10:58:12 PST
Created attachment 363107 [details]
Patch
Comment 16 Brent Fulgham 2019-02-27 11:03:06 PST
Comment on attachment 363107 [details]
Patch

r=me (assuming EWS is happy)
Comment 17 EWS Watchlist 2019-02-27 12:59:34 PST
Comment on attachment 363107 [details]
Patch

Attachment 363107 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11306081

New failing tests:
js/dom/custom-constructors.html
Comment 18 EWS Watchlist 2019-02-27 12:59:46 PST
Created attachment 363119 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 19 Per Arne Vollan 2019-02-28 11:53:35 PST
Created attachment 363246 [details]
Patch
Comment 20 Per Arne Vollan 2019-02-28 12:28:39 PST
Created attachment 363249 [details]
Patch
Comment 21 Per Arne Vollan 2019-02-28 13:13:49 PST
Comment on attachment 363249 [details]
Patch

Thanks for reviewing!
Comment 22 WebKit Commit Bot 2019-02-28 13:39:58 PST
Comment on attachment 363249 [details]
Patch

Clearing flags on attachment: 363249

Committed r242222: <https://trac.webkit.org/changeset/242222>
Comment 23 Truitt Savell 2019-03-04 15:12:03 PST
Reverted r242222 for reason:

Broke internal debug testing

Committed r242396: <https://trac.webkit.org/changeset/242396>
Comment 24 Truitt Savell 2019-03-04 16:09:20 PST
Reverted r242396 for reason:

Found issue to be unrelated. reverting my rollout.

Committed r242400: <https://trac.webkit.org/changeset/242400>