[webkit-reviews] review granted: [Bug 46399] Implement plugin quirks in WebKit2 : [Attachment 93333] [PATCH] Mozilla User Agent Quirk

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 46399: Implement plugin quirks in WebKit2
https://bugs.webkit.org/show_bug.cgi?id=46399

Attachment 93333: [PATCH] Mozilla User Agent Quirk
https://bugs.webkit.org/attachment.cgi?id=93333&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=93333&action=review

> Source/WebKit2/Shared/Plugins/Netscape/win/NetscapePluginModuleWin.cpp:126
> +    // Pre Flash v10 only requests windowless plugins if we use a Mozilla
user agent.
> +
> +    // To test: Install Flash version < 10
(http://kb2.adobe.com/cps/142/tn_14266.html)
> +    // and test at
http://communitymx.com/content/source/E5141/wmodetrans.htm. You should
> +    // see a star behind the bouncing ball.

I'd move these comments inside the if (mimeTypes[i].type == ...) test. That way
they are closer to the code that is actually dealing with Flash.

I think it would be a little better to put the test case info in the bug and
have the comment just reference the bug.

> Source/WebKit2/Shared/Plugins/Netscape/win/NetscapePluginModuleWin.cpp:129
> +	   if (mimeTypes[i].type == "application/x-shockwave-flash") {

You could add a FIXME here saying that it's a little strange to assume that any
plugin that handles this MIME type needs this quirk. (Maybe we should be
checking the plugin's name instead, e.g.)

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:137
> +#if PLUGIN_ARCHITECTURE(MAC)
> +	   "Macintosh; U; Intel Mac OS X;"

I don't think Apple's Mac port ever had this behavior before. (Only Qt/mac and
GTK+/mac did.) We should test to make sure it's really desired.