[Qt][WK2] Extend QRawWebView classes for use as base API.

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Michael Brüning]

Subject says most. 

In principal, QRawWebView, QRawWebViewPrivate and QRawWebViewClient need to be extended so they can act as a real base layer for the QQuickWebView and also for C++ clients.

For this, the base is being set for moving the PageViewportControllerClientQt to the QRawWebView, as well as using the QRawWebViewPrivate as a functional page client.

[Michael Brüning]

Created attachment 193899 [details]
Patch

[Early Warning System Bot]

Comment on attachment 193899 [details]
Patch

Attachment 193899 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17257048

[Michael Brüning]

Created attachment 194001 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 194001 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:529
> +    WebCore::CoordinatedGraphicsScene* scene = coordinatedGraphicsScene();

why not just call is scene() you dont have any other, just like WebPageProxy is called page() many places

> Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:577
> +#endif // ENABLE(GESTURE_EVENTS)

I dint this comment quite useless

> Source/WebKit2/UIProcess/API/qt/raw/qrawwebview_p.h:81
> +    virtual void handleAuthenticationRequiredRequest(const QString&, const QString&, const QString&, QString&, QString&) = 0;

Well I know we like not adding things like const QString& string as it is obvious what string is, but here it is not obvious what those strings mean at all

> Source/WebKit2/UIProcess/API/qt/raw/qrawwebview_p.h:173
> +    void setShowsAsSource(bool showsAsSource);

Shows? why trailing s?

[Michael Brüning]

(In reply to comment #4)
> (From update of attachment 194001 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194001&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:529
> > +    WebCore::CoordinatedGraphicsScene* scene = coordinatedGraphicsScene();
> 
> why not just call is scene() you dont have any other, just like WebPageProxy is called page() many places
> 
I thought that the coordinatedGraphicsScene makes it a bit clearer which scene this is referring to and also, this is consistent with efl and nix and I tried to keep those as aligned as possible should we decide to move to a common WebView and / or C-API implementation.

> > Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:577
> > +#endif // ENABLE(GESTURE_EVENTS)
> 
> I dint this comment quite useless

I usually add those comments to the closing #endifs as I think it adds clarity should the code inside the #if block become larger. We don't really have a rule for this in the style guide, but I think it does not add significant noise. If there's agreement though that it should be removed, I am also fine with that.
> 
> > Source/WebKit2/UIProcess/API/qt/raw/qrawwebview_p.h:81
> > +    virtual void handleAuthenticationRequiredRequest(const QString&, const QString&, const QString&, QString&, QString&) = 0;
True, I'll add those.
> 
> Well I know we like not adding things like const QString& string as it is obvious what string is, but here it is not obvious what those strings mean at all
> 
> > Source/WebKit2/UIProcess/API/qt/raw/qrawwebview_p.h:173
> > +    void setShowsAsSource(bool showsAsSource);
> 
> Shows? why trailing s?
This is for consistency with efl as well (see comment above regarding coordinateGraphicsScene).

[Anders Carlsson]

Comment on attachment 194001 [details]
Patch

Qt has been removed, clearing review flags.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.