Bug 32826 - [Qt] Delegation client
Summary: [Qt] Delegation client
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-21 08:03 PST by Luiz Agostini
Modified: 2010-01-11 06:21 PST (History)
6 users (show)

See Also:


Attachments
Qt delegation client (26.66 KB, patch)
2009-12-21 08:15 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
Qt delegation client (29.24 KB, patch)
2009-12-21 08:33 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
Qt delegation client second try (24.51 KB, patch)
2010-01-08 13:25 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luiz Agostini 2009-12-21 08:03:25 PST
The objective is to implement a delegation client class that will be provided by the WebKit layer and accessible in WebCore layer.
Comment 1 Luiz Agostini 2009-12-21 08:15:14 PST
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.
Comment 2 WebKit Review Bot 2009-12-21 08:20:33 PST
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
Comment 3 Luiz Agostini 2009-12-21 08:33:46 PST
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.
Comment 4 WebKit Review Bot 2009-12-21 08:36:26 PST
style-queue ran check-webkit-style on attachment 45332 [details] without any errors.
Comment 5 Kenneth Rohde Christiansen 2009-12-29 06:20:03 PST
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?
Comment 6 Luiz Agostini 2009-12-29 15:05:02 PST
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.
Comment 7 Kenneth Rohde Christiansen 2009-12-30 02:03:10 PST
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.
Comment 8 Simon Hausmann 2009-12-30 06:53:16 PST
(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.
Comment 9 Luiz Agostini 2010-01-08 13:25:21 PST
Created attachment 46152 [details]
Qt delegation client second try
Comment 10 WebKit Review Bot 2010-01-08 13:27:35 PST
style-queue ran check-webkit-style on attachment 46152 [details] without any errors.
Comment 11 WebKit Commit Bot 2010-01-08 13:48:11 PST
Comment on attachment 46152 [details]
Qt delegation client second try

Clearing flags on attachment: 46152

Committed r53005: <http://trac.webkit.org/changeset/53005>
Comment 12 WebKit Commit Bot 2010-01-08 13:48:19 PST
All reviewed patches have been landed.  Closing bug.