Bug 226267 – Refactor some data detector code in WebCore::Frame

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

[Wenson Hsieh]

.

[Wenson Hsieh]

Created attachment 429763 [details]
Patch

[Andy Estes]

Comment on attachment 429763 [details]
Patch

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

> Source/WebCore/editing/cocoa/DataDetection.h:44
> +class HTMLElement;

Nit: T < i

> Source/WebCore/editing/cocoa/DataDetection.h:49
> +#if ENABLE(IMAGE_EXTRACTION)
> +struct ImageExtractionDataDetectorInfo;
> +#endif

Nit: I think it's preferable to remove the `#if` here since the forward declaration is harmless even when `ENABLE(IMAGE_EXTRACTION)` is false.

> Source/WebCore/page/Frame.h:104
> +#if ENABLE(DATA_DETECTION)
> +class DataDetectionResultsStorage;
> +#endif

Nit: ditto

> Source/WebCore/page/cocoa/DataDetectionResultsStorage.h:43
> +    DataDetectionResultsStorage() = default;
> +    ~DataDetectionResultsStorage() = default;

Nit: I think the compiler will generate a default constructor and destructor without you having to declare them.

Note that you also get a default copy constructor. Do you want to `WTF_MAKE_NONCOPYABLE` this?

[Devin Rousso]

Comment on attachment 429763 [details]
Patch

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

r=me as well :)

> Source/WebCore/page/Frame.cpp:112
> +#include "DataDetectionResultsStorage.h"

I'd remove the `#if` here since everything in the file has the same `#if`.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:273
> +#include <WebCore/DataDetectionResultsStorage.h>

ditto

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:156
> +#if ENABLE(DATA_DETECTION)

ditto

[Wenson Hsieh]

Comment on attachment 429763 [details]
Patch

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

>> Source/WebCore/editing/cocoa/DataDetection.h:44
>> +class HTMLElement;
> 
> Nit: T < i

Fixed!

>> Source/WebCore/editing/cocoa/DataDetection.h:49
>> +#endif
> 
> Nit: I think it's preferable to remove the `#if` here since the forward declaration is harmless even when `ENABLE(IMAGE_EXTRACTION)` is false.

Sounds good! Removed the guard.

>> Source/WebCore/page/Frame.cpp:112
>> +#include "DataDetectionResultsStorage.h"
> 
> I'd remove the `#if` here since everything in the file has the same `#if`.

👍🏻

>> Source/WebCore/page/Frame.h:104
>> +#endif
> 
> Nit: ditto

Fixed!

>> Source/WebCore/page/cocoa/DataDetectionResultsStorage.h:43
>> +    ~DataDetectionResultsStorage() = default;
> 
> Nit: I think the compiler will generate a default constructor and destructor without you having to declare them.
> 
> Note that you also get a default copy constructor. Do you want to `WTF_MAKE_NONCOPYABLE` this?

Good catch! Removed the ctor/dtor definitions here, and added `WTF_MAKE_NONCOPYABLE(DataDetectionResultsStorage)` above.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:273
>> +#include <WebCore/DataDetectionResultsStorage.h>
> 
> ditto

I'm not actually sure this will work, since DataDetectionResultsStorage.h (which is in a cocoa directory in WebCore) would need to be added to the Headers.cmake list to be exported. I'll see what the bots think!

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:156
>> +#if ENABLE(DATA_DETECTION)
> 
> ditto

Sounds good! Removed the #if (this is Cocoa-specific code so this should be safe to import).

[Wenson Hsieh]

Created attachment 429906 [details]
For EWS

[Wenson Hsieh]

Created attachment 429911 [details]
Fix GTK/WPE build

[EWS]

Committed r278175 (238218@main): <https://commits.webkit.org/238218@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/78584057>