Bug 179397 – [iOS] Use new class name from UIKit when checking UITextSuggestion type for WebKitLegacy

• Component: New Bugs
• Status: RESOLVED FIXED

[Frederik Riedel]

Use new class name from UIKit when checking UITextSuggestion type.
It seems like https://bugs.webkit.org/show_bug.cgi?id=178416 only covers the WebKit2 part of this but not WebKit1.

The old UITextSuggestion type is still being used in DOMHTMLInputElement and should be replaced.

[Frederik Riedel]

Created attachment 326268 [details]
Patch

[Wenson Hsieh]

Comment on attachment 326268 [details]
Patch

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

Good catch, Frederik! Just a few comments, inline.

> Source/WebKitLegacy/mac/DOM/DOMHTMLInputElement.mm:39
> +// FIXME: This can be safely removed once <rdar://problem/34583628> lands in the SDK.

This @implementation can be safely removed now :)

Instead of making pieces of DOMHTMLInputElement.mm contingent on USE(APPLE_INTERNAL_SDK), I think we should instead define the new interfaces as needed to build against an SDK without this header. In other words, we should be doing something along the lines of:

#if TARGET_OS_IPHONE
#if __has_include(<UIKit/UITextAutofillSuggestion.h>)

// Just import the header directly.

#else

// Declare an @interface for UITextAutofillSuggestion, with whichever properties we'll need to build.

#endif
#endif // TARGET_OS_IPHONE

> Source/WebKitLegacy/mac/DOM/DOMHTMLInputElement.mm:706
>  #if USE(APPLE_INTERNAL_SDK) && TARGET_OS_IPHONE

I know this isn't new with this patch, but we should really make this guarded on just TARGET_OS_PHONE, and not additionally guard it with USE(APPLE_INTERNAL_SDK). In general, we strive to only have USE(APPLE_INTERNAL_SDK) in headers rather than implementation files. That way, WebKit built with the internal SDK won't behave differently than WebKit built using OpenSource (there are some exceptions, as you probably recall, but this case in particular isn't one where we need to draw a distinction between internal/open source configurations).

[Frederik Riedel]

Comment on attachment 326268 [details]
Patch

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

>> Source/WebKitLegacy/mac/DOM/DOMHTMLInputElement.mm:39
>> +// FIXME: This can be safely removed once <rdar://problem/34583628> lands in the SDK.
> 
> This @implementation can be safely removed now :)
> 
> Instead of making pieces of DOMHTMLInputElement.mm contingent on USE(APPLE_INTERNAL_SDK), I think we should instead define the new interfaces as needed to build against an SDK without this header. In other words, we should be doing something along the lines of:
> 
> #if TARGET_OS_IPHONE
> #if __has_include(<UIKit/UITextAutofillSuggestion.h>)
> 
> // Just import the header directly.
> 
> #else
> 
> // Declare an @interface for UITextAutofillSuggestion, with whichever properties we'll need to build.
> 
> #endif
> #endif // TARGET_OS_IPHONE

Done. Import if available and otherwise declaring the interface with @properties for username and password, because we use them below.

>> Source/WebKitLegacy/mac/DOM/DOMHTMLInputElement.mm:706
>>  #if USE(APPLE_INTERNAL_SDK) && TARGET_OS_IPHONE
> 
> I know this isn't new with this patch, but we should really make this guarded on just TARGET_OS_PHONE, and not additionally guard it with USE(APPLE_INTERNAL_SDK). In general, we strive to only have USE(APPLE_INTERNAL_SDK) in headers rather than implementation files. That way, WebKit built with the internal SDK won't behave differently than WebKit built using OpenSource (there are some exceptions, as you probably recall, but this case in particular isn't one where we need to draw a distinction between internal/open source configurations).

That makes sense, I removed the APPLE_INTERNAL_SDK guard here. This should work now because the @interface declaration for UITextAutofillSuggestion above sits behind the same guard.

[Frederik Riedel]

Created attachment 326438 [details]
Patch

[Frederik Riedel]

Created attachment 326444 [details]
Patch

[Wenson Hsieh]

Comment on attachment 326444 [details]
Patch

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

> Source/WebKitLegacy/mac/ChangeLog:3
> +        Replaced the old placeholder UIKit class with the real class name in WebKitLegacy.

This is a minor nit, but I just noticed - the ChangeLog title here should be the same as the title of the bug ([iOS] Use new class name...). Is it OK if you make this tweak and upload a new patch?

[Frederik Riedel]

Created attachment 326576 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 326576 [details]
Patch

Clearing flags on attachment: 326576

Committed r224682: <https://trac.webkit.org/changeset/224682>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35562181>