Bug 37323

(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.

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 on attachment 54060 [details] proposed patch Thanks a lot Yi! :-)

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

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

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

(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.

(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.

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

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 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 on attachment 54565 [details] fix the svn-apply issue Let's go for Antonio's solution :)

Comment on attachment 54562 [details] alternative approach Clearing flags on attachment: 54562 Committed r58427: <http://trac.webkit.org/changeset/58427>

Thank you Simon Hausmann!

(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 :(

(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 :)

Revision r58427 cherry-picked into qtwebkit-2.0 with commit adc47622d204163a416403abd925d2eff1817f0b

(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...

(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 ...

(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?

simon, ideas on what can be done here?

(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?

(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.

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.

Marking as closed, this is not meant for inclusion in the release branch.