[webkit-reviews] review denied: [Bug 84532] [Qt][WK2] Private non-QtQuick API : [Attachment 146595] patch

[bugzilla-daemon at webkit.org]

Tor Arne Vestbø <vestbo at webkit.org> has denied Luiz Agostini
<luiz at webkit.org>'s request for review:
Bug 84532: [Qt][WK2] Private non-QtQuick API
https://bugs.webkit.org/show_bug.cgi?id=84532

Attachment 146595: patch
https://bugs.webkit.org/attachment.cgi?id=146595&action=review

------- Additional Comments from Tor Arne Vestbø <vestbo at webkit.org>
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.