Bug 33617 – [Qt] DRT missing setUserStyleSheetLocation and setUserStyleSheetEnabled in LayoutTestController

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Diego Gonzalez]

The setUserStyleSheetLocation and setUserStyleSheetEnabled are missing in LayoutTestController,
which fails the http/tests/security/local-user-CSS-from-remote.html LayoutTest

[Diego Gonzalez]

Created attachment 46492 [details]
Proposed patch

[WebKit Review Bot]

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

[Jakub Wieczorek]

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.

[Diego Gonzalez]

(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

[Diego Gonzalez]

Created attachment 46507 [details]
Proposed patch v0.2

Correct style and add Jakub suggestions

[Antonio Gomes]

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

[Kenneth Rohde Christiansen]

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?

[WebKit Commit Bot]

Comment on attachment 46507 [details]
Proposed patch v0.2

Clearing flags on attachment: 46507

Committed r53249: <http://trac.webkit.org/changeset/53249>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Kenneth Rohde Christiansen]

I guess Diego will first be around in 2 hours or so. He is in the Manaus, Brazil time zone.

[Eric Seidel (no email)]

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>

[Antonio Gomes]

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>

[Antonio Gomes]

(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

[Eric Seidel (no email)]

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

[Antonio Gomes]

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

[Eric Seidel (no email)]

It's unclear what changed between checkins and why this new one isn't going to cause failures. :)  The bots will tell. :)