Summary: | Hide password echo when screen is being captured. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Megan Gardner <megan_gardner> | ||||||||||||||
Component: | New Bugs | Assignee: | Megan Gardner <megan_gardner> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | bdakin, bfulgham, darin, ews-watchlist, mifenton, simon.fraser, thorton, wenson_hsieh | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Megan Gardner
2020-05-18 20:45:25 PDT
Created attachment 399708 [details]
Patch
Comment on attachment 399708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399708&action=review > Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:56 > + bool screenIsCaptured = document().editor().client()->isScreenCaptured(); IMO the editor client method should be named similarly to the setting (shouldEchoPassword or something) > Source/WebKit/UIProcess/ios/WKContentView.mm:172 > + _page->setIsScreenCaptured([[UIScreen mainScreen] isCaptured]); I’m not sure it makes sense to mirror the main screen’s state, we should send the message (with the right window’s state) when we’re parented instead. > Source/WebKit/UIProcess/ios/WKContentView.mm:778 > + _page->setIsScreenCaptured([[UIScreen mainScreen] isCaptured]); Should always be using the WebView or content view screen Also, nothing is pushing the right state in the process swap or crash case. You should add a bit to WebPageCreationParameters that, if parented, provides the current window’s state. Created attachment 399797 [details]
Patch
Created attachment 399801 [details]
Patch
Comment on attachment 399801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399801&action=review > Source/WebCore/page/EditorClient.h:74 > + virtual bool surpressPasswordEcho() const { return false; }; Nit - `suppressPasswordEcho`. > Source/WebKitLegacy/ios/DefaultDelegates/WebDefaultUIKitDelegate.m:271 > +- (BOOL)surpressPasswordEcho Nit - `suppressPasswordEcho`. Comment on attachment 399801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399801&action=review > Source/WebKit/UIProcess/ios/WKContentView.mm:776 > +- (void)_screenCapturedDidChange:(NSNotification*)notification Nit - space before the *. > Source/WebKit/WebProcess/WebPage/WebPage.h:2002 > + WebCore::FloatSize m_screenCaptured; I think you meant to delete this. Comment on attachment 399801 [details]
Patch
Oh also — is UIScreenCapturedDidChangeNotification dispatched when we’re unparented or parented? (e.g. what happens if the screen is being captured in a WKWebView, but then the web view is removed from the view hierarchy and added later after screen capture has ended?)
Comment on attachment 399801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399801&action=review > Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:56 > + bool suppressPasswordEcho = document().editor().client()->surpressPasswordEcho(); shouldSuppressPasswordEcho > Source/WebKit/UIProcess/WebPageProxy.h:722 > + void setIsScreenCaptured(bool); setScreenIsBeingCaptured. > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1583 > +void WebPageProxy::setIsScreenCaptured(bool screenMirroring) The dissonance between "captured" and "mirroring" is confusing. > Source/WebKit/WebProcess/WebCoreSupport/ios/WebEditorClientIOS.mm:122 > +bool WebEditorClient::surpressPasswordEcho() const shouldSurpressPasswordEcho > Source/WebKit/WebProcess/WebPage/WebPage.h:670 > + bool isScreenCaptured() const { return m_isScreenCaptured; } > + void setIsScreenCaptured(bool); isScreenBeingCaptured? > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4281 > +void WebPage::setIsScreenCaptured(bool captured) I would say "being captured" here. We're not capturing the screen, someone else is. "captured" is also vague here (easily confused with pointer capture). Can we say "being mirrored" instead? > Source/WebKitLegacy/ios/WebView/WebUIKitDelegate.h:131 > +- (BOOL)surpressPasswordEcho; Sounds like a verb. Should be isPasswordEchoSurpressed. > Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:833 > +bool WebEditorClient::surpressPasswordEcho() const isPasswordEchoSurpressed > Source/WebKitLegacy/mac/WebView/WebViewData.h:248 > + BOOL surpressPasswordEcho; isPasswordEchoSurpressed? Created attachment 399867 [details]
Patch
Created attachment 399883 [details]
Patch for landing
Comment on attachment 399883 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=399883&action=review > Source/WebKit/WebProcess/WebPage/WebPage.h:669 > + bool screenIsCaptured() const { return m_screenIsCaptured; } Missing a "being" here. Created attachment 399896 [details]
Patch for landing
Committed r261966: <https://trac.webkit.org/changeset/261966> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399896 [details]. Comment on attachment 399867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399867&action=review > Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:56 > + bool shouldSuppressPasswordEcho = document().editor().client()->shouldSuppressPasswordEcho(); I think this should be added to the passwordEchoEnabled expression, rather than put in a separate boolean. > Source/WebKitLegacy/mac/WebView/WebView.mm:1528 > + if ([[self _UIKitDelegateForwarder] respondsToSelector:@selector(shouldSuppressPasswordEcho)]) > + _private->shouldSuppressPasswordEcho = [[self _UIKitDelegateForwarder] shouldSuppressPasswordEcho]; I think this code should be removed. > Source/WebKitLegacy/mac/WebView/WebViewData.h:248 > + BOOL shouldSuppressPasswordEcho; I think this should be removed. It’s set but never read. Post commit comments address in https://trac.webkit.org/changeset/262272/webkit |