Bug 36092 – noscript tag should render when @sandbox disables JavaScript

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Adam Barth]

noscript tag should render when @sandbox disables JavaScript

[Adam Barth]

Created attachment 50664 [details]
Patch

[Darin Adler]

Comment on attachment 50664 [details]
Patch

It's great that you found all six call sites and fixed them. But the test checks only one or two of them. It would be better to make tests that cover as many of these as possible.

It might be good to have a comment in the Settings.h header file warning people away from using the isJavaScriptEnabled function to decide whether JavaScript is enabled. It's almost always wrong to use that function unless the code is simply trying to tweak settings. We don't want to make that header too confusing, but we also don't want anyone adding any new call sites like these.

r=me despite the limited test coverage

[Adam Barth]

I presume XHTMLMP isn't defined in our build, so I can't really test those cases.  Is the Canvas change testable?  I wasn't sure how to see what kind of render object gets created given that I have no way to draw into it without JavaScript.  The plugin case is hard to test too because @sandbox stops you from including plugins...  I'd certainly like to add tests if you had advice for how to do it.

(In any case, the comment is a good idea.)

[Eric Seidel (no email)]

Attachment 50664 [details] was posted by a committer and has review+, assigning to Adam Barth for commit.

[Adam Barth]

Created attachment 50760 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 50760 [details]
Patch for landing

Clearing flags on attachment: 50760

Committed r56046: <http://trac.webkit.org/changeset/56046>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Looks like this might have broken Gtk somehow?
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r56046%20(872)/plugins/embed-inside-object-diffs.txt

--- /var/lib/buildbot/build/gtk-linux-64-release/build/layout-test-results/plugins/embed-inside-object-expected.txt	2010-03-16 00:37:06.000000000 -0700
+++ /var/lib/buildbot/build/gtk-linux-64-release/build/layout-test-results/plugins/embed-inside-object-actual.txt	2010-03-16 00:37:06.000000000 -0700
@@ -1,5 +1,5 @@
+FAIL: Timed out waiting for notifyDone to be called
 
 This tests that it's possible to control an embed that is nested inside an object with a span tag in between.
 plugin object is: [object HTMLEmbedElement]
-SUCCESS

[Gustavo Noronha (kov)]

Waterfall indicates this change regressed plugins/embed-inside-object.html for GTK+. Any idea why that might be?

[Gustavo Noronha (kov)]

Qt too (not surprising since the plugin code is very similar with GTK+) http://build.webkit.org/results/Qt Linux Release/r56052 (8597)/results.html

[Adam Barth]

I bet the problem is the missing "!".  Will try to fix.

[Adam Barth]

Created attachment 50798 [details]
Patch

[Adam Barth]

Committed r56064: <http://trac.webkit.org/changeset/56064>