Bug 32461 – [Qt] Adding QPixmap/QImage support for the Qt hybrid layer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Noam Rosenthal]

Created attachment 44725 [details]
Qt hybrid support for QPixmap/QImage + a test

Attached is a patch that adds support for sending pixmaps/images from Qt to webkit and vise-versa, via QObject signals/slots/properties.
The change is limited to the Qt bridge (qt_runtime.cpp), and also includes a test case.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/tests/hybridPixmap/widget.cpp:1:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/tests/hybridPixmap/widget.cpp:3:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/tests/hybridPixmap/widget.cpp:15:  Missing space after ,  [whitespace/comma] [3]
WebKit/qt/tests/hybridPixmap/widget.cpp:20:  Missing space after ,  [whitespace/comma] [3]
WebKit/qt/tests/hybridPixmap/widget.cpp:21:  Missing space after ,  [whitespace/comma] [3]
WebKit/qt/tests/hybridPixmap/widget.cpp:26:  Missing space after ,  [whitespace/comma] [3]
WebKit/qt/tests/hybridPixmap/widget.cpp:27:  Missing space after ,  [whitespace/comma] [3]
WebKit/qt/tests/hybridPixmap/widget.cpp:31:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/qt/tests/hybridPixmap/widget.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/tests/hybridPixmap/widget.cpp:45:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/qt/tests/hybridPixmap/widget.cpp:52:  Missing space after ,  [whitespace/comma] [3]
WebCore/bridge/qt/qt_runtime.cpp:53:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bridge/qt/qt_runtime.cpp:54:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bridge/qt/qt_runtime.cpp:56:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bridge/qt/qt_runtime.cpp:58:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bridge/qt/qt_runtime.cpp:61:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/tests/hybridPixmap/widget.h:5:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/tests/hybridPixmap/widget.h:6:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/tests/hybridPixmap/widget.h:8:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/qt/tests/hybridPixmap/widget.h:18:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/qt/tests/hybridPixmap/widget.h:20:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/qt/tests/hybridPixmap/main.cpp:1:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/tests/hybridPixmap/main.cpp:2:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/tests/hybridPixmap/main.cpp:20:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/qt/tests/hybridPixmap/main.cpp:22:  Declaration has space between type name and * in Widget *wdg  [whitespace/declaration] [3]
WebKit/qt/tests/hybridPixmap/main.cpp:25:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 26

[Adam Barth]

Comment on attachment 44725 [details]
Qt hybrid support for QPixmap/QImage + a test

widget.cpp seems somewhat drastically out of compliance with the style guide.  Would you be willing to revise your patch to address the style nits?

[Noam Rosenthal]

Sure, my first patch so please bear with me :)

[Adam Barth]

Welcome to the project.  Thanks for contributing a patch.  :)

If you like, you can run the style checker locally using the check-webkit-style script in the WebKitTools/Scripts directory.  The checker might have some false positives (it's new), so feel free to ask questions if you're not sure what to do.

[Noam Rosenthal]

One question about the style checker - it seems to fail on my main.cpp because it doesn't have a main header it implements (it's expecting main.h). 

Now my test doesn't have main.h, and neither does any of the other Qt tests. Do I need to add a dummy file, does the script need to be revised, or is there another trick I'm missing?

Cheers
No'am

[Adam Barth]

I'm not that familiar with the Qt unit testing framework, but I believe this is a false positive in the style checker.  Almost all the source files in the project represent classes, which have a corresponding header file.  Someone who knows more about Qt might have more specific advice, but I think you should ignore this complaint.  We'll fix the style checker to know not to complain in this case.

[Noam Rosenthal]

Created attachment 44745 [details]
support for QPixmap/QImage handling in QtWebkit hybrid layer, revised with adherence to webkit coding style

New patch, after comment re. coding style.
There are still a couple of false positives but the major coding style issues have been fixed.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/tests/hybridPixmap/widget.cpp:20:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/tests/hybridPixmap/tst_hybridPixmap.cpp:20:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2

[Noam Rosenthal]

Note that the above style-checker errors are false positives, as none of the other Qt tests have config.h or a main header file included (a unit-test usually doesn't have a main header file). These issues should probably be dealt with at the style-checker level.

[Simon Hausmann]

This appears to be like an extremely convenient feature to have, but I think we have to be very careful if this is exactly what we want.

I'd like to avoid adding feature to the existing qt_runtime bridge. The code is old (as you may have noticed :) and I wish for it disappear as much as possible as part of bug #31863 .

At the same time we are here silently extending the documented DOM interfaces (indirectly appendChild, as the conversion is not visible in the source code). We are extending the interface with an undocumented extension.

I think this feature deserves the input of a wider audience than just the WebKit reviewers, I would like to hear the opinions of the other developers. Given that this is somewhat specific to the Qt port perhaps the webkit-qt mailing list would be a good forum.

[Eric Seidel (no email)]

(In reply to comment #9)
> Note that the above style-checker errors are false positives, as none of the
> other Qt tests have config.h or a main header file included (a unit-test
> usually doesn't have a main header file). These issues should probably be dealt
> with at the style-checker level.

Please file a bug about the false positives.

[Noam Rosenthal]

We didn't get much input from the mailing list. I think that we should pick a route and go with it - I can implement whatever we decide as long as we decide on something soon :)

[Antti Koivisto]

What is the problem this patch is trying to solve? Why can't it be solved using the existing mechanisms?

[Noam Rosenthal]

This tries to solve a big performance issue we're tackling.

Sometimes we want a native component to handle heavy lifting of image handling - either because it's complicated, or because it's the only way. We still, however, want the images to be displayed and laid out in HTML.
A good example is extracting images from a video that has a proprietary C API to get those images.

Currently taking those images from the Qt native layer and displaying them in HTML requires:

- encoding the image in Qt (PNG/BMP)
- sending the image to webkit, either in base64 or in a custom QNetworkReply
- Setting the image source to the base64-encoded string, or to the url that would trigger the custom network reply.

You might imagine how slow this path is. 

Since QtWebkit uses pixmaps internally, it should be relatively smooth and trivial to get those pixmaps to the webkit layer. The problem - there's no current public API to do that! 

This also opens a possibility to offload some of the more complicated rendering problems to Qt when goings get tough with doing them with HTML, or if the project has no time to make changes to webkit. For example a slot can send a few web-element IDs to Qt, Qt would do the heavy rendering, and will return a QPixmap to the web layer which in turn would decide whether to display it in the DOM or to draw it using canvas.


So, any current mechanism that would allow me to make an existing QPixmap available for the web-dev layer, without encoding/decoding/data-copy would work better than a new one. The problem is that there's no such mechanism :)

[WebKit Review Bot]

Attachment 44745 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/144763

[Antti Koivisto]

Makes sense. A custom QNetworkReply with uncompressed image format would probably not perform much worse but would be clumsier to use. I don't have any philosophical objections to this patch.

However, I'm not the right person to evaluate the bindings code or API impact. Simon seemed to have some concerns.

[Simon Hausmann]

Comment on attachment 44745 [details]
support for QPixmap/QImage handling in QtWebkit hybrid layer, revised with adherence to webkit coding style

As convenient as it seems, I'm still worried about extending the DOM API in Qt with an implicit conversion from a QPixmap to an automatically created DOM element.

In addition this is limited to HTML image elements, it would not be possible to use application provided pixmaps for backgrounds for example.

It might be easier to be able to bind pixmaps to a central instance in the API and get an URI for it, that could be used in anywhere pixmaps/images are needed.

[Noam Rosenthal]

I completely understand the concerns.
Note that the implicity is in webkit and not in Qt: 
var img = new Image;
really creates an image element even when you don't use the DOM API...

Alternative approaches:
(1)
The returned property/signal-argument doesn't create an image element implicitly, but rather gives you an option to do so implicitly:

myObject.somePixmapProperty.createImageElement();

it would also allow you to generate a data URL, if you want to use it for a background instead:

myObject.somePixmapProperty.toDataUrl() // or encodeDataUrl()

(2)
We modify CachedImage::createImage(), in a #ifdef PLATFORM(QT), to recognize the url and see that the image really needs to be native and loaded directly.
This is the only place I can see where I can do a direct URL->WebCore::Image conversion, all the other places require me to translate the pixmap to a QImage and back, which beats the purpose, especially if the pixmap is in video memory.

(3)
Any other option - you choose it, I'll implement.

I don't mind implementing either of those, but I wouldn't want to see them rejected again because of an architectural concern... can we pick an approach and go with it? :) Sometimes perfect is the enemy of good-enough, and in this case we're really trying to do something rather simple.

[Noam Rosenthal]

Created attachment 45678 [details]
Refactor of hybrid pixmap transferring, with an intermediate object

Refactored the solution to use an intermediate object (in qt_pixmapruntime.cpp)
- The object has width & height properties, and toHTMLImageElement() and toDataUrl() functions.
- The Qt bridge still accepts HTMLImageElement directly when sending a pixmap to Qt, as there's no way to construct this intermediate object, and in this case there's no implicit creation of DOM elements.
- The intermediate object converts to a QImage/QPixmap when the actual data is requested: QImage when dataUrl is requested, QPixmap when HTMLImageElement is requested.

Note that the test still fails the webkit style checker, I already posted a relevant bug about the style-checker tool.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
mapruntime.cpp:119:  Missing space after ,  [whitespace/comma] [3]
WebCore/bridge/qt/qt_pixmapruntime.cpp:130:  Missing space after ,  [whitespace/comma] [3]
WebCore/bridge/qt/qt_pixmapruntime.cpp:133:  Missing space after ,  [whitespace/comma] [3]
WebCore/bridge/qt/qt_pixmapruntime.cpp:138:  Missing space before {  [whitespace/braces] [5]
WebCore/bridge/qt/qt_pixmapruntime.cpp:187:  Missing space after ,  [whitespace/comma] [3]
WebCore/bridge/qt/qt_pixmapruntime.cpp:185:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bridge/qt/qt_pixmapruntime.cpp:205:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bridge/qt/qt_pixmapruntime.cpp:215:  Missing space after ,  [whitespace/comma] [3]
WebCore/bridge/qt/qt_pixmapruntime.cpp:216:  Missing space after ,  [whitespace/comma] [3]
WebCore/bridge/qt/qt_pixmapruntime.cpp:217:  Missing space after ,  [whitespace/comma] [3]
WebCore/bridge/qt/qt_pixmapruntime.cpp:218:  Missing space after ,  [whitespace/comma] [3]
WebCore/bridge/qt/qt_pixmapruntime.cpp:225:  Missing spaces around |  [whitespace/operators] [3]
WebCore/bridge/qt/qt_pixmapruntime.cpp:227:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
WebCore/bridge/qt/qt_pixmapruntime.cpp:236:  Missing space after ,  [whitespace/comma] [3]
WebCore/bridge/qt/qt_pixmapruntime.cpp:237:  Missing space after ,  [whitespace/comma] [3]
WebCore/bridge/qt/qt_pixmapruntime.cpp:242:  Missing space after ,  [whitespace/comma] [3]
WebCore/bridge/qt/qt_pixmapruntime.cpp:247:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bridge/qt/qt_pixmapruntime.cpp:257:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bridge/qt/qt_pixmapruntime.cpp:267:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bridge/qt/qt_pixmapruntime.cpp:279:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bridge/qt/qt_pixmapruntime.cpp:289:  Missing space after ,  [whitespace/comma] [3]
WebCore/bridge/qt/qt_pixmapruntime.cpp:312:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bridge/qt/qt_pixmapruntime.cpp:320:  Missing space after ,  [whitespace/comma] [3]
WebCore/bridge/qt/qt_pixmapruntime.cpp:323:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 49

[WebKit Review Bot]

Attachment 45678 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/153325

[Noam Rosenthal]

Created attachment 45684 [details]
Had to resubmit due to some build/style failures

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/tests/hybridPixmap/widget.cpp:20:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/tests/hybridPixmap/tst_hybridPixmap.cpp:20:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2

[Simon Hausmann]

Comment on attachment 45684 [details]
Had to resubmit due to some build/style failures


In general your patch looks good to me. I'm sorry it takes so long to review this big patch.

My only criticism left are coding style issues and missing ChangeLog entries before we
can land this. r- because of that.

(Kent, could you have a look at the JS API, too? Thanks!)

> Index: WebKit/qt/tests/hybridPixmap/hybridPixmap.pro
> ===================================================================
> --- WebKit/qt/tests/hybridPixmap/hybridPixmap.pro	(revision 0)
> +++ WebKit/qt/tests/hybridPixmap/hybridPixmap.pro	(revision 0)
> @@ -0,0 +1,15 @@
> +# -------------------------------------------------
> +# Project created by QtCreator 2009-12-10T11:25:02
> +# -------------------------------------------------
> +include(../../../../WebKit.pri)
> +QT += network gui testlib
> +TARGET = hybridPixmap
> +TEMPLATE = app
> +SOURCES += tst_hybridPixmap.cpp \
> +    widget.cpp
> +HEADERS += widget.h
> +FORMS += widget.ui
> +RESOURCES += resources.qrc
> +OTHER_FILES += test.html
> +CONFIG += console
> +QMAKE_RPATH_DIR=$$OUTPUT_DIR/lib $$QMAKE_RPATH_DIR

This could probably be simplified to use ../tests.pri.

> Index: WebKit/qt/tests/hybridPixmap/test.html
> ===================================================================
> --- WebKit/qt/tests/hybridPixmap/test.html	(revision 0)
> +++ WebKit/qt/tests/hybridPixmap/test.html	(revision 0)
> @@ -0,0 +1,35 @@
> +<html>
> +    <head>
> +        <style>
> +            img { display: block; border-style: groove}
> +        </style>
> +        <script>
> +            function startTest()
> +            {
> +                var obj = myWidget.image;
> +                var pxm = myWidget.pixmap;
> +                var img = obj.toHTMLImageElement();
> +                var img1 = document.getElementById("img1");
> +                var img2 = document.getElementById("img2");
> +                document.body.appendChild(img);
> +                document.body.appendChild(pxm.toHTMLImageElement());

I wonder if this method should take a document as argument, instead of
always defaulting to the global document. Otherwise it might happen that
the element is created in the "global" document but it is intended to be
inserted into another (local) document. Perhaps even in another frame.

I guess that could be fixed later on though.

The rest of the API looks good to me.

> +Widget::Widget(QWidget *parent) :

Coding style (* placement).

> +void Widget::setPixmap(const QPixmap & p)

Coding style (space before &)

> +void Widget::setImage(const QImage & img)

Coding style (space before &)

> +void Widget::changeEvent(QEvent *e)

Coding style (* placement).

> +    Widget(QWidget *parent = 0);

Coding style (* placement).

> +    ~Widget();
> +    void setPixmap(const QPixmap &);

Ditto.

> +    QPixmap pixmap() const;
> +    void setImage(const QImage &);

Ditto. (there are a few more instances, I believe)

> +    static const char* name() {return "width"; }

Space before "return";

> +    virtual JSValue valueFromInstance(ExecState* exec, const Instance* pixmap) const
> +    {
> +        return jsNumber(exec, static_cast<const QtPixmapInstance*>(pixmap)->width());
> +    }
> +    virtual void setValueToInstance(ExecState*, const Instance*, JSValue) const {}
> +};
> +class QtPixmapHeightField : public Field {
> +public:
> +    static const char* name() {return "height"; }

Same here :)

> +        JSDOMGlobalObject* global = (JSDOMGlobalObject*)root->globalObject();

Any reason for a C style cast here? I think C++ style casts are preferred.

> +        const QString b64 = QString("data:image/png;base64,")+ba.toBase64();

Coding style (missing space before and after }).

[Noam Rosenthal]

Created attachment 46751 [details]
Fixed style issues reported by Simon

Mainly made sure that all the * and & are in the right place, and fixed all the other style issues reported by the previous review (e.g. use tests.pri)
I think it should be sufficient by now - unless there are style issues not reported in the previous review.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/tests/hybridPixmap/widget.cpp:20:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/tests/hybridPixmap/tst_hybridPixmap.cpp:20:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/bridge/qt/qt_pixmapruntime.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3

[Kenneth Rohde Christiansen]

(In reply to comment #26)
> Attachment 46751 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebKit/qt/tests/hybridPixmap/widget.cpp:20:  Found header this file implements
> before WebCore config.h. Should be: config.h, primary header, blank line, and
> then alphabetically sorted.  [build/include_order] [4]
> WebKit/qt/tests/hybridPixmap/tst_hybridPixmap.cpp:20:  Found other header
> before WebCore config.h. Should be: config.h, primary header, blank line, and
> then alphabetically sorted.  [build/include_order] [4]
> WebCore/bridge/qt/qt_pixmapruntime.cpp:27:  Alphabetical sorting problem. 
> [build/include_order] [4]
> Total errors found: 3

You can ignore the first two, but you should probably take a look at the last one.

[Kenneth Rohde Christiansen]

Comment on attachment 46751 [details]
Fixed style issues reported by Simon

I agree with Simon that it looks good, but sometimes you have some weird spacing/newlining.

> +            function startTest()
> +            {
> +                var obj = myWidget.image;
> +                var pxm = myWidget.pixmap;
> +
> +
> +                var img = obj.toHTMLImageElement();

Like above. Why two lines?


> +                var img1 = document.getElementById("img1");
> +                var img2 = document.getElementById("img2");
> +                document.body.appendChild(img);
> +                document.body.appendChild(pxm.toHTMLImageElement());
> +                function completeIfDone()
> +                {
> +                    if (img1.complete && img2.complete) {
> +                        myWidget.completeTest();
> +                    }
> +                }
> +                img2.onload = completeIfDone;
> +                img1.onload = completeIfDone;

Why not img1.onload before img2.onload?


In classes like below, we would normally add a new line before public: private slots: etc lines.

> +class Widget : public QWidget {
> +    Q_OBJECT
> +    Q_PROPERTY(QPixmap pixmap READ pixmap WRITE setPixmap)
> +    Q_PROPERTY(QImage image READ image WRITE setImage)
> +public:
> +    Widget(QWidget* parent = 0);
> +    ~Widget();
> +    void setPixmap(const QPixmap&);
> +    QPixmap pixmap() const;
> +    void setImage(const QImage&);
> +    QImage image() const;
> +private slots:
> +    void refreshJS();
> +public slots:
> +    void completeTest();
> +    void start();
> +signals:
> +    void testComplete();
> +protected:
> +    void changeEvent(QEvent* e);
> +
> +private:
> +    Ui::Widget* ui;
> +};

> +    }
> +private:
> +    Widget* wdg;
> +
> +
> +};

Weird extra newline above. Also, we try not to abbreviate things like widget into wdg, pixmap into pxm, etc. Please avoid that.


> +    MethodList ml;
> +    if (identifier == QtPixmapToDataUrlMethod::name())

For instance in the above instead of 'ml', I would prefer 'methods'. That makes the below code easier to read.

> +        ml.append(&qt_pixmap_metaData.toDataUrlMethod);
> +    else if (identifier == QtPixmapCreateElementMethod::name())
> +        ml.append(&qt_pixmap_metaData.createElementMethod);
> +    return ml;
> +}

> +int QtPixmapInstance::width() const
> +{
> +    if (data.type() == (QVariant::Type)qMetaTypeId<QPixmap>())
> +        return data.value<QPixmap>().width();
> +    if (data.type() == (QVariant::Type)qMetaTypeId<QImage>())
> +        return data.value<QImage>().width();

We disencourage C style casts.

[Noam Rosenthal]

Created attachment 46760 [details]
Fix some extra style issues reported by Kenneth

Note that some of the c-style castings are a style-copy from qt_runtime.cpp, mainly where enums are involved.
However, I changed all of them to c++ style, except for the ones that can only be done c-style, such as (UChar*).

Hope this is stylish enough, sorry to waste your time this coding-style stuff - I come from a more hack-and-run background but it's getting better :)
N

[Noam Rosenthal]

Created attachment 46773 [details]
Fix a couple of more c-style casts

Oops, still had a couple of more c-style casts... those are hard to find :)

[Kenneth Rohde Christiansen]

Comment on attachment 46773 [details]
Fix a couple of more c-style casts

Good work! r=me

[Kenneth Rohde Christiansen]

No'am, have you thought about writing documentation about the hybrid layer? Would be good to have that in the Qt documentation, I think, and you know pretty well how that works.

Kenneth

[Noam Rosenthal]

Yes, I've actually written a draft of that documentation 3 months ago but finalizing it got lost somewhere in the pipeline...
I think one more component is missing in the hybrid layer: transporting QWebElements. With that and the new QPixmap/QImage transporting you can do something like that in Javascript:

var renderedElementImage = QtRenderer.renderElement(document.getElementById("someElement")).toHTMLImageElement();

which would allow QGraphicsView-like performance inside a QtWebkit based HTML canvas, for example.

I focused on the QPixmap task first to make it easier to review...

but after the second one is done (and the compositing stuff) I'd be glad to take on the work of documenting the hybrid layer.

Prepare for some more reviews :)
N

[Kenneth Rohde Christiansen]

Great! Looking forward to that!

[Kent Hansen]

(In reply to comment #30)
> Created an attachment (id=46773) [details]
> Fix a couple of more c-style casts
> 
> Oops, still had a couple of more c-style casts... those are hard to find :)

Hi No'am :)

The qt_runtime.cpp part doesn't apply cleanly for me (against r53396).

Compile issues:

/home/khansen/dev/webkit/WebCore/bridge/qt/qt_pixmapruntime.cpp: In member function 'virtual JSC::JSValue JSC::Bindings::QtPixmapCreateElementMethod::invoke(JSC::ExecState*, QVariant&, WTF::PassRefPtr<JSC::Bindings::RootObject>)':
/home/khansen/dev/webkit/WebCore/bridge/qt/qt_pixmapruntime.cpp:89: error: expected `>' before ')' token

/home/khansen/dev/webkit/WebCore/bridge/qt/qt_pixmapruntime.cpp: In member function 'virtual JSC::JSValue JSC::Bindings::QtPixmapInstance::defaultValue(JSC::ExecState*, JSC::PreferredPrimitiveType) const':
/home/khansen/dev/webkit/WebCore/bridge/qt/qt_pixmapruntime.cpp:222: warning: suggest parentheses around && within ||

/home/khansen/dev/webkit/WebCore/bridge/qt/qt_pixmapruntime.cpp: In member function 'QPixmap JSC::Bindings::QtPixmapInstance::toPixmap()':
/home/khansen/dev/webkit/WebCore/bridge/qt/qt_pixmapruntime.cpp:262: error: expected `)' before '{' token


The code looks OK to me. It would be nice if the test had QCOMPAREs for the result of toDataUrl() and pixmap.height too. And QtPixmapInstance::valueOf() doesn't seem to be tested.
Does this test cover all the new paths in convertValueToQVariant() and convertQVariantToValue? And the two cases for QtPixmapInstance::variantFromObject()? E.g., what happens if you call a slot that takes a QPixmap argument and you pass it a JSHTMLImageElement? Should be possible to write tests for that stuff. :)

[Noam Rosenthal]

Created attachment 46835 [details]
Increased test coverage to Kent's request - fixed a compilation issue that slipped through...

This should be a pretty straightforward review, as it changes very little from the last patch (which apparently had a compilation issue that I ovelooked). It mainly adds more tests like having a signal with image/pixmap, testing toString for the intermediate object, and sending garbage to a slot that wants an image/pixmap.

[Kenneth Rohde Christiansen]

Comment on attachment 46835 [details]
Increased test coverage to Kent's request - fixed a compilation issue that slipped through...

Do you want this added to the commit-queue? if so set the commit queue to "?"

[WebKit Commit Bot]

Comment on attachment 46835 [details]
Increased test coverage to Kent's request - fixed a compilation issue that slipped through...

Rejecting patch 46835 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Kenneth Rohde Christiansen', '--force']" exit_code: 2
Last 500 characters of output:
 file WebKit/qt/tests/hybridPixmap/widget.h
patching file WebKit/qt/tests/hybridPixmap/tst_hybridPixmap.cpp
patching file WebKit/qt/tests/hybridPixmap/resources.qrc
patching file WebKit/qt/tests/tests.pro
patching file WebCore/WebCore.pro
patch: **** Only garbage was found in the patch input.
patch: **** Only garbage was found in the patch input.
patching file WebCore/bridge/qt/qt_pixmapruntime.cpp
patching file WebCore/bridge/qt/qt_runtime.cpp
patching file WebCore/bridge/qt/qt_pixmapruntime.h

Full output: http://webkit-commit-queue.appspot.com/results/199125

[Noam Rosenthal]

(In reply to comment #38)
> (From update of attachment 46835 [details])
> Rejecting patch 46835 from commit-queue.
> 
> Failed to run
> "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply',
> '--reviewer', 'Kenneth Rohde Christiansen', '--force']" exit_code: 2
> Last 500 characters of output:
>  file WebKit/qt/tests/hybridPixmap/widget.h
> patching file WebKit/qt/tests/hybridPixmap/tst_hybridPixmap.cpp
> patching file WebKit/qt/tests/hybridPixmap/resources.qrc
> patching file WebKit/qt/tests/tests.pro
> patching file WebCore/WebCore.pro
> patch: **** Only garbage was found in the patch input.
> patch: **** Only garbage was found in the patch input.
> patching file WebCore/bridge/qt/qt_pixmapruntime.cpp
> patching file WebCore/bridge/qt/qt_runtime.cpp
> patching file WebCore/bridge/qt/qt_pixmapruntime.h
> 
> Full output: http://webkit-commit-queue.appspot.com/results/199125

hmm... do I need to do anything special about new files?

[Kenneth Rohde Christiansen]

Try creating the patch with WebKitTools/Scripts/webkit-patch or bugzilla-tool.

git format-patch -1 should also work.

> hmm... do I need to do anything special about new files?

[Noam Rosenthal]

Created attachment 46848 [details]
Use /dev/null for new files

The only difference between this and the previous patch is that I use /dev/null instead of empty files for the new files.
Should that fix the commit problem? I have no way of testing and this is my first commit

[Kent Hansen]

(In reply to comment #41)
> Created an attachment (id=46848) [details]
> Use /dev/null for new files
> 
> The only difference between this and the previous patch is that I use /dev/null
> instead of empty files for the new files.
> Should that fix the commit problem? I have no way of testing and this is my
> first commit

Well, the first thing to try would be to apply the patch to the master branch of your local clone.
It doesn't apply for me, I need to add a/ and b/ to the paths in the --- and +++ lines. After that it applies nicely (even the qt_runtime.cpp part this time).

Did you use git-format-patch like Kenneth suggested? (Your patch doesn't look like git diff output, even)

[Simon Hausmann]

Comment on attachment 46848 [details]
Use /dev/null for new files

The patch is fine with me, and in fact I can apply it locally with patch -p0. I was about to review and land it, but unfortunately it is missing ChangeLog entries.

Noam, can you add those with explanations of your changes? AFAICS that's the only thing missing we need to land it.

(If the bots have problems, it's okay, we can land it manually)

Sorry :(

[Noam Rosenthal]

No problem, this is my newbie fault - I unknowingly skipped a couple of the Webkit contribution guidelines. I'll clean it up and prepare an applicable patch and a changelog.

[Noam Rosenthal]

Created attachment 46974 [details]
ChangeLog added, created with svn-create-patch

Same patch, just with a ChangeLog and created with svn-create-patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Ignoring WebKit/qt/tests/hybridPixmap/widget.cpp; This file is exempt from the style guide.
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:19:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:23:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:28:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:31:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:34:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:36:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:39:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:42:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:46:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:49:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:58:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:60:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:62:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:66:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:68:  Line contains tab character.  [whitespace/tab] [5]
Ignoring WebKit/qt/tests/hybridPixmap/tst_hybridPixmap.cpp; This file is exempt from the style guide.
Ignoring WebKit/qt/tests/hybridPixmap/widget.h; This file is exempt from the style guide.
WebKit/qt/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 21


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

[Noam Rosenthal]

Created attachment 46977 [details]
oops, some auto-tabs

[Simon Hausmann]

Comment on attachment 46977 [details]
oops, some auto-tabs

Re-applying earlier r+. Let's try this on the bot.

[WebKit Commit Bot]

Comment on attachment 46977 [details]
oops, some auto-tabs

Rejecting patch 46977 from commit-queue.

Failed to parse ChangeLog: /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog

[Noam Rosenthal]

what does that mean and how can I fix it?

[Kenneth Rohde Christiansen]

Comment on attachment 46977 [details]
oops, some auto-tabs

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 53519)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,72 @@
> [phone]  No'am Rosenthal  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] Adding QPixmap/QImage support for the Qt hybrid layer
> +       Allows accesing QPixmap and QImage based arguments from Qt signals,
> +       slots and properties
> +       This is done by an intermediate object that can be turned into
> +       web-based objects by calling either toHTMLImageElement() or
> +       toDataURL()
> +        https://bugs.webkit.org/show_bug.cgi?id=32461

I'm not sure, but your identation looks wrong.

[Simon Hausmann]

(In reply to comment #51)
> (From update of attachment 46977 [details])
> > Index: WebCore/ChangeLog
> > ===================================================================
> > --- WebCore/ChangeLog	(revision 53519)
> > +++ WebCore/ChangeLog	(working copy)
> > @@ -1,3 +1,72 @@
> > [phone]  No'am Rosenthal  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        [Qt] Adding QPixmap/QImage support for the Qt hybrid layer
> > +       Allows accesing QPixmap and QImage based arguments from Qt signals,
> > +       slots and properties
> > +       This is done by an intermediate object that can be turned into
> > +       web-based objects by calling either toHTMLImageElement() or
> > +       toDataURL()
> > +        https://bugs.webkit.org/show_bug.cgi?id=32461
> 
> I'm not sure, but your identation looks wrong.

Yeah, maybe it was confused by that. I'm landing it manually now.

[Simon Hausmann]

The patch didn't build with current ToT, as runtime.h has been renamed to Bridge.h. I'll fix that when landing.

[Simon Hausmann]

Committed r53611: <http://trac.webkit.org/changeset/53611>

[Simon Hausmann]

I landed http://trac.webkit.org/changeset/53613 on top, to make widget.ui not depend on Qt being built with QtWebKit, as that's how the Qt bot is configured.

[Noam Rosenthal]

wow, this was painful... but I think my next contributions are going to have a lot less overhead :)

[Eric Seidel (no email)]

Filed bug 33975 about the strange bot failure.  Sorry about the trouble.