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.
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)?
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.
Created attachment 67836 [details] patch, now with a style fix
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.
(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 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
(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!
Created attachment 67904 [details] patch with review fixes
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 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 on attachment 67904 [details] patch with review fixes Clearing flags on attachment: 67904 Committed r67787: <http://trac.webkit.org/changeset/67787>
All reviewed patches have been landed. Closing bug.
(In reply to comment #12) > All reviewed patches have been landed. Closing bug. Nice work!
*** Bug 45881 has been marked as a duplicate of this bug. ***