Bug 78821 – [Qt] API changes to QWebNavigationRequest

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Simon Hausmann]

As discussed in the API review session in Szeged, there are a few simple API changes we should do before the release:


   * Try to change the type of the action property to the real enum
   * Removing originatingUrl property (no clear use-case known. miight be useful, but we should add it if we have a real use-case)
   * Rename modifiers to keyboardModifiers
   * Rename button to mouseButton

[Caio Marcelo de Oliveira Filho]

Created attachment 129096 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 129096 [details]
Patch

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

> Source/WebKit2/ChangeLog:7
> +        [Qt] API changes to QWebNavigationRequest
> +        https://bugs.webkit.org/show_bug.cgi?id=78821
> +
> +        Reviewed by NOBODY (OOPS!).
> +

A summarized info about what changes are being made would be nice

[Caio Marcelo de Oliveira Filho]

Created attachment 129236 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 129236 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qwebnavigationrequest.cpp:29
> +    QWebNavigationRequestPrivate(const QUrl& url, Qt::MouseButton mouseButton,
> +                                 Qt::KeyboardModifiers keyboardModifiers, QQuickWebView::NavigationType navigationType)

I beliuve that the coding style tell to not split up these up into two lines, but keep them as one

> Source/WebKit2/UIProcess/API/qt/qwebnavigationrequest.cpp:50
> +QWebNavigationRequest::QWebNavigationRequest(const QUrl& url, Qt::MouseButton mouseButton,
> +                                             Qt::KeyboardModifiers keyboardModifiers, QQuickWebView::NavigationType navigationType, QObject* parent)

Same here

> Source/WebKit2/UIProcess/API/qt/qwebnavigationrequest_p.h:41
> +    QWebNavigationRequest(const QUrl& url, Qt::MouseButton mouseButton, Qt::KeyboardModifiers keyboardModifiers,
>                            QQuickWebView::NavigationType navigationType, QObject* parent = 0);

and here

> Source/WebKit2/UIProcess/qt/QtWebPagePolicyClient.cpp:45
> +void QtWebPagePolicyClient::decidePolicyForNavigationAction(const QUrl& url, Qt::MouseButton mouseButton,
>                                                              Qt::KeyboardModifiers keyboardModifiers, QQuickWebView::NavigationType navigationType, WKFramePolicyListenerRef listener)

And here

[Caio Marcelo de Oliveira Filho]

Committed r109106: <http://trac.webkit.org/changeset/109106>

[Csaba Osztrogonác]

(In reply to comment #5)
> Committed r109106: <http://trac.webkit.org/changeset/109106>

Reopen, because it broke the build:

cc1plus: warnings being treated as errors
/ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/qt/QtWebPagePolicyClient.cpp: In member function ‘void QtWebPagePolicyClient::decidePolicyForNavigationAction(const QUrl&, Qt::MouseButton, Qt::KeyboardModifiers, QQuickWebView::NavigationType, const OpaqueWKFramePolicyListener*)’:
/ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/qt/QtWebPagePolicyClient.cpp:55: error: case label value exceeds maximum value for type

[Caio Marcelo de Oliveira Filho]

I reverted the part that changes action to be an enum type in bug 79800 because was making our bot GCC unhappy when switching one type with labels from the other.

The 'int' solution works and is OK since we are taking values from 2 different enums (one in WebView other in Experimental). It might be the price to pay for having the Experimental enums, but I wonder if there are other approaches we could take.

Simon, Kenneth: do you have other ideas?

[Caio Marcelo de Oliveira Filho]

For reference: https://bugs.webkit.org/show_bug.cgi?id=73818 moved the DownloadRequest to a separate enum in experimental and http://trac.webkit.org/changeset/103331 fixes the build afterwards in a "similar" way we did here.

[Caio Marcelo de Oliveira Filho]

Patch for bug 80164 changes the 'action' property to the proper enum.