Bug 112735 - [Qt][WK2] Extend QRawWebView classes for use as base API.
Summary: [Qt][WK2] Extend QRawWebView classes for use as base API.
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Brüning
URL:
Keywords:
Depends on:
Blocks: 109204
  Show dependency treegraph
 
Reported: 2013-03-19 12:31 PDT by Michael Brüning
Modified: 2014-02-03 03:25 PST (History)
5 users (show)

See Also:


Attachments
Patch (54.65 KB, patch)
2013-03-19 12:49 PDT, Michael Brüning
no flags Details | Formatted Diff | Diff
Patch (55.39 KB, patch)
2013-03-20 01:48 PDT, Michael Brüning
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Brüning 2013-03-19 12:31:50 PDT
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.
Comment 1 Michael Brüning 2013-03-19 12:49:39 PDT
Created attachment 193899 [details]
Patch
Comment 2 Early Warning System Bot 2013-03-19 13:11:46 PDT
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
Comment 3 Michael Brüning 2013-03-20 01:48:50 PDT
Created attachment 194001 [details]
Patch
Comment 4 Kenneth Rohde Christiansen 2013-03-20 02:11:10 PDT
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?
Comment 5 Michael Brüning 2013-03-20 04:41:24 PDT
(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).
Comment 6 Anders Carlsson 2013-10-02 21:26:35 PDT
Comment on attachment 194001 [details]
Patch

Qt has been removed, clearing review flags.
Comment 7 Jocelyn Turcotte 2014-02-03 03:25:22 PST
=== 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.