RESOLVED WONTFIX 100031
Make creating platform strategies easier 
https://bugs.webkit.org/show_bug.cgi?id=100031
Summary Make creating platform strategies easier
Alexey Proskuryakov
Reported 2012-10-22 14:23:12 PDT
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.
Attachments
proposed patch (21.45 KB, patch)
2012-10-22 14:37 PDT, Alexey Proskuryakov 		webkit-ews: commit-queue-
DetailsFormatted DiffDiff
with build fixes (24.37 KB, patch)
2012-10-22 15:15 PDT, Alexey Proskuryakov 		beidson: review+
buildbot: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2012-10-22 14:37:09 PDT
Created attachment 169988 [details] proposed patch
Early Warning System Bot
Comment 2 2012-10-22 14:43:49 PDT
Comment on attachment 169988 [details] proposed patch Attachment 169988 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14500235
Early Warning System Bot
Comment 3 2012-10-22 14:44:41 PDT
Comment on attachment 169988 [details] proposed patch Attachment 169988 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14487579
Build Bot
Comment 4 2012-10-22 15:09:47 PDT
Comment on attachment 169988 [details] proposed patch Attachment 169988 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14488577
Alexey Proskuryakov
Comment 5 2012-10-22 15:15:20 PDT
Created attachment 169998 [details] with build fixes
Brady Eidson
Comment 6 2012-10-22 15:35:47 PDT
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.
Build Bot
Comment 7 2012-10-22 16:09:14 PDT
Comment on attachment 169998 [details] with build fixes Attachment 169998 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14492640
Adam Barth
Comment 8 2012-10-22 16:26:34 PDT
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.
Adam Barth
Comment 9 2012-10-22 16:27:12 PDT
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.
Alexey Proskuryakov
Comment 10 2012-10-22 16:57:55 PDT
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.
Alexey Proskuryakov
Comment 11 2012-10-23 15:06:52 PDT
> This Page dependency looks like a fake one - no port ever looks at this argument. Not true - Qt does.
Note You need to log in before you can comment on or make changes to this bug.