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.
Created attachment 326268 [details] Patch
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).
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.
Created attachment 326438 [details] Patch
Created attachment 326444 [details] Patch
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?
Created attachment 326576 [details] Patch
Comment on attachment 326576 [details] Patch Clearing flags on attachment: 326576 Committed r224682: <https://trac.webkit.org/changeset/224682>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35562181>