Bug 32550 – [Qt] Implement combobox delegate for Qt

• Component: Platform
• Status: RESOLVED FIXED

[Luiz Agostini]

Implementing a delegation API to enable combobox popup customizations.

[Luiz Agostini]

Created attachment 44878 [details]
Moving list populate methods from PopupMenuQt to QWebPopup.

The methods that populate the combobox popup will be implemented by the popup customization class. They could not be in PopupMenu and were moved to QWebPopup.

[WebKit Review Bot]

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

[Kenneth Rohde Christiansen]

It should be noted that the patch just attached is just one of many needed to implement the feature.

[Kenneth Rohde Christiansen]

Comment on attachment 44878 [details]
Moving list populate methods from PopupMenuQt to QWebPopup.

LGTM

[WebKit Commit Bot]

Comment on attachment 44878 [details]
Moving list populate methods from PopupMenuQt to QWebPopup.

Clearing flags on attachment: 44878

Committed r52151: <http://trac.webkit.org/changeset/52151>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Luiz Agostini]

Created attachment 44883 [details]
Creating abstract popup menu class.

Creating QAbstractPopupMenu class and QDefaultPopupMenu class that inherits from QAbstractPopupMenu.
Custom combobox popup menu implementations will inherit from QAbstractPopupMenu.
QDefaultPopupMenu needs to be improved to provide standard features that are missing in QtWebKit.

No behaviour changes.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/PopupMenu.h:45:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1

[Luiz Agostini]

Created attachment 44888 [details]
Correcting coding style

[Luiz Agostini]

Bug was closed by the commit-bot, but the feature is still not fully implemented.

[WebKit Review Bot]

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

[Eric Seidel (no email)]

Generally we try to do one patch per bug, and open additional bugs for additional patches.  You can always use the "depends on" and "blocks" features to indicate that bugs are related.

[Luiz Agostini]

Created attachment 44920 [details]
Some more refactoring.

Class QWebPopup and QWebPopup.* files renamed to QtAbstractWebPopup.
Class QDefaultWebPopup renamed to QtFallbackWebPopup and moved to its own file.
    
No behavior changes.

[Luiz Agostini]

Created attachment 44925 [details]
Abstract web popup factory

No behavior changes.

[Kenneth Rohde Christiansen]

Comment on attachment 44920 [details]
Some more refactoring.

Could you merge this patch with the previous one?

Due to the layering new delegate implementations cannot inherit directly from the QtAbstractWebPopup, so that should be more clear in the ChangeLog.

[Kenneth Rohde Christiansen]

Comment on attachment 44888 [details]
Correcting coding style

QAbstractPopupMenu and QDefaultPopupMenu are bad names as they are Qt specific WebCore classes, and not Qt public classes. In other places we have prefixes these with Qt* instead.

[Kenneth Rohde Christiansen]

Comment on attachment 44925 [details]
Abstract web popup factory

Please explain better in the ChangeLog where you are heading and why you did this particular change.

[Luiz Agostini]

Created attachment 44978 [details]
Refactoring of class QWebPopup.

Class QWebPopup has been split in QtAbstractWebPopup and QtFallbackWebPopup.
Both new classes are now in corresponding files and files QWebPopup.* have been removed.

Custom combo popup classes will inherit from QtAbstractWebPopup. It is not the public API as it is
in WebCore and will not be visible by users of QtWebKit. It will be used in implementation of
public QtWebKit combobox popup delegation API.

Class QtFallbackWebPopup inherits from QtAbstractWebPopup and implements the currently used combobox popup.
It needs to be improved to provide standard features that are missing like style or indentation.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Skipping input 'WebCore/platform/qt/QWebPopup.cpp': Can't open for reading
WebCore/platform/qt/QtFallbackWebPopup.cpp:22:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebCore/platform/qt/QtAbstractWebPopup.cpp:22:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Skipping input 'WebCore/platform/qt/QWebPopup.h': Can't open for reading
WebCore/platform/qt/QtAbstractWebPopup.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3

[Luiz Agostini]

Created attachment 44983 [details]
Refactoring of class QWebPopup.

Class QWebPopup has been split in QtAbstractWebPopup and QtFallbackWebPopup.
Both new classes are now in corresponding files and files QWebPopup.* have been removed.
    
Custom combo popup classes will inherit from QtAbstractWebPopup. It is not the public API as it is
in WebCore and will not be visible by users of QtWebKit. It will be used in implementation of
public QtWebKit combobox popup delegation API.
    
Class QtFallbackWebPopup inherits from QtAbstractWebPopup and implements the currently used combobox popup.
It needs to be improved to provide standard features that are missing like style or indentation.

[WebKit Review Bot]

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

[WebKit Commit Bot]

Comment on attachment 44983 [details]
Refactoring of class QWebPopup.

Clearing flags on attachment: 44983

Committed r52199: <http://trac.webkit.org/changeset/52199>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Luiz Agostini]

Bug was closed by the commit-bot, but the feature is still not fully
implemented.

I am trying to send just really small patches on these first times. That is why I am sending several patches on this same bug. There is no sense on creating one bug for each such small patches.

[Luiz Agostini]

Created attachment 45015 [details]
Abstract popup menu factory.

The objective is to make it easy to replace the combobox popup at WebCore layer providing
support to the combobox popup delegation API. Future patches will make it possible to
replace the combobox popup at WebKit layer.

[WebKit Review Bot]

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

[WebKit Commit Bot]

Comment on attachment 45015 [details]
Abstract popup menu factory.

Clearing flags on attachment: 45015

Committed r52223: <http://trac.webkit.org/changeset/52223>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.