Bug 45831 – [Qt] Turn on PLATFORM_STRATEGIES

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Ademar Reis]

We need to support/enable PLATFORM_STRATEGIES on the Qt port in order to support plugins and other features on WebKit2. Right now only mac/windows support it.

Some references:

 - https://lists.webkit.org/pipermail/webkit-dev/2010-June/013270.html (discussion on the webkit-dev ml)
 - Bug 43431 (adding support on Windows)
 - http://trac.webkit.org/changeset/64645 (commit adding support on windows)

I'm working on it. A first patch should be available for review soon.

[Ademar Reis]

Created attachment 67833 [details]
Initial patch

Initial patch for review.

Question for review: Inside our localization implementation, the following methods were inside an #if ENABLE(VIDEO):

localizedMediaControlElementString()
localizedMediaControlElementHelpText()
localizedMediaTimeDescription()
(these methods are relatively large, ~40 strings total)

But the WebCore::LocalizationStrategy base-class (pure virtual) doesn't have such conditional.

In my patch I added an #else and implemented stubs for these methods in case VIDEO is disabled. Should I add #if ENABLE(VIDEO) in the base class instead (and change the other ports as well)?

[WebKit Review Bot]

Attachment 67833 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/qt/WebCoreSupport/WebPlatformStrategies.cpp:32:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ademar Reis]

Created attachment 67836 [details]
patch, now with a style fix

[Ademar Reis]

I have another couple of questions for the reviewer :-)

While moving the implementation from WebCore/platform/qt/Localizations.cpp to 
WebKit/qt/WebCoreSupport/WebPlatformStrategies.cpp, I added notImplemented() to a few localization methods which are just returning String(), based on other examples I found in the same file.

I'm not sure if that was really needed/correct. Example:

(from WebCore/platform/qt/Localizations.cpp)
-String AXLinkActionVerb()
-{
-    return String();
-}

(WebKit/qt/WebCoreSupport/WebPlatformStrategies.cpp)
+String WebPlatformStrategies::AXLinkActionVerb()
+{
+    notImplemented();
+    return String();
+}

Also, there was no multipleFileUploadText() implemented in the Qt localization, so I added a stub.

Sorry for not bringing these points when attaching the patch a few minutes ago.

[Kenneth Rohde Christiansen]

(In reply to comment #1)
> Created an attachment (id=67833) [details]
> Initial patch
> 
> Initial patch for review.
> 
> Question for review: Inside our localization implementation, the following methods were inside an #if ENABLE(VIDEO):
> 
> localizedMediaControlElementString()
> localizedMediaControlElementHelpText()
> localizedMediaTimeDescription()
> (these methods are relatively large, ~40 strings total)
> 
> But the WebCore::LocalizationStrategy base-class (pure virtual) doesn't have such conditional.
> 
> In my patch I added an #else and implemented stubs for these methods in case VIDEO is disabled. Should I add #if ENABLE(VIDEO) in the base class instead (and change the other ports as well)?

That is probably just a bug, I guess you can leave them out for now.

[Kenneth Rohde Christiansen]

Comment on attachment 67836 [details]
patch, now with a style fix

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

[Ademar Reis]

(In reply to comment #6)
> (From update of attachment 67836 [details])
> 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

Done

> 
> > WebCore/platform/qt/Language.cpp:29
> > +
> 
> Why all these empty lines?
> 
> > WebCore/platform/qt/Language.cpp:31
> > +
> 
> Here again

Apparently I misunderstood check-webkit-style complains somewhere. Fixed.

> 
> > 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.

Not even when mixing internal and external headers (e.g. WebCore and Qt headers)? Apparently there's no rule or consistent example for such cases, so I just ordered them.

We should also have a guideline for when to use "" or <>, but that's off-topic now :)

> 
> > WebKit/qt/WebCoreSupport/WebPlatformStrategies.cpp:86
> > +    QWebPluginFactory* factory = m_page->pluginFactory();
> 
> Will this work for webkit2? Can be fixes later!

WebKit2 has it's own WebPlatformStrategies.cpp implementation, which right now is shared among the three platforms supporting it and has a few FIXMEs inside.

I plan to invest some time on a second task focused on WebKit2, which will probably require changes in code which is common between all platforms. The idea of this patch/bug is just to enable platform strategies, "AS IS".

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

Was copy&pasted, but done :)

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

Done

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

Done

Thanks for the review!

[Ademar Reis]

Created attachment 67904 [details]
patch with review fixes

[Kenneth Rohde Christiansen]

Comment on attachment 67904 [details]
patch with review fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=67904&action=prettypatch

> WebKit/qt/WebCoreSupport/WebPlatformStrategies.cpp:375
> +    return String();

Maybe we should file a bug about implementing these.

[Eric Seidel (no email)]

Comment on attachment 67836 [details]
patch, now with a style fix

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 67836 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 67904 [details]
patch with review fixes

Clearing flags on attachment: 67904

Committed r67787: <http://trac.webkit.org/changeset/67787>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

(In reply to comment #12)
> All reviewed patches have been landed.  Closing bug.

Nice work!

[Balazs Kelemen]

*** Bug 45881 has been marked as a duplicate of this bug. ***