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: | P2 | Keywords: | Qt, QtTriaged | ||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||||||||||
Bug Depends on: | 62790 | ||||||||||||||||||||||||||||||
Bug Blocks: | 38654 | ||||||||||||||||||||||||||||||
Attachments: |
|
Description
Simon Hausmann
2010-04-09 02:55:29 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)"). (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. (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. I added the slot with QWebElement - it definitely passed when I added it. Maybe it only works on Linux? (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. ok - misunderstood one of the comments here. 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. 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.
Created attachment 53462 [details]
Patch: mark as expected failure
Edit: added the bug url to the changelog
Comment on attachment 53462 [details] Patch: mark as expected failure Clearing flags on attachment: 53462 Committed r57669: <http://trac.webkit.org/changeset/57669> All reviewed patches have been landed. Closing bug. Revision r57669 cherry-picked into qtwebkit-2.0 with commit 1dc6dfe537a3532becfe17ee790755166c3c641e The QEXPECT_FAIL macro is not a right solution :-) *** Bug 46748 has been marked as a duplicate of this bug. *** Created attachment 80608 [details]
Patch
Created attachment 80610 [details]
Patch
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? (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. 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? (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/ Comment on attachment 80610 [details]
Patch
Looks like there are unanswered concerns for this patch.
Comment on attachment 80610 [details]
Patch
r=me , but fast/dom/nodesFromRect-basic.html should be checked before/after landing, as Laszlo pointed out.
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. (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. Created attachment 92382 [details]
Patch
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.
Created attachment 92540 [details]
Patch
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. 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.
Created attachment 96633 [details]
Patch
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. Created attachment 96649 [details]
Patch
Created attachment 96746 [details]
Patch
Comment on attachment 96746 [details]
Patch
Rebasing
Created attachment 96778 [details]
Patch
Sorry for the noise. Patch now applies. Comment on attachment 96778 [details] Patch Clearing flags on attachment: 96778 Committed r88796: <http://trac.webkit.org/changeset/88796> All reviewed patches have been landed. Closing bug. Revision r88796 cherry-picked into qtwebkit-2.2 with commit f49c89c <http://gitorious.org/webkit/qtwebkit/commit/f49c89c> (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 Created attachment 98740 [details]
Patch
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. Comment on attachment 98740 [details]
Patch
r=me
Good luck with the recursion issue if you're attacking that. :)
Comment on attachment 98740 [details] Patch Clearing flags on attachment: 98740 Committed r89830: <http://trac.webkit.org/changeset/89830> All reviewed patches have been landed. Closing bug. Revision r89830 cherry-picked into qtwebkit-2.2 with commit 2b104d4 <http://gitorious.org/webkit/qtwebkit/commit/2b104d4> |