[webkit-reviews] review granted: [Bug 234323] Adopt ChromeClient::classifyModalContainerControls() in ModalContainerObserver : [Attachment 447563] For EWS

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 234323: Adopt ChromeClient::classifyModalContainerControls() in
ModalContainerObserver
https://bugs.webkit.org/show_bug.cgi?id=234323

Attachment 447563: For EWS

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




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 447563
  --> https://bugs.webkit.org/attachment.cgi?id=447563
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(controlTextsToCl
assify), [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?