WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
234323
Adopt ChromeClient::classifyModalContainerControls() in ModalContainerObserver
https://bugs.webkit.org/show_bug.cgi?id=234323
Summary
Adopt ChromeClient::classifyModalContainerControls() in ModalContainerObserver
Wenson Hsieh
Reported
2021-12-14 15:50:21 PST
.
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 attachment
proposed patch, testcase, etc.
Wenson Hsieh
Comment 1
2021-12-17 12:59:42 PST
Comment hidden (obsolete)
Created
attachment 447470
[details]
Patch
Wenson Hsieh
Comment 2
2021-12-17 13:05:43 PST
Comment hidden (obsolete)
Created
attachment 447471
[details]
Fix title
Wenson Hsieh
Comment 3
2021-12-19 15:23:24 PST
Created
attachment 447563
[details]
For EWS
Devin Rousso
Comment 4
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?
Wenson Hsieh
Comment 5
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.
Wenson Hsieh
Comment 6
2021-12-20 13:31:06 PST
Comment hidden (obsolete)
Created
attachment 447627
[details]
For EWS
Wenson Hsieh
Comment 7
2021-12-20 13:47:11 PST
Comment hidden (obsolete)
Created
attachment 447632
[details]
For EWS (fixed)
Wenson Hsieh
Comment 8
2021-12-20 13:56:34 PST
Created
attachment 447635
[details]
For EWS (actually fixed?)
EWS
Comment 9
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]
.
Radar WebKit Bug Importer
Comment 10
2021-12-20 15:03:15 PST
<
rdar://problem/86742167
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug