We really need to get this in for 15 as zoomAndPan fragments can crash the browser without this.

Comment on attachment 627721 [details] [diff] [review] update uuid too >-const PRUint16* >+const PRUint16 > nsSVGSVGElement::GetZoomAndPanProperty() const > { > void* valPtr = GetProperty(nsGkAtoms::zoomAndPan); > if (valPtr) { >- return reinterpret_cast<PRUint16*>(valPtr); >+ return reinterpret_cast<PRUint16>(valPtr); This doesn't compile for me -- GCC says: > nsSVGSVGElement.cpp:1434:45: error: cast from ‘void*’ to > ‘PRUint16 {aka short unsigned int}’ loses precision [-fpermissive] Presumably we actually want: return *reinterpret_cast<PRUint16*>(valPtr); (the same as it was before, but with a * at the beginning)?

(In reply to Daniel Holbert [:dholbert] from comment #3) > > Presumably we actually want: > return *reinterpret_cast<PRUint16*>(valPtr); > (the same as it was before, but with a * at the beginning)? Actually no, we don't want to dereference. Fortunately there's a uintptr_t that seems to fix this.

> void > SVGFragmentIdentifier::SaveOldZoomAndPan(nsSVGSVGElement *root) > { >- const PRUint16 *oldZoomAndPanPtr = root->GetZoomAndPanProperty(); >- if (!oldZoomAndPanPtr) { >+ PRUint16 oldZoomAndPan = root->GetZoomAndPanProperty(); >+ if (oldZoomAndPan != nsIDOMSVGZoomAndPan::SVG_ZOOMANDPAN_UNKNOWN) { > root->SetZoomAndPanProperty(root->mEnumAttributes[nsSVGSVGElement::ZOOMANDPAN].GetBaseValue()); > } Previously, this was saying "if root doesn't have a ZoomAndPanProperty stored, then store its current attribute baseval there." Now it seems to be saying "if root _has_ a (non-unknown) ZoomAndPanProperty stored, then store its current attribute baseval there." So, I think this logic might've been unintentionally flipped (and I think the old logic made more sense)... I might be misunderstanding though.

Comment on attachment 628296 [details] [diff] [review] remove silly const Expanding on Comment 7 slightly -- so it looks like with the current patch, SaveOldZoomAndPan() will never actually end up saving anything, since we'll always fail the "if" check. It worries me a bit that we don't have any tests to verify our behavior on that... (assuming the current patch passes tests) Could you add a test for that? (In reply to Robert Longson from comment #5) > Actually no, we don't want to dereference. Fortunately there's a uintptr_t > that seems to fix this. (Righto -- I hadn't groked what was actually going on before & was just focusing on the compile error. uintptr_t looks like a reasonable solution.) > bool > SVGFragmentIdentifier::ProcessSVGViewSpec(const nsAString &aViewSpec, > nsSVGSVGElement *root) > { >@@ -103,16 +103,19 @@ SVGFragmentIdentifier::ProcessSVGViewSpe > const nsAString *preserveAspectRatioParams = nsnull; > const nsAString *zoomAndPanParams = nsnull; > > // Each token is a SVGViewAttribute > PRInt32 bracketPos = aViewSpec.FindChar('('); > CharTokenizer<';'>tokenizer( > Substring(aViewSpec, bracketPos + 1, aViewSpec.Length() - bracketPos - 2)); > >+ if (!tokenizer.hasMoreTokens()) { >+ return false; >+ } > while (tokenizer.hasMoreTokens()) { Nit: The "while(tokenizer.hasMoreTokens())" check is now guaranteed to be true when we first enter the loop, so this could now be converted to do { ... } while (tokenizer.hasMoreTokens()); to eliminate that unnecessary check. >diff --git a/content/svg/content/test/test_fragments.html b/content/svg/content/test/test_fragments.html >new file mode 100644 >--- /dev/null >+++ b/content/svg/content/test/test_fragments.html [...] >+ for (var i = 0; i < tests.length; i++) { >+ var test = tests[i]; >+ svg.setAttribute("src", src + "#" + test.svgFragmentIdentifier); >+ is(doc.rootElement.useCurrentView, test.valid, "Expected " + test.svgFragmentIdentifier + " to be " + (test.valid ? "valid" : "invalid")); That last line is pretty long -- wrap it so it's reasonably-sized. :) r=me with the above (& comment 7) addressed.

(In reply to Daniel Holbert [:dholbert] from comment #7) > > void > > SVGFragmentIdentifier::SaveOldZoomAndPan(nsSVGSVGElement *root) > > { > >- const PRUint16 *oldZoomAndPanPtr = root->GetZoomAndPanProperty(); > >- if (!oldZoomAndPanPtr) { > >+ PRUint16 oldZoomAndPan = root->GetZoomAndPanProperty(); > >+ if (oldZoomAndPan != nsIDOMSVGZoomAndPan::SVG_ZOOMANDPAN_UNKNOWN) { > > root->SetZoomAndPanProperty(root->mEnumAttributes[nsSVGSVGElement::ZOOMANDPAN].GetBaseValue()); > > } > > Previously, this was saying "if root doesn't have a ZoomAndPanProperty > stored, then store its current attribute baseval there." > > Now it seems to be saying "if root _has_ a (non-unknown) ZoomAndPanProperty > stored, then store its current attribute baseval there." > > So, I think this logic might've been unintentionally flipped (and I think > the old logic made more sense)... I might be misunderstanding though. It was unintentionally flipped. I'll be doing another patch to add currentView DOM support to the SVGSVGElement, I can add tests once I've done that but I don't think there's any way to test it automatically right now.

OK, that sounds fine. Thanks!

Thanks for the review Daniel. https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b479f90848