Bug 37319 - [Qt] tst_QWebFrame::overloadedSlots() fails
Summary: [Qt] tst_QWebFrame::overloadedSlots() fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Major
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords: Qt, QtTriaged
Depends on: 62790
Blocks: 38654
  Show dependency treegraph
 
Reported: 2010-04-09 02:55 PDT by Simon Hausmann
Modified: 2011-06-27 12:28 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 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.
Comment 1 Kent Hansen 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)").
Comment 2 Jakub Wieczorek 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.
Comment 3 Kent Hansen 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.
Comment 4 Noam Rosenthal 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?
Comment 5 Kent Hansen 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.
Comment 6 Noam Rosenthal 2010-04-09 08:33:25 PDT
ok - misunderstood one of the comments here.
Comment 7 Kent Hansen 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.
Comment 8 Jocelyn Turcotte 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.
Comment 9 Jocelyn Turcotte 2010-04-15 12:06:24 PDT
Created attachment 53462 [details]
Patch: mark as expected failure

Edit: added the bug url to the changelog
Comment 10 WebKit Commit Bot 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>
Comment 11 WebKit Commit Bot 2010-04-15 13:44:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Simon Hausmann 2010-04-15 14:41:24 PDT
Revision r57669 cherry-picked into qtwebkit-2.0 with commit 1dc6dfe537a3532becfe17ee790755166c3c641e
Comment 13 Jędrzej Nowacki 2010-06-18 06:11:07 PDT
The QEXPECT_FAIL macro is not a right solution :-)
Comment 14 Noam Rosenthal 2011-01-30 17:23:01 PST
*** Bug 46748 has been marked as a duplicate of this bug. ***
Comment 15 Noam Rosenthal 2011-01-30 18:10:42 PST
Created attachment 80608 [details]
Patch
Comment 16 Noam Rosenthal 2011-01-30 18:37:43 PST
Created attachment 80610 [details]
Patch
Comment 17 Kenneth Rohde Christiansen 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?
Comment 18 Noam Rosenthal 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.
Comment 19 Andreas Kling 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?
Comment 20 Laszlo Gombos 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/
Comment 21 Eric Seidel 2011-04-26 17:10:54 PDT
Comment on attachment 80610 [details]
Patch

Looks like there are unanswered concerns for this patch.
Comment 22 Simon Hausmann 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.
Comment 23 Caio Marcelo de Oliveira Filho 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.
Comment 24 Noam Rosenthal 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.
Comment 25 Noam Rosenthal 2011-05-04 23:55:54 PDT
Created attachment 92382 [details]
Patch
Comment 26 WebKit Review Bot 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.
Comment 27 Noam Rosenthal 2011-05-05 22:21:08 PDT
Created attachment 92540 [details]
Patch
Comment 28 Caio Marcelo de Oliveira Filho 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.
Comment 29 Caio Marcelo de Oliveira Filho 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.
Comment 30 Caio Marcelo de Oliveira Filho 2011-06-09 13:47:03 PDT
Created attachment 96633 [details]
Patch
Comment 31 Andreas Kling 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.
Comment 32 Caio Marcelo de Oliveira Filho 2011-06-09 14:55:45 PDT
Created attachment 96649 [details]
Patch
Comment 33 Caio Marcelo de Oliveira Filho 2011-06-10 08:26:27 PDT
Created attachment 96746 [details]
Patch
Comment 34 Caio Marcelo de Oliveira Filho 2011-06-10 11:30:06 PDT
Comment on attachment 96746 [details]
Patch

Rebasing
Comment 35 Caio Marcelo de Oliveira Filho 2011-06-10 13:43:41 PDT
Created attachment 96778 [details]
Patch
Comment 36 Caio Marcelo de Oliveira Filho 2011-06-10 14:02:21 PDT
Sorry for the noise. Patch now applies.
Comment 37 WebKit Review Bot 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>
Comment 38 WebKit Review Bot 2011-06-14 06:53:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 39 Ademar Reis 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>
Comment 40 Csaba Osztrogonác 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
Comment 41 Caio Marcelo de Oliveira Filho 2011-06-27 09:55:45 PDT
Created attachment 98740 [details]
Patch
Comment 42 Caio Marcelo de Oliveira Filho 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.
Comment 43 Andreas Kling 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. :)
Comment 44 WebKit Review Bot 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>
Comment 45 WebKit Review Bot 2011-06-27 10:18:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 46 Ademar Reis 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>