Bug 78821 - [Qt] API changes to QWebNavigationRequest
Summary: [Qt] API changes to QWebNavigationRequest
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords:
Depends on: 78867 79800 80164
Blocks: 74403
  Show dependency treegraph
 
Reported: 2012-02-16 08:15 PST by Simon Hausmann
Modified: 2012-03-07 04:54 PST (History)
6 users (show)

See Also:


Attachments
Patch (13.12 KB, patch)
2012-02-27 14:01 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (13.34 KB, patch)
2012-02-28 05:58 PST, Caio Marcelo de Oliveira Filho
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2012-02-16 08:15:16 PST
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
Comment 1 Caio Marcelo de Oliveira Filho 2012-02-27 14:01:28 PST
Created attachment 129096 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-02-28 04:51:01 PST
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
Comment 3 Caio Marcelo de Oliveira Filho 2012-02-28 05:58:26 PST
Created attachment 129236 [details]
Patch
Comment 4 Kenneth Rohde Christiansen 2012-02-28 06:02:14 PST
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
Comment 5 Caio Marcelo de Oliveira Filho 2012-02-28 08:12:14 PST
Committed r109106: <http://trac.webkit.org/changeset/109106>
Comment 6 Csaba Osztrogonác 2012-02-28 08:31:02 PST
(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
Comment 7 Caio Marcelo de Oliveira Filho 2012-02-28 09:14:03 PST
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?
Comment 8 Caio Marcelo de Oliveira Filho 2012-02-28 09:20:04 PST
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.
Comment 9 Caio Marcelo de Oliveira Filho 2012-03-02 09:09:56 PST
Patch for bug 80164 changes the 'action' property to the proper enum.