Bug 235598 – Data detectors sometimes show up in the wrong place when resizing images with Live Text

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

.

[Radar WebKit Bug Importer]

<rdar://problem/88032375>

[Wenson Hsieh]

Created attachment 449990 [details]
Patch

[Wenson Hsieh]

Comment on attachment 449990 [details]
Patch

Thanks for the review!

[EWS]

Committed r288621 (246440@main): <https://commits.webkit.org/246440@main>

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

[Darin Adler]

Comment on attachment 449990 [details]
Patch

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

> Source/WebCore/dom/ImageOverlay.cpp:215
> +            return { };

While I love using { } and not being too specific about type when just trying to return something like an empty vector or any empty object, when the value is "nullptr" I think I prefer to write that. What do you think?

> Source/WebCore/testing/Internals.cpp:5821
> +        , dataDetectors.map([] (auto& dataDetector) -> TextRecognitionDataDetector {
> +            return TextRecognitionDataDetector { fakeDataDetectorResultForTesting(), { getQuad(dataDetector) } };
> +        })

I don’t think we need to write TextRecognitionDataDetector twice. Either we don’t need to specify it after the "->" or we don’t need to specify it in the return statement.

Also, Yusuke told me our style does *not* include a space between the capture list and the argument list, so "[](auto& dataDetector)" is the style, although we are certainly not consistent about this.

> Source/WebCore/testing/Internals.mm:182
> +    static NeverDestroyed result = ([]() -> RetainPtr<DDScannerResult> {

I am surprised that we needed the outer parentheses. I would normally leave them out. I think you can just call a lambda without putting it into parentheses.

Also, we can write "[]" instead of "[]()".

> Source/WebCore/testing/Internals.mm:184
> +        auto stringToScan = CFSTR("webkit.org");

Not sure we need a local variable for this, but OK if you think it makes things clearer.

> Source/WebCore/testing/Internals.mm:187
> +            return { };

Same thought as above, I would use "nil" here instead of "{ }".

[Wenson Hsieh]

Comment on attachment 449990 [details]
Patch

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

Thanks for taking a look!

>> Source/WebCore/dom/ImageOverlay.cpp:215
>> +            return { };
> 
> While I love using { } and not being too specific about type when just trying to return something like an empty vector or any empty object, when the value is "nullptr" I think I prefer to write that. What do you think?

Sounds good! I don't have a strong preference for this, but I can see how `nullptr` would be more clear. I'll change it to that.

>> Source/WebCore/testing/Internals.cpp:5821
>> +        })
> 
> I don’t think we need to write TextRecognitionDataDetector twice. Either we don’t need to specify it after the "->" or we don’t need to specify it in the return statement.
> 
> Also, Yusuke told me our style does *not* include a space between the capture list and the argument list, so "[](auto& dataDetector)" is the style, although we are certainly not consistent about this.

Good catch! Fixed both issues.

>> Source/WebCore/testing/Internals.mm:182
>> +    static NeverDestroyed result = ([]() -> RetainPtr<DDScannerResult> {
> 
> I am surprised that we needed the outer parentheses. I would normally leave them out. I think you can just call a lambda without putting it into parentheses.
> 
> Also, we can write "[]" instead of "[]()".

Removed the parentheses!

Regarding the "[]()", I tried to do that initially, but got this error:

```
/…/OpenSource/Source/WebCore/testing/Internals.mm:182:39: error: lambda without a parameter clause is a C++2b extension [-Werror,-Wc++2b-extensions]
    static NeverDestroyed result = [] -> RetainPtr<DDScannerResult> {
1 error generated.
```

>> Source/WebCore/testing/Internals.mm:184
>> +        auto stringToScan = CFSTR("webkit.org");
> 
> Not sure we need a local variable for this, but OK if you think it makes things clearer.

So I pulled this out into a local variable here because I use `stringToScan` twice below — I think I'll keep the local here.

>> Source/WebCore/testing/Internals.mm:187
>> +            return { };
> 
> Same thought as above, I would use "nil" here instead of "{ }".

Changed these to return `nil`.

[Wenson Hsieh]

Reopening to attach new patch.

[Wenson Hsieh]

Created attachment 450041 [details]
Patch

[Darin Adler]

Comment on attachment 449990 [details]
Patch

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

>>> Source/WebCore/testing/Internals.mm:182
>>> +    static NeverDestroyed result = ([]() -> RetainPtr<DDScannerResult> {
>> 
>> I am surprised that we needed the outer parentheses. I would normally leave them out. I think you can just call a lambda without putting it into parentheses.
>> 
>> Also, we can write "[]" instead of "[]()".
> 
> Removed the parentheses!
> 
> Regarding the "[]()", I tried to do that initially, but got this error:
> 
> ```
> /…/OpenSource/Source/WebCore/testing/Internals.mm:182:39: error: lambda without a parameter clause is a C++2b extension [-Werror,-Wc++2b-extensions]
>     static NeverDestroyed result = [] -> RetainPtr<DDScannerResult> {
> 1 error generated.
> ```

Oh, yes, forgot that when you specify a return type the empty arguments list is needed.

[EWS]

Committed r288654 (246462@main): <https://commits.webkit.org/246462@main>

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