[webkit-reviews] review granted: [Bug 45831] [Qt] Turn on PLATFORM_STRATEGIES : [Attachment 67836] patch, now with a style fix

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Ademar Reis
<ademar.reis at openbossa.org>'s request for review:
Bug 45831: [Qt] Turn on PLATFORM_STRATEGIES
https://bugs.webkit.org/show_bug.cgi?id=45831

Attachment 67836: patch, now with a style fix
https://bugs.webkit.org/attachment.cgi?id=67836&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=67836&action=prettypatch

> WebCore/platform/qt/Language.cpp:2
> + * Copyright (C) 2010 INdT - Instituto Nokia de Tecnologia

I think you need to add Nokia copyright as well

> WebCore/platform/qt/Language.cpp:29
> +

Why all these empty lines?

> WebCore/platform/qt/Language.cpp:31
> +

Here again

> WebKit/qt/WebCoreSupport/WebPlatformStrategies.cpp:33
> +

Please read the part about includes in the WebKit style guide. We normally
don't add sections of includes, thought we might have done so earlier on.

> WebKit/qt/WebCoreSupport/WebPlatformStrategies.cpp:86
> +    QWebPluginFactory* factory = m_page->pluginFactory();

Will this work for webkit2? Can be fixes later!

> WebKit/qt/WebCoreSupport/WebPlatformStrategies.cpp:88
> +

remove newline

> WebKit/qt/WebCoreSupport/WebPlatformStrategies.cpp:606
> +// XXX: #if ENABLE(VIDEO) should be in the base class

We normally use FIXME:

> WebKit/qt/WebCoreSupport/WebPlatformStrategies.h:137
> +    QWebPage *m_page;

* is placed wrongly