Bug 34507 – [Qt] Minor improvement to hybrid QPixmap

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Noam Rosenthal]

The original patch for hybrid transport of pixmaps between Qt and webkit allows for creating new HTML img elements, but in most cases the HTML image element would already be there. This patch allows adding the QPixmap into an already existing img element without the overhead of toDataUrl, by allowing an optional single parameter to toHTMLImageElement() - an existing img element.

[Noam Rosenthal]

Created attachment 47984 [details]
minor adjustment to hybrid pixmap: allow sending an existing img element as an argument

Note: this is coming from a real world use-case where we have a native SDK that exposes images, and we want to use HTML to display them.

[Kenneth Rohde Christiansen]

Comment on attachment 47984 [details]
minor adjustment to hybrid pixmap: allow sending an existing img element as an argument

e.g. myQObject.myPixmapProperty.toHTMLImageElement(myHtmlImg);

I don't like this API very much. It is like the to* operated on the myPixmapProperty, but still it takes an image.

[Noam Rosenthal]

I see your point. Would something like myQObject.myPixmapProperty.attachToHTMLImageElement(myHtmlImg) be better? Any name is fine with me as long as I can get the functionality in!

[Noam Rosenthal]

Created attachment 48620 [details]
enable applying a QPixmap from a native QObject to an HTML img element

Based on comments from Kenneth, renamed toHTMLImageElement to two functions: 
createHTMLImageElement and applyToHTMLImageElement (which takes an existing img element as an argument).
Also added some in-code comments.

[Noam Rosenthal]

Created attachment 48653 [details]
The native QPixmap/QImage can now assign its value to an existing image element

Replaced toHTMLImageElement() which creates an element, with assignToHTMLImageElement(element), which takes an existing element as an arg.
Also fixed the style to conform to the webkit style guide.

[Kenneth Rohde Christiansen]

Comment on attachment 48653 [details]
The native QPixmap/QImage can now assign its value to an existing image element

Looks fine, but there is at least a minor style issue you could fix before committing.

> +        if (hint == qMetaTypeId<QPixmap >())

[Noam Rosenthal]

Created attachment 48659 [details]
enable applying a QPixmap from a native QObject to an HTML img element + minor style fixes

ok - fixed those stray spaces, should be good to go :)

[Noam Rosenthal]

Created attachment 48681 [details]
The native QPixmap/QImage can now assign its value to an existing image element

Even better style!
Hope this is sufficient
N

[Eric Seidel (no email)]

Comment on attachment 48653 [details]
The native QPixmap/QImage can now assign its value to an existing image element

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 48653 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 48681 [details]
The native QPixmap/QImage can now assign its value to an existing image element

Clearing flags on attachment: 48681

Committed r54986: <http://trac.webkit.org/changeset/54986>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.