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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Dec 20 13:06:36 PST 2021

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

Attachment 447563: For EWS


--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 447563
  --> https://bugs.webkit.org/attachment.cgi?id=447563

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


> Source/WebCore/page/ModalContainerObserver.cpp:284
> +	  
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?

More information about the webkit-reviews mailing list