[Webkit-unassigned] [Bug 32461] [Qt] Adding QPixmap/QImage support for the Qt hybrid layer

Sat Jan 16 00:32:29 PST 2010

https://bugs.webkit.org/show_bug.cgi?id=32461

Simon Hausmann <hausmann at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #45684|review?                     |review-
               Flag|                            |

--- Comment #24 from Simon Hausmann <hausmann at webkit.org>  2010-01-16 00:32:26 PST ---
(From update of attachment 45684)

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 }).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.