Bug 186578

Summary:

Supported plugin check should be based on plugin identifier

Product:

WebKit

Reporter:

youenn fablet <youennf>

Component:

WebCore Misc.

Assignee:

youenn fablet <youennf>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

a.tion.surf, cdumez, commit-queue, darin, dbates, ews-watchlist, ggaren, japhet, jberlin, rmondello, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch for landing	none

youenn fablet

Reported 2018-06-12 16:11:53 PDT

Detect WebKit built-in PDF plug-in using a bundle identifier

Attachments
Patch (24.78 KB, patch) 2018-06-12 16:18 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Patch for landing (24.96 KB, patch) 2018-06-13 15:00 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

youenn fablet

Comment 1 2018-06-12 16:12:11 PDT

<rdar://problem/40523828>

youenn fablet

Comment 2 2018-06-12 16:18:23 PDT

Created attachment 342608 [details] Patch

Darin Adler

Comment 3 2018-06-12 23:39:22 PDT

Comment on attachment 342608 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342608&action=review > Source/WebCore/plugins/PluginData.cpp:57 > + return plugin.bundleIdentifier == "com.apple.webkit.builtInPDFPlugin"; Seems a little unsafe to have this be a case sensitive check since bundle identifiers are not case sensitive. Could avoid the case sensitivity by using equalLettersIgnoringASCIICase or equalIgnoringASCIICase. Would also be nice to not risk this getting out of sync if we change the identifier in the future for some reason, but I suppose it’s not that terrible to have it here.

Zach Li

Comment 4 2018-06-13 13:33:28 PDT

Comment on attachment 342608 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342608&action=review > Source/WebKit/UIProcess/Plugins/PluginInfoStore.h:68 > void addSupportedPlugin(String&& matchingDomain, String&& name, HashSet<String>&& mimeTypes, HashSet<String> extensions); This should be changed to `identifier` from `name`.

youenn fablet

Comment 5 2018-06-13 15:00:41 PDT

Created attachment 342696 [details] Patch for landing

youenn fablet

Comment 6 2018-06-13 15:02:41 PDT

Thanks for the reviews. > > Source/WebCore/plugins/PluginData.cpp:57 > > + return plugin.bundleIdentifier == "com.apple.webkit.builtInPDFPlugin"; > > Seems a little unsafe to have this be a case sensitive check since bundle > identifiers are not case sensitive. Could avoid the case sensitivity by > using equalLettersIgnoringASCIICase or equalIgnoringASCIICase. Done > Would also be nice to not risk this getting out of sync if we change the > identifier in the future for some reason, but I suppose it’s not that > terrible to have it here. Right, I also thought it was not great but not too bad either. > > Source/WebKit/UIProcess/Plugins/PluginInfoStore.h:68 > > void addSupportedPlugin(String&& matchingDomain, String&& name, HashSet<String>&& mimeTypes, HashSet<String> extensions); > > This should be changed to `identifier` from `name`. Done

WebKit Commit Bot

Comment 7 2018-06-13 15:22:14 PDT

Comment on attachment 342696 [details] Patch for landing Clearing flags on attachment: 342696 Committed r232815: <https://trac.webkit.org/changeset/232815>

WebKit Commit Bot

Comment 8 2018-06-13 15:22:16 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.