Bug 62191 – [Qt] Creating webkit2 popup menu hooks and adding popup menus to MiniBrowser.

• Component: WebKit2
• Status: RESOLVED FIXED

[Luiz Agostini]

Creating a simple API for popup menus in QWKPage and implementing MiniBrowser's popup menus.

[Kenneth Rohde Christiansen]

Are you upstreaming the patches from the branch?

[Luiz Agostini]

Created attachment 96214 [details]
patch

[Luiz Agostini]

(In reply to comment #1)
> Are you upstreaming the patches from the branch?

not actually.

[Kenneth Rohde Christiansen]

(In reply to comment #3)
> (In reply to comment #1)
> > Are you upstreaming the patches from the branch?
> 
> not actually.

Well we will need to upsteam them at some point, so it would be nice to know the difference. Anything blocking in you upstreaming those?

[Luiz Agostini]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #1)
> > > Are you upstreaming the patches from the branch?
> > 
> > not actually.
> 
> Well we will need to upsteam them at some point, so it would be nice to know the difference. Anything blocking in you upstreaming those?

No. Just consider this thing quite simple and straightforward.
And MiniBrowser is not that usefull without it.

[Kenneth Rohde Christiansen]

> No. Just consider this thing quite simple and straightforward.
> And MiniBrowser is not that usefull without it.

OK, just make sure that it at least does not block upstreaming the other patches. Upstreaming those would be great of course

[Luiz Agostini]

(In reply to comment #6)
> > No. Just consider this thing quite simple and straightforward.
> > And MiniBrowser is not that usefull without it.
> 
> OK, just make sure that it at least does not block upstreaming the other patches. Upstreaming those would be great of course

Of course both are similar because they do the same job. But there are differences:

1 - This patch just uses the features that are now available in webkit2. So it does not support multiple selections and possible types are only 'Separator' and 'Item'.
2 - To enable the use of this API to implement not full screen popups that try to position themselves near to the <select> component the geometry of the component and current scale factor are available.
3 - There are no slots.
4 - A client class is used to communicate back to WebPopupMenuProxyQt instead of using signals.
5 - There are separated pure virtual classes for the contents (QWKPopupMenuData), the client(QWKPopupMenuClient) and for the popup itself(QWKPopupMenu).
5 - WebPopupMenuProxyQt just destroys QWKPopupMenu objects when they are not needed any more, without calling a 'hide' method.

[Andreas Kling]

Comment on attachment 96214 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96214&action=review

I like where this is going. Let's do another iteration though. :)

> Source/WebKit2/UIProcess/API/qt/qwkpage.h:108
> +    typedef QWKPopupMenu* (*CreatePopupMenuFn)(void *);

Style, void * -> void*.

> Source/WebKit2/UIProcess/API/qt/qwkpage.h:110
> +    QWKPopupMenu* craetePopupMenu();

Typo, createPopupMenu().

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:45
> +

Unnecessary newline.

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:49
> +            return Separator;

Not sure it makes sense that invalid indices claim to be Separators.. is there a precedent for this?
Same comment repeated for all functions in this class, basically. :)

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:78
> +

Unnecessary newline.

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:84
> +    QRect m_rect;
> +    WebCore::TextDirection m_direction;
> +    double m_scaleFactor;
> +    Vector<WebKit::WebPopupItem> m_items;
> +    int m_selectedIndex;

It's good practice to order member variables by size in descending order, to avoid ballooning the class with padding.

> Tools/MiniBrowser/qt/PopupMenu.cpp:117
> +    m_timer.start(1);

Why not start(0)?

[Luiz Agostini]

(In reply to comment #8)
> (From update of attachment 96214 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96214&action=review
> 
> > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:49
> > +            return Separator;
> 
> Not sure it makes sense that invalid indices claim to be Separators.. is there a precedent for this?
> Same comment repeated for all functions in this class, basically. :)

Invalid indices are bad anyway. Would it make sense for them to claim to be Items?
Do you think that we should have a type Invalid to be the type of invalid indexes? 

> 
> > Tools/MiniBrowser/qt/PopupMenu.cpp:117
> > +    m_timer.start(1);
> 
> Why not start(0)?

start(0) is ok.

[Luiz Agostini]

Created attachment 96787 [details]
patch

This patch is a bit different from the previous one. 

The main difference is that in previous patch the QWKPopupMenuData object was only alive during the QWKPopupMenu::show execution. Now the QWKPopupMenu object owns a QWKPopupMenuData object.

[Sam Weinig]

I don't understand why there is a need to allow the client of WebKit2 to control the popup at all.  Can you explain why that level of control is necessary.

[Luiz Agostini]

(In reply to comment #11)

Hi Sam.

> I don't understand why there is a need to allow the client of WebKit2 to control the popup at all.  Can you explain why that level of control is necessary.

Because different platforms may need to have different popups in its browsers.

For example, Maemo and Symbian want to have full screen popups, Meego has its own different needs, and all of them want popups that are very different from the ones we usually see in traditional browsers.

QtWebKit is a library and the actual browser developers are the users of it. We want to allow the browser developers to have full control over the appearance and behavior of the popup menus. Look how different the popup menus are in iPhone, iPad and in MacBook for example.

Do you see problems in this approach?

[Andreas Kling]

Comment on attachment 96787 [details]
patch

r- because this won't apply after the view class refactoring.

Thinking about this, QWKPopupMenu{,Data} is really not the kind of API we want to expose to the end-user, but rather something that belongs in a platform plugin. Popup menus should Just Work(tm) when slapping a QDesktopWebView or QTouchWebView into your application.

[Yael]

(In reply to comment #13)
> (From update of attachment 96787 [details])
> r- because this won't apply after the view class refactoring.
> 
> Thinking about this, QWKPopupMenu{,Data} is really not the kind of API we want to expose to the end-user, but rather something that belongs in a platform plugin. Popup menus should Just Work(tm) when slapping a QDesktopWebView or QTouchWebView into your application.

I think there is a value in customizing the popup menu. The same menu might not be suitable for all applications using QDesktopWebView or QTouchWebView.
In webkit1 we achieved that with the actions that QWebPage was exposing, and I hope we can have the same flexibility in webkit2.

[Luiz Agostini]

*** Bug 47900 has been marked as a duplicate of this bug. ***

[Igor Trindade Oliveira]

Created attachment 103369 [details]
Patch

Proposed patch.

[Benjamin Poulain]

Comment on attachment 103369 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103369&action=review

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:71
> +    void setCreateNewPopupMenuFunction(QWKPopupMenuFactory*, void* data);
> +    QWKPopupMenu* createPopupMenu(const QWKPopupMenuData*, QWKPopupMenuClient*);
> +

No no no no no no
The whole idea is to have a simple component you integrate into QML and provide everything you need.
Especially in the current public views, which are supposed to be simplified as much as possible.

QWKPopupMenu -> We have dropped the "WK" prefix in trunk.

If it is required to have a popup menu exposed in the views, then this needs to be hidden by an other in QML. Ideally, if the feature is implemented correct, you would have it working without making any change to MiniBrowser.

[Igor Trindade Oliveira]

Created attachment 103638 [details]
Patch

Update patch with suggestions discussed in the irc.

[Igor Trindade Oliveira]

Created attachment 103639 [details]
Patch

Changelog updated.

[Alexis Menard (darktears)]

Comment on attachment 103639 [details]
Patch

Could that all thing could be something like :
class QWebKitPopup {
    setItems(const Vector<WebPopupItem>& items) // I oversimplify here
    show(const QRect& geometry);
    hide();
    selectItem(int index) 
}

and QDesktopWebView/QTouchWebView has a void registerWebKitPopup(QWebKitPopup*);

The implementation in desktopwebview creates a default QWebKitPopup which implements a QComboBox or whatever is coming after.

At least it makes the API looks nicer for the touch view. It also allows 3rd party to override the way the popup is shown/rendered. I could totally do a QWebKitPopup based on QML.

[Alexis Menard (darktears)]

(In reply to comment #20)
> (From update of attachment 103639 [details])
> Could that all thing could be something like :
> class QWebKitPopup {
>     setItems(const Vector<WebPopupItem>& items) // I oversimplify here
>     show(const QRect& geometry);
>     hide();
>     selectItem(int index) 
> }
> 
> and QDesktopWebView/QTouchWebView has a void registerWebKitPopup(QWebKitPopup*);
> 
> The implementation in desktopwebview creates a default QWebKitPopup which implements a QComboBox or whatever is coming after.
> 
> At least it makes the API looks nicer for the touch view. It also allows 3rd party to override the way the popup is shown/rendered. I could totally do a QWebKitPopup based on QML.

That was the old patch more or less. I really liked the previous one rather than the new approach, sorry to say.

[Luiz Agostini]

Comment on attachment 103639 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103639&action=review

I think that we could have different WebPopupMenuProxy implementations. We could implement a simple one using QComboBox and use it for now. Then we would have something very similar to what was proposed in previous patch, having QtWebPageProxy::createPopupMenuProxy as the factory.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:174
> +void QDesktopWebViewPrivate::showPopupMenu(QSharedPointer<QComboBox> comboBox)

Why to use a shared pointer if the ownership of this object is not shared?

I don't like it. Why is QComboBox been used by more then one class? It seems dirty and ugly.

> Source/WebKit2/UIProcess/qt/TouchViewInterface.cpp:201
> +void TouchViewInterface::showPopupMenu(QSharedPointer<QComboBox>)

Is touch interface going to use QComboBox as well?

[Kenneth Rohde Christiansen]

Did you guys look at what API we are using for the N9 browser?

[Alexis Menard (darktears)]

(In reply to comment #22)
> (From update of attachment 103639 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103639&action=review
> 
> I think that we could have different WebPopupMenuProxy implementations. We could implement a simple one using QComboBox and use it for now. Then we would have something very similar to what was proposed in previous patch, having QtWebPageProxy::createPopupMenuProxy as the factory.
> 
> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:174
> > +void QDesktopWebViewPrivate::showPopupMenu(QSharedPointer<QComboBox> comboBox)
> 
> Why to use a shared pointer if the ownership of this object is not shared?
> 
> I don't like it. Why is QComboBox been used by more then one class? It seems dirty and ugly.
> 
> > Source/WebKit2/UIProcess/qt/TouchViewInterface.cpp:201
> > +void TouchViewInterface::showPopupMenu(QSharedPointer<QComboBox>)
> 
> Is touch interface going to use QComboBox as well?
 
+1 I don't like it too.

[Kenneth Rohde Christiansen]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 103639 [details] [details])
> > Could that all thing could be something like :
> > class QWebKitPopup {

I really dislike the name popup, I think we call our class something with selector on the N9 branch.

[Benjamin Poulain]

Comment on attachment 103639 [details]
Patch

I'l review later but I am still missing something with this solution.
Before patching anything, I would like someone who know QML better to find a way to have:
-a nice public API for QML developers
-a way for use to write QML code (like creating the popup window with QtComponents) without breaking the nice API

Yes, we will have to cheat with QComboBox for now since Qt components is not ready. But, that should not prevent us from doing the right thing in WebKit. Once we have a clear view of where we go, then we can decide if this is the right solution for now.

[Alexis Menard (darktears)]

(In reply to comment #26)
> (From update of attachment 103639 [details])
> I'l review later but I am still missing something with this solution.
> Before patching anything, I would like someone who know QML better to find a way to have:
> -a nice public API for QML developers
> -a way for use to write QML code (like creating the popup window with QtComponents) without breaking the nice API

The way I think it could be done is to expose a property Q_PROPERTY(QDesktopWebViewExperimental* private READ ...) which contains all the private API that is not tested/stable. It's the QML way to do in C++ what you can achieve when you include Qt private headers. The property is not documented but accessible. There is no need to worry about the binary compatibility and we can remove this property as we want in the future. It's also easy for a code to move from desktopwebview.private.foo() to desktopwebview.foo() when the API is promoted. We can support revision as well. It is much better than a regular C++ subclass that is exposed in QML as a separate object/component because I don't need to change the name of the component/import if I decide to remove the usage of private stuff (or to use it). If you are worry about the getter of the private property being public in the QDesktopWebView h file, we can just use Q_PRIVATE_PROPERTY and it is possible to move the getter in the d pointer, so no public leak of that property :D.

A factory is good enough as you will give the responsibility of the popup creator passed to the factory to create the popup and styles it the way it wants. Though a default implementation based on QCombobox could be provided. I don't think it matters with QtComponents as to the popup can't be integrated with the scene QDesktopWebView/TouchWebView uses (the popup need to go out of the window, at least on desktop or it could be a fullscreen separate top level widget like Maemo/Symbian). Modality is also something solved easily with a regular Qt::Popup widget that is *really* tricky in QML and doesn't follow the DE you are using.


> 
> Yes, we will have to cheat with QComboBox for now since Qt components is not ready. But, that should not prevent us from doing the right thing in WebKit. Once we have a clear view of where we go, then we can decide if this is the right solution for now.

[Igor Trindade Oliveira]

Created attachment 103664 [details]
Patch

This patch is a working in progress. The factory was moved to the private implementation and QDesktopWebView/QTouchWebView can implement it own combobox.

[Luiz Agostini]

Comment on attachment 103664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103664&action=review

> Source/WebKit2/UIProcess/qt/DesktopPopupMenu.cpp:55
> +DesktopPopupMenu::DesktopPopupMenu(const QWKPopupMenuData* data, QWKPopupMenuClient* client, QWidget* parent)

QWK prefix have been dropped.

> Source/WebKit2/UIProcess/qt/qwkpopupmenu.h:27
> +class QWKPopupMenuData {

These classes (QWKPopupMenuData and QWKPopupMenu) does not make sense if they are not going to be public.
They are redundant, add a lot of complexity and no value. I think that we should not have them.

If we are not going to have public classes then we should inherit straight from WebPopupMenuProxy and return the customized object from QtWebPageProxy::createPopupMenuProxy.

For example, we could have

    class QComboBoxWebPopupMenuProxy : public WebPopupMenuProxy {
    public:
        QComboBoxWebPopupMenuProxy(..., QWidget* parentWidget);

    ...
    }

QDesktopWebPageProxy could then return an instance of this class by overriding QtWebPageProxy::createPopupMenuProxy.

I am considering that there will be no public API for popup menu delegates. But we should wait for the discussion about QML API to go further before taking any decision.

[Igor Trindade Oliveira]

Created attachment 104076 [details]
Patch

Proposed patch.

[Igor Trindade Oliveira]

Created attachment 104171 [details]
Patch

Updated patch.

[Alexis Menard (darktears)]

Comment on attachment 104171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104171&action=review

I didn't do a full review yet but I like better that change.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2399
> +#if !PLATFORM(QT)

A comment here on why? I can't see why you do that.

[Igor Trindade Oliveira]

After showPopupMenu is called, WebPopupMenuProxy::invalidate() clears the m_client pointer reference. This is not interesting for us because we are using m_client to send the hidePopup event for WebProcess when needed.

(In reply to comment #32)
> (From update of attachment 104171 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104171&action=review
> 
> I didn't do a full review yet but I like better that change.
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:2399
> > +#if !PLATFORM(QT)
> 
> A comment here on why? I can't see why you do that.

[Benjamin Poulain]

Comment on attachment 104171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104171&action=review

I talked with Jens about using the desktop QML components. That project is just not gonna cut it, and we should not rely on it for now.

Since we have to get rid of QWidget, Jens suggest we put all dependencies to QWidget in plugins. That will be quite some work, but that is something to keep in mind.

I apologize for blocking this patch for a while. I believed the Desktop components were going to make it to Qt5.


When it comes to this patch...this patch looks very weird. Nothing is used in the standard way. That might be correct, but that requires explanations.

> Source/WebKit2/ChangeLog:7
> +        This patch implements a simple QtWebKit WebKit2 popup menu in DesktopWebView.
> +

This changelog needs a lot more explanations.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2401
> +#if !PLATFORM(QT)
>      protectedActivePopupMenu->invalidate();
> +#endif

This invalidate() is here for good reasons. The way WebPopupMenuProxy:: showPopupMenu() is designed is to be used as a blocking API.

I would prefer to refactor this assumption if necessary instead of adding a #ifdef for Qt. Otherwise you are abusing the design of the class and it is opening gates for future bugs when refactoring WebPopupMenuProxy.

> Source/WebKit2/UIProcess/qt/QComboBoxWebPopupMenuProxy.cpp:116
> +
> +    QMouseEvent event(QEvent::MouseButtonPress, QCursor::pos(), Qt::LeftButton,
> +                      Qt::LeftButton, Qt::NoModifier);
> +    QCoreApplication::sendEvent(m_combo, &event);

This needs comments because that looks really weird.

[Igor Trindade Oliveira]

Created attachment 104551 [details]
Patch

Updated patch. Use QEventLoop to block  the main event loop when the popup is being showed and improve ChangeLog.

[Igor Trindade Oliveira]

(In reply to comment #34)
> (From update of attachment 104171 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104171&action=review
> 
> I talked with Jens about using the desktop QML components. That project is just not gonna cut it, and we should not rely on it for now.
> 
> Since we have to get rid of QWidget, Jens suggest we put all dependencies to QWidget in plugins. That will be quite some work, but that is something to keep in mind.

This can be the next step.

> 
> I apologize for blocking this patch for a while. I believed the Desktop components were going to make it to Qt5.
> 
> 
> When it comes to this patch...this patch looks very weird. Nothing is used in the standard way. That might be correct, but that requires explanations.
> 
> > Source/WebKit2/ChangeLog:7
> > +        This patch implements a simple QtWebKit WebKit2 popup menu in DesktopWebView.
> > +
> 
> This changelog needs a lot more explanations.

Ok

> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:2401
> > +#if !PLATFORM(QT)
> >      protectedActivePopupMenu->invalidate();
> > +#endif
> 
> This invalidate() is here for good reasons. The way WebPopupMenuProxy:: showPopupMenu() is designed is to be used as a blocking API.
> 
> I would prefer to refactor this assumption if necessary instead of adding a #ifdef for Qt. Otherwise you are abusing the design of the class and it is opening gates for future bugs when refactoring WebPopupMenuProxy.
> 
> > Source/WebKit2/UIProcess/qt/QComboBoxWebPopupMenuProxy.cpp:116
> > +
> > +    QMouseEvent event(QEvent::MouseButtonPress, QCursor::pos(), Qt::LeftButton,
> > +                      Qt::LeftButton, Qt::NoModifier);
> > +    QCoreApplication::sendEvent(m_combo, &event);
> 
> This needs comments because that looks really weird.

Ok. This is explained in the ChangeLog now.

[Benjamin Poulain]

Comment on attachment 104551 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104551&action=review

> Source/WebKit2/WebKit2.pro:240
>      UIProcess/qt/ClientImpl.h \
> +    UIProcess/qt/QComboBoxWebPopupMenuProxy.h \
>      UIProcess/qt/TouchViewInterface.h \

Alphabetical order.

> Source/WebKit2/WebKit2.pro:473
>      UIProcess/qt/ClientImpl.cpp \
> +    UIProcess/qt/QComboBoxWebPopupMenuProxy.cpp \
>      UIProcess/qt/TouchViewInterface.cpp \

Alphabetical order.

[Igor Trindade Oliveira]

Created attachment 104682 [details]
Patch

Proposed patch. Put the files in alphabetical order.

[Igor Trindade Oliveira]

Created attachment 104691 [details]
Patch

Proposed patch.

[Igor Trindade Oliveira]

Created attachment 105215 [details]
Patch

Proposed patch. Share code with WK1.

[Alexis Menard (darktears)]

Comment on attachment 105215 [details]
Patch

I like that one.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 105215 [details]
Patch

Removing r=? for now. After discussing this patch with Igor, we decided to make another version and upload it soon.

[Caio Marcelo de Oliveira Filho]

Finally closing this. Bug 73560.

[Caio Marcelo de Oliveira Filho]

Finally closing this. Bug 73560 added the itemSelector property to experimental, that can be used to customize popup menus using QML. MiniBrowser was updated to use it.