RESOLVED FIXED 36092
noscript tag should render when @sandbox disables JavaScript
https://bugs.webkit.org/show_bug.cgi?id=36092
Summary noscript tag should render when @sandbox disables JavaScript
Adam Barth
Reported 2010-03-13 16:33:23 PST
noscript tag should render when @sandbox disables JavaScript
Attachments
Patch (6.58 KB, patch)
2010-03-13 16:50 PST, Adam Barth
no flags
Patch for landing (7.21 KB, patch)
2010-03-15 21:41 PDT, Adam Barth
no flags
Patch (1.30 KB, patch)
2010-03-16 09:09 PDT, Adam Barth
no flags
Adam Barth
Comment 1 2010-03-13 16:50:48 PST
Darin Adler
Comment 2 2010-03-14 13:51:17 PDT
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
Comment 3 2010-03-15 00:07:10 PDT
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)
Comment 4 2010-03-15 16:07:03 PDT
Attachment 50664 [details] was posted by a committer and has review+, assigning to Adam Barth for commit.
Adam Barth
Comment 5 2010-03-15 21:41:30 PDT
Created attachment 50760 [details] Patch for landing
WebKit Commit Bot
Comment 6 2010-03-16 00:27:30 PDT
Comment on attachment 50760 [details] Patch for landing Clearing flags on attachment: 50760 Committed r56046: <http://trac.webkit.org/changeset/56046>
WebKit Commit Bot
Comment 7 2010-03-16 00:27:35 PDT
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 8 2010-03-16 03:05:02 PDT
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)
Comment 9 2010-03-16 04:48:29 PDT
Waterfall indicates this change regressed plugins/embed-inside-object.html for GTK+. Any idea why that might be?
Gustavo Noronha (kov)
Comment 10 2010-03-16 04:51:06 PDT
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
Comment 11 2010-03-16 09:02:50 PDT
I bet the problem is the missing "!". Will try to fix.
Adam Barth
Comment 12 2010-03-16 09:09:09 PDT
Adam Barth
Comment 13 2010-03-16 09:09:38 PDT
Note You need to log in before you can comment on or make changes to this bug.