Last Comment Bug 652442 - Script feature support should depend on whether javascript is enabled
: Script feature support should depend on whether javascript is enabled
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Robert Longson
:
Mentors:
Depends on: 695677
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-24 10:03 PDT by Robert Longson
Modified: 2011-10-19 23:38 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.22 KB, patch)
2011-04-24 10:03 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review
return false for SVG images too (9.34 KB, patch)
2011-04-28 01:04 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review
hg changeset patch with final review comments addressed (8.79 KB, patch)
2011-04-29 04:29 PDT, Robert Longson
no flags Details | Diff | Splinter Review
new attempt at unreliable reftest (8.82 KB, patch)
2011-04-30 02:53 PDT, Robert Longson
no flags Details | Diff | Splinter Review

Description Robert Longson 2011-04-24 10:03:22 PDT
Created attachment 528010 [details] [diff] [review]
patch

http://www.w3.org/TR/SVG11/feature#Script should only resolve if javascript is enabled.
Comment 1 Daniel Holbert [:dholbert] 2011-04-24 14:26:45 PDT
Comment on attachment 528010 [details] [diff] [review]
patch

Hm... Is "javascript.enabled" really the way we want to check for javascript being enabled?

In e.g. SVG-as-an-image (and external resource documents, too, I think), javascript is disabled as far as the SVG document is concerned, but the "javascript.enabled" pref won't reveal that.

If an author is taking the trouble to query whether scripting is enabled, they'd probably want their query to fail in those cases, right?
Comment 2 Daniel Holbert [:dholbert] 2011-04-24 14:38:50 PDT
Comment on attachment 528010 [details] [diff] [review]
patch

Ah, nevermind -- it looks like nsSVGFeatures' methods are all static, so I guess they can't tell what sort of document they're being called for.

So, if we want to have special handling for the image/resource doc case, we'd probably need to do that up two levels, in the non-static method nsGenericElement:::IsSupported.  And that's probably too generic of a place to have special-handling code for SVG...

Do you think it'd be appropriate to fail this feature-check for image/resource docs?  (If so, perhaps you could file a followup on that?)

r=dholbert on this, either way.
Comment 3 Robert Longson 2011-04-28 01:04:41 PDT
Created attachment 528808 [details] [diff] [review]
return false for SVG images too
Comment 4 Daniel Holbert [:dholbert] 2011-04-28 15:57:27 PDT
Comment on attachment 528808 [details] [diff] [review]
return false for SVG images too

>+nsSVGFeatures::HaveFeature(nsISupports* aObject, const nsAString& aFeature)
> {
>-#define SVG_SUPPORTED_FEATURE(str) if (aFeature.Equals(NS_LITERAL_STRING(str).get())) return PR_TRUE;
>+  if (aFeature.EqualsLiteral("http://www.w3.org/TR/SVG11/feature#Script")) {
>+    nsCOMPtr<nsIContent> content(do_QueryInterface(aObject));
>+    if (content) {
>+      nsIDocument *doc = content->GetCurrentDoc();
>+      if (doc && doc->IsBeingUsedAsImage()) {
>+        // no scripting in SVG images
>+        return PR_FALSE;

This wants:
s/IsBeingUsedAsImage/IsResourceDoc/
s/in SVG images/in SVG images and external resource documents/

(Assuming we want to consider scripting unsupported in external resource documents, which I imagine we do)
That'll need a testcase using an external resource doc, too, then
  (e.g. <use xlink:href="someOtherFile.svg#something">)

>+++ b/content/svg/content/src/nsSVGFeatures.h
>    * Check whether we support the given feature string.
>    *
>    * @param aFeature one of the feature strings specified at
>    *    http://www.w3.org/TR/SVG11/feature.html
>    */
>   static PRBool
>-  HaveFeature(const nsAString& aFeature);
>+  HaveFeature(nsISupports* aObject, const nsAString& aFeature);

This documentation needs updating with the new @param.

Also, why is |aObject| nsISupports rather than just nsIContent?  If it's always going to be nsIContent, can we just make it that? (then we won't need the QI dance inside of the HaveFeature impl)

> private:
>   /**
>    * Check whether we support the given list of feature strings.
>    *
>    * @param aFeatures a whitespace separated list containing one or more of the
>    *   feature strings specified at http://www.w3.org/TR/SVG11/feature.html
>    */
>   static PRBool
>-  HaveFeatures(const nsSubstring& aFeatures);
>+  HaveFeatures(nsISupports* aObject, const nsSubstring& aFeatures);

This documentation needs updating with the new @param, too.


>+++ b/layout/reftests/svg/as-image/script100x100.svg
[...]
>+++ b/layout/reftests/svg/as-image/svg-image-script-1.svg

Could you use a different failure-color in script100x100.svg vs. in svg-image-script-1.svg?  Right now they both use red, so if this test failed, we wouldn't be able to figure out what part had failed. (the image rendering vs. the script feature-detection)

r=dholbert with the above addressed.
Comment 5 Robert Longson 2011-04-28 16:32:27 PDT
(In reply to comment #4)
> Also, why is |aObject| nsISupports rather than just nsIContent?  If it's always
> going to be nsIContent, can we just make it that? (then we won't need the QI
> dance inside of the HaveFeature impl)

HaveFeature can be called with things other than nodes e.g. nsDOMAttribute or  nsDOMImplementation. These aren't derived from nsIContent.

I'll do the other comments.
Comment 6 Daniel Holbert [:dholbert] 2011-04-28 16:43:40 PDT
Ah, right. I figured as much, but I didn't notice any non-nsIContent-passing callers when I first read the patch.

Looking back now, I see that  nsGenericElement::InternalIsSupported will pass in aObject which is an nsISupports.  Makes sense.  Thanks!
Comment 7 Robert Longson 2011-04-29 04:29:23 PDT
Created attachment 529073 [details] [diff] [review]
hg changeset patch with final review comments addressed
Comment 8 Dão Gottwald [:dao] 2011-04-29 06:53:54 PDT
http://hg.mozilla.org/mozilla-central/rev/00f6e702f095
Comment 9 Dão Gottwald [:dao] 2011-04-29 08:13:51 PDT
Backed out, since svg-image-script-2.svg failed intermittently.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1304086959.1304088668.21248.gz

http://hg.mozilla.org/mozilla-central/rev/96ea841d7f4a
Comment 10 Robert Longson 2011-04-30 02:53:12 PDT
Created attachment 529273 [details] [diff] [review]
new attempt at unreliable reftest
Comment 11 Robert Longson 2011-04-30 02:53:24 PDT
http://tbpl.mozilla.org/?tree=Try&rev=3ccc4d8ab375
Comment 12 Robert Longson 2011-04-30 08:15:36 PDT
pushed http://hg.mozilla.org/mozilla-central/rev/1be13b4c6d80
Thanks for trying this before Dao,my apologies for the intermittent problem with the previous reftest
Comment 13 George Carstoiu 2011-07-27 06:28:04 PDT
 Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue on Ubuntu 11.04, Win7 x86, WinXP and Mac OS X 10.6 using the following steps:
1. Go to  http://www.w3.org/TR/SVG11/images/script/script01.svg
2. Click on red circle
3. Disable javascript
4. Reload page and click again on the red circle

Action performed only if js is enabled.

Setting status Verified Fixed.
Comment 14 Robert Longson 2011-07-27 06:44:55 PDT
(In reply to comment #13)
>  Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0
> 

That's not what this fix is about. Here's how you test this fix though.

1. Enable javascript
2. Goto http://www.codedread.com/svgtest.svg
3. Check that http://www.w3.org/TR/SVG11/feature#Script displays in green (it's about 3/4 of the way down the list)
4. Disable javascript
5. Refresh the page
6. Check that http://www.w3.org/TR/SVG11/feature#Script displays in red now.
Comment 15 George Carstoiu 2011-07-27 07:02:14 PDT
(In reply to comment #14)
> (In reply to comment #13)
> >  Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0
> > 
> 
> That's not what this fix is about. Here's how you test this fix though.
> 
> 1. Enable javascript
> 2. Goto http://www.codedread.com/svgtest.svg
> 3. Check that http://www.w3.org/TR/SVG11/feature#Script displays in green
> (it's about 3/4 of the way down the list)
> 4. Disable javascript
> 5. Refresh the page
> 6. Check that http://www.w3.org/TR/SVG11/feature#Script displays in red now.

Thanks for the heads-up.

Re-checked using steps from Comment 14 and everything seems in order.

Note You need to log in before you can comment on or make changes to this bug.