Last Comment Bug 759124 - Implement useCurrentView
: Implement useCurrentView
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Robert Longson
:
: Jet Villegas (:jet)
Mentors:
Depends on: 512525
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-28 08:56 PDT by Robert Longson
Modified: 2012-06-01 08:42 PDT (History)
2 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (11.59 KB, patch)
2012-05-28 08:58 PDT, Robert Longson
no flags Details | Diff | Splinter Review
update uuid too (12.27 KB, patch)
2012-05-28 09:02 PDT, Robert Longson
no flags Details | Diff | Splinter Review
Fix for gcc (12.78 KB, patch)
2012-05-30 03:20 PDT, Robert Longson
no flags Details | Diff | Splinter Review
remove silly const (12.76 KB, patch)
2012-05-30 03:23 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review

Description Robert Longson 2012-05-28 08:56:11 PDT

    
Comment 1 Robert Longson 2012-05-28 08:58:21 PDT
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.
Comment 2 Robert Longson 2012-05-28 09:02:07 PDT
Created attachment 627721 [details] [diff] [review]
update uuid too
Comment 3 Daniel Holbert [:dholbert] 2012-05-29 13:44:36 PDT
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 2012-05-30 03:20:22 PDT
Created attachment 628295 [details] [diff] [review]
Fix for gcc
Comment 5 Robert Longson 2012-05-30 03:22:46 PDT
(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 2012-05-30 03:23:14 PDT
Created attachment 628296 [details] [diff] [review]
remove silly const
Comment 7 Daniel Holbert [:dholbert] 2012-05-31 00:08:43 PDT
> 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] 2012-05-31 00:48:20 PDT
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 2012-05-31 01:04:13 PDT
(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] 2012-05-31 01:50:01 PDT
OK, that sounds fine. Thanks!
Comment 11 Robert Longson 2012-05-31 03:39:59 PDT
Thanks for the review Daniel.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b479f90848
Comment 12 Ed Morley [:emorley] 2012-06-01 08:42:56 PDT
https://hg.mozilla.org/mozilla-central/rev/f9b479f90848

Note You need to log in before you can comment on or make changes to this bug.