Closed Bug 783915 Opened 12 years ago Closed 12 years ago

Bug fixes for SVGFragmentIdentifier

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: longsonr, Assigned: longsonr)

Details

Attachments

(1 file, 1 obsolete file)

Managed to write some tests which threw up some bugs. I'm hoping this might fix bug 761994 as it ensures that the attribute updates take place.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #653206 - Flags: review?(dholbert)
OS: Windows Vista → All
Hardware: x86 → All
Comment on attachment 653206 [details] [diff] [review]
patch

Haven't been through the whole patch yet, but noticed this when skimming the test tweaks & reviewing the spec:

>diff --git a/content/svg/content/test/test_fragments.html b/content/svg/content/test/test_fragments.html
>--- a/content/svg/content/test/test_fragments.html
>+++ b/content/svg/content/test/test_fragments.html
>   var tests = [
>-      new Test("svgView(preserveAspectRatio(xMaxYMin slice))", true),
[...]
>+      new Test("svgView(preserveAspectRatio(xMaxYMin slice))", true, null, "xMaxYMin slice", null),

I'm wondering about this------------------------------^ space character, which IIUC should make this fragment identifier _invalid_, per this chunk of spec:
  # Spaces are not allowed in fragment specifications; thus,
  # commas are used to separate numeric values within an SVG
  # view specification (e.g., #svgView(viewBox(0,0,200,200)))
  http://www.w3.org/TR/SVG/linking.html#SVGFragmentIdentifiers

From that spec chunk, I'd expect we'd need to reject that space in fragment IDs, and instead we'd need a comma there.

(Also: it doesn't look like we have any tests to check that we reject spaces in other spots (like in viewBox(), per spec above) right now.  Maybe that's because we don't reject spaces, as shown by the preserveAspectRatio() chunk quoted from the test above.  But: if & when we do reject them, we should add some tests here to verify that we reject them.)
(Comment 2 may merit a separate bug, too. I just commented on it here to be sure I was understanding correctly.)
Comment on attachment 653206 [details] [diff] [review]
patch

>+++ b/content/svg/content/src/SVGFragmentIdentifier.cpp
>-  const nsAString *viewBoxParams = nullptr;
>-  const nsAString *preserveAspectRatioParams = nullptr;
>-  const nsAString *zoomAndPanParams = nullptr;
>+  nsAutoString viewBoxParams;
>+  nsAutoString preserveAspectRatioParams;
>+  nsAutoString zoomAndPanParams;
[...]
>-      if (viewBoxParams) {
>+      if (!viewBoxParams.IsEmpty()) {
[etc]

To be clear -- were these s/nsAString-pointer/nsAutoString/ changes just for safety?  (no intended functional changes?)

It looks like this change will prevent us from accepting specifications like this:
  "svgView(viewBox())"
which I think should be accepted, if your <svg> originally has a viewBox (or a pAR value, or zoomAndPan) and you're trying to remove that with your svgView() identifier.  e.g. if you want the equivalent of root.setAttribute(viewBox, ""), basically.  (or setAttributeNS or whatever)

>   CharTokenizer<';'>tokenizer(
>     Substring(aViewSpec, bracketPos + 1, aViewSpec.Length() - bracketPos - 2));

This line is actually the only use of the "CharTokenizer" class in the entire mozilla-central codebase.  I suspect we should be using the more-generic & more-tested nsCharSeparatedTokenizer here, instead. (and I think CharTokenizer should probably go away, as I've just posted in bug 672641 comment 17 / bug 672641 comment 18)

So: as long as you're fixing up the neighboring code, could you change this to
 nsCharSeparatedTokenizer nsCharSeparatedTokenizer(Substring(...), ';')
?

>+  if (!preserveAspectRatioParams.IsEmpty()) {
>+    if (NS_FAILED(root->mPreserveAspectRatio.SetBaseValueString(preserveAspectRatioParams, root, true))) {

That's 107 characters long -- maybe linewrap after "preserveAspectRatioParams"?  (still ~90 characters long, but better.

>-  if (zoomAndPanParams) {
>-    nsCOMPtr<nsIAtom> valAtom = do_GetAtom(*zoomAndPanParams);
>+  if (!zoomAndPanParams.IsEmpty()) {
>+    nsCOMPtr<nsIAtom> valAtom = do_GetAtom(zoomAndPanParams);
>     const nsSVGEnumMapping *mapping = root->sZoomAndPanMap;

Shouldn't this be "nsSVGSVGElement::sZoomAndPanMap"?  (I could be wrong, but don't think I've seen mozilla code referring to static data via an instance variable.)

>     while (mapping->mKey) {
>       if (valAtom == *(mapping->mKey)) {
>-        root->mEnumAttributes[nsSVGSVGElement::ZOOMANDPAN].SetBaseValue(mapping->mVal, root);
>+        if (NS_FAILED(root->mEnumAttributes[nsSVGSVGElement::ZOOMANDPAN].SetBaseValue(mapping->mVal, root))) {
>+          return false;
>+        }
>         break;
>       }
>       mapping++;
>     }

That longest line there could use some long-line-treatment too. (111 characters long)

This is also a bunch of uncommented code that could use a little overview-comment to explain what's going on, so a reader doesn't have to stare quite as hard at it to know what it's doing.

Maybe above the "while" loop, something like
 // If we've got a valid zoomAndPan value, then set it on our root elem.

>+    if (!mapping->mKey) {
>+      return false;
>+    }

This new chunk could also use a comment.  Maybe inside the "if" clause, something like:
  // Unrecognized zoomAndPan value.
Comment on attachment 653206 [details] [diff] [review]
patch

>+    is(doc.rootElement.useCurrentView, test.valid,
>+       "Expected " + test.svgFragmentIdentifier + " to be " +
>+       (test.valid ? "valid" : "invalid"));
>+    
  ^^^^
nit: nix whitespace on blank line there

>+    var viewBoxString =
>+      doc.rootElement.hasAttribute("viewBox") ?
>+        doc.rootElement.getAttribute("viewBox") : null;
>+    is(viewBoxString, test.viewBoxString, "unexpected viewBox");

nit: here and afterwords, I think the "hasAttribute() / : null" dance is unnecessary.  If the element doesn't have the attribute, then getAttribute() will return null -- so I think you can just boldly call getAttribute() and let it return null if it fails.
Also, RE the "nsAString* vs nsAutoString" replacement that I mentioned at the beginning of comment 4 -- won't the assignment to nsAutoString trigger a string-copy?

It might be better to use nsDependentSubstring instead here, though I don't know if that has operator=().  (though maybe we can fake it with Rebind())

It'd be great to be sure we're avoiding a string-copy, if possible.
(In reply to Daniel Holbert [:dholbert] from comment #4)
> To be clear -- were these s/nsAString-pointer/nsAutoString/ changes just for
> safety?  (no intended functional changes?)

Nope, this is a functional fix. All the pointers end up pointing to whatever you parsed last so viewBox(...);preserveAspectRatio(...) ends up with both string pointers containing the preserveAspectRatio contents and the viewBox fails to parse. The additional mochitests picked this up.

> 
> It looks like this change will prevent us from accepting specifications like
> this:
>   "svgView(viewBox())"

Arguable as to whether that's really an error since a viewBox is 4 numbers. Nowhere does it say an empty viewBox is the same as no viewBox at all as far as I know. I'll see what Opera does with this.

> 
> Shouldn't this be "nsSVGSVGElement::sZoomAndPanMap"?  (I could be wrong, but
> don't think I've seen mozilla code referring to static data via an instance
> variable.)

Too much Java in my day job I guess.

I'll sort the other comments out and post an update.
(In reply to Daniel Holbert [:dholbert] from comment #6)
> Also, RE the "nsAString* vs nsAutoString" replacement that I mentioned at
> the beginning of comment 4 -- won't the assignment to nsAutoString trigger a
> string-copy?

Indeed and as far as I can tell I need that.

> 
> It might be better to use nsDependentSubstring instead here, though I don't
> know if that has operator=().  (though maybe we can fake it with Rebind())

I can try it.

> 
> It'd be great to be sure we're avoiding a string-copy, if possible.

Well yes, but it has to work too :-)
(In reply to Robert Longson from comment #7)
> Nope, this is a functional fix. All the pointers end up pointing to whatever
> you parsed last

Ah, of course.

> > Also, RE the "nsAString* vs nsAutoString" replacement that I mentioned at
> > the beginning of comment 4 -- won't the assignment to nsAutoString trigger a
> > string-copy?
> 
> Indeed and as far as I can tell I need that.

At least in theory, it should be doable without a string-copy, right?   Since viewBoxParams/preserveAspectRatioParams/zoomAndPanParams are all strict substrings of aViewSpec, so they can share the underlying memory (just like each |params| value will).
Attached patch updated patchSplinter Review
I've not addressed the spaces comma issue in comment 2. 

a) The quote only talks about what to do with numeric values and preserveAspectRatio doesn't have numeric values
b) In a triumph of interoperability Batik only accepts commas and Opera only accepts spaces.

So my view is that we ask w3c what to do and meanwhile I'll go with comment 3.

Also nobody accepts svgView(viewBox()) as clearing an existing view and we don't either with or without this patch. Well spotted for the hole in the specification though.

Everything else should be done. I consume the parameters directly now so there's no string copying.
Attachment #653206 - Attachment is obsolete: true
Attachment #653206 - Flags: review?(dholbert)
Attachment #654186 - Flags: review?(dholbert)
Comment on attachment 654186 [details] [diff] [review]
updated patch

>diff --git a/content/svg/content/src/SVGFragmentIdentifier.cpp b/content/svg/content/src/SVGFragmentIdentifier.cpp
>+  // SVGViewAttribute may occur in any order, but each type may only occur
>+  // at most one time in a correctly formed SVGViewSpec.
>+  // If we encounter any element more than once or get any syntax errors
>+  // we're going to return false and cancel any changes.

Nit: the word "element" might be misleading here (implying an XML element) -- maybe "component" or "attribute name"?

>+  nsCharSeparatedTokenizerTemplate<IgnoreWhitespace>tokenizer(
>+    Substring(aViewSpec, bracketPos + 1, aViewSpec.Length() - bracketPos - 2), ';');

Add a space between ">" and "tokenizer".

Also: "aViewSpec.Length() - bracketPos - 2" is somewhat magical there.  Looks like that's the expected length of the "..." in our "svgView(...)" string. Could you declare a helper-variable to make that value more obvious / self-describing, e.g.:
   uint32_t lengthOfViewSpec = aViewSpec.Length() - bracketPos - 2;
or (if you'd prefer) add a brief comment explaining the length?

(Note the "uint32_t" there -- bug 579517 just landed, so you'll want to use stdint types.  There's a script (attachment 650572 [details]) you can use to auto-unbitrot any patches that you've got in your .hg/patches/ directory)

>+      nsCOMPtr<nsIAtom> valAtom = do_GetAtom(params);
>+      const nsSVGEnumMapping *mapping = nsSVGSVGElement::sZoomAndPanMap;
>+      while (mapping->mKey) {
>+        if (valAtom == *(mapping->mKey)) {

I think do_GetAtom is be a bit wasteful here.  If |params| is a huge gibberish string, then I'm pretty sure do_GetAtom will gladly generate a new giant atom for us, and that's not really useful, since there are really only two atoms we're looking for -- nsGkAtoms::disable and nsGkAtoms::magnify.

I suspect that "NS_GetStaticAtom" would be better here:
  https://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsIAtom.idl#132
since we know that the values we're interested in will have static atoms associated with them.

We can store the result in a raw nsIAtom*, too, since static atoms aren't refcounted. And we can return early (skipping the loop) if we get null.

>+  if (root->mUseCurrentView) {
>+    // A previous SVGViewSpec may have overridden some attributes.
>+    // If they are no longer overridden we need to restore the old values

Nit: Add a period at the end of that comment.

>diff --git a/content/svg/content/test/test_fragments.html b/content/svg/content/test/test_fragments.html
>-function Test(svgFragmentIdentifier, valid) {
>+function Test(svgFragmentIdentifier, valid, viewBoxString, preserveAspectRatioString, zoomAndPanString) {

Long line -- add newline before " preserveAspectRatioString"

>   var tests = [
>+      new Test("view", true, "0 200 100 100", "none", null),
>+      new Test("unknown", false, null, null, null),
>+      new Test("svgView(viewBox(0,0,200,200))", true, "0 0 200 200", null, null),
>+      new Test("svgView(preserveAspectRatio(xMaxYMin slice))", true, null, "xMaxYMin slice", null),
>+      new Test("svgView(viewBox(1,2,3,4);preserveAspectRatio(xMinYMax))", true, "1 2 3 4", "xMinYMax meet", null),
>+      new Test("svgView(zoomAndPan(disable))", true, null, null, "disable"),

Could you add some tests to check our handling of the last character in the string?  (I initially thought we might not be checking for a terminal ")", but it looks like IsMatchingParameter() does handle that.  But I don't think that behavior is tested right now.)

e.g. after that last valid Test quoted above, add a few more variants with invalid input, like these:
      // Be sure we verify that there's a closing paren for svgView()
      // (and not too many closing parens)
      new Test("svgView(zoomAndPan(disable)", false, null, null, null),
      new Test("svgView(zoomAndPan(disable) ", false,  null, null, null),
      new Test("svgView(zoomAndPan(disable)]", false,  null, null, null),
      new Test("svgView(zoomAndPan(disable)))", false,  null, null, null),

and maybe also one like this...
      new Test("svgView(zoomAndPan(disable)) ", false,  null, null, null),
...if we want to reject junk (including whitespace) after the final closing paren. (I suspect we do...)

r=me with the above.
Attachment #654186 - Flags: review?(dholbert) → review+
(Could you also file a followup bug on Comment 2?  Whichever way the w3c thread shakes out, we'll at least want to add tests about how we handle svgView(viewBox(0 0 100 100)) -- valid or invalid -- and we may need to change the code too.)
(In reply to Daniel Holbert [:dholbert] from comment #11)
> and maybe also one like this...
>       new Test("svgView(zoomAndPan(disable)) ", false,  null, null, null),
> ...if we want to reject junk (including whitespace) after the final closing
> paren. (I suspect we do...)

That one passes as the string is trimmed before we get it. The other tests pass so I'll include them.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8247f7b038ce
Flags: in-testsuite+
Target Milestone: --- → mozilla17
Raised bug 785606 to cover the viewBox removal case and bug 785608 to cover spaces.
(In reply to Robert Longson from comment #15)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8247f7b038ce

(That commit message said "bug 785017" instead of this bug #, so I added a note over there (bug 785017 comment 7) directing people back here.)
For sanity's sake / correct hg-blame, I backed out 8247f7b038ce and re-landed it with the correct bug number.
  backout:    https://hg.mozilla.org/integration/mozilla-inbound/rev/4b46b309c126
  re-landing: https://hg.mozilla.org/integration/mozilla-inbound/rev/9245897374c1
Oops. Thanks Daniel.
https://hg.mozilla.org/mozilla-central/rev/9245897374c1
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.