Bug 212060 – Hide password echo when screen is being captured.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Hide password echo when screen is being captured.

[Megan Gardner]

Created attachment 399708 [details]
Patch

[Megan Gardner]

<rdar://problem/47653578>

[Tim Horton]

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

[Tim Horton]

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.

[Megan Gardner]

Created attachment 399797 [details]
Patch

[Megan Gardner]

Created attachment 399801 [details]
Patch

[Wenson Hsieh]

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`.

[Wenson Hsieh]

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.

[Wenson Hsieh]

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?)

[Simon Fraser (smfr)]

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?

[Megan Gardner]

Created attachment 399867 [details]
Patch

[Megan Gardner]

Created attachment 399883 [details]
Patch for landing

[Simon Fraser (smfr)]

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.

[Megan Gardner]

Created attachment 399896 [details]
Patch for landing

[EWS]

Committed r261966: <https://trac.webkit.org/changeset/261966>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399896 [details].

[Darin Adler]

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.

[Megan Gardner]

Post commit comments address in https://trac.webkit.org/changeset/262272/webkit