[webkit-reviews] review granted: [Bug 234440] Add support for a UI delegate method to decide how to handle detected modal containers : [Attachment 447678] Fix Windows build

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 234440: Add support for a UI delegate method to decide how to handle
detected modal containers
https://bugs.webkit.org/show_bug.cgi?id=234440

Attachment 447678: Fix Windows build

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 447678
  --> https://bugs.webkit.org/attachment.cgi?id=447678
Fix Windows build

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

> Source/WebCore/loader/DocumentLoader.h:151
> -enum class ModalContainerObservationPolicy : uint8_t {
> +enum class ModalContainerObservationPolicy : bool {
>      Disabled,
>      Prompt,
> -    Allow,
> -    Disallow,
>  };

I really prefer that such simple enums are done as one liners instead of
vertically. Possibly others don’t agree.

> Source/WebCore/page/ModalContainerObserver.cpp:314
> +		   ClassifiedControls(ClassifiedControls&& other)
> +		       : positive(WTFMove(other.positive))
> +		       , neutral(WTFMove(other.neutral))
> +		       , negative(WTFMove(other.negative))
> +		   {
> +		   }

This can just be:

    ClassifiedControls(ClassifiedControls&&) = default;

But also, if you just omit all constructors you should get these without
writing anything.

> Source/WebCore/page/ModalContainerObserver.cpp:326
> +		       case ModalContainerDecision::Show:
> +			   break;
> +		       case ModalContainerDecision::HideAndIgnore:
> +			   break;

Idiomatic to just share a break.

> Source/WebCore/page/ModalContainerObserver.cpp:341
> +		   OptionSet<ModalContainerControlType> controlTypes() const

In the context of a class named ClassifiedControls, seems this can just be
called types().