.
<rdar://problem/88032375>
Created attachment 449990 [details] Patch
Comment on attachment 449990 [details] Patch Thanks for the review!
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].
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 "{ }".
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`.
Reopening to attach new patch.
Created attachment 450041 [details] Patch
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.
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].