Bug 42679

<rdar://problem/8213865>

Created attachment 142830 [details] Patch

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.

Created attachment 142832 [details] Patch

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.

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

Created attachment 143690 [details] Test for EWS

Created attachment 143691 [details] Test Patch for EWS

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

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

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

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

Created attachment 143884 [details] Patch

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.

Created attachment 143949 [details] Patch with fixes

(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

Can someone please review the latest patch?

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.

Created attachment 156093 [details] Rebased patch for landing

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

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

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

Created attachment 156103 [details] Rebased patch

Comment on attachment 156103 [details] Rebased patch Clearing flags on attachment: 156103 Committed r124477: <http://trac.webkit.org/changeset/124477>

All reviewed patches have been landed. Closing bug.