Closed Bug 470911 Opened 11 years ago Closed 11 years ago

New style nsSVGPreserveAspectRatio

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: longsonr, Assigned: longsonr)

Details

Attachments

(1 file, 8 obsolete files)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #354292 - Flags: review?(jwatt)
Couple questions since I'm planning on doing the new style viewbox which should be similar to this.

Is the PreserveAspectRatioAttributeInfo struct really necessary? It's got a single pointer variable. Couldn't the GetPreserveAspectRatioInfo just return the pointer? You've gotta do the null check on the variable in the class anyway.
It appears that nsSVGUtils.h includes nsSVGPreserveAspectRatio.h but the patch does not include the Makefile.in change needed to export that header file.
(In reply to comment #1)
> 
> Is the PreserveAspectRatioAttributeInfo struct really necessary? It's got a
> single pointer variable. Couldn't the GetPreserveAspectRatioInfo just return
> the pointer? You've gotta do the null check on the variable in the class
> anyway.

It has a reset method that makes some other code slightly simpler.
(In reply to comment #2)
> It appears that nsSVGUtils.h includes nsSVGPreserveAspectRatio.h but the patch
> does not include the Makefile.in change needed to export that header file.

It builds for me without that. Does it not build for you?
Attached patch update to tip (obsolete) — Splinter Review
Attachment #354292 - Attachment is obsolete: true
Attachment #354679 - Flags: superreview?(roc)
Attachment #354679 - Flags: review?(roc)
Attachment #354292 - Flags: review?(jwatt)
(In reply to comment #2)
> It appears that nsSVGUtils.h includes nsSVGPreserveAspectRatio.h but the patch
> does not include the Makefile.in change needed to export that header file.

I see what you mean now that I've rebuilt everything. I've restructured the code so that the export is no longer necessary.
Attached patch update to tip (obsolete) — Splinter Review
Attachment #354679 - Attachment is obsolete: true
Attachment #354683 - Flags: superreview?(roc)
Attachment #354683 - Flags: review?(roc)
Attachment #354679 - Flags: superreview?(roc)
Attachment #354679 - Flags: review?(roc)
I haven't done a full review yet but I suspect that it would be better to get rid of PreserveAspectRatioAttributeInfo. Since there can only be one of these attributes, we don't really get the benefit that we do for the other types. Reset or whatever else gets shared via the info object should be easily shareable in other ways.
Attached patch address review comment (obsolete) — Splinter Review
use raw nsSVGPreserveAspectRatio pointer rather than info struct
Attachment #354683 - Attachment is obsolete: true
Attachment #354741 - Flags: superreview?(roc)
Attachment #354741 - Flags: review?(roc)
Attachment #354683 - Flags: superreview?(roc)
Attachment #354683 - Flags: review?(roc)
Comment on attachment 354741 [details] [diff] [review]
address review comment

+    if (!foundMatch) {
+      // Check for nsSVGPreserveAspectRatio attribute

Make that !foundMatch && aAttribute == nsGkAtoms::preserveAspectRatio for lower overhead.

+    if (!foundMatch) {
+      // Check if this is a preserveAspectRatio attribute going away

Same here

+  virtual nsSVGPreserveAspectRatio *GetPreserveAspectRatio();

Worth adding a comment to explain why this is different.

It looks like a bug that nsSVGSVGElement does not respond to changes in preserveaspectratio --- it should be overriding DidChangePreserveAspectRatio. Please file a bug on that.

Everything else looks great.

Another piece of cleanup we should do (in a separate patch) is remove the foundMatch boolean from ParseAttribute by moving all the foundMatch-setting code into helper function that returns the value of foundMatch. This would be nice because all the "foundMatch = PR_TRUE; break" code would become "return PR_TRUE", so the "if (!foundMatch)" tests can go away. Same for UnsetAttr.
Attachment #354741 - Flags: superreview?(roc)
Attachment #354741 - Flags: superreview+
Attachment #354741 - Flags: review?(roc)
Attachment #354741 - Flags: review+
Attached patch for check-in (obsolete) — Splinter Review
Attachment #354741 - Attachment is obsolete: true
hg add nsSVGImageElement.h
hg remove nsSVGAnimatedPreserveAspectRatio.cpp nsSVGAnimatedPreserveAspectRatio.h
Keywords: checkin-needed
Whiteboard: [needs landing] files added/removed see comment 12
if you "hg add" and "hg remove" appropriate files, and generate the patch with "hg diff" or "hg commit; hg export", then the patch will contain the add/remove instructions and the committer can use "hg import" to apply the patch with the adds and removes.

By the way, why haven't you got hg access yet?
Attached patch for check-in (obsolete) — Splinter Review
Attachment #354793 - Attachment is obsolete: true
(In reply to comment #13)
> if you "hg add" and "hg remove" appropriate files, and generate the patch with
> "hg diff" or "hg commit; hg export", then the patch will contain the add/remove
> instructions and the committer can use "hg import" to apply the patch with the
> adds and removes.

Thanks for the tip, I've done that.

> 
> By the way, why haven't you got hg access yet?

I do have hg access I even land stuff from time to time. The tree is red and I'm off out soon though :-)
Whiteboard: [needs landing] files added/removed see comment 12 → [needs landing]
Patch is missing the change to Makefile.in to remove nsSVGAnimatedPreserveAspectRatio.cpp from the source list.
> It looks like a bug that nsSVGSVGElement does not respond to changes in
> preserveaspectratio --- it should be overriding DidChangePreserveAspectRatio.
> Please file a bug on that.
 
I believe this was previously handled in DidModifySVGObservable. There is a call to InvalidateTransformNotifyFrame() that would have been executed. Additionally MarkerElement has "mViewBoxToViewportTransform = nsnull" in its DidModifySVGObservable that needs to be in DidChangePreserveAspectRatio
Well caught. That code is weird. So I guess Robert's patch needs to be fixed to handle that.
This patch adds support to make the SVG and marker elements respond to changes in the preserve aspect ratio attribute.

Also adds cycle collection to the DOM classes for bug 467671.
Attachment #354926 - Flags: superreview?(roc)
Attachment #354926 - Flags: review?(roc)
Do we care that the mapping of the align enumeration to the strings is overly complex given the enumeration values? Each entry in the mapping array is storing its index plus 1. The solution here is useful for a space with holes in it, but this space is packed. This obviously makes the code break if the values change for some reason, but given these are part of the DOM they can't change.
I think we should depend on them being packed and just do an array lookup without the enum value, yeah.

+  if (mBaseVal.mDefer) {
+    aValueAsString.AssignLiteral("defer ");
+    nsAutoString alignString;
+    GetAlignString(alignString, mBaseVal.mAlign);
+    aValueAsString.Append(alignString);
+  } else {
+    GetAlignString(aValueAsString, mBaseVal.mAlign);
+  }

This can be slightly simplified; just make the AssignLiteral("defer ") inside the if, and unconditionally append alignString (the optimization to avoid the temporary alignString when !mBaseVal.mDefer is totally not worth it).

Otherwise looks great.
Attached patch Addresses last round of comments (obsolete) — Splinter Review
I attempted to fix the GetBaseValueString routine, but I'm not sure that I really made it better. Not sure if I succeeded since I had to had a Truncate to make sure the string is clear before the appending of the align string.
Attachment #354926 - Attachment is obsolete: true
Attachment #354926 - Flags: superreview?(roc)
Attachment #354926 - Flags: review?(roc)
Attachment #354976 - Flags: superreview?(roc)
Attachment #354976 - Flags: review?(roc)
Comment on attachment 354976 [details] [diff] [review]
Addresses last round of comments

+  NS_ASSERTION(aMeetOrSlice >= nsIDOMSVGPreserveAspectRatio::SVG_MEETORSLICE_MEET, "Unknown meetOrSlice");
+  NS_ASSERTION(aMeetOrSlice <= nsIDOMSVGPreserveAspectRatio::SVG_MEETORSLICE_SLICE, "Unknown meetOrSlice");
+
+  aMeetOrSliceString.AssignASCII(sMeetOrSliceStrings[aMeetOrSlice - nsIDOMSVGPreserveAspectRatio::SVG_MEETORSLICE_MEET]);

break before 80 chars, here and elsewhere
Attachment #354976 - Flags: superreview?(roc)
Attachment #354976 - Flags: superreview+
Attachment #354976 - Flags: review?(roc)
Attachment #354976 - Flags: review+
Attached patch Shortened linesSplinter Review
Attachment #354976 - Attachment is obsolete: true
Attachment #354804 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed 1fb9d185d8f1
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Also pushed bustage fixes 6fe82507c1f3, 4a76631e1a66 and 4a76631e1a66.

I also noticed that the animVal tearoff calls GetBaseValue instead of GetAnimValue, and pushed the obvious fix for that as f53765e8a713.
(In reply to comment #26)
> Also pushed bustage fixes 6fe82507c1f3, 4a76631e1a66 and 4a76631e1a66.

That last one was cd7bf39ff7b5.
You need to log in before you can comment on or make changes to this bug.