RESOLVED FIXED 193683
[iOS] Move calls to [UIKeyboard isInHardwareKeyboardMode] to the UI process.
https://bugs.webkit.org/show_bug.cgi?id=193683
Summary [iOS] Move calls to [UIKeyboard isInHardwareKeyboardMode] to the UI process.
Per Arne Vollan
Reported 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.
Attachments
Patch (6.28 KB, patch)
2019-01-22 14:24 PST, Per Arne Vollan
no flags
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
Patch (6.40 KB, patch)
2019-01-23 12:22 PST, Per Arne Vollan
no flags
Patch (8.07 KB, patch)
2019-02-01 14:26 PST, Per Arne Vollan
no flags
Patch (8.00 KB, patch)
2019-02-01 15:30 PST, Per Arne Vollan
no flags
Patch (8.59 KB, patch)
2019-02-04 09:43 PST, Per Arne Vollan
no flags
Patch (8.86 KB, patch)
2019-02-26 12:49 PST, Per Arne Vollan
no flags
Patch (8.87 KB, patch)
2019-02-27 10:58 PST, Per Arne Vollan
bfulgham: review+
ews-watchlist: commit-queue-
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
Patch (8.87 KB, patch)
2019-02-28 11:53 PST, Per Arne Vollan
no flags
Patch (8.87 KB, patch)
2019-02-28 12:28 PST, Per Arne Vollan
no flags
Per Arne Vollan
Comment 1 2019-01-22 14:24:00 PST
EWS Watchlist
Comment 2 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
EWS Watchlist
Comment 3 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
Per Arne Vollan
Comment 4 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
Daniel Bates
Comment 5 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?
Per Arne Vollan
Comment 6 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.
Per Arne Vollan
Comment 7 2019-01-23 12:22:11 PST
Per Arne Vollan
Comment 8 2019-02-01 14:26:23 PST
Per Arne Vollan
Comment 9 2019-02-01 15:30:15 PST
Brent Fulgham
Comment 10 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.
Per Arne Vollan
Comment 11 2019-02-04 09:43:56 PST
Per Arne Vollan
Comment 12 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!
Per Arne Vollan
Comment 13 2019-02-26 12:49:48 PST
Per Arne Vollan
Comment 14 2019-02-27 10:55:11 PST
Per Arne Vollan
Comment 15 2019-02-27 10:58:12 PST
Brent Fulgham
Comment 16 2019-02-27 11:03:06 PST
Comment on attachment 363107 [details] Patch r=me (assuming EWS is happy)
EWS Watchlist
Comment 17 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
EWS Watchlist
Comment 18 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
Per Arne Vollan
Comment 19 2019-02-28 11:53:35 PST
Per Arne Vollan
Comment 20 2019-02-28 12:28:39 PST
Per Arne Vollan
Comment 21 2019-02-28 13:13:49 PST
Comment on attachment 363249 [details] Patch Thanks for reviewing!
WebKit Commit Bot
Comment 22 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>
Truitt Savell
Comment 23 2019-03-04 15:12:03 PST
Reverted r242222 for reason: Broke internal debug testing Committed r242396: <https://trac.webkit.org/changeset/242396>
Truitt Savell
Comment 24 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>
Note You need to log in before you can comment on or make changes to this bug.