Bug 45831 - [Qt] Turn on PLATFORM_STRATEGIES
Summary: [Qt] Turn on PLATFORM_STRATEGIES
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Ademar Reis
URL:
Keywords: Qt
: 45881 (view as bug list)
Depends on: 46181
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-15 13:39 PDT by Ademar Reis
Modified: 2010-09-21 06:16 PDT (History)
7 users (show)

See Also:


Attachments
Initial patch (67.94 KB, patch)
2010-09-16 13:34 PDT, Ademar Reis
no flags Details | Formatted Diff | Diff
patch, now with a style fix (67.95 KB, patch)
2010-09-16 13:40 PDT, Ademar Reis
no flags Details | Formatted Diff | Diff
patch with review fixes (68.07 KB, patch)
2010-09-17 06:11 PDT, Ademar Reis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ademar Reis 2010-09-15 13:39:06 PDT
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.
Comment 1 Ademar Reis 2010-09-16 13:34:10 PDT
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)?
Comment 2 WebKit Review Bot 2010-09-16 13:35:59 PDT
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.
Comment 3 Ademar Reis 2010-09-16 13:40:06 PDT
Created attachment 67836 [details]
patch, now with a style fix
Comment 4 Ademar Reis 2010-09-16 13:50:03 PDT
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.
Comment 5 Kenneth Rohde Christiansen 2010-09-17 05:25:25 PDT
(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.
Comment 6 Kenneth Rohde Christiansen 2010-09-17 05:30:33 PDT
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
Comment 7 Ademar Reis 2010-09-17 06:03:39 PDT
(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!
Comment 8 Ademar Reis 2010-09-17 06:11:34 PDT
Created attachment 67904 [details]
patch with review fixes
Comment 9 Kenneth Rohde Christiansen 2010-09-17 06:37:33 PDT
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.
Comment 10 Eric Seidel (no email) 2010-09-18 03:19:12 PDT
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.
Comment 11 WebKit Commit Bot 2010-09-18 13:16:02 PDT
Comment on attachment 67904 [details]
patch with review fixes

Clearing flags on attachment: 67904

Committed r67787: <http://trac.webkit.org/changeset/67787>
Comment 12 WebKit Commit Bot 2010-09-18 13:16:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Antonio Gomes 2010-09-18 13:23:50 PDT
(In reply to comment #12)
> All reviewed patches have been landed.  Closing bug.

Nice work!
Comment 14 Balazs Kelemen 2010-09-20 02:21:57 PDT
*** Bug 45881 has been marked as a duplicate of this bug. ***