Bug 37323 - [Qt] tst_QWebHistoryInterface::visitedLinks() fails
Summary: [Qt] tst_QWebHistoryInterface::visitedLinks() fails
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P1 Blocker
Assignee: Yi Shen
URL:
Keywords: Qt
Depends on:
Blocks: 38654
  Show dependency treegraph
 
Reported: 2010-04-09 03:00 PDT by Simon Hausmann
Modified: 2010-05-07 07:58 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (4.08 KB, patch)
2010-04-22 08:43 PDT, Yi Shen
commit-queue: commit-queue-
Details | Formatted Diff | Diff
alternative approach (4.22 KB, patch)
2010-04-28 07:16 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
fix the svn-apply issue (3.99 KB, patch)
2010-04-28 08:09 PDT, Yi Shen
hausmann: review-
hausmann: commit-queue-
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 03:00:47 PDT
FAIL!  : tst_QWebHistoryInterface::visitedLinks() Compared values are not the same
   Actual (m_page->mainFrame()->evaluateJavaScript("document.querySelectorAll(':visited').length;").toString()): 0
   Expected (QString::fromLatin1("1")): 1
   Loc: [/home/shausman/src/webkit/trunk/WebKit/qt/tests/qwebhistoryinterface/tst_qwebhistoryinterface.cpp(90)]

Reproducible on Linux/X11.
Comment 1 Jakub Wieczorek 2010-04-09 06:57:35 PDT
(In reply to comment #0)
> FAIL!  : tst_QWebHistoryInterface::visitedLinks() Compared values are not the
> same
>    Actual
> (m_page->mainFrame()->evaluateJavaScript("document.querySelectorAll(':visited').length;").toString()):
> 0
>    Expected (QString::fromLatin1("1")): 1
>    Loc:
> [/home/shausman/src/webkit/trunk/WebKit/qt/tests/qwebhistoryinterface/tst_qwebhistoryinterface.cpp(90)]
> 
> Reproducible on Linux/X11.

That would be the new :visited patch that has just landed: https://bugs.webkit.org/show_bug.cgi?id=24300.
Comment 2 Yi Shen 2010-04-22 08:43:37 PDT
Created attachment 54060 [details]
proposed patch

Fix the tst_QWebHistoryInterface::visitedLinks by using DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo to get the correct info.
I have run the test on my local many many times and I am sure DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo won't cause any crash.
Please take a look at the comments of https://bugs.webkit.org/show_bug.cgi?id=37759. Thanks a lot!
Comment 3 Simon Hausmann 2010-04-27 14:01:04 PDT
Comment on attachment 54060 [details]
proposed patch

Thanks a lot Yi! :-)
Comment 4 WebKit Commit Bot 2010-04-27 19:38:38 PDT
Comment on attachment 54060 [details]
proposed patch

Rejecting patch 54060 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Simon Hausmann', u'--force']" exit_code: 1
Last 500 characters of output:
g file WebKit/qt/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp
Hunk #2 succeeded at 416 with fuzz 2 (offset 45 lines).
patching file WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.h
Hunk #1 FAILED at 24.
Hunk #2 succeeded at 86 (offset 3 lines).
1 out of 2 hunks FAILED -- saving rejects to file WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.h.rej
patching file WebKit/qt/tests/qwebhistoryinterface/tst_qwebhistoryinterface.cpp

Full output: http://webkit-commit-queue.appspot.com/results/1905083
Comment 5 Yi Shen 2010-04-27 20:00:45 PDT
Is it because my patch is out of date? I will create a new one then.

(In reply to comment #4)
> (From update of attachment 54060 [details])
> Rejecting patch 54060 from commit-queue.
> 
> Failed to run
> "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply',
> u'--reviewer', u'Simon Hausmann', u'--force']" exit_code: 1
> Last 500 characters of output:
> g file WebKit/qt/ChangeLog
> Hunk #1 succeeded at 1 with fuzz 3.
> patching file WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp
> Hunk #2 succeeded at 416 with fuzz 2 (offset 45 lines).
> patching file WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.h
> Hunk #1 FAILED at 24.
> Hunk #2 succeeded at 86 (offset 3 lines).
> 1 out of 2 hunks FAILED -- saving rejects to file
> WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.h.rej
> patching file WebKit/qt/tests/qwebhistoryinterface/tst_qwebhistoryinterface.cpp
> 
> Full output: http://webkit-commit-queue.appspot.com/results/1905083
Comment 6 Antonio Gomes 2010-04-27 20:31:32 PDT
hum, patch works, but doing this include from WebKit/qt/tests looks a bit strange to me =(

+#include "../WebCoreSupport/DumpRenderTreeSupportQt.h"
 #include <QtTest/QtTest>
 
+    QString linkColor = DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo(m_page->mainFrame(), "vlink")["color"].toString();


and this looks a bit intrusive ...

simon, any idea to make it work differently? if no, just ignore me
Comment 7 Simon Hausmann 2010-04-28 01:02:54 PDT
(In reply to comment #6)
> hum, patch works, but doing this include from WebKit/qt/tests looks a bit
> strange to me =(
> 
> +#include "../WebCoreSupport/DumpRenderTreeSupportQt.h"
>  #include <QtTest/QtTest>
> 
> +    QString linkColor =
> DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo(m_page->mainFrame(),
> "vlink")["color"].toString();
> 
> 
> and this looks a bit intrusive ...
> 
> simon, any idea to make it work differently? if no, just ignore me

I don't see much of a choice at this point :( It's late in the game to add new APIs, so the DRT function for unit testing is fine with me.
Comment 8 Antonio Gomes 2010-04-28 07:14:05 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > hum, patch works, but doing this include from WebKit/qt/tests looks a bit
> > strange to me =(
> > 
> > +#include "../WebCoreSupport/DumpRenderTreeSupportQt.h"
> >  #include <QtTest/QtTest>
> > 
> > +    QString linkColor =
> > DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo(m_page->mainFrame(),
> > "vlink")["color"].toString();
> > 
> > 
> > and this looks a bit intrusive ...
> > 
> > simon, any idea to make it work differently? if no, just ignore me
> 
> I don't see much of a choice at this point :( It's late in the game to add new
> APIs, so the DRT function for unit testing is fine with me.

Simon, I think we can use

QWebElement::styleProperty(const QString &name, StyleResolveStrategy strategy)

i will upload a patch that shows what I have in mind.
Comment 9 Antonio Gomes 2010-04-28 07:16:02 PDT
Created attachment 54562 [details]
alternative approach

this patch makes QWebElement::styleProperty method instead of DumpRenderTreeQt additional method.

ps: Yi's authorship is kept in the ChangeLog
Comment 10 Yi Shen 2010-04-28 08:09:49 PDT
Created attachment 54565 [details]
fix the svn-apply issue

Thanks Antonio. Your patch looks nice for me but I am not familiar with QWebElement::styleProperty() changes. I also updated my patch and attached it for review. Let Simon decide which patch should be used :)

The reasons why I prefer to add DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo to handle this test case are
1) using DRT function for unit testing can also be found in tst_qwebpage.cpp.
2) we may need an implementation of LayoutTestController::computedStyleIncludingVisitedInfo for Qt, which can invoke DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo.
see https://bugs.webkit.org/show_bug.cgi?id=37759. I was fighting against the test script which causes the new interface crashes.
Comment 11 Simon Hausmann 2010-04-28 13:08:17 PDT
Comment on attachment 54562 [details]
alternative approach

You're right Antonio, the reason for the change in behaviour of computedStyle() was to protect the user from malicious javascript.

There's little point in providing much security through our C++ API, given that it gives full control anyway.

Therefore your patch is much simpler :)
Comment 12 Simon Hausmann 2010-04-28 13:08:46 PDT
Comment on attachment 54565 [details]
fix the svn-apply issue

Let's go for Antonio's solution :)
Comment 13 Antonio Gomes 2010-04-28 21:37:23 PDT
Comment on attachment 54562 [details]
alternative approach

Clearing flags on attachment: 54562

Committed r58427: <http://trac.webkit.org/changeset/58427>
Comment 14 Antonio Gomes 2010-04-28 21:37:51 PDT
Thank you Simon Hausmann!
Comment 15 Antonio Gomes 2010-04-28 21:39:34 PDT
(In reply to comment #13)
> (From update of attachment 54562 [details])
> Clearing flags on attachment: 54562
> 
> Committed r58427: <http://trac.webkit.org/changeset/58427>

Err, I had a typo in the commit message:

"Reviewed by NOBODY Simon Hausmann."

hope it does not hurt much. sorry :(
Comment 16 Simon Hausmann 2010-04-28 22:45:43 PDT
(In reply to comment #15)
> (In reply to comment #13)
> > (From update of attachment 54562 [details] [details])
> > Clearing flags on attachment: 54562
> > 
> > Committed r58427: <http://trac.webkit.org/changeset/58427>
> 
> Err, I had a typo in the commit message:
> 
> "Reviewed by NOBODY Simon Hausmann."
> 
> hope it does not hurt much. sorry :(

Not a problem at all, I had a good laugh :)
Comment 17 Simon Hausmann 2010-04-29 00:08:54 PDT
Revision r58427 cherry-picked into qtwebkit-2.0 with commit adc47622d204163a416403abd925d2eff1817f0b
Comment 18 Simon Hausmann 2010-04-29 01:14:14 PDT
(In reply to comment #17)
> Revision r58427 cherry-picked into qtwebkit-2.0 with commit
> adc47622d204163a416403abd925d2eff1817f0b

Rolled out with commit a3a6be07b35ace4738aab1f3b1dc2caa755175e9:


   ../../../WebCore/css/CSSComputedStyleDeclaration.h: In member function â<U+0080><U+0098>QString QWebElement::styleProperty(const QString&, QWebElement::StyleResolveStrategy) constâ<U+0080><U+0099>:
    ../../../WebCore/css/CSSComputedStyleDeclaration.h:75: error: too many arguments to function â<U+0080><U+0098>WTF::PassRefPtr WebCore::computedStyle(WTF::PassRefPtr)â<U+0080><U+0099>
    ../../../WebKit/qt/Api/qwebelement.cpp:824: error: at this point in file



The private links patch is not part of the branch. Not sure if we should actually pick it, it's quite intrusive...
Comment 19 Antonio Gomes 2010-04-29 03:51:20 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > Revision r58427 cherry-picked into qtwebkit-2.0 with commit
> > adc47622d204163a416403abd925d2eff1817f0b
> 
> Rolled out with commit a3a6be07b35ace4738aab1f3b1dc2caa755175e9:
> 
> 
>    ../../../WebCore/css/CSSComputedStyleDeclaration.h: In member function
> â<U+0080><U+0098>QString QWebElement::styleProperty(const QString&,
> QWebElement::StyleResolveStrategy) constâ<U+0080><U+0099>:
>     ../../../WebCore/css/CSSComputedStyleDeclaration.h:75: error: too many
> arguments to function â<U+0080><U+0098>WTF::PassRefPtr
> WebCore::computedStyle(WTF::PassRefPtr)â<U+0080><U+0099>
>     ../../../WebKit/qt/Api/qwebelement.cpp:824: error: at this point in file
> 
> 
> 
> The private links patch is not part of the branch. Not sure if we should
> actually pick it, it's quite intrusive...

Ahh! :( should we go with the other approach for the branch? I really did not realize it could happen ...
Comment 20 Antonio Gomes 2010-04-29 05:22:19 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > Revision r58427 cherry-picked into qtwebkit-2.0 with commit
> > > adc47622d204163a416403abd925d2eff1817f0b
> > 
> > Rolled out with commit a3a6be07b35ace4738aab1f3b1dc2caa755175e9:
> > 
> > 
> >    ../../../WebCore/css/CSSComputedStyleDeclaration.h: In member function
> > â<U+0080><U+0098>QString QWebElement::styleProperty(const QString&,
> > QWebElement::StyleResolveStrategy) constâ<U+0080><U+0099>:
> >     ../../../WebCore/css/CSSComputedStyleDeclaration.h:75: error: too many
> > arguments to function â<U+0080><U+0098>WTF::PassRefPtr
> > WebCore::computedStyle(WTF::PassRefPtr)â<U+0080><U+0099>
> >     ../../../WebKit/qt/Api/qwebelement.cpp:824: error: at this point in file
> > 

> Ahh! :( should we go with the other approach for the branch? I really did not
> realize it could happen ...

ahh (again) the "other" approach also makes use of this the new parameter introduced by hyatt :(

> > The private links patch is not part of the branch. Not sure if we should
> > actually pick it, it's quite intrusive...

humm, it is considered a security fix. Are you considering to cherry pick it at all, simon, or it is out of the question?
Comment 21 Antonio Gomes 2010-05-05 04:34:41 PDT
simon, ideas on what can be done here?
Comment 22 Simon Hausmann 2010-05-05 08:13:24 PDT
(In reply to comment #21)
> simon, ideas on what can be done here?

I've looked into cherry-picking the private links feature, but it turns out to be a big effort. There have been a _lot_ of follow-up patches after the initial landing, mostly performance related.

I don't feel comfortable to cherry-pick a huge series of patches and risk missing one, to be honest. If one of you wants to do it, I'd be happy to review the picks.

My feeling is that it may be simpler to take this into the next 2.1 release. What do you think?
Comment 23 Antonio Gomes 2010-05-05 08:28:42 PDT
(In reply to comment #22)
> (In reply to comment #21)
> > simon, ideas on what can be done here?
> 
> I've looked into cherry-picking the private links feature, but it turns out to
> be a big effort. There have been a _lot_ of follow-up patches after the initial
> landing, mostly performance related.
> 
> I don't feel comfortable to cherry-pick a huge series of patches and risk
> missing one, to be honest. If one of you wants to do it, I'd be happy to review
> the picks.
> 
> My feeling is that it may be simpler to take this into the next 2.1 release.
> What do you think?

I see. My concern is what to do with this test in the 2.0 branch? Maybe "skip" it, like a known test failure?

on trunk it is ok and will be in 2.1.
Comment 24 Antonio Gomes 2010-05-07 07:51:40 PDT
It seems that tronical reported this bug because auto tests was failing on trunk, not on the qtwebkit-2.0 release branch. Hence, since the release branch does not have hyatt's "visited links fixes' series of patches either, this autotest does not fail on release branch, and this fix does not need to be cherry-picked.

I just build qtwebkit-2.0 b4aa5e1ddc41edab895132aba3cc66d9d7129444 , and auto test passes fine.
Comment 25 Simon Hausmann 2010-05-07 07:58:55 PDT
Marking as closed, this is not meant for inclusion in the release branch.