Bug 234323 - Adopt ChromeClient::classifyModalContainerControls() in ModalContainerObserver
Summary: Adopt ChromeClient::classifyModalContainerControls() in ModalContainerObserver
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:
Blocks: 234440
  Show dependency treegraph
 
Reported: 2021-12-14 15:50 PST by Wenson Hsieh
Modified: 2021-12-20 15:03 PST (History)
8 users (show)

See Also:


Attachments
Patch (5.88 KB, patch)
2021-12-17 12:59 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix title (5.87 KB, patch)
2021-12-17 13:05 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
For EWS (5.94 KB, patch)
2021-12-19 15:23 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
For EWS (5.95 KB, patch)
2021-12-20 13:31 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
For EWS (fixed) (5.96 KB, patch)
2021-12-20 13:47 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
For EWS (actually fixed?) (6.48 KB, patch)
2021-12-20 13:56 PST, Wenson Hsieh
no flags 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-14 15:50:21 PST
.
Comment 1 Wenson Hsieh 2021-12-17 12:59:42 PST Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-12-17 13:05:43 PST Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2021-12-19 15:23:24 PST
Created attachment 447563 [details]
For EWS
Comment 4 Devin Rousso 2021-12-20 13:06:36 PST
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?
Comment 5 Wenson Hsieh 2021-12-20 13:13:12 PST
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.
Comment 6 Wenson Hsieh 2021-12-20 13:31:06 PST Comment hidden (obsolete)
Comment 7 Wenson Hsieh 2021-12-20 13:47:11 PST Comment hidden (obsolete)
Comment 8 Wenson Hsieh 2021-12-20 13:56:34 PST
Created attachment 447635 [details]
For EWS (actually fixed?)
Comment 9 EWS 2021-12-20 15:02:42 PST
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].
Comment 10 Radar WebKit Bug Importer 2021-12-20 15:03:15 PST
<rdar://problem/86742167>