Bug 37319

Summary:

[Qt] tst_QWebFrame::overloadedSlots() fails

Product:

WebKit

Reporter:

Simon Hausmann <hausmann>

Component:

Text

Assignee:

Caio Marcelo de Oliveira Filho <cmarcelo>

Status:

RESOLVED FIXED

Severity:

Major

CC:

ademar, cmarcelo, commit-queue, jedrzej.nowacki, jwieczorek, kent.hansen, laszlo.gombos, ossy, twerner, webkit.review.bot

Priority:

Keywords:

Qt, QtTriaged

Version:

528+ (Nightly build)

Hardware:

All

OS:

Linux

Bug Depends on:

62790

Bug Blocks:

38654

Attachments:

Description	Flags
Patch: mark as expected failure	none
Patch: mark as expected failure	none
Patch	none
Patch	cmarcelo: review-, hausmann: commit-queue-
Patch	none
Patch	none
Patch	none
Rebased	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Simon Hausmann

Reported 2010-04-09 02:55:29 PDT

FAIL! : tst_QWebFrame::overloadedSlots() Compared values are not the same Actual (m_myObject->qtFunctionInvoked()): 41 Expected (36): 36 Loc: [/home/shausman/src/webkit/trunk/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp(1911)] Failure reproducible on Linux, but likely cross-platform.

Attachments
Patch: mark as expected failure (1.07 KB, patch) 2010-04-15 12:04 PDT, Jocelyn Turcotte	no flags	Details Formatted Diff Diff
Patch: mark as expected failure (1.12 KB, patch) 2010-04-15 12:06 PDT, Jocelyn Turcotte	no flags	Details Formatted Diff Diff
Patch (4.74 KB, patch) 2011-01-30 18:10 PST, Noam Rosenthal	no flags	Details Formatted Diff Diff
Patch (4.53 KB, patch) 2011-01-30 18:37 PST, Noam Rosenthal	cmarcelo: review- hausmann: commit-queue-	Details Formatted Diff Diff
Patch (4.41 KB, patch) 2011-05-04 23:55 PDT, Noam Rosenthal	no flags	Details Formatted Diff Diff
Patch (4.67 KB, patch) 2011-05-05 22:21 PDT, Noam Rosenthal	no flags	Details Formatted Diff Diff
Patch (16.46 KB, patch) 2011-05-17 16:03 PDT, Caio Marcelo de Oliveira Filho	no flags	Details Formatted Diff Diff
Rebased (16.19 KB, patch) 2011-06-09 09:53 PDT, Caio Marcelo de Oliveira Filho	no flags	Details Formatted Diff Diff
Patch (16.11 KB, patch) 2011-06-09 13:47 PDT, Caio Marcelo de Oliveira Filho	no flags	Details Formatted Diff Diff
Patch (17.27 KB, patch) 2011-06-09 14:55 PDT, Caio Marcelo de Oliveira Filho	no flags	Details Formatted Diff Diff
Patch (19.89 KB, patch) 2011-06-10 08:26 PDT, Caio Marcelo de Oliveira Filho	no flags	Details Formatted Diff Diff
Patch (19.93 KB, patch) 2011-06-10 13:43 PDT, Caio Marcelo de Oliveira Filho	no flags	Details Formatted Diff Diff
Patch (5.02 KB, patch) 2011-06-27 09:55 PDT, Caio Marcelo de Oliveira Filho	no flags	Details Formatted Diff Diff
Show Obsolete (12) View All Add attachment proposed patch, testcase, etc.

Kent Hansen

Comment 1 2010-04-09 03:42:24 PDT

JS:myOverloadedSlot(document.body) is picking myOverloadedSlot(QObject*) instead of myOverloadedSlot(const QWebElement &). Did this ever pass? Also the comment looks wrong ("should pick myOverloadedSlot(QRegExp)").

Jakub Wieczorek

Comment 2 2010-04-09 06:55:31 PDT

(In reply to comment #1) > JS:myOverloadedSlot(document.body) is picking myOverloadedSlot(QObject*) > instead of myOverloadedSlot(const QWebElement &). > Did this ever pass? > Also the comment looks wrong ("should pick myOverloadedSlot(QRegExp)"). I faced this kind of problem in https://bugs.webkit.org/show_bug.cgi?id=37060 (the LayoutTestController change) and solved it by registering the QWebElement type with qRegisterMetaType. I presume this is necessary to use QWebElement as a slot argument.

Kent Hansen

Comment 3 2010-04-09 07:55:16 PDT

(In reply to comment #2) > (In reply to comment #1) > > JS:myOverloadedSlot(document.body) is picking myOverloadedSlot(QObject*) > > instead of myOverloadedSlot(const QWebElement &). > > Did this ever pass? > > Also the comment looks wrong ("should pick myOverloadedSlot(QRegExp)"). > > I faced this kind of problem in https://bugs.webkit.org/show_bug.cgi?id=37060 > (the LayoutTestController change) and solved it by registering the QWebElement > type with qRegisterMetaType. I presume this is necessary to use QWebElement as > a slot argument. Thanks Jakub. I tried to register the type but it didn't make a difference. It's possible there's a bug in WebCore/bridge/qt/qt_runtime.cpp findMethodIndex() when calling an overloaded slot with a custom type as argument. I'll try to debug it next week.

Noam Rosenthal

Comment 4 2010-04-09 08:23:25 PDT

I added the slot with QWebElement - it definitely passed when I added it. Maybe it only works on Linux?

Kent Hansen

Comment 5 2010-04-09 08:30:51 PDT

(In reply to comment #4) > I added the slot with QWebElement - it definitely passed when I added it. Maybe > it only works on Linux? Only fails on Linux you mean? Simon is on Linux, I'm on Linux, and it doesn't work. Haven't tried other platforms yet.

Noam Rosenthal

Comment 6 2010-04-09 08:33:25 PDT

ok - misunderstood one of the comments here.

Kent Hansen

Comment 7 2010-04-12 06:12:35 PDT

OK, figured out why it's picking the overload that takes a QObject*; "document.body" evaluates to null. Setting some HTML on the frame, e.g. m_page->mainFrame()->setHtml("<html><head><body></body></html>") in the beginning of the test fixes that. I don't know if that's needed due to a recent change in behavior. But it still doesn't work. First, Jakub's comment about QWebElement needing to be registered as a metatype is correct after all. Calling qMetaTypeId<QWebElement>() in convertValueToQVariant() is too late; it has to be registered already when the argument types are resolved (otherwise we'll never get as far as the conversion anyways). QWebElement should be registered as a metatype somewhere (not in the test; either in the QWebFrame constructor or (ideally) some singleton). Second, even with the type registered, the conversion seems to get confused; apparently the DOM node can be converted to several other types, all of which are reported as equally good: ambiguous call of overloaded function myOverloadedSlot(); candidates were myOverloadedSlot(bool) myOverloadedSlot(QStringList) myOverloadedSlot(double) myOverloadedSlot(float) myOverloadedSlot(int) myOverloadedSlot(QString) myOverloadedSlot(QVariant) myOverloadedSlot(QWebElement) This is because all conversions report a match distance of 10. You want to report a lower one for the QWebElement case (match distance 0 == perfect match; isn't that the case here since QWebElement is just a thin wrapper around the DOM node? Well, 1 at most :) ) Fixing the above three issues makes the test pass for me.

Jocelyn Turcotte

Comment 8 2010-04-15 12:04:34 PDT

Created attachment 53460 [details] Patch: mark as expected failure The patch that was commited by in qt/src/3rdparty/webkit. The ideal would be to soon have something that prevent the failure in the qtwebkit-2.0 branch to ease the next import. Either this patch or a fix for the test.

Jocelyn Turcotte

Comment 9 2010-04-15 12:06:24 PDT

Created attachment 53462 [details] Patch: mark as expected failure Edit: added the bug url to the changelog

WebKit Commit Bot

Comment 10 2010-04-15 13:44:28 PDT

Comment on attachment 53462 [details] Patch: mark as expected failure Clearing flags on attachment: 53462 Committed r57669: <http://trac.webkit.org/changeset/57669>

WebKit Commit Bot

Comment 11 2010-04-15 13:44:33 PDT

All reviewed patches have been landed. Closing bug.

Simon Hausmann

Comment 12 2010-04-15 14:41:24 PDT

Revision r57669 cherry-picked into qtwebkit-2.0 with commit 1dc6dfe537a3532becfe17ee790755166c3c641e

Jędrzej Nowacki

Comment 13 2010-06-18 06:11:07 PDT

The QEXPECT_FAIL macro is not a right solution :-)

Noam Rosenthal

Comment 14 2011-01-30 17:23:01 PST

*** Bug 46748 has been marked as a duplicate of this bug. ***

Noam Rosenthal

Comment 15 2011-01-30 18:10:42 PST

Created attachment 80608 [details] Patch

Noam Rosenthal

Comment 16 2011-01-30 18:37:43 PST

Created attachment 80610 [details] Patch

Kenneth Rohde Christiansen

Comment 17 2011-01-31 00:37:27 PST

Comment on attachment 80610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80610&action=review > Source/WebCore/bridge/qt/qt_runtime.cpp:775 > + dist = 0; > + break; > + } > + > + ret = QVariant::fromValue<QWebElement>(QWebElement()); > + dist = 10; So these 0 and 10 are arbitary values?

Noam Rosenthal

Comment 18 2011-01-31 03:37:02 PST

(In reply to comment #17) > (From update of attachment 80610 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80610&action=review > > > Source/WebCore/bridge/qt/qt_runtime.cpp:775 > > + dist = 0; > > + break; > > + } > > + > > + ret = QVariant::fromValue<QWebElement>(QWebElement()); > > + dist = 10; > > So these 0 and 10 are arbitary values? Yes, they're just a way to sort the different overloads and how well they match the JS datatype.

Andreas Kling

Comment 19 2011-04-26 16:01:07 PDT

Comment on attachment 80610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80610&action=review > Source/WebCore/bridge/qt/qt_runtime.cpp:-769 > - else if (object && object->inherits(&JSDocument::s_info)) > - ret = QVariant::fromValue<QWebElement>(QtWebElementRuntime::create((static_cast<JSDocument*>(object))->impl()->documentElement())); You're removing some logic re: JSDocument here, what's the reason?

Laszlo Gombos

Comment 20 2011-04-26 16:59:49 PDT

(In reply to comment #19) > (From update of attachment 80610 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80610&action=review > > > Source/WebCore/bridge/qt/qt_runtime.cpp:-769 > > - else if (object && object->inherits(&JSDocument::s_info)) > > - ret = QVariant::fromValue<QWebElement>(QtWebElementRuntime::create((static_cast<JSDocument*>(object))->impl()->documentElement())); > > You're removing some logic re: JSDocument here, what's the reason? Removing this code might brake fast/dom/nodesFromRect-basic.html - see http://trac.webkit.org/changeset/71004/

Eric Seidel (no email)

Comment 21 2011-04-26 17:10:54 PDT

Comment on attachment 80610 [details] Patch Looks like there are unanswered concerns for this patch.

Simon Hausmann

Comment 22 2011-04-26 17:15:13 PDT

Comment on attachment 80610 [details] Patch r=me , but fast/dom/nodesFromRect-basic.html should be checked before/after landing, as Laszlo pointed out.

Caio Marcelo de Oliveira Filho

Comment 23 2011-05-04 18:08:10 PDT

Comment on attachment 80610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80610&action=review Noam, do you still plan to work on this? > Source/WebCore/bridge/qt/qt_runtime.cpp:235 > + else if (object->inherits(&JSHTMLElement::s_info)) > + hint = static_cast<QMetaType::Type>(qMetaTypeId<QWebElement>()); This doesn't affect overloadedSlots tests but do fix tst_QWebFrame::getSetStaticProperty for me. >>> Source/WebCore/bridge/qt/qt_runtime.cpp:-769 >>> - ret = QVariant::fromValue<QWebElement>(QtWebElementRuntime::create((static_cast<JSDocument*>(object))->impl()->documentElement())); >> >> You're removing some logic re: JSDocument here, what's the reason? > > Removing this code might brake fast/dom/nodesFromRect-basic.html - see > http://trac.webkit.org/changeset/71004/ It breaks. From my understand you shouldn't remove JSDocument case here. Actually, you could even 'dist = 0' for this case too.

Noam Rosenthal

Comment 24 2011-05-04 21:31:01 PDT

(In reply to comment #23) > (From update of attachment 80610 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80610&action=review > > Noam, do you still plan to work on this? I hope to do that at some point; but if someone else wants to take a stab I'm all for it. > >> You're removing some logic re: JSDocument here, what's the reason? Otherwise tst_QWebFrame::domCycles() fails. That test was based on the old handling of the document element. > > > > Removing this code might brake fast/dom/nodesFromRect-basic.html - see > > http://trac.webkit.org/changeset/71004/ > > It breaks. From my understand you shouldn't remove JSDocument case here. Actually, you could even 'dist = 0' for this case too. Then we have to fix the tst_QWebFrame::domCycles() test.

Noam Rosenthal

Comment 25 2011-05-04 23:55:54 PDT

Created attachment 92382 [details] Patch

WebKit Review Bot

Comment 26 2011-05-04 23:58:03 PDT

Attachment 92382 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bridge/qt/qt_runtime.cpp:768: 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] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Noam Rosenthal

Comment 27 2011-05-05 22:21:08 PDT

Created attachment 92540 [details] Patch

Caio Marcelo de Oliveira Filho

Comment 28 2011-05-17 16:03:06 PDT

Created attachment 93840 [details] Patch I asked Noam to pick up this one. This patch is on top of another patch for bug 60984, so will not compile yet. A summary of the changes: - Be more agressive when converting JSElement to QWebElement. If we have no hint for a JSElement, offer one. Once we get a QWebElement, identify it as a "perfect match" (dist = 0). This change was present in the latest version of Noam's patch. - Remove the special handling for 'document' object. The 'document' vs. 'document.documentElement' is complicated enough, as well as our logic for choosing the right slot. This implicit conversion was added for the sake of DRT nodesFromRect, but we can use QVariantMap conversion and pick the 'documentElement' instead. - This patch depends on the other because when creating the QVariantMap we hit into situation that an exception 'leaks' from the conversion function. The layout test and the autotests mentioned all pass.

Caio Marcelo de Oliveira Filho

Comment 29 2011-06-09 09:53:33 PDT

Created attachment 96598 [details] Rebased This is just a rebased version of the patch. I think we should land this fix before the patches of bridge refactoring land, since I think this one is fine for being cherry-picked for 2.2 release, OTOH I'm not sure we want the bridge refactoring there.

Caio Marcelo de Oliveira Filho

Comment 30 2011-06-09 13:47:03 PDT

Created attachment 96633 [details] Patch

Andreas Kling

Comment 31 2011-06-09 14:06:40 PDT

Comment on attachment 96633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96633&action=review r=me as is, but please consider making the following adjustments before landing: > Source/WebKit/qt/ChangeLog:23 > + (tst_QWebFrame::documentHasDocumentElement): Evaluate 'document' and extracts Evaluate -> Evaluates > Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:985 > - WebCore::Element* webElement = document.m_element; > + QWebElement documentElement = document.value(QLatin1String("documentElement")).value<QWebElement>(); We could ASSERT that the QVariant returned by document.value() here is valid. > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2313 > + QVariant v = m_page->mainFrame()->evaluateJavaScript("document"); This variable is used for a bit of this and that, the function would be more readable if you used separate variables with clearer names.

Caio Marcelo de Oliveira Filho

Comment 32 2011-06-09 14:55:45 PDT

Created attachment 96649 [details] Patch

Caio Marcelo de Oliveira Filho

Comment 33 2011-06-10 08:26:27 PDT

Created attachment 96746 [details] Patch

Caio Marcelo de Oliveira Filho

Comment 34 2011-06-10 11:30:06 PDT

Comment on attachment 96746 [details] Patch Rebasing

Caio Marcelo de Oliveira Filho

Comment 35 2011-06-10 13:43:41 PDT

Created attachment 96778 [details] Patch

Caio Marcelo de Oliveira Filho

Comment 36 2011-06-10 14:02:21 PDT

Sorry for the noise. Patch now applies.

WebKit Review Bot

Comment 37 2011-06-14 06:53:50 PDT

Comment on attachment 96778 [details] Patch Clearing flags on attachment: 96778 Committed r88796: <http://trac.webkit.org/changeset/88796>

WebKit Review Bot

Comment 38 2011-06-14 06:53:57 PDT

All reviewed patches have been landed. Closing bug.

Ademar Reis

Comment 39 2011-06-14 09:25:10 PDT

Revision r88796 cherry-picked into qtwebkit-2.2 with commit f49c89c <http://gitorious.org/webkit/qtwebkit/commit/f49c89c>

Csaba Osztrogonác

Comment 40 2011-06-16 01:55:09 PDT

(In reply to comment #37) > (From update of attachment 96778 [details]) > Clearing flags on attachment: 96778 > > Committed r88796: <http://trac.webkit.org/changeset/88796> Reopen, because it was rolled out by http://trac.webkit.org/changeset/89019

Caio Marcelo de Oliveira Filho

Comment 41 2011-06-27 09:55:45 PDT

Created attachment 98740 [details] Patch

Caio Marcelo de Oliveira Filho

Comment 42 2011-06-27 09:59:33 PDT

Reduced the scope of the patch. - Removing the special implicit conversion of document -> QWebElement was causing the Qt Debug build to timeout. This is because conversion of document -> QVariantMap hits the cyclic references in DOMMimeType and DOMPlugin, and stop after 200 levels of recursion. We can deal with that in a different bug. - Making the conversion of JSElement -> QWebElement more agressive isn't backwards compatible. IOW, existing code might rely that evaluateJavaScript("document.body") will return a QVariant with a QVariantMap inside, instead of a QWebElement.

Andreas Kling

Comment 43 2011-06-27 10:04:14 PDT

Comment on attachment 98740 [details] Patch r=me Good luck with the recursion issue if you're attacking that. :)

WebKit Review Bot

Comment 44 2011-06-27 10:18:37 PDT

Comment on attachment 98740 [details] Patch Clearing flags on attachment: 98740 Committed r89830: <http://trac.webkit.org/changeset/89830>

WebKit Review Bot

Comment 45 2011-06-27 10:18:45 PDT

All reviewed patches have been landed. Closing bug.

Ademar Reis

Comment 46 2011-06-27 12:28:55 PDT

Revision r89830 cherry-picked into qtwebkit-2.2 with commit 2b104d4 <http://gitorious.org/webkit/qtwebkit/commit/2b104d4>

Note You need to log in before you can comment on or make changes to this bug.