WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Adam Barth
Comment 1
2010-03-13 16:50:48 PST
Created
attachment 50664
[details]
Patch
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
Created
attachment 50798
[details]
Patch
Adam Barth
Comment 13
2010-03-16 09:09:38 PDT
Committed
r56064
: <
http://trac.webkit.org/changeset/56064
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug