Bug 212580

<rdar://problem/63813283>

Created attachment 410855 [details] Patch

Created attachment 410856 [details] Patch add log when `changeTimeInterval` is overridden by `NSUserDefaults`

Comment on attachment 410856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410856&action=review > Source/WebKit/ChangeLog:16 > + Use `+[UIScribbleInteraction isPencilInputExpected]` as a proxy for the presence of stylus > + devices as there's no way of directly querying for connected stylus devices (especially if > + there is no active connection). When the value is changed to `YES`, notify all WebProcess > + immediately. When the value is changed to `NO`, use an `NSTimer` to delay notifying all > + WebProcess for 10min in case the user starts using their stylus again. > + > + For testing/debugging purposes this 10min timeout can be adjusted before the UIProcess is > + created with the `WKStylusDeviceObserverChangeTimeInterval` key in `NSUserDefaults`. I think this can and should be generalized for all platforms that support styluses. While the implementation to check for a stylus on iOS can't be shared, it would be good to create a more general pipeline for passing this information down to the WebProcess that is not iOS specific like this. Additionally, please avoid using NSUserDefaults directly for testing. Rather, please add a new preference to the WebPreferences yaml files for your needs. This allows us to better keep track of all the settings in one place, rather than having them scattered around in different places.

Comment on attachment 410856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410856&action=review >> Source/WebKit/ChangeLog:16 >> + created with the `WKStylusDeviceObserverChangeTimeInterval` key in `NSUserDefaults`. > > I think this can and should be generalized for all platforms that support styluses. While the implementation to check for a stylus on iOS can't be shared, it would be good to create a more general pipeline for passing this information down to the WebProcess that is not iOS specific like this. > > Additionally, please avoid using NSUserDefaults directly for testing. Rather, please add a new preference to the WebPreferences yaml files for your needs. This allows us to better keep track of all the settings in one place, rather than having them scattered around in different places. Could you give an example of what you mean by "generalized for all platforms"? My thinking is that any other platform that wants to add this UIProcess->WebProcess communication of "we now have a stylus" (and the opposite) would adjust the `#if HAVE(PENCILKIT_TEXT_INPUT) && PLATFORM(IOS)` around `WebProcessProxy::notifyHasStylusDeviceChanged`, `WebProcess::setHasStylusDevice`, etc. and likely have it's own way of calling those functions based on it's own platform-specific way of knowing "we now have a stylus" (and the opposite). When I said "testing" I mean this more as a QA form of testing (i.e. "live on"), where we could set the default and see how it feels (I chose 10min as a sort of gut guesstimate rather than a value backed by some collected metric/statistic). The API tests included in this patch do not actually use `NSUserDefaults` for testing this behavior. I initially went with `NSUserDefaults` because I wanted something global as we only really expect `WKStylusDeviceObserver` to be used from it's `sharedInstance`. Is it possible for `WebPreferences` (or something similar) to be global and configurable like this, or is there perhaps another approach to getting the same outcome?

(In reply to Devin Rousso from comment #5) > Comment on attachment 410856 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410856&action=review > > >> Source/WebKit/ChangeLog:16 > >> + created with the `WKStylusDeviceObserverChangeTimeInterval` key in `NSUserDefaults`. > > > > I think this can and should be generalized for all platforms that support styluses. While the implementation to check for a stylus on iOS can't be shared, it would be good to create a more general pipeline for passing this information down to the WebProcess that is not iOS specific like this. > > > > Additionally, please avoid using NSUserDefaults directly for testing. Rather, please add a new preference to the WebPreferences yaml files for your needs. This allows us to better keep track of all the settings in one place, rather than having them scattered around in different places. > > Could you give an example of what you mean by "generalized for all > platforms"? My thinking is that any other platform that wants to add this > UIProcess->WebProcess communication of "we now have a stylus" (and the > opposite) would adjust the `#if HAVE(PENCILKIT_TEXT_INPUT) && PLATFORM(IOS)` > around `WebProcessProxy::notifyHasStylusDeviceChanged`, > `WebProcess::setHasStylusDevice`, etc. and likely have it's own way of > calling those functions based on it's own platform-specific way of knowing > "we now have a stylus" (and the opposite). Pretty sure he means "leave the plumbing in place on all platforms, so they just have to add /callers/ of it". The plumbing will all be the same.

Created attachment 410884 [details] Patch

Comment on attachment 410884 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410884&action=review > Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:45 > + if (WebProcessProxy::allProcesses().size() == 1) { > #if HAVE(UIKIT_WITH_MOUSE_SUPPORT) && PLATFORM(IOS) > - if (WebProcessProxy::allProcesses().size() == 1) > [[WKMouseDeviceObserver sharedInstance] start]; > #endif > +#if HAVE(PENCILKIT_TEXT_INPUT) && PLATFORM(IOS) I don't think the PLATFORM(IOS) checks are needed here, since this file should only be compiled on iOS platforms. > Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:59 > +#if HAVE(PENCILKIT_TEXT_INPUT) && PLATFORM(IOS) > + [[WKStylusDeviceObserver sharedInstance] stop]; > +#endif This seems like an error prone way to use a shared observer. If any other client ever get's created, it will be confusing that this call site completely stops it. A common pattern for things like this is to have the shared instance maintain a reference count, and rather than explicitly starting and stoping the observer, the clients increment / decrement the count, and when it reaches 0, it stops observing.

Comment on attachment 410884 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410884&action=review >> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:45 >> +#if HAVE(PENCILKIT_TEXT_INPUT) && PLATFORM(IOS) > > I don't think the PLATFORM(IOS) checks are needed here, since this file should only be compiled on iOS platforms. Is there a way to connect a stylus/pencil to a simulator? I didn't think there was, so I figured it wouldn't be desirable, but I suppose it couldn't hurt to have it. >> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:59 >> +#endif > > This seems like an error prone way to use a shared observer. If any other client ever get's created, it will be confusing that this call site completely stops it. > > A common pattern for things like this is to have the shared instance maintain a reference count, and rather than explicitly starting and stoping the observer, the clients increment / decrement the count, and when it reaches 0, it stops observing. Wow yeah I dunno why I didn't do that :| I'll adjust `WKMouseDeviceObserver` too while im at it :)

Comment on attachment 410884 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410884&action=review >>> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:45 >>> +#if HAVE(PENCILKIT_TEXT_INPUT) && PLATFORM(IOS) >> >> I don't think the PLATFORM(IOS) checks are needed here, since this file should only be compiled on iOS platforms. > > Is there a way to connect a stylus/pencil to a simulator? I didn't think there was, so I figured it wouldn't be desirable, but I suppose it couldn't hurt to have it. err, s/simulator/catalyst

Created attachment 410970 [details] Patch

(In reply to Devin Rousso from comment #9) > Comment on attachment 410884 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410884&action=review > > >> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:45 > >> +#if HAVE(PENCILKIT_TEXT_INPUT) && PLATFORM(IOS) > > > > I don't think the PLATFORM(IOS) checks are needed here, since this file should only be compiled on iOS platforms. > > Is there a way to connect a stylus/pencil to a simulator? I didn't think > there was, so I figured it wouldn't be desirable, but I suppose it couldn't > hurt to have it. PLATFORM(IOS) is true for both the simulator and hardware.

Created attachment 410972 [details] Patch rebase

Created attachment 411132 [details] Patch add another test for when the stylus disconnected timeout should be cancelled (e.g. a stylus is reconnected within the 10min window)

Comment on attachment 411132 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411132&action=review > Source/WebKit/Shared/WebProcessCreationParameters.cpp:91 > + encoder << hasStylusDevice; Probably can drop the word "device" everywhere :) > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:43 > #import "WKMouseDeviceObserver.h" But you've got it for mouse too so maybe not > Source/WebKit/UIProcess/ios/WKMouseDeviceObserver.mm:50 > + while (_startCount) Looping in dealloc is /very weird/. Just shut it down! > Source/WebKit/UIProcess/ios/WKStylusDeviceObserver.mm:36 > +static Seconds s_changeTimeInterval { 10_min }; I think the s_ rule from the style guide (which you might be following here) is about static data members; this should just be 'changeTimeInterval' > Source/WebKit/UIProcess/ios/WKStylusDeviceObserver.mm:69 > + while (_startCount) Ditto from the other place > Source/WebKit/UIProcess/ios/WKStylusDeviceObserver.mm:77 > +- (void)_setHasStylusDevice:(BOOL)hasStylusDevice Don't need leading underscores for methods on internal classes, only exported ones

Comment on attachment 411132 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411132&action=review >> Source/WebKit/UIProcess/ios/WKMouseDeviceObserver.mm:50 >> + while (_startCount) > > Looping in dealloc is /very weird/. Just shut it down! Also, do we ever expect this -dealloc method to be hit?

Comment on attachment 411132 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411132&action=review >>> Source/WebKit/UIProcess/ios/WKMouseDeviceObserver.mm:50 >>> + while (_startCount) >> >> Looping in dealloc is /very weird/. Just shut it down! > > Also, do we ever expect this -dealloc method to be hit? I was trying to avoid duplicating the unregister logic, but yeah this is maybe a bit too odd. I'll just copy it :) No, this is not expected to be called (at least so long as the UIProcess exists).

Created attachment 411154 [details] Patch

Comment on attachment 411154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411154&action=review > Source/WebKit/UIProcess/ios/WKStylusDeviceObserver.mm:67 > +- (void)dealloc I still think we should just remove these `-dealloc`s, since it's dead code. > Tools/TestWebKitAPI/Tests/WebKitCocoa/iOSStylusSupport.mm:173 > + WKStylusDeviceObserver *stylusDeviceObserver = [NSClassFromString(@"WKStylusDeviceObserver") sharedInstance]; > + [stylusDeviceObserver start]; > + stylusDeviceObserver.hasStylusDevice = YES; > + > + auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]); > + > + [webView synchronouslyLoadHTMLString:@""]; > + > + [stylusDeviceObserver startChangeTimer:0.01]; Nit - it might be a bit cleaner if you pulled this test setup logic out into a separate helper function.

Comment on attachment 411154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411154&action=review >> Source/WebKit/UIProcess/ios/WKStylusDeviceObserver.mm:67 >> +- (void)dealloc > > I still think we should just remove these `-dealloc`s, since it's dead code. oh yeah that's fine I can remove this i'll add this too ``` + (instancetype)new NS_UNAVAILABLE; - (instancetype)init NS_UNAVAILABLE; ``` to be extra clear :) >> Tools/TestWebKitAPI/Tests/WebKitCocoa/iOSStylusSupport.mm:173 >> + [stylusDeviceObserver startChangeTimer:0.01]; > > Nit - it might be a bit cleaner if you pulled this test setup logic out into a separate helper function. good idea

Created attachment 411184 [details] Patch

Committed r268384: <https://trac.webkit.org/changeset/268384> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411184 [details].