Bug 32550 - [Qt] Implement combobox delegate for Qt
Summary: [Qt] Implement combobox delegate for Qt
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-15 04:47 PST by Luiz Agostini
Modified: 2009-12-21 10:37 PST (History)
6 users (show)

See Also:


Attachments
Moving list populate methods from PopupMenuQt to QWebPopup. (6.68 KB, patch)
2009-12-15 07:51 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
Creating abstract popup menu class. (6.75 KB, patch)
2009-12-15 09:13 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
Correcting coding style (8.51 KB, patch)
2009-12-15 10:37 PST, Luiz Agostini
kenneth: review-
Details | Formatted Diff | Diff
Some more refactoring. (19.89 KB, patch)
2009-12-15 16:08 PST, Luiz Agostini
kenneth: review-
Details | Formatted Diff | Diff
Abstract web popup factory (4.12 KB, patch)
2009-12-15 16:54 PST, Luiz Agostini
kenneth: review-
Details | Formatted Diff | Diff
Refactoring of class QWebPopup. (20.21 KB, patch)
2009-12-16 08:22 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
Refactoring of class QWebPopup. (20.66 KB, patch)
2009-12-16 09:11 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
Abstract popup menu factory. (5.59 KB, patch)
2009-12-16 14:46 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-15 04:47:40 PST
Implementing a delegation API to enable combobox popup customizations.
Comment 1 Luiz Agostini 2009-12-15 07:51:57 PST
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.
Comment 2 WebKit Review Bot 2009-12-15 07:55:59 PST
style-queue ran check-webkit-style on attachment 44878 [details] without any errors.
Comment 3 Kenneth Rohde Christiansen 2009-12-15 08:01:31 PST
It should be noted that the patch just attached is just one of many needed to implement the feature.
Comment 4 Kenneth Rohde Christiansen 2009-12-15 08:03:51 PST
Comment on attachment 44878 [details]
Moving list populate methods from PopupMenuQt to QWebPopup.

LGTM
Comment 5 WebKit Commit Bot 2009-12-15 08:14:00 PST
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>
Comment 6 WebKit Commit Bot 2009-12-15 08:14:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Luiz Agostini 2009-12-15 09:13:16 PST
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.
Comment 8 WebKit Review Bot 2009-12-15 09:17:35 PST
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
Comment 9 Luiz Agostini 2009-12-15 10:37:07 PST
Created attachment 44888 [details]
Correcting coding style
Comment 10 Luiz Agostini 2009-12-15 10:38:31 PST
Bug was closed by the commit-bot, but the feature is still not fully implemented.
Comment 11 WebKit Review Bot 2009-12-15 10:40:11 PST
style-queue ran check-webkit-style on attachment 44888 [details] without any errors.
Comment 12 Eric Seidel (no email) 2009-12-15 11:20:26 PST
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.
Comment 13 Luiz Agostini 2009-12-15 16:08:20 PST
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.
Comment 14 Luiz Agostini 2009-12-15 16:54:26 PST
Created attachment 44925 [details]
Abstract web popup factory

No behavior changes.
Comment 15 Kenneth Rohde Christiansen 2009-12-16 04:52:44 PST
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.
Comment 16 Kenneth Rohde Christiansen 2009-12-16 04:56:48 PST
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.
Comment 17 Kenneth Rohde Christiansen 2009-12-16 05:01:21 PST
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.
Comment 18 Luiz Agostini 2009-12-16 08:22:32 PST
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.
Comment 19 WebKit Review Bot 2009-12-16 08:23:58 PST
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
Comment 20 Luiz Agostini 2009-12-16 09:11:36 PST
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.
Comment 21 WebKit Review Bot 2009-12-16 09:15:38 PST
style-queue ran check-webkit-style on attachment 44983 [details] without any errors.
Comment 22 WebKit Commit Bot 2009-12-16 09:27:16 PST
Comment on attachment 44983 [details]
Refactoring of class QWebPopup.

Clearing flags on attachment: 44983

Committed r52199: <http://trac.webkit.org/changeset/52199>
Comment 23 WebKit Commit Bot 2009-12-16 09:27:22 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Luiz Agostini 2009-12-16 13:57:35 PST
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.
Comment 25 Luiz Agostini 2009-12-16 14:46:55 PST
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.
Comment 26 WebKit Review Bot 2009-12-16 14:51:34 PST
style-queue ran check-webkit-style on attachment 45015 [details] without any errors.
Comment 27 WebKit Commit Bot 2009-12-16 15:52:46 PST
Comment on attachment 45015 [details]
Abstract popup menu factory.

Clearing flags on attachment: 45015

Committed r52223: <http://trac.webkit.org/changeset/52223>
Comment 28 WebKit Commit Bot 2009-12-16 15:52:54 PST
All reviewed patches have been landed.  Closing bug.