Bug 234440 - Add support for a UI delegate method to decide how to handle detected modal containers
Summary: Add support for a UI delegate method to decide how to handle detected modal c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on: 234322 234323
Blocks:
  Show dependency treegraph
 
Reported: 2021-12-17 13:08 PST by Wenson Hsieh
Modified: 2021-12-21 12:09 PST (History)
16 users (show)

See Also:


Attachments
Patch (59.13 KB, patch)
2021-12-17 13:43 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Depends on #234322 (60.17 KB, patch)
2021-12-20 17:14 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix Windows build (62.67 KB, patch)
2021-12-20 20:20 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
For EWS (63.06 KB, patch)
2021-12-21 08:46 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix Windows build (63.81 KB, patch)
2021-12-21 09:09 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2021-12-17 13:08:42 PST
.
Comment 1 Wenson Hsieh 2021-12-17 13:43:06 PST Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-12-20 17:14:40 PST Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2021-12-20 20:20:30 PST
Created attachment 447678 [details]
Fix Windows build
Comment 4 Darin Adler 2021-12-21 01:26:23 PST
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().
Comment 5 Wenson Hsieh 2021-12-21 08:20:15 PST
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()`.
Comment 6 Wenson Hsieh 2021-12-21 08:24:32 PST
rdar://77073735
Comment 7 Wenson Hsieh 2021-12-21 08:46:37 PST Comment hidden (obsolete)
Comment 8 Wenson Hsieh 2021-12-21 09:09:02 PST
Created attachment 447721 [details]
Fix Windows build
Comment 9 EWS 2021-12-21 10:18:54 PST
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].