To create a platform strategy, one needs to touch every platform's override, even if most of the platforms will just use a default implementation. That's quite a bit of copy/paste code. Also, the implementation uses an incorrect idiom - createXXXStrategy functions return a result that shouldn't be deleted despite having "create" in the name.
Created attachment 169988 [details] proposed patch
Comment on attachment 169988 [details] proposed patch Attachment 169988 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14500235
Comment on attachment 169988 [details] proposed patch Attachment 169988 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14487579
Comment on attachment 169988 [details] proposed patch Attachment 169988 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14488577
Created attachment 169998 [details] with build fixes
Comment on attachment 169998 [details] with build fixes Nit pick: For bonus points anyone who includes PlatformStrategies.h now no longer has to include the header for the specific strategy they meant to use. R+ after EWS signs off.
Comment on attachment 169998 [details] with build fixes Attachment 169998 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14492640
Comment on attachment 169998 [details] with build fixes View in context: https://bugs.webkit.org/attachment.cgi?id=169998&action=review > Source/WebCore/platform/PlatformStrategies.h:33 > +#include "PluginStrategy.h" Isn't this a layering violation? PluginStrategy.h isn't in the platform directory.
http://trac.webkit.org/browser/trunk/Source/WebCore/plugins/PluginStrategy.h Notice that PluginStrategy.h depends on Page, which is a concept that isn't allowed in WebCore/platform.
Thanks for catching this! >Notice that PluginStrategy.h depends on Page, which is a concept that isn't allowed in WebCore/platform. This Page dependency looks like a fake one - no port ever looks at this argument. But including PluginStrategy (or possibly even VisitedLinkStrategy) in PlatformStrategies seems flawed. The design still has the flaws mentioned in description, but we need to approach improving it in some other way.
> This Page dependency looks like a fake one - no port ever looks at this argument. Not true - Qt does.