WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
Per Arne Vollan
Comment 1
2019-01-22 14:24:00 PST
Created
attachment 359772
[details]
Patch
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
Created
attachment 359920
[details]
Patch
Per Arne Vollan
Comment 8
2019-02-01 14:26:23 PST
Created
attachment 360902
[details]
Patch
Per Arne Vollan
Comment 9
2019-02-01 15:30:15 PST
Created
attachment 360912
[details]
Patch
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
Created
attachment 361064
[details]
Patch
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
Created
attachment 363007
[details]
Patch
Per Arne Vollan
Comment 14
2019-02-27 10:55:11 PST
rdar://problem/47634345
Per Arne Vollan
Comment 15
2019-02-27 10:58:12 PST
Created
attachment 363107
[details]
Patch
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
Created
attachment 363246
[details]
Patch
Per Arne Vollan
Comment 20
2019-02-28 12:28:39 PST
Created
attachment 363249
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug