Bug 100031

Summary:

Make creating platform strategies easier

Product:

WebKit

Reporter:

Alexey Proskuryakov <ap>

Component:

WebCore Misc.

Assignee:

Alexey Proskuryakov <ap>

Status:

RESOLVED WONTFIX

Severity:

Normal

CC:

abarth, andersca, beidson, gyuyoung.kim, rakuco, sam, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

100043

Attachments:

Description	Flags
proposed patch	webkit-ews: commit-queue-
with build fixes	beidson: review+, buildbot: commit-queue-

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-	Details Formatted Diff Diff
with build fixes (24.37 KB, patch) 2012-10-22 15:15 PDT, Alexey Proskuryakov	beidson: review+ buildbot: commit-queue-	Details Formatted Diff Diff
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.