The setUserStyleSheetLocation and setUserStyleSheetEnabled are missing in LayoutTestController, which fails the http/tests/security/local-user-CSS-from-remote.html LayoutTest
Created attachment 46492 [details] Proposed patch
Attachment 46492 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:174: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 1
Let me add my 3 cents... > --- a/WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp > +++ b/WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp > @@ -171,6 +171,7 @@ void WebPage::resetSettings() > m_drt->layoutTestController()->setXSSAuditorEnabled(false); > > QWebSettings::setMaximumPagesInCache(0); // reset to default > + settings()->setUserStyleSheetUrl(QUrl("")); //reset to default The default constructor (QUrl()) would be fine as well. > --- a/WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp > +++ b/WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp > @@ -391,3 +391,22 @@ void LayoutTestController::overridePreference(const QString& name, const QVarian > else if (name == "WebKitUsesPageCachePreferenceKey") > QWebSettings::setMaximumPagesInCache(value.toInt()); > } > + > +static QUrl userStyleSheetLocation = QUrl(""); I would make it a member variable instead. > +void LayoutTestController::setUserStyleSheetLocation(const QString& url) > +{ > + if (!url.isEmpty()) Is this check even needed? > +void LayoutTestController::setUserStyleSheetEnabled(bool enabled) > +{ > + QWebSettings* settings = m_topLoadingFrame->page()->settings(); > + if (settings) { I doubt settings can be null here. > + if (enabled) > + settings->setUserStyleSheetUrl(userStyleSheetLocation); > + else > + settings->setUserStyleSheetUrl(QUrl("")); Same as above, I would use the default constructor.
(In reply to comment #3) > Let me add my 3 cents... > > > --- a/WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp > > +++ b/WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp > > @@ -171,6 +171,7 @@ void WebPage::resetSettings() > > m_drt->layoutTestController()->setXSSAuditorEnabled(false); > > > > QWebSettings::setMaximumPagesInCache(0); // reset to default > > + settings()->setUserStyleSheetUrl(QUrl("")); //reset to default > > The default constructor (QUrl()) would be fine as well. > > > --- a/WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp > > +++ b/WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp > > @@ -391,3 +391,22 @@ void LayoutTestController::overridePreference(const QString& name, const QVarian > > else if (name == "WebKitUsesPageCachePreferenceKey") > > QWebSettings::setMaximumPagesInCache(value.toInt()); > > } > > + > > +static QUrl userStyleSheetLocation = QUrl(""); > > I would make it a member variable instead. > > > +void LayoutTestController::setUserStyleSheetLocation(const QString& url) > > +{ > > + if (!url.isEmpty()) > > Is this check even needed? > > > +void LayoutTestController::setUserStyleSheetEnabled(bool enabled) > > +{ > > + QWebSettings* settings = m_topLoadingFrame->page()->settings(); > > + if (settings) { > > I doubt settings can be null here. > > > + if (enabled) > > + settings->setUserStyleSheetUrl(userStyleSheetLocation); > > + else > > + settings->setUserStyleSheetUrl(QUrl("")); > > Same as above, I would use the default constructor. Good suggestions! Thanks a lot. Reviewing now
Created attachment 46507 [details] Proposed patch v0.2 Correct style and add Jakub suggestions
LGTM ! simon, kenneth , could one of you guys please review ? thx (In reply to comment #5) > Created an attachment (id=46507) [details] > Proposed patch v0.2 > > Correct style and add Jakub suggestions
Comment on attachment 46507 [details] Proposed patch v0.2 LGTM, r=me > + if (enabled) > + m_drt->webPage()->settings()->setUserStyleSheetUrl(m_userStyleSheetLocation); > + else > + m_drt->webPage()->settings()->setUserStyleSheetUrl(QUrl()); Do you know what happens if I give an invalid url to setUserStyleSheetUrl? Is that handled?
Comment on attachment 46507 [details] Proposed patch v0.2 Clearing flags on attachment: 46507 Committed r53249: <http://trac.webkit.org/changeset/53249>
All reviewed patches have been landed. Closing bug.
This looks like this caused one test to regress: http://build.webkit.org/results/Qt%20Linux%20Release/r53249%20(5974)/http/tests/security/local-user-CSS-from-remote-pretty-diff.html I'm going to look at rolling this out unless someone is around to comment.
In case the buildbot results are purged by the time someone looks at this, here is the failing diff: --- layout-test-results/http/tests/security/local-user-CSS-from-remote-expected.txt 2010-01-14 01:54:07.000000000 -0800 +++ layout-test-results/http/tests/security/local-user-CSS-from-remote-actual.txt 2010-01-14 01:54:07.000000000 -0800 @@ -2,4 +2,4 @@ To run this test manually you must set your user style sheet in your Safari preferences to LayoutTests/http/tests/security/resources/cssStyle.css If the background is yellow then the user stylesheet was loaded. -Test Passed: Local user stylesheet loaded. +Test Failed: Local user stylesheet not loaded into remote document.
I guess Diego will first be around in 2 hours or so. He is in the Manaus, Brazil time zone.
Reverted r53249 for reason: This caused http/tests/security/local-user-CSS-from-remote.html to fail on the Qt Release Build Bot. Committed r53253: <http://trac.webkit.org/changeset/53253>
guys that test regressed because bug 33614 (see dependency field) has to land first ! kenneth , could look over that one and we re-try this ? (In reply to comment #13) > Reverted r53249 for reason: > > This caused http/tests/security/local-user-CSS-from-remote.html to fail on the > Qt Release Build Bot. > > Committed r53253: <http://trac.webkit.org/changeset/53253>
(In reply to comment #7) > (From update of attachment 46507 [details]) > LGTM, r=me > > > + if (enabled) > > + m_drt->webPage()->settings()->setUserStyleSheetUrl(m_userStyleSheetLocation); > > + else > > + m_drt->webPage()->settings()->setUserStyleSheetUrl(QUrl()); > > Do you know what happens if I give an invalid url to setUserStyleSheetUrl? Is > that handled? in this case, it is like unsetting the userStyleSheet
Yup. It's just better to roll-out first and ask questions later. That way we keep the bots green more of the time. In this case that's extra important because we have over a days worth of commits pent up in the commit-queue because it was blocked all day due to bug 33638. This will be easy to roll back in once we can do so without breaking the bots. Just go ahead and set r+/cq+ on the patch once it's ready to land again :)
Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog M LayoutTests/platform/qt/Skipped M WebKitTools/ChangeLog M WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp M WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp M WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.h Committed r53281
It's unclear what changed between checkins and why this new one isn't going to cause failures. :) The bots will tell. :)