Bug 32826 – [Qt] Delegation client

• Component: Platform
• Status: RESOLVED FIXED

[Luiz Agostini]

The objective is to implement a delegation client class that will be provided by the WebKit layer and accessible in WebCore layer.

[Luiz Agostini]

Created attachment 45329 [details]
Qt delegation client

The new class QtPlatformPageClient replaced the class QWebPageClient
as type PlataformPageClient (Widget.h 79). QtPlatformPageClient is composed
by an instance of QWebPageClient and an instance of the new class
QtDelegationClient. This way an object of class QtDelegationClient provided by
the WebKit layer is easily accessible in WebCore layer through the
HostWindow intreface.
    
All related changes have been made in WebCore and WebKit layers.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/qt/QtDelegationClient.h:1:  #ifndef header guard has wrong style, please use: QtDelegationClient_h  [build/header_guard] [5]
WebCore/platform/qt/QtDelegationClient.h:14:  #endif line should be "#endif // QtDelegationClient_h"  [build/header_guard] [5]
WebCore/platform/qt/QtFallbackWebPopup.cpp:52:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 3

[Luiz Agostini]

Created attachment 45332 [details]
Qt delegation client

The new class QtPlatformPageClient replaced the class QWebPageClient
as type PlataformPageClient (Widget.h 79). QtPlatformPageClient is composed
by an instance of QWebPageClient and an instance of the new class
QtDelegationClient. This way an object of class QtDelegationClient provided by
the WebKit layer is easily accessible in WebCore layer through the
HostWindow intreface.

All related changes have been made in WebCore and WebKit layers.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45332 [details] without any errors.

[Kenneth Rohde Christiansen]

I didn't like this change very much. It seems a bit the wrong way around. You could have added a new delegate() method or so to our QWebPageClient, but I'm not sure this is really the right place for it.

For me, the delegates belong to the chrome, so I would just add a Qt special method for creating combos etc, like ::createSelectionPopup() or so. You could make it Qt specific and do a cast to ChromeClientQt when creating the combos.

What do you think Simon?

[Luiz Agostini]

The first idea was to put new methods in class QWebPageClient but after a talk with you Kenneth I was convinced that it was not the place for it.

I did not use the cast because at the point where the methods are needed I just have access to an instance of the abstract class HostWindow. I know it is indeed an instance of chrome but using this information I would be throwing away the HostWindow abstraction.

I think that chrome has a standard way to provide this kind of service through the method platformPageClient of the HostWindow interface. Today platformPageClient is returning an instance of the class QWebPageClient. The proposal is to return an instance of a new class (QtPlatformPageClient) that aggregates an instance of QWebPageClient and an instance of another new class (QtDelegationClient) responsible for delegates. I believe that chrome would be then providing the desired functionality in a standard way without messing with QWebPageClient's scope or responsibilities.

This solution is Qt specific. Although the patch makes changes in several files the changes are all very simple. QWebPageClient is still available and QtDelegationClient can be used for combobox popups and for the delegates that will come in future.

[Kenneth Rohde Christiansen]

Why return another class returning the original one plus the delegate. Why not just add a new method to OUR QWebPageClient class in QWebPageClient.h. We are free to do so. 

(In reply to comment #6)
> The first idea was to put new methods in class QWebPageClient but after a talk
> with you Kenneth I was convinced that it was not the place for it.
> 
> I did not use the cast because at the point where the methods are needed I just
> have access to an instance of the abstract class HostWindow. I know it is
> indeed an instance of chrome but using this information I would be throwing
> away the HostWindow abstraction.
> 
> I think that chrome has a standard way to provide this kind of service through
> the method platformPageClient of the HostWindow interface. Today
> platformPageClient is returning an instance of the class QWebPageClient. The
> proposal is to return an instance of a new class (QtPlatformPageClient) that
> aggregates an instance of QWebPageClient and an instance of another new class
> (QtDelegationClient) responsible for delegates. I believe that chrome would be
> then providing the desired functionality in a standard way without messing with
> QWebPageClient's scope or responsibilities.
> 
> This solution is Qt specific. Although the patch makes changes in several files
> the changes are all very simple. QWebPageClient is still available and
> QtDelegationClient can be used for combobox popups and for the delegates that
> will come in future.

[Simon Hausmann]

(In reply to comment #5)
> I didn't like this change very much. It seems a bit the wrong way around. You
> could have added a new delegate() method or so to our QWebPageClient, but I'm
> not sure this is really the right place for it.
> 
> For me, the delegates belong to the chrome, so I would just add a Qt special
> method for creating combos etc, like ::createSelectionPopup() or so. You could
> make it Qt specific and do a cast to ChromeClientQt when creating the combos.
> 
> What do you think Simon?

I agree, this should go through the chrome layer.

[Luiz Agostini]

Created attachment 46152 [details]
Qt delegation client second try

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46152 [details] without any errors.

[WebKit Commit Bot]

Comment on attachment 46152 [details]
Qt delegation client second try

Clearing flags on attachment: 46152

Committed r53005: <http://trac.webkit.org/changeset/53005>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.