Bug 240044 – Remove deprecated 'JavaEnabled' feature flag and related code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

We removed support for Java Plug-Ins in macOS 10.15, but never removed the preference. To reduce code complexity and build times we should remove this unused preference, leaving non-functional stubs to avoid breaking binaries that expect to call their accessor methods.

[Brent Fulgham]

Created attachment 458765 [details]
Patch

[Darin Adler]

Comment on attachment 458765 [details]
Patch

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

> Source/WebCore/loader/SubframeLoader.cpp:132
> +    if (MIMETypeRegistry::isJavaAppletMIMEType(mimeType))
> +        return false;

I understand that this preserves behavior. But do we need this? Why exactly? I think we don’t.

> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:106
> +// Defaults to false.

Comment could say something more specific like "does nothing, always returns false".

> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:108
> +WK_EXPORT void WKPreferencesSetJavaEnabled(WKPreferencesRef preferences, bool javaEnabled) WK_C_API_DEPRECATED;
> +WK_EXPORT bool WKPreferencesGetJavaEnabled(WKPreferencesRef preferences) WK_C_API_DEPRECATED;

Do we need this? Can we just remove it instead?

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:585
> +WK_EXPORT void WKPreferencesSetJavaEnabledForLocalFiles(WKPreferencesRef preferences, bool javaEnabled) WK_C_API_DEPRECATED;
> +WK_EXPORT bool WKPreferencesGetJavaEnabledForLocalFiles(WKPreferencesRef preferences) WK_C_API_DEPRECATED;

Do we need this? Can we just remove it instead?

> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.mm:1641
>  #endif
>  
> +
> +#if PLATFORM(MAC)

No need for the extra blank line.

> Source/WebKit/UIProcess/API/Cocoa/WKPreferencesPrivate.h:236
> +@property (nonatomic, setter=_setJavaEnabledForLocalFiles:) BOOL _javaEnabledForLocalFiles WK_API_DEPRECATED("Java is no longer supported", macos(10.13.4, 10.15));

Do we need this? Can we just remove it instead?

> Source/WebKitLegacy/mac/WebView/WebPreferenceKeysPrivate.h:293
> +#define WebKitJavaEnabledPreferenceKey @"WebKitJavaEnabled"

Why do we need this? I suggest we remove it.

> Source/WebKitLegacy/mac/WebView/WebPreferences.h:172
>  /*!
>      @property javaEnabled
>  */
> -@property (nonatomic, getter=isJavaEnabled) BOOL javaEnabled;
> +@property (nonatomic, getter=isJavaEnabled) BOOL javaEnabled WEBKIT_DEPRECATED_MAC(10_3, 10_15);

I suggest we have the comment say this does nothing and always returns false.

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:3474
> +- (BOOL)isJavaEnabled
> +{
> +    return NO;
> +}
> +
> +- (void)setJavaEnabled:(BOOL)flag
> +{
> +}

Why move this to the bottom of the file instead of changing it in place?

> Tools/TestWebKitAPI/Tests/WebKit/WKPreferences.cpp:84
> -    EXPECT_TRUE(WKPreferencesGetJavaEnabled(preference));
> +    EXPECT_FALSE(WKPreferencesGetJavaEnabled(preference));

I suggest we just remove this.

[Brent Fulgham]

Comment on attachment 458765 [details]
Patch

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

>> Source/WebCore/loader/SubframeLoader.cpp:132
>> +        return false;
> 
> I understand that this preserves behavior. But do we need this? Why exactly? I think we don’t.

I'll just delete it and confirm that passes all tests. My only hesitation is that this method returns 'true' by default, which might give us bad behavior if we just delete.

>> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:106
>> +// Defaults to false.
> 
> Comment could say something more specific like "does nothing, always returns false".

Will do!

>> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:108
>> +WK_EXPORT bool WKPreferencesGetJavaEnabled(WKPreferencesRef preferences) WK_C_API_DEPRECATED;
> 
> Do we need this? Can we just remove it instead?

I'll confirm we don't use it anywhere. I didn't do a thorough search, so I'll double check and then remove. I'm a little worried about removing it since it's API.

>> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:585
>> +WK_EXPORT bool WKPreferencesGetJavaEnabledForLocalFiles(WKPreferencesRef preferences) WK_C_API_DEPRECATED;
> 
> Do we need this? Can we just remove it instead?

I'll bet we can remove both of these, since they are SPI.

>> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.mm:1641
>> +#if PLATFORM(MAC)
> 
> No need for the extra blank line.

Will fix.

>> Source/WebKit/UIProcess/API/Cocoa/WKPreferencesPrivate.h:236
>> +@property (nonatomic, setter=_setJavaEnabledForLocalFiles:) BOOL _javaEnabledForLocalFiles WK_API_DEPRECATED("Java is no longer supported", macos(10.13.4, 10.15));
> 
> Do we need this? Can we just remove it instead?

I'll confirm and remove.

>> Source/WebKitLegacy/mac/WebView/WebPreferenceKeysPrivate.h:293
>> +#define WebKitJavaEnabledPreferenceKey @"WebKitJavaEnabled"
> 
> Why do we need this? I suggest we remove it.

Will do.

>> Source/WebKitLegacy/mac/WebView/WebPreferences.h:172
>> +@property (nonatomic, getter=isJavaEnabled) BOOL javaEnabled WEBKIT_DEPRECATED_MAC(10_3, 10_15);
> 
> I suggest we have the comment say this does nothing and always returns false.

Will do.

>> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:3474
>> +}
> 
> Why move this to the bottom of the file instead of changing it in place?

I moved it to the 'WKPreferences (WKPrivateDeprecated)' category, along with the other deprecated items.

>> Tools/TestWebKitAPI/Tests/WebKit/WKPreferences.cpp:84
>> +    EXPECT_FALSE(WKPreferencesGetJavaEnabled(preference));
> 
> I suggest we just remove this.

Will do.

[Brent Fulgham]

Created attachment 458767 [details]
Patch for landing

[EWS]

Patch 458767 does not build

[Brent Fulgham]

Created attachment 458803 [details]
Patch for landing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 458803 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Brent Fulgham]

(In reply to EWS from comment #7)
> Tools/Scripts/svn-apply failed to apply attachment 458803 [details] to trunk.
> Please resolve the conflicts and upload a new patch.

Huge EWS fail:
"patch: **** Can't create file /var/folders/33/15r3ggyd4fb52p4t38_rbd9w0000gn/T//pp0VQ5bq : No space left on device
"

We should recognize space failures and not mark a patch as broken when that happens.

[EWS]

Committed r293785 (250264@main): <https://commits.webkit.org/250264@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/92750902>

[Darin Adler]

Comment on attachment 458765 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:108
>>> +WK_EXPORT bool WKPreferencesGetJavaEnabled(WKPreferencesRef preferences) WK_C_API_DEPRECATED;
>> 
>> Do we need this? Can we just remove it instead?
> 
> I'll confirm we don't use it anywhere. I didn't do a thorough search, so I'll double check and then remove. I'm a little worried about removing it since it's API.

Despite the name of the directory this is in, it is absolutely *not* API on Apple platforms. Our C interface for modern WebKit has only ever been SPI and I’m pretty sure we don’t ever plan to make it API.

I suppose on non-Apple platforms it might be API, so if those need this to stay around, I understand why you might not remove it.