Closed
Bug 783915
Opened 12 years ago
Closed 12 years ago
Bug fixes for SVGFragmentIdentifier
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: longsonr, Assigned: longsonr)
Details
Attachments
(1 file, 1 obsolete file)
15.88 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
Assignee: nobody → longsonr
Attachment #653206 -
Flags: review?(dholbert)
Assignee | ||
Updated•12 years ago
|
OS: Windows Vista → All
Hardware: x86 → All
Comment 2•12 years ago
|
||
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 3•12 years ago
|
||
(Comment 2 may merit a separate bug, too. I just commented on it here to be sure I was understanding correctly.)
Comment 4•12 years ago
|
||
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 5•12 years ago
|
||
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.
Comment 6•12 years ago
|
||
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.
Assignee | ||
Comment 7•12 years ago
|
||
(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.
Assignee | ||
Comment 8•12 years ago
|
||
(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 :-)
Comment 9•12 years ago
|
||
(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).
Assignee | ||
Comment 10•12 years ago
|
||
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 11•12 years ago
|
||
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+
Comment 12•12 years ago
|
||
(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.)
Assignee | ||
Comment 13•12 years ago
|
||
(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.
Assignee | ||
Comment 14•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=4611e77e25d1
Assignee | ||
Comment 15•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8247f7b038ce
Flags: in-testsuite+
Target Milestone: --- → mozilla17
Assignee | ||
Comment 16•12 years ago
|
||
Raised bug 785606 to cover the viewBox removal case and bug 785608 to cover spaces.
Comment 17•12 years ago
|
||
(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.)
Comment 18•12 years ago
|
||
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
Assignee | ||
Comment 19•12 years ago
|
||
Oops. Thanks Daniel.
Comment 20•12 years ago
|
||
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.
Description
•