Bug 182748 – Allow specifying which plug-ins are supported

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

[youenn fablet]

Allow specifying which plug-ins are supported

[youenn fablet]

Created attachment 333739 [details]
Patch

[youenn fablet]

Created attachment 333747 [details]
Patch

[youenn fablet]

Created attachment 333755 [details]
Patch

[youenn fablet]

Created attachment 333757 [details]
Patch

[Chris Dumez]

Comment on attachment 333755 [details]
Patch

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

> Source/WebCore/html/HTMLPlugInElement.cpp:408
> +        ASCIILiteral consoleMessage("Tried to use an unsupported plugin.");

I do not think this local variable is needed.

> Source/WebCore/plugins/SupportedPluginNames.h:37
> +struct SupportedPluginNames {

Given the API, it seems like ignorance could be a class with the data members private.

> Source/WebCore/plugins/SupportedPluginNames.h:50
> +    auto specific = originSpecificPlugins.get(&origin);

This may construct an empty HashSet unnecessarily. We may want to use find() here.

> Source/WebCore/plugins/SupportedPluginNames.h:66
> +template<class Decoder> inline std::optional<HashSet<String>> decodePluginNames(Decoder& decoder)

Doesn’t HashSet already have encoding / decoding functions?

> Source/WebCore/testing/Internals.cpp:3277
> +    if (!renderer || !is<RenderEmbeddedObject>(*renderer))

I believe we can omit the first check and pass a pointer to the is() function.

> Source/WebCore/testing/Internals.cpp:3280
> +    auto result = downcast<RenderEmbeddedObject>(*renderer).pluginReplacementTextIfUnavailable();

I do not think this local variable is useful.

> Source/WebKit/WebProcess/Plugins/WebPluginInfoProvider.cpp:130
> +    for (int32_t i = plugins.size() - 1; i >= 0; --i) {

Cannot this use removeAllMatching()?

[EWS Watchlist]

Comment on attachment 333757 [details]
Patch

Attachment 333757 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6494850

New failing tests:
http/tests/plugins/supported-plugin-origin-specific-visibility.html
http/tests/plugins/nounsupported-plugin.html

[EWS Watchlist]

Created attachment 333765 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[youenn fablet]

Created attachment 333836 [details]
Patch

[youenn fablet]

Created attachment 333839 [details]
Patch

[youenn fablet]

Created attachment 333843 [details]
Patch

[youenn fablet]

Created attachment 333852 [details]
Patch

[youenn fablet]

Created attachment 333859 [details]
Patch

[youenn fablet]

Created attachment 333865 [details]
Patch

[youenn fablet]

Created attachment 333911 [details]
Patch

Comment on attachment 333911 [details]
Patch

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

> Source/WebCore/platform/LocalizedStrings.cpp:665
> +    return WEB_UI_STRING_KEY("Unsupported Plug-in", "Unsupported Plug-In", "Label text to be used when an unsupported plug-in was blocked from loading");

Why does the key need to be different from the English here?

[Chris Dumez]

Comment on attachment 333911 [details]
Patch

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

r=me

> Source/WebCore/PAL/pal/SessionID.h:98
> +std::optional<SessionID> SessionID::decode(Decoder& decoder)

Please update the legacy coder to call this one if possible. This would avoid code duplication.

> Source/WebCore/html/HTMLPlugInElement.cpp:408
> +        document().addConsoleMessage(MessageSource::JS, MessageLevel::Warning, ASCIILiteral("Tried to use an unsupported plugin."));

plug-in ?

>> Source/WebCore/platform/LocalizedStrings.cpp:665
>> +    return WEB_UI_STRING_KEY("Unsupported Plug-in", "Unsupported Plug-In", "Label text to be used when an unsupported plug-in was blocked from loading");
> 
> Why does the key need to be different from the English here?

At least it is consistent with Blocked Plug-in above.

> Source/WebKit/Platform/IPC/ArgumentCoders.h:429
> +    static std::optional<HashSetType> decode(Decoder& decoder)

Please update the legacy coder to call this one if possible. This would avoid code duplication.

[youenn fablet]

Created attachment 334063 [details]
Patch for landing

[youenn fablet]

Created attachment 334069 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 334069 [details]
Patch for landing

Rejecting attachment 334069 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 334069, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
in.html
patching file LayoutTests/platform/ios-wk1/TestExpectations
patching file LayoutTests/platform/mac-wk1/TestExpectations
Hunk #2 FAILED at 504.
1 out of 2 hunks FAILED -- saving rejects to file LayoutTests/platform/mac-wk1/TestExpectations.rej
patching file LayoutTests/plugins/unsupported-plugin-expected.txt
patching file LayoutTests/plugins/unsupported-plugin.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/6542471

[youenn fablet]

Created attachment 334071 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 334071 [details]
Patch for landing

Clearing flags on attachment: 334071

Committed r228587: <https://trac.webkit.org/changeset/228587>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37621938>

[Chris Dumez]

WebKit.UnavailablePlugIn API test seems to be failing after this.

[youenn fablet]

(In reply to Chris Dumez from comment #24)
> WebKit.UnavailablePlugIn API test seems to be failing after this.

Not reproduced on bot AFAICS.
Locally tested, timing out if flash is installed, passing otherwise.
I guess the test could be made more robust.