Bug 234323 – Adopt ChromeClient::classifyModalContainerControls() in ModalContainerObserver

• Component: Platform
• Status: RESOLVED FIXED

[Wenson Hsieh]

.

[Wenson Hsieh]

Created attachment 447470 [details]
Patch

[Wenson Hsieh]

Created attachment 447471 [details]
Fix title

[Wenson Hsieh]

Created attachment 447563 [details]
For EWS

[Devin Rousso]

Comment on attachment 447563 [details]
For EWS

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

r=me

> Source/WebCore/page/ModalContainerObserver.cpp:284
> +        page->chrome().client().classifyModalContainerControls(WTFMove(controlTextsToClassify), [weakDocument, observer, controls = WTFMove(classifiableControls)](auto&& types) {

NIT: Can/Should we `weakDocument = WTFMove(weakDocument)`?

> Source/WebCore/page/ModalContainerObserver.cpp:305
> +            Vector<Ref<HTMLElement>> positiveControls;
> +            Vector<Ref<HTMLElement>> neutralControls;
> +            Vector<Ref<HTMLElement>> negativeControls;

NIT: I was gonna suggest that instead of creating three separate `Vector` you could just do the iteration inside each of the `case` below to avoid these allocations, but I see in Bug 234440 that you do use these so I guess it's fine :)

> Source/WebCore/page/ModalContainerObserver.cpp:350
> +            observer->m_hasAttemptedToFulfillPolicy = true;

When is this set back to `false`?  Or is it a one-way state that can not be turned back off?

[Wenson Hsieh]

Comment on attachment 447563 [details]
For EWS

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

Thanks for the review!

>> Source/WebCore/page/ModalContainerObserver.cpp:284
>> +        page->chrome().client().classifyModalContainerControls(WTFMove(controlTextsToClassify), [weakDocument, observer, controls = WTFMove(classifiableControls)](auto&& types) {
> 
> NIT: Can/Should we `weakDocument = WTFMove(weakDocument)`?

Indeed! Changed to `weakDocument = WTFMove(weakDocument)`.

>> Source/WebCore/page/ModalContainerObserver.cpp:305
>> +            Vector<Ref<HTMLElement>> negativeControls;
> 
> NIT: I was gonna suggest that instead of creating three separate `Vector` you could just do the iteration inside each of the `case` below to avoid these allocations, but I see in Bug 234440 that you do use these so I guess it's fine :)

Ah, yes — this code is actually sort of temporary, until I land support for the final (well, more final :P) version of the client method in #234440

>> Source/WebCore/page/ModalContainerObserver.cpp:350
>> +            observer->m_hasAttemptedToFulfillPolicy = true;
> 
> When is this set back to `false`?  Or is it a one-way state that can not be turned back off?

Yes, that is the idea (a document *should* only need to observe modal containers once).

It's possible I'll need to adjust how this works in the future, but for now this one-way state should suffice.

[Wenson Hsieh]

Created attachment 447627 [details]
For EWS

[Wenson Hsieh]

Created attachment 447632 [details]
For EWS (fixed)

[Wenson Hsieh]

Created attachment 447635 [details]
For EWS (actually fixed?)

[EWS]

Committed r287281 (245436@main): <https://commits.webkit.org/245436@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/86742167>