I think it would be preferable to have these calls in the UI process, not in the WebContent process.
Created attachment 359772 [details] Patch
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
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
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 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?
(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.
Created attachment 359920 [details] Patch
Created attachment 360902 [details] Patch
Created attachment 360912 [details] Patch
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.
Created attachment 361064 [details] Patch
(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!
Created attachment 363007 [details] Patch
rdar://problem/47634345
Created attachment 363107 [details] Patch
Comment on attachment 363107 [details] Patch r=me (assuming EWS is happy)
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
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
Created attachment 363246 [details] Patch
Created attachment 363249 [details] Patch
Comment on attachment 363249 [details] Patch Thanks for reviewing!
Comment on attachment 363249 [details] Patch Clearing flags on attachment: 363249 Committed r242222: <https://trac.webkit.org/changeset/242222>
Reverted r242222 for reason: Broke internal debug testing Committed r242396: <https://trac.webkit.org/changeset/242396>
Reverted r242396 for reason: Found issue to be unrelated. reverting my rollout. Committed r242400: <https://trac.webkit.org/changeset/242400>