Bug 193683 – [iOS] Move calls to [UIKeyboard isInHardwareKeyboardMode] to the UI process.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Per Arne Vollan]

I think it would be preferable to have these calls in the UI process, not in the WebContent process.

[Per Arne Vollan]

Created attachment 359772 [details]
Patch

[EWS Watchlist]

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]

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]

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

(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]

Created attachment 359920 [details]
Patch

[Per Arne Vollan]

Created attachment 360902 [details]
Patch

[Per Arne Vollan]

Created attachment 360912 [details]
Patch

[Brent Fulgham]

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]

Created attachment 361064 [details]
Patch

[Per Arne Vollan]

(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]

Created attachment 363007 [details]
Patch

[Per Arne Vollan]

rdar://problem/47634345

[Per Arne Vollan]

Created attachment 363107 [details]
Patch

[Brent Fulgham]

Comment on attachment 363107 [details]
Patch

r=me (assuming EWS is happy)

[EWS Watchlist]

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]

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]

Created attachment 363246 [details]
Patch

[Per Arne Vollan]

Created attachment 363249 [details]
Patch

[Per Arne Vollan]

Comment on attachment 363249 [details]
Patch

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 363249 [details]
Patch

Clearing flags on attachment: 363249

Committed r242222: <https://trac.webkit.org/changeset/242222>

[Truitt Savell]

Reverted r242222 for reason:

Broke internal debug testing

Committed r242396: <https://trac.webkit.org/changeset/242396>

[Truitt Savell]

Reverted r242396 for reason:

Found issue to be unrelated. reverting my rollout.

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