[webkit-reviews] review granted: [Bug 234322] Add ModalContainerControlClassifier and use it to implement classifyModalContainerControls() : [Attachment 447648] Fix macOS 10.14 build

[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 234322: Add ModalContainerControlClassifier and use it to implement
classifyModalContainerControls()
https://bugs.webkit.org/show_bug.cgi?id=234322

Attachment 447648: Fix macOS 10.14 build

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




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

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

r=me, neato :)

> Source/WebKit/UIProcess/Cocoa/ModalContainerControlClassifier.h:28
> +#import <wtf/CompletionHandler.h>

NIT: Could/Should we use `<wtf/Forward.h>` for these instead?

> Source/WebKit/UIProcess/Cocoa/ModalContainerControlClassifier.h:46
> +    ModalContainerControlClassifier();

Should this be in `private:`?

> Source/WebKit/UIProcess/Cocoa/ModalContainerControlClassifier.h:48
> +    void classify(Vector<String>&&,
CompletionHandler<void(Vector<WebCore::ModalContainerControlType>&&)>&&);

Style: I'd make this `Vector<String>&& texts` as it's not really clear what
that parameter is supposed to be used for just by looking at the type.

> Source/WebKit/UIProcess/Cocoa/ModalContainerControlClassifier.mm:125
> +static std::unique_ptr<ModalContainerControlClassifier>&
sharedModalContainerControlClassifier()

NIT: I'd inline this

> Source/WebKit/UIProcess/Cocoa/ModalContainerControlClassifier.mm:156
> +    for (NSInteger index = 0; index < [batch count]; ++index) {

Why not `[resultProvider count]`?

> Source/WebKit/UIProcess/Cocoa/ModalContainerControlClassifier.mm:175
> +    m_queue->dispatch([this, texts = texts.isolatedCopy(), completion =
WTFMove(completion)]() mutable {

Are you doing `texts.isolatedCopy()` instead of `WTFMove(texts)` to avoid
deallocing on a different thread?  Is that an issue for `Vector`/`String`?