The default bug view has changed. See this FAQ.

Implement useCurrentView

RESOLVED FIXED in mozilla15

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Robert Longson, Assigned: Robert Longson)

Tracking

({dev-doc-needed})

Trunk
mozilla15
dev-doc-needed
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 627720 [details] [diff] [review]
patch

We really need to get this in for 15 as zoomAndPan fragments can crash the browser without this.
Assignee: nobody → longsonr
Attachment #627720 - Flags: review?(jwatt)
(Assignee)

Comment 2

5 years ago
Created attachment 627721 [details] [diff] [review]
update uuid too
Attachment #627720 - Attachment is obsolete: true
Attachment #627720 - Flags: review?(jwatt)
Attachment #627721 - Flags: review?(jwatt)
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)?
Depends on: 512525
(Assignee)

Comment 4

5 years ago
Created attachment 628295 [details] [diff] [review]
Fix for gcc
Attachment #627721 - Attachment is obsolete: true
Attachment #627721 - Flags: review?(jwatt)
Attachment #628295 - Flags: review?(jwatt)
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
Created attachment 628296 [details] [diff] [review]
remove silly const
Attachment #628295 - Attachment is obsolete: true
Attachment #628295 - Flags: review?(jwatt)
Attachment #628296 - Flags: review?(jwatt)
(Assignee)

Updated

5 years ago
Attachment #628296 - Flags: review?(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 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.
Attachment #628296 - Flags: review?(dholbert) → review+
(Assignee)

Comment 9

5 years ago
(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!
(Assignee)

Updated

5 years ago
Attachment #628296 - Flags: review?(jwatt)
(Assignee)

Comment 11

5 years ago
Thanks for the review Daniel.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b479f90848
Flags: in-testsuite+
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla15
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/f9b479f90848
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.