Bug 46345 - [Qt] Refactor QtWebKitPlatformPlugin interface
Summary: [Qt] Refactor QtWebKitPlatformPlugin interface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Tor Arne Vestbø (not active)
URL:
Keywords:
Depends on: 46402
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-23 03:48 PDT by Tor Arne Vestbø (not active)
Modified: 2010-09-23 15:16 PDT (History)
9 users (show)

See Also:


Attachments
Patch (8.97 KB, patch)
2010-09-23 03:50 PDT, Tor Arne Vestbø (not active)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tor Arne Vestbø (not active) 2010-09-23 03:48:03 PDT
[Qt] Refactor QtWebKitPlatformPlugin interface
Comment 1 Tor Arne Vestbø (not active) 2010-09-23 03:50:45 PDT
Created attachment 68507 [details]
Patch
Comment 2 Tor Arne Vestbø (not active) 2010-09-23 03:54:14 PDT
Committed r68128: <http://trac.webkit.org/changeset/68128>
Comment 3 WebKit Review Bot 2010-09-23 04:22:49 PDT
http://trac.webkit.org/changeset/68128 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, and Qt Linux ARMv7 Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/68128
http://trac.webkit.org/changeset/68129
Comment 4 Tor Arne Vestbø (not active) 2010-09-23 04:31:15 PDT
Commited r68130: <http://trac.webkit.org/changeset/68130>
Comment 5 Ademar Reis 2010-09-23 10:25:29 PDT
Cherry-picking it to 2.1 requires a backport due to conflicts with r64829  <http://trac.webkit.org/changeset/64829>, Bug 43427

The most important change affecting the cherry-pick appears to be the change of the createSelectInputMethod() signature in r64829:

(WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp)
-QtAbstractWebPopup* QtPlatformPlugin::createSelectInputMethod()
+QWebSelectMethod* QtPlatformPlugin::createSelectInputMethod()

BTW, r64829 is a large patch and trying to cherry-pick it I see even more conflicts (6 conflicting files). Apparently it requires the changes from Bug 42592 (also a large patch, with 9 file conflicts when cherry-picking to 2.1)

/o\ :-)

Does it make sense to backport only this patch to 2.1, without the previous changes?
Comment 6 Luiz Agostini 2010-09-23 11:03:58 PDT
(In reply to comment #5)
> Cherry-picking it to 2.1 requires a backport due to conflicts with r64829  <http://trac.webkit.org/changeset/64829>, Bug 43427
> 
> The most important change affecting the cherry-pick appears to be the change of the createSelectInputMethod() signature in r64829:
> 
> (WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp)
> -QtAbstractWebPopup* QtPlatformPlugin::createSelectInputMethod()
> +QWebSelectMethod* QtPlatformPlugin::createSelectInputMethod()
> 
> BTW, r64829 is a large patch and trying to cherry-pick it I see even more conflicts (6 conflicting files). Apparently it requires the changes from Bug 42592 (also a large patch, with 9 file conflicts when cherry-picking to 2.1)
> 
> /o\ :-)
> 
> Does it make sense to backport only this patch to 2.1, without the previous changes?

I think that in 2.1 it should be:

QtAbstractWebPopup* QtPlatformPlugin::createSelectInputMethod()
{
     QWebKitPlatformPlugin* p = plugin();
     if (!p)
       return 0;

-    QWebSelectMethod* selector = p->createSelectInputMethod();
+   QWebSelectMethod* selector =qobject_cast<QWebSelectMethod*>(p->createExtension(QWebKitPlatformPlugin::MultipleSelections)) : 0;

    if (!selector)
        return 0;

    return new SelectInputMethodWrapper(selector);
}
Comment 7 Luiz Agostini 2010-09-23 11:09:06 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Cherry-picking it to 2.1 requires a backport due to conflicts with r64829  <http://trac.webkit.org/changeset/64829>, Bug 43427
> > 
> > The most important change affecting the cherry-pick appears to be the change of the createSelectInputMethod() signature in r64829:
> > 
> > (WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp)
> > -QtAbstractWebPopup* QtPlatformPlugin::createSelectInputMethod()
> > +QWebSelectMethod* QtPlatformPlugin::createSelectInputMethod()
> > 
> > BTW, r64829 is a large patch and trying to cherry-pick it I see even more conflicts (6 conflicting files). Apparently it requires the changes from Bug 42592 (also a large patch, with 9 file conflicts when cherry-picking to 2.1)
> > 
> > /o\ :-)
> > 
> > Does it make sense to backport only this patch to 2.1, without the previous changes?
> 
> I think that in 2.1 it should be:
> 
> QtAbstractWebPopup* QtPlatformPlugin::createSelectInputMethod()
> {
>      QWebKitPlatformPlugin* p = plugin();
>      if (!p)
>        return 0;
> 
> -    QWebSelectMethod* selector = p->createSelectInputMethod();
> +   QWebSelectMethod* selector =qobject_cast<QWebSelectMethod*>(p->createExtension(QWebKitPlatformPlugin::MultipleSelections)) : 0;
> 
>     if (!selector)
>         return 0;
> 
>     return new SelectInputMethodWrapper(selector);
> }

please ignore the ": 0" in inserted line in the previous comment. :)

the correct line is:

+   QWebSelectMethod* selector =qobject_cast<QWebSelectMethod*>(p->createExtension(QWebKitPlatformPlugin::MultipleSelections));
Comment 8 Ademar Reis 2010-09-23 13:38:08 PDT
Besides the already discussed change on QtPlatformPlugin::createSelectInputMethod(), we also need the patch from Bug 46402 and the following change:

--- a/WebCore/platform/qt/PlatformHapticsQt.cpp
+++ b/WebCore/platform/qt/PlatformHapticsQt.cpp
@@ -33,7 +33,7 @@ void initHaptics()
 
     if (!pluginLoaded) {
         if (platformPlugin.plugin() && platformPlugin.plugin()->supportsExtension(QWebKitPlatformPlugin::Haptics))
-            player = platformPlugin.plugin()->createHapticFeedbackPlayer();
+            player = qobject_cast<QWebHapticFeedbackPlayer*>(platformPlugin.plugin()->createExtension(QWebKitPlatformPlugin::Haptics));
         pluginLoaded = true;
     }
 }

With those changes I can build and run QtTestBrowser - although I couldn't manage to test the new plugin interface. Anything else?

Does somebody has a testcase or a plugin I can use to test?
Comment 9 Tor Arne Vestbø (not active) 2010-09-23 14:05:57 PDT
(In reply to comment #8)
> Besides the already discussed change on QtPlatformPlugin::createSelectInputMethod(), we also need the patch from Bug 46402 and the following change:
> 
> --- a/WebCore/platform/qt/PlatformHapticsQt.cpp
> +++ b/WebCore/platform/qt/PlatformHapticsQt.cpp
> @@ -33,7 +33,7 @@ void initHaptics()
> 
>      if (!pluginLoaded) {
>          if (platformPlugin.plugin() && platformPlugin.plugin()->supportsExtension(QWebKitPlatformPlugin::Haptics))
> -            player = platformPlugin.plugin()->createHapticFeedbackPlayer();
> +            player = qobject_cast<QWebHapticFeedbackPlayer*>(platformPlugin.plugin()->createExtension(QWebKitPlatformPlugin::Haptics));
>          pluginLoaded = true;
>      }
>  }
> 

This part should ideally be refactored into the WebCore-side helper QtPlatformPlugin like the others, so the qobject_cast is hidden.
Comment 10 Ademar Reis 2010-09-23 14:34:40 PDT
(In reply to comment #9)
> (In reply to comment #8)
>
<snip>
> --- a/WebCore/platform/qt/PlatformHapticsQt.cpp
> +++ b/WebCore/platform/qt/PlatformHapticsQt.cpp
<snip>
> 
> This part should ideally be refactored into the WebCore-side helper QtPlatformPlugin like the others, so the qobject_cast is hidden.

Sure. It also applies to trunk. Bug 46402 has been updated (includes patch)
Comment 11 Ademar Reis 2010-09-23 15:16:08 PDT
Revision r68128 cherry-picked into qtwebkit-2.1 with commit 9b5a4b4 <http://gitorious.org/webkit/qtwebkit/commit/9b5a4b4>
Revision r68130 cherry-picked into qtwebkit-2.1 with commit 0f90c4b <http://gitorious.org/webkit/qtwebkit/commit/0f90c4b>