759124 - Implement useCurrentView

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

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

Comment 2

[Robert Longson [:longsonr]]

Attachment: update uuid too

Comment 3

[Daniel Holbert [:dholbert]]

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)?

Comment 4

[Robert Longson [:longsonr]]

Attachment: Fix for gcc

Comment 5

[Robert Longson [:longsonr]]

(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.

Comment 6

[Robert Longson [:longsonr]]

Attachment: remove silly const

Comment 7

[Daniel Holbert [:dholbert]]

> 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 8

[Daniel Holbert [:dholbert]]

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.

Comment 9

[Robert Longson [:longsonr]]

(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.

Comment 10

[Daniel Holbert [:dholbert]]

OK, that sounds fine. Thanks!

Comment 11

[Robert Longson [:longsonr]]

Thanks for the review Daniel.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b479f90848

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f9b479f90848