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 tree / graph
 
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
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

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>