Bug 234440 – Add support for a UI delegate method to decide how to handle detected modal containers

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

.

[Wenson Hsieh]

Created attachment 447472 [details]
Patch

[Wenson Hsieh]

Created attachment 447667 [details]
Depends on #234322

[Wenson Hsieh]

Created attachment 447678 [details]
Fix Windows build

[Darin Adler]

Comment on attachment 447678 [details]
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().

[Wenson Hsieh]

Comment on attachment 447678 [details]
Fix Windows build

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

Thanks for the review!

>> Source/WebCore/loader/DocumentLoader.h:151
>>  };
> 
> I really prefer that such simple enums are done as one liners instead of vertically. Possibly others don’t agree.

Makes sense! Changed to `enum class ModalContainerObservationPolicy : bool { Disabled, Prompt };`.

>> Source/WebCore/page/ModalContainerObserver.cpp:314
>> +                }
> 
> This can just be:
> 
>     ClassifiedControls(ClassifiedControls&&) = default;
> 
> But also, if you just omit all constructors you should get these without writing anything.

Good catch — removed these constructors.

>> Source/WebCore/page/ModalContainerObserver.cpp:326
>> +                        break;
> 
> Idiomatic to just share a break.

Fixed!

>> 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().

That's true — renamed to just `types()`.

[Wenson Hsieh]

rdar://77073735

[Wenson Hsieh]

Created attachment 447719 [details]
For EWS

[Wenson Hsieh]

Created attachment 447721 [details]
Fix Windows build

[EWS]

Committed r287321 (245471@main): <https://commits.webkit.org/245471@main>

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