Bug 234322 – Add ModalContainerControlClassifier and use it to implement classifyModalContainerControls()

• Component: Platform
• Status: RESOLVED FIXED

[Wenson Hsieh]

.

[Wenson Hsieh]

Created attachment 447469 [details]
Patch

[Wenson Hsieh]

Created attachment 447640 [details]
Rebase on trunk

[Wenson Hsieh]

Created attachment 447648 [details]
Fix macOS 10.14 build

[Devin Rousso]

Comment on attachment 447648 [details]
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`?

[Wenson Hsieh]

Comment on attachment 447648 [details]
Fix macOS 10.14 build

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

Thanks for the review!

>> Source/WebKit/UIProcess/Cocoa/ModalContainerControlClassifier.h:28
>> +#import <wtf/CompletionHandler.h>
> 
> NIT: Could/Should we use `<wtf/Forward.h>` for these instead?

Sure.

>> Source/WebKit/UIProcess/Cocoa/ModalContainerControlClassifier.h:46
>> +    ModalContainerControlClassifier();
> 
> Should this be in `private:`?

Hm…I might be able to, if I `friend std::unique_ptr<~>`…

Will give that a try.

>> 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

Good point. Moved into the implementation of `sharedClassifier()` below.

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

Good point — changed to use `resultProvider.count` here.

>> 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`?

I believe it is. These strings passed into `classify()` were created on the main thread, so they should also be destroyed on the main thread…AFAICT.

[Wenson Hsieh]

Created attachment 447663 [details]
Style error is false positive

[Wenson Hsieh]

Created attachment 447665 [details]
Ignore style error

[EWS]

Committed r287296 (245448@main): <https://commits.webkit.org/245448@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447665 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86750278>