The objective is to implement a delegation client class that will be provided by the WebKit layer and accessible in WebCore layer.
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.
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
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.
style-queue ran check-webkit-style on attachment 45332 [details] without any errors.
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?
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.
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.
(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.
Created attachment 46152 [details] Qt delegation client second try
style-queue ran check-webkit-style on attachment 46152 [details] without any errors.
Comment on attachment 46152 [details] Qt delegation client second try Clearing flags on attachment: 46152 Committed r53005: <http://trac.webkit.org/changeset/53005>
All reviewed patches have been landed. Closing bug.