[webkit-reviews] review denied: [Bug 171551] Allow forbidding javascript prompts : [Attachment 308825] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Claudio Saavedra
<csaavedra at igalia.com>'s request for review:
Bug 171551: Allow forbidding javascript prompts
https://bugs.webkit.org/show_bug.cgi?id=171551

Attachment 308825: Patch

https://bugs.webkit.org/attachment.cgi?id=308825&action=review




--- Comment #3 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 308825
  --> https://bugs.webkit.org/attachment.cgi?id=308825
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308825&action=review

Prompting is not built into WebKit by default. Instead, WebKit sends a delegate
message to the client, and then the client has the option to prompt. 

If a client doesn't want any prompting ever, isn't the best way to get that
behavior simply not to implement the delegate messages?

Or maybe you envision a client disabling prompts but only temporarily? If so,
under what conditions (other than page unload) do you expect clients to disable
prompts? And why do a client need WebKit to track those conditions (instead of
tracking them itself)?

> Source/WebCore/loader/FrameLoader.cpp:208
> +	   m_page->forbidPrompts(Page::PromptPolicy::DisabledByPageUnload);

I would rename this class to DisablePromptsForPageUnload and rename the enum
value DisabledForPageUnload.

It seems wrong for an abstract ForbidPromptsScope to assume that it is used
only for page unload.

> Source/WebCore/page/DOMWindow.cpp:1121
> +    if (page->arePromptsAllowed() ==
Page::PromptPolicy::DisabledByPageUnload) {

I would rename arePromptsAllowed to promptPolicy, since you've changed it to an
enum.

I think this comparison has a bug in it. In the DisabledByUser case, the
equality comparison will return false, and you'll still print. I believe you
could have caught this bug with a simple regression test. Can you add
regression tests? We usually require a regression test for every patch.

> Source/WebCore/page/DOMWindow.cpp:1155
> +    case Page::PromptPolicy::DisabledByUser:
>	   return;

I would name this case just "Disabled". There's nothing about the setting or
its behavior that ties to a user action.

> Source/WebCore/page/DOMWindow.cpp:1160
> +    default:
> +	   break;

This code is meaningless, since the default behavior of an unmatched switch is
to do nothing.

The net effect of writing this code is that you disable the compiler's ability
to verify that you've handled all possible enum values. That's bad. You should
add a case for Enabled and remove default.

> Source/WebCore/page/DOMWindow.cpp:1187
> +    default:
> +	   break;

Ditto.

> Source/WebCore/page/DOMWindow.cpp:1214
> +    default:
> +	   break;

Ditto.

> Source/WebCore/page/DOMWindow.cpp:2327
> +    if (page->arePromptsAllowed() ==
Page::PromptPolicy::DisabledByPageUnload) {

Same bug here as above.

> Source/WebCore/page/Page.cpp:2067
> +    default:
> +	   ASSERT_NOT_REACHED();

Same comment about default.

> Source/WebCore/page/Page.h:771
>      unsigned m_lastSpatialNavigationCandidatesCount;
>      unsigned m_forbidPromptsDepth;
> +    bool m_userDisabledSimplePrompts;

I would call m_forbidPromptsDepth "m_disablePromptsForPageUnloadDepth" and
m_userDisabledSimplePrompts "m_disablePrompts".

> Source/WebKit2/UIProcess/WebPageProxy.h:1668
> +    bool m_promptsForbidden { false };

Why do we need this extra state? Do we expect clients to needlessly call this
API repeatedly?