[Qt][WK2] Private non-QtQuick API

• Component: Platform
• Status: RESOLVED FIXED

[Noam Rosenthal]

Right now the only way to use WebKit2 is either inside QML or using a C++ QQuickCanvas.
This is ok for what we want to support publicly at this point, but there has been requests for an API that can work without QtQuick, e.g. just rendering the web-view directly to the current OpenGL context, and passing events directly through the API.

[Noam Rosenthal]

The current thinking is to use the WebKit2 C API, with an additional WKView-like file that can render to a GL context. We're experimenting with this in a branch.

[Kenneth Rohde Christiansen]

(In reply to comment #1)
> The current thinking is to use the WebKit2 C API, with an additional WKView-like file that can render to a GL context. We're experimenting with this in a branch.

What about the whole interaction model?

[Noam Rosenthal]

(In reply to comment #2)
> (In reply to comment #1)
> > The current thinking is to use the WebKit2 C API, with an additional WKView-like file that can render to a GL context. We're experimenting with this in a branch.
> 
> What about the whole interaction model?

There are two options:
1. expose that via the WKView API
2. Allow people to do whatever they want in their browser code, and expose only what's necessary for them to implement them.

But we haven't gotten that far yet. What do you think?
Maybe we should discuss this over IRC at some point rather than on this bug :)

[Kenneth Rohde Christiansen]

Yes, let's do that.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #3)
> But we haven't gotten that far yet. What do you think?
> Maybe we should discuss this over IRC at some point rather than on this bug :)

Would be nice to post a summary of the discussion here.

[Noam Rosenthal]

(In reply to comment #5)
> (In reply to comment #3)
> > But we haven't gotten that far yet. What do you think?
> > Maybe we should discuss this over IRC at some point rather than on this bug :)
> 
> Would be nice to post a summary of the discussion here.

The main issues are the viewport interaction engine and the gesture handler. Some of the decisions we're making inside WebKit2 are more of a particular browser behavior than something that can be used by a non-browser web application.

We agreed to keep that in mind when we get to those issues - at least the first version we're doing might not even need some of those features.

We'll keep communicating as we go along :)

[Noam Rosenthal]

Currently the plan is to put this under the codename "qtcb" (Qt C bindings). We'll have UIProcess/API/qt/qtcb
.. and MiniBrowser/qtcb

This is the best we could come up with, if someone has better suggestions please don't be shy.

[Luiz Agostini]

Created attachment 145712 [details]
patch

This is a first version of the patch. I am submitting it as it is, without changelog.

This patch contains a very simple Api to be used together with the C Api. It contais as well a demo browser named MiniBrowser_cb (c bindings). Both, Api and browser, are not complete. The idea is to increment the Api as needed, but to keep it as small and simple as possible.

[Luiz Agostini]

Created attachment 145713 [details]
patch

a small correction to the previous patch.

[Alexis Menard (darktears)]

(In reply to comment #9)
> Created an attachment (id=145713) [details]
> patch
> 
> a small correction to the previous patch.

Some questions :
- Is that a long term commitment? Noam can you guarantee that people will maintain that API? What happen if your project is discontinued? The Qt Quick API goes to Qt 5 which means that we will have users of it other than Nokia and that worst case the community will step up to work on it.

- The API doesn't respect Qt guidelines API, e.g. the d pointer.

- What about tests? I'm sorry to be the annoying guy but every new API needs to be tested. None of us out there have the thing you are working on therefore without test if we break that API we can't know for sure if it works or not. I'm not very sure the MiniBrowser is good enough in that matter.

I know I'm annoying but I'd like to make sure that what we are checking in is not research work, we commit to it and it follows our usual guidelines.

Other than that nice to see you back luis!

[Noam Rosenthal]

(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=145713) [details] [details]
> > patch
> > 
> > a small correction to the previous patch.
> 
> Some questions :
> - Is that a long term commitment? Noam can you guarantee that people will maintain that API? 

It's a private API, it should have the proper warnings. It's not a committed part of the Qt port, nor is it a committed part of any port. It's there for the sole purpose of having C++ glue to WebKit without exposing WebCore classes, and we're upstreaming it so that it doesn't break.

> - The API doesn't respect Qt guidelines API, e.g. the d pointer.
It's not a Qt API. It's C/C++ bindings to the Qt port.

[Noam Rosenthal]

Comment on attachment 145713 [details]
patch

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

I think this needs a test, to make sure people don't break it. Also some nitpicks :)

> Source/WebKit2/ChangeLog:6
> +        [Qt][WK2] Private non-QtQuick API
> +        https://bugs.webkit.org/show_bug.cgi?id=84532
> +
> +        Reviewed by NOBODY (OOPS!).

Needs some explanation about this being a private API (meaning not packaged together with Qt or part of the Qt port).
Also explain that it's guarded by a qmake CONFIG flag.

> Source/WebKit2/UIProcess/API/qt/WKView.h:23
> +    the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> +    Boston, MA 02110-1301, USA.
> +*/
> +
> +#ifndef WKView_h
> +#define WKView_h
> +#include "qwebkitglobal.h"
> +

I think this needs a warning that it's not part of the Qt API in any way.

> Source/WebKit2/UIProcess/API/qt/WKViewImpl.cpp:118
> +void WKViewImpl::setUrl(const QUrl& url)
> +{
> +    m_url = url;
> +    m_webPageProxy->loadURL(url.toString());
> +}

I'd rather if we just had loadURL function.

> Source/WebKit2/UIProcess/API/qt/WKViewImpl.h:163
> +
> +

Extra lines

> Tools/MiniBrowser/qtcb/View.cpp:151
> +    qDebug() << __PRETTY_FUNCTION__ << __LINE__;

Need to remove these debug messages before upstreaming.

[Noam Rosenthal]

Comment on attachment 145713 [details]
patch

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

> Source/WebKit2/UIProcess/API/qt/WKViewImpl.cpp:99
> +    WebPreferences* wkPrefs = m_webPageProxy->pageGroup()->preferences();
> +    wkPrefs->setDeviceDPI(160);
> +    wkPrefs->setDeviceWidth(m_size.width());
> +    wkPrefs->setDeviceHeight(m_size.height());

Can we do this via the regular C API?

>> Source/WebKit2/UIProcess/API/qt/WKViewImpl.cpp:118
>> +void WKViewImpl::setUrl(const QUrl& url)
>> +{
>> +    m_url = url;
>> +    m_webPageProxy->loadURL(url.toString());
>> +}
> 
> I'd rather if we just had loadURL function.

Actually, I'd prefer if we didn't expose it at all and just call WKPageLoadURL

[Luiz Agostini]

(In reply to comment #13)
> (From update of attachment 145713 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145713&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/WKViewImpl.cpp:99
> > +    WebPreferences* wkPrefs = m_webPageProxy->pageGroup()->preferences();
> > +    wkPrefs->setDeviceDPI(160);
> > +    wkPrefs->setDeviceWidth(m_size.width());
> > +    wkPrefs->setDeviceHeight(m_size.height());
> 
> Can we do this via the regular C API?

We don't need those lines. I will double check it.

> 
> >> Source/WebKit2/UIProcess/API/qt/WKViewImpl.cpp:118
> >> +void WKViewImpl::setUrl(const QUrl& url)
> >> +{
> >> +    m_url = url;
> >> +    m_webPageProxy->loadURL(url.toString());
> >> +}
> > 
> > I'd rather if we just had loadURL function.
> 
> Actually, I'd prefer if we didn't expose it at all and just call WKPageLoadURL

sure.

[Luiz Agostini]

(In reply to comment #12)
> (From update of attachment 145713 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145713&action=review
> 
> I think this needs a test, to make sure people don't break it. Also some nitpicks :)

> Needs some explanation about this being a private API (meaning not packaged together with Qt or part of the Qt port).
> Also explain that it's guarded by a qmake CONFIG flag.

> I think this needs a warning that it's not part of the Qt API in any way.

> I'd rather if we just had loadURL function.

> Extra lines

> Need to remove these debug messages before upstreaming.

I will work on those issues today. I uploaded it as it was yesterday for the guys to see it, because of the time difference.

[Luiz Agostini]

(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=145713) [details] [details]
> > patch

> Other than that nice to see you back luis!

Thanks! It is nice to be back!

[Tor Arne Vestbø]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Created an attachment (id=145713) [details] [details] [details]
> > > patch
> > > 
> > > a small correction to the previous patch.
> > 
> > Some questions :
> > - Is that a long term commitment? Noam can you guarantee that people will maintain that API? 
> 
> It's a private API, it should have the proper warnings. It's not a committed part of the Qt port, nor is it a committed part of any port. It's there for the sole purpose of having C++ glue to WebKit without exposing WebCore classes, and we're upstreaming it so that it doesn't break.

I think the bar for landing stuff in trunk is higher than that.

I'd like to hear your comment on Alexi's question about tests.

> > - The API doesn't respect Qt guidelines API, e.g. the d pointer.
> It's not a Qt API. It's C/C++ bindings to the Qt port.

If it's part of the Qt port (as a side project) it should follow Qt standards.

[Noam Rosenthal]

> I think the bar for landing stuff in trunk is higher than that.
We're 
 
> I'd like to hear your comment on Alexi's question about tests.
Of course we will include tests, I think I've commented on that already. We wanted to put a patch that contains the general direction early.

> > It's not a Qt API. It's C/C++ bindings to the Qt port.
> If it's part of the Qt port (as a side project) it should follow Qt standards.
Sure, we can go down that route. Though I thought those Qt standards were about Qt public APIs in order to maintain binary-compatibility, which is not the goal of this API.

[Noam Rosenthal]

(In reply to comment #18)
> > I think the bar for landing stuff in trunk is higher than that.
> We're 
(oops, forgot to finish the sentence).
We're setting the bar as high as any other WebKit contribution, and as high as other private Qt APIs. If there's a specific case where you feel this is below that bar, please speak up.

[Luiz Agostini]

Created attachment 145953 [details]
patch

[Luiz Agostini]

The tests in tst_qtcb are pixel tests. WKView is actually very convenient to implement pixel tests.

[Jocelyn Turcotte]

I think we have to be careful with this kind of direction since it can become far more complex than these initial patches in 1 year, after it evolved.

You'll need a different PageClient and a complete different set of UIProcess proxy classes. You'll eventually duplicate at least half of the code in WebKit2/UIProcess/qt and API/qt.
You'll start needing different messages and web process behaviors, and there won't be implementations for every features each API has, so we'll need to start guarding stuff with #if !USE(QTCB) or #if PLATFORM(QT) && !USE(QTCB).

You're essentially starting a new port of WebKit2.

I understand how upstreaming this can be beneficial for you, but the cost is however I think considerably high for QtWebKit as a whole. I'm not saying that we shouldn't do it, but that this should be thought through first.

After all, all this is just to avoid the overhead of QQuickCanvas, is it worth it? Did anybody try to measure how much performance/memory this could actually save?

[Noam Rosenthal]

(In reply to comment #22)
> I think we have to be careful with this kind of direction since it can become far more complex than these initial patches in 1 year, after it evolved.
> 
> You'll need a different PageClient and a complete different set of UIProcess proxy classes. You'll eventually duplicate at least half of the code in WebKit2/UIProcess/qt and API/qt.

We'll need to do that anyway if we want a custom behavior that's not like the one in UIProcess/qt. I'd rather have the API and do it outside WebKit trunk, to avoid introducing noise to the current implementation.

> You'll start needing different messages and web process behaviors, and there won't be implementations for every features each API has, so we'll need to start guarding stuff with #if !USE(QTCB) or #if PLATFORM(QT) && !USE(QTCB).

We're not going to do any of that. What we're talking about is essentially two cross-process state machines - viewport handling and gesture handling. If we want to reuse them, we'll expose them the same way we do with LayerTreeHostProxy, which handles the async state-machine internally but exposes a simple internal non-Qt API.

> You're essentially starting a new port of WebKit2.
> 
> I understand how upstreaming this can be beneficial for you, but the cost is however I think considerably high for QtWebKit as a whole. I'm not saying that we shouldn't do it, but that this should be thought through first.

I think this would benefit QtWebKit, not just "our project". Having a clean (private) API to *WebKit* rather than to some default browser behavior we decided to put in the QML API layer can help us test WebKit in a much cleaner way, separate the QML-API concerns and test them specifically in the API tests. For example it would help us clean out the current code paths that work differently in WTR than they do in MiniBrowser.
 
> After all, all this is just to avoid the overhead of QQuickCanvas, is it worth it? Did anybody try to measure how much performance/memory this could 
actually save?

It's more about removing a huge chunk of code from the equation that we spend all of our time working around because it does things differently than how we need them to behave. It's not about performance yet, but I'm sure that would come up as well.

Right now WebKit and the default browser behavior are entangled together, while people who need to productize browsers usually have to fork that behavior anyway. I propose to separate those two, leaving the "default behavior for QML views" feature in its own space (I think even outside trunk, but that's a separate discussion), and allowing lower-level access via the existing C APIs when you don't want that default behavior.

[Tor Arne Vestbø]

(In reply to comment #23)
> Right now WebKit and the default browser behavior are entangled together, while people who need to productize browsers usually have to fork that behavior anyway. I propose to separate those two, leaving the "default behavior for QML views" feature in its own space (I think even outside trunk, but that's a separate discussion), and allowing lower-level access via the existing C APIs when you don't want that default behavior.

Can you expand on where specifically there are entanglements, and how they are fundamental to the QML bindings and not some features/settings that are just missing? 

Can you also expand on what you mean by leaving the "default behavior for QML views" outside of trunk?

[Noam Rosenthal]

> Can you expand on where specifically there are entanglements, and how they are fundamental to the QML bindings and not some features/settings that are just missing? 

I specifically mean that the API only exposes the default browser behavior (e.g. the gesture state-machine), and to use that you have to use a QQuickWebView inside a QQuickCanvas.
If we had a browser that wanted to implement a slightly different sequence for gestures, there's no way to do it without forking webkit or introducing a lot more complexity to the gesture state-machine, which is already rather complex.

> Can you also expand on what you mean by leaving the "default behavior for QML views" outside of trunk?

If I look at the WebKit project goals, it's stated that "WebKit is a content engine and not a browser". Some of the behaviors we implement in the API layer (see above) are specific to an interaction paradigm or UI decisions, and can even be specific to a device or OS. That code is also very dynamic, and change rather rapidly as Qt5 is still evolving - that layer I think changes much more than any other part we're working on, and is the most prone to changes in Qt (it relies on 2-3 modules outside of qtbase).

IMO if we could expose the C+WKView API in webkit trunk, then we could develop the QtQuick bindings as part of the Qt project in a Qt repo and host it there, making it the public API if you're getting WebKit via Qt. For QML embedders it wouldn't change a thing, but for us it would make it much easier to develop those QtQuick bindings, without bringing core QtWebKit development to a halt.
In the context of the Qt project we can choose any goals we like, probably good QtQuick default behavior is a good goal for that project.

[Noam Rosenthal]

(In reply to comment #25)
> > Can you expand on where specifically there are entanglements, and how they are fundamental to the QML bindings and not some features/settings that are just missing? 
> 
> I specifically mean that the API only exposes the default browser behavior (e.g. the gesture state-machine), and to use that you have to use a QQuickWebView inside a QQuickCanvas.
> If we had a browser that wanted to implement a slightly different sequence for gestures, there's no way to do it without forking webkit or introducing a lot more complexity to the gesture state-machine, which is already rather complex.
> 
> > Can you also expand on what you mean by leaving the "default behavior for QML views" outside of trunk?
> 
> If I look at the WebKit project goals, it's stated that "WebKit is a content engine and not a browser". Some of the behaviors we implement in the API layer (see above) are specific to an interaction paradigm or UI decisions, and can even be specific to a device or OS. That code is also very dynamic, and change rather rapidly as Qt5 is still evolving - that layer I think changes much more than any other part we're working on, and is the most prone to changes in Qt (it relies on 2-3 modules outside of qtbase).
> 
> IMO if we could expose the C+WKView API in webkit trunk, then we could develop the QtQuick bindings as part of the Qt project in a Qt repo and host it there, making it the public API if you're getting WebKit via Qt. For QML embedders it wouldn't change a thing, but for us it would make it much easier to develop those QtQuick bindings, without bringing core QtWebKit development to a halt.
> In the context of the Qt project we can choose any goals we like, probably good QtQuick default behavior is a good goal for that project.

btw this is just my opinion as an answer to your question, not a proposal, a plan or something that's my responsibility. I think we should not drag this patch to a discussion on QtWebkit project goals.

[Luiz Agostini]

Created attachment 146393 [details]
patch

some test improvements.

[WebKit Review Bot]

Attachment 146393 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit2/ChangeLog', u..." exit_code: 1
Source/WebKit2/UIProcess/API/qt/tests/qtcb/tst_qtcb.cpp:97:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebKit2/UIProcess/API/qt/tests/qtcb/tst_qtcb.cpp:135:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Luiz Agostini]

Created attachment 146595 [details]
patch

coding style fix

[Simon Hausmann]

Let's discuss this in detail face-to-face in Berlin.

On a higher level I think one thing that this should be landed either together with or at least very close by is the enabling of TestWebKitAPI. This is an opportunity to re-use those tests and it would seem like a fit.

In terms of build flags I think this should always be enabled, just not be shipped with public header files at this point.

[Tor Arne Vestbø]

Comment on attachment 146595 [details]
patch

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

> Source/WebKit2/Target.pri:798
> +contains(CONFIG, qtcb) {

I think in general, if we're gonna do this, then it shouldn't be behind a build flag, it should be built by default. That means we can skip the awkward "qtcb" prefix as well.

> Source/WebKit2/Target.pri:801
> +        UIProcess/API/cpp/qt/WKView.h \
> +        UIProcess/API/cpp/qt/WKViewImpl.h

QWKView_p.h
QWKView_p_p.h

> Source/WebKit2/Target.pri:803
> +        UIProcess/API/cpp/qt/WKViewImpl.cpp

QWKView.cpp

> Source/WebKit2/UIProcess/API/cpp/qt/WKViewImpl.h:53
> +    virtual void handleDownloadRequest(DownloadProxy* download) { /* FIXME: Implement. */ }

Perhaps these FIXME's should be replaced with notImplemente();

> Source/tests.pri:57
> +    contains(CONFIG, qtcb) {
> +        SUBDIRS += \
> +            $$WEBKIT2_TESTS_DIR/qtcb
> +    }

$$WEBKIT2_TESTS_DIR/qwkview, without any conditionals

We should also have an implementation of TestWebKitAPI based on this work.

> Tools/MiniBrowser/qtcb/View.cpp:61
> +View::View(const char* url)

QString?

> Tools/MiniBrowser/qtcb/View.cpp:133
> +    QGuiApplication a(argc, argv);

app / application

> Tools/MiniBrowser/qtcb/View.cpp:135
> +    View view(argc > 1 ? argv[1] : "http://www.google.com");

app.arguments().at()

> Tools/Tools.pro:22
> +contains(CONFIG, qtcb) {
> +    SUBDIRS += MiniBrowser/qtcb/MiniBrowser_cb.pro
> +}

Ideally we'd be able to run minibrowser with --wkview, but for now this is fine I think.

[Luiz Agostini]

Created attachment 147208 [details]
patch

[Noam Rosenthal]

Comment on attachment 147208 [details]
patch

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

> Source/WebKit2/ChangeLog:10
> +        This is not public API. It may change without notice at any time
> +        in future and we make no commitment regarding source compatibility
> +        or binary compatibility.

This warning should be in the header file, the changelog should describe what the API actually does.

> Source/WebKit2/Target.pri:180
> +    UIProcess/API/cpp/qt/QWKView_p.h \
> +    UIProcess/API/cpp/qt/QWKView_p_p.h \

Probably needs to be qwkview_p and qwkview_p_p.

> Source/WebKit2/UIProcess/API/cpp/qt/QWKView.cpp:96
> +        return;
> +
> +

Extra line

> Source/WebKit2/UIProcess/API/cpp/qt/QWKView.cpp:98
> +    drawingArea->setVisibleContentsRect(WebCore::IntRect(WebCore::IntPoint(), m_size), 1, WebCore::FloatPoint());

What does 1 mean? Needs a comment

> Source/WebKit2/UIProcess/API/cpp/qt/QWKView.cpp:116
> +    renderer->setVisibleContentsRect(WebCore::IntRect(WebCore::IntPoint(), m_size), 1, WebCore::FloatPoint());

ditto

[Luiz Agostini]

Created attachment 149650 [details]
patch

[Noam Rosenthal]

Comment on attachment 149650 [details]
patch

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

I'm generally ok with the direction this patch is going, though of course there are still some nitpicks.
Does anyone else have a major issue with how things look?

> Source/WebKit2/UIProcess/API/C/WKPreferences.cpp:400
> +void WKPreferencesSetForceCompositingMode(WKPreferencesRef preferencesRef, bool flag)
> +{
> +    toImpl(preferencesRef)->setForceCompositingMode(flag);
> +}
> +
> +bool WKPreferencesGetForceCompositingMode(WKPreferencesRef preferencesRef)
> +{
> +    return toImpl(preferencesRef)->forceCompositingMode();
> +}
> +

Maybe instead of exposing this, we should always force compositing in the Qt port.

> Source/WebKit2/UIProcess/API/cpp/qt/qwkview.cpp:96
> +
> +
> +    DrawingAreaProxy* drawingArea = m_webPageProxy->drawingArea();
> +    if (!drawingArea)
> +        return;
> +
> +

Lot's of empty lines :)

> Source/WebKit2/UIProcess/API/cpp/qt/qwkview_p_p.h:38
> +class QWKViewImpl : public PageClient , public QWKView {

extra space

> Source/WebKit2/UIProcess/API/cpp/qt/qwkview_p_p.h:68
> +    virtual void didReceiveMessageFromNavigatorQtObject(const String& message) { notImplemented(); }
> +    virtual void didChangeViewportProperties(const WebCore::ViewportAttributes& attr) { notImplemented(); }
> +    virtual void handleDownloadRequest(DownloadProxy* download) { notImplemented(); }
> +
> +    virtual void handleAuthenticationRequiredRequest(const String& hostname, const String& realm, const String& prefilledUsername, String& username, String& password) { notImplemented(); }
> +    virtual void handleCertificateVerificationRequest(const String& hostname, bool& ignoreErrors) { notImplemented(); }
> +    virtual void handleProxyAuthenticationRequiredRequest(const String& hostname, uint16_t port, const String& prefilledUsername, String& username, String& password) { notImplemented(); }
> +
> +    virtual void registerEditCommand(PassRefPtr<WebEditCommandProxy>, WebPageProxy::UndoOrRedo) { notImplemented(); }
> +    virtual void clearAllEditCommands() { }
> +    virtual bool canUndoRedo(WebPageProxy::UndoOrRedo undoOrRedo) { notImplemented(); return false; }
> +    virtual void executeUndoRedo(WebPageProxy::UndoOrRedo undoOrRedo) { notImplemented(); }
> +
> +    virtual WebCore::FloatRect convertToDeviceSpace(const WebCore::FloatRect& rect) { notImplemented(); return rect; }
> +    virtual WebCore::FloatRect convertToUserSpace(const WebCore::FloatRect& rect) { notImplemented(); return rect; }
> +    virtual WebCore::IntPoint screenToWindow(const WebCore::IntPoint& point) { notImplemented(); return point; }
> +    virtual WebCore::IntRect windowToScreen(const WebCore::IntRect& rect) { notImplemented(); return rect; }

I'd prefer if those notImplemented() calls were in the cpp file, makes it easier later.

[Simon Hausmann]

Comment on attachment 149650 [details]
patch

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

> Source/WebKit2/UIProcess/API/cpp/qt/qwkview.cpp:148
> +void QWKViewImpl::touchEvent(QTouchEvent* event)
> +{
> +    m_webPageProxy->handleTouchEvent(NativeWebTouchEvent(event, QTransform()));
> +}
> +
> +void QWKViewImpl::doneWithKeyEvent(const NativeWebKeyboardEvent& event, bool wasEventHandled)
> +{
> +    m_client->doneWithKeyEvent(event.nativeEvent(), wasEventHandled);
> +}
> +
> +#if ENABLE(TOUCH_EVENTS)

There's an #if here but not above in ::touchEvent.

>> Source/WebKit2/UIProcess/API/cpp/qt/qwkview_p_p.h:38
>> +class QWKViewImpl : public PageClient , public QWKView {
> 
> extra space

Is there a particular reason for choosing the design of an abstract interface (QWKView) and an in-library implementation (QWKViewImpl) over what would seem a more common pattern in Qt of having one class and a d-pointer for things that would require including webcore headers in the semi-public header?

The Apple Windows build seems to use entirely a C API, the Apple Mac port is using an object C class due to what seems to be a requirement of subclassing NSView.

> Source/WebKit2/UIProcess/API/cpp/qt/qwkview_p_p.h:160
> +    QUrl m_url;

This appears to be an unused member variable.

> Tools/MiniBrowser/qwkview/View.cpp:72
> +    m_webView = WebKit::QWKView::create(createWebContext(), createWebPageGroup(QString()), this);
> +    m_webView->initialize();

Is there a particular reason for this pattern over say giving QWKView a constructor that implies initialize?

[Noam Rosenthal]

Jocelyn has raised some concern in IRC about the name QWKView being confusing vs. QQuickWebView. 
I propsoed to call this QRawWebView instead.
This way we have QQuickWebView and QRawWebView, you either go the quick way or the raw way. Any objections?

[Luiz Agostini]

Created attachment 150279 [details]
patch

[WebKit Review Bot]

Attachment 150279 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit2/ChangeLog', u..." exit_code: 1
Source/WebKit2/UIProcess/API/qt/raw/qrawwebview_p.h:63:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebKit2/UIProcess/API/qt/raw/qrawwebview_p_p.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:21:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:23:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Noam Rosenthal]

Comment on attachment 150279 [details]
patch

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

OK, this seems to cover comments from everyone, also we agreed with several people on the naming and directory scheme with torarne and others.
It's time to move this forward - if there are any additional concerns/changes let's make them on top.

> Source/WebKit2/ChangeLog:8
> +

Please describe the test, and the fact that right now most of the functionality is not implemented other than the minimum needed to get web content on the screen.

> Source/WebKit2/UIProcess/API/qt/raw/qrawwebview_p.h:88
> +    void paint(const QMatrix4x4& transform, float opacity, unsigned paintFlags);

Add a comment saying that this assumes a current GL context.

> Source/WebKit2/UIProcess/API/qt/raw/qrawwebview_p.h:95
> +    void keyEvent(QKeyEvent*);
> +    void mouseEvent(QMouseEvent*, int clickCount = 0);
> +    void wheelEvent(QWheelEvent*);
> +    void touchEvent(QTouchEvent*);

sendKeyEvent etc.

[Luiz Agostini]

Created attachment 150292 [details]
patch

[WebKit Review Bot]

Comment on attachment 150292 [details]
patch

Clearing flags on attachment: 150292

Committed r121620: <http://trac.webkit.org/changeset/121620>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.