Bug 113639

Summary: [Qt] assignToHTMLImageElement no longer exists in Qt5
Product: WebKit Reporter: Kristof Provost <kristof>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, hausmann, jturcotte, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Kristof Provost 2013-03-30 04:58:58 PDT
assignToHTMLImageElement no longer exists in Qt5
Comment 1 Kristof Provost 2013-03-30 04:59:39 PDT
Created attachment 195855 [details]
Patch
Comment 2 Kristof Provost 2013-03-30 05:00:21 PDT
QWebKit allows developers to export QImage objects to Javascript and use Javascript to assign the QImage to an <img/> element.
This is documented in http://qt-project.org/doc/qt-5.0/qtwebkit/qtwebkit-bridge.html

The problem is that the documentation no longer matches reality. The (Javascript) function was indeed called assignToHTMLImageElement in Qt 4.8, but in Qt5 it's simply 'assignTo'.

The responsible code can be found in qtwebkit/Source/WebCore/bridge/qt/qt_pixmapruntime.cpp (QtPixmapRuntime::getClassRef()).

I feel very strongly that there was no good reason to change the function name so rather than updating the documentation to reflect the code the code should simply be changed back to expose the old name, or to allow both. It should be trivial to allow both assignTo and assignToHTMLImageElement to be used.

See also https://bugreports.qt-project.org/browse/QTBUG-30208
Comment 3 Jocelyn Turcotte 2013-04-02 06:09:41 PDT
Comment on attachment 195855 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        assignToHTMLImageElement no longer exists in Qt5

We usually add a [Qt] prefix before the titles and ChangeLog headlines of Qt-specific WebKit bugs.

> Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:257
> +        { "assignToHTMLImageElement", assignToHTMLImageElement, 0 },
>          { "assignTo", assignToHTMLImageElement, 0 },

This was a mistake so I think that we can just replace the "assignTo" entry instead of having two names for the function.
Comment 4 Kristof Provost 2013-04-02 10:05:37 PDT
I chose to keep the old name so things wouldn't break for anyone using qt-5.0.1 with javascript code using 'assignTo'.

I'll submit a new patch to fix both remarks.
Comment 5 Kristof Provost 2013-04-02 10:09:40 PDT
Created attachment 196172 [details]
Patch
Comment 6 Jocelyn Turcotte 2013-04-02 10:45:23 PDT
Comment on attachment 196172 [details]
Patch

Great :)
Comment 7 Jocelyn Turcotte 2013-04-02 10:48:13 PDT
(In reply to comment #4)
> I chose to keep the old name so things wouldn't break for anyone using qt-5.0.1 with javascript code using 'assignTo'.

That also makes sense. Since the documentation wasn't matching normallly they should be as skeptical as you were (that is if they bothered digging in the code) and they shouldn't be surprised that it gets changed back.

I'll pick this change into 5.1.
Comment 8 WebKit Review Bot 2013-04-02 10:55:53 PDT
Comment on attachment 196172 [details]
Patch

Rejecting attachment 196172 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 196172, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 14075 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
55>At revision 14075.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://webkit-commit-queue.appspot.com/results/17334471
Comment 9 Jocelyn Turcotte 2013-04-02 11:17:00 PDT
Comment on attachment 196172 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Please state that this bug is already covered by the tst_hybridPixmap API test (which doesn't seem to be run by the bot for some reason).

The OOPS in the "Reviewed by" line will be replaced by the commit queue.
Comment 10 Kristof Provost 2013-04-02 11:54:31 PDT
Created attachment 196202 [details]
Patch
Comment 11 WebKit Review Bot 2013-04-03 04:13:48 PDT
Comment on attachment 196202 [details]
Patch

Clearing flags on attachment: 196202

Committed r147539: <http://trac.webkit.org/changeset/147539>
Comment 12 WebKit Review Bot 2013-04-03 04:13:51 PDT
All reviewed patches have been landed.  Closing bug.