Bug 42679 – WebKitTestRunner needs layoutTestController.setUserStyleSheetEnabled

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Sam Weinig]

WebKitTestRunner needs layoutTestController.setUserStyleSheetEnabled

[Sam Weinig]

<rdar://problem/8213865>

[Dinu Jacob]

Created attachment 142830 [details]
Patch

[WebKit Review Bot]

Attachment 142830 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.h:101:  The parameter name "location" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dinu Jacob]

Created attachment 142832 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Thanks for the patch Dinu Jacob!

However setUserStyleSheetEnabled seems a good candidate to be added to window.internals.settings (WebCore/testing/InternalSettings) instead of LayoutTestController, because it just sets a property in the page's Settings.

That would avoid the creation of new private C API for the bundle, and possibly remove code from other ports. I did a similar patch for another setting in bug 42689. I am not a reviewer, but my suggestion is you go that route: add it to window.internals.settings.

[Dinu Jacob]

(In reply to comment #5)
> Thanks for the patch Dinu Jacob!
> 
> However setUserStyleSheetEnabled seems a good candidate to be added to window.internals.settings (WebCore/testing/InternalSettings) instead of LayoutTestController, because it just sets a property in the page's Settings.
> 
> That would avoid the creation of new private C API for the bundle, and possibly remove code from other ports. I did a similar patch for another setting in bug 42689. I am not a reviewer, but my suggestion is you go that route: add it to window.internals.settings.

Yes, it came to me as an after thought :( I will update the patch use Internals.

[Dinu Jacob]

Created attachment 143690 [details]
Test for EWS

[Dinu Jacob]

Created attachment 143691 [details]
Test Patch for EWS

[Build Bot]

Comment on attachment 143691 [details]
Test Patch for EWS

Attachment 143691 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12800026

[WebKit Review Bot]

Comment on attachment 143691 [details]
Test Patch for EWS

Attachment 143691 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12791064

New failing tests:
http/tests/security/local-user-CSS-from-remote.html
fast/loader/user-style-sheet-resource-load-callbacks.html

[WebKit Review Bot]

Created attachment 143703 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dinu Jacob]

(In reply to comment #6)
> (In reply to comment #5)
> > Thanks for the patch Dinu Jacob!
> > 
> > However setUserStyleSheetEnabled seems a good candidate to be added to window.internals.settings (WebCore/testing/InternalSettings) instead of LayoutTestController, because it just sets a property in the page's Settings.
> > 
> > That would avoid the creation of new private C API for the bundle, and possibly remove code from other ports. I did a similar patch for another setting in bug 42689. I am not a reviewer, but my suggestion is you go that route: add it to window.internals.settings.
> 
> Yes, it came to me as an after thought :( I will update the patch use Internals.

setUserStyleSheetEnabled is tied to setUserStyleSheetLocation. Currently Chromium retrieves the data from the css file and that is the string being set instead of the local file path in its dumpRenderTree implementation. Hence, can't use Internals Settings to replace the current implementation.

[Dinu Jacob]

Created attachment 143884 [details]
Patch

[Darin Adler]

Comment on attachment 143884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143884&action=review

> Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp:404
> +    WKRetainPtr<WKStringRef> emptyUrl = WKStringCreateWithUTF8CString("");

This leaks. It needs an adoptWK.

> Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp:411
> +    m_userStyleSheetLocation = WKStringCreateWithJSString(location);

This leaks. It needs an adoptWK.

[Dinu Jacob]

Created attachment 143949 [details]
Patch with fixes

[Dinu Jacob]

(In reply to comment #14)
> (From update of attachment 143884 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143884&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp:404
> > +    WKRetainPtr<WKStringRef> emptyUrl = WKStringCreateWithUTF8CString("");
> 
> This leaks. It needs an adoptWK.
> 
> > Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp:411
> > +    m_userStyleSheetLocation = WKStringCreateWithJSString(location);
> 
> This leaks. It needs an adoptWK.

Thanks. Fixed and new patch uploaded

[Dinu Jacob]

Can someone please review the latest patch?

[Eric Seidel (no email)]

Comment on attachment 143949 [details]
Patch with fixes

This looks reasonable.  I would think we'd want to move this to window.internals these days though.

[Dinu Jacob]

Created attachment 156093 [details]
Rebased patch for landing

[Build Bot]

Comment on attachment 156093 [details]
Rebased patch for landing

Attachment 156093 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13421460

[Build Bot]

Comment on attachment 156093 [details]
Rebased patch for landing

Attachment 156093 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13422444

[Early Warning System Bot]

Comment on attachment 156093 [details]
Rebased patch for landing

Attachment 156093 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13411921

[Dinu Jacob]

Created attachment 156103 [details]
Rebased patch

[WebKit Review Bot]

Comment on attachment 156103 [details]
Rebased patch

Clearing flags on attachment: 156103

Committed r124477: <http://trac.webkit.org/changeset/124477>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.