[webkit-reviews] review granted: [Bug 100838] [Qt][WK2] setPlatformStrategies always asserts after r132744 : [Attachment 171974] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has granted Balazs Kelemen
<kbalazs at webkit.org>'s request for review:
Bug 100838: [Qt][WK2] setPlatformStrategies always asserts after r132744
https://bugs.webkit.org/show_bug.cgi?id=100838

Attachment 171974: Patch
https://bugs.webkit.org/attachment.cgi?id=171974&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=171974&action=review


r=me with a few minor glitches that should be fixed before landing :)

> Source/WebCore/ChangeLog:16
> +	   in non production mode.

Very good reasoning, I fully agree.

There are some dependencies left from WTR to the WK1 API, right?
(DumpRenderTreeSupport?)

If not, then we can elimiate the dependency mentioned in Tools/Tools.pro to
build wtr only if build?(webkit1) is true.

> Source/WebCore/Target.pri:4084
> +!production_build {

While this will work most of the time, I think the correct condition to use is

if(build?(drt)|build?(wtr))

Those will be disabled if !production_build of course.

> Source/WebCore/WebCore.pri:289
> +!production_build:have?(FONTCONFIG) PKGCONFIG += fontconfig

I think a colon is missing at the end here, at least to be on the safe side :)

> Tools/DumpRenderTree/qt/DumpRenderTree.pro:-35
> -    QtInitializeTestFonts.h \

I don't see those files as removed in the diff. Is that a bug in the review
tool?