Bug 36092 - noscript tag should render when @sandbox disables JavaScript
Summary: noscript tag should render when @sandbox disables JavaScript
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-13 16:33 PST by Adam Barth
Modified: 2010-03-16 09:09 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.58 KB, patch)
2010-03-13 16:50 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (7.21 KB, patch)
2010-03-15 21:41 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (1.30 KB, patch)
2010-03-16 09:09 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-03-13 16:33:23 PST
noscript tag should render when @sandbox disables JavaScript
Comment 1 Adam Barth 2010-03-13 16:50:48 PST
Created attachment 50664 [details]
Patch
Comment 2 Darin Adler 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
Comment 3 Adam Barth 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.)
Comment 4 Eric Seidel (no email) 2010-03-15 16:07:03 PDT
Attachment 50664 [details] was posted by a committer and has review+, assigning to Adam Barth for commit.
Comment 5 Adam Barth 2010-03-15 21:41:30 PDT
Created attachment 50760 [details]
Patch for landing
Comment 6 WebKit Commit Bot 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>
Comment 7 WebKit Commit Bot 2010-03-16 00:27:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Eric Seidel (no email) 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
Comment 9 Gustavo Noronha (kov) 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?
Comment 10 Gustavo Noronha (kov) 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
Comment 11 Adam Barth 2010-03-16 09:02:50 PDT
I bet the problem is the missing "!".  Will try to fix.
Comment 12 Adam Barth 2010-03-16 09:09:09 PDT
Created attachment 50798 [details]
Patch
Comment 13 Adam Barth 2010-03-16 09:09:38 PDT
Committed r56064: <http://trac.webkit.org/changeset/56064>