Closed
Bug 238530
Opened 20 years ago
Closed 20 years ago
Implement SVG markers
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
People
(Reporter: tor, Assigned: tor)
Details
Attachments
(2 files, 12 obsolete files)
125.57 KB,
patch
|
scootermorris
:
review+
|
Details | Diff | Splinter Review |
36.05 KB,
patch
|
scootermorris
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
Checkpointing - going to split some of the backfill into a different bug. 3000+ line patch and still not close to writing the actual marker logic.
Working markers, at least with the cairo backend. Needs some more work for the GDI+ and libart backends due to the rentained rendering nature of those backends.
Attachment #150812 -
Attachment is obsolete: true
Accidentally tagged all the new files as part of the cairo renderer - fixed.
Attachment #156367 -
Attachment is obsolete: true
Attachment #156382 -
Attachment is obsolete: true
Attachment #156821 -
Attachment is obsolete: true
Attachment #156925 -
Attachment is obsolete: true
Attachment #160649 -
Flags: review?(scootermorris)
Attachment #160650 -
Flags: review?(scootermorris)
Attachment #160649 -
Attachment is obsolete: true
Attachment #160650 -
Attachment is obsolete: true
Attachment #160649 -
Flags: review?(scootermorris)
Attachment #160650 -
Flags: review?(scootermorris)
Attachment #162229 -
Flags: review?(scootermorris)
Attachment #162230 -
Flags: review?(scootermorris)
Comment 10•20 years ago
|
||
Comment on attachment 162229 [details] [diff] [review] changes to shared mozilla files (updated to tip) Looks good to me.
Attachment #162229 -
Flags: review?(scootermorris) → review+
Comment 11•20 years ago
|
||
Comment on attachment 162230 [details] [diff] [review] changes/new svg-only files (updated top tip) > // properties and attributes > SVG_ATOM(alignment_baseline, "alignment-baseline") A comment as to why this is 'Auto' vs 'auto' might be helpful. Any particular reason you used 'Auto' as apposed to '_auto' as they did for '_class'? >+SVG_ATOM(Auto, "auto") > SVG_ATOM(baseline_shift, "baseline-shift") > SVG_ATOM(_class, "class") > SVG_ATOM(clip_path, "clip-path") > SVG_ATOM(clip_rule, "clip-rule") > SVG_ATOM(color, "color") > SVG_ATOM(cursor, "cursor") > SVG_ATOM(cx, "cx") > SVG_ATOM(cy, "cy") >+ * the Initial Developer. All Rights Reserved. >+ * Shouldn't you be the contributor? (In case we need to track you down later :-)) >+ * Contributor(s): >+ * >+ * Alternatively, the contents of this file may be used under the terms of >+ * either of the GNU General Public License Version 2 or later (the "GPL"), >+ * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+ * >+ * Contributor(s): >+ * See above, and further entries... >+ * Alternatively, the contents of this file may be used under the terms of >+ * either of the GNU General Public License Version 2 or later (the "GPL"), >+ * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+ * in which case the provisions of the GPL or the LGPL are applicable instead >+ * of those above. If you wish to allow use of your version of this file only >+ * under the terms of either the GPL or the LGPL, and not to allow others to >+//////////////////////////////////////////////////////////////////////// >+// nsSVGAngle class >+ >+class nsSVGAngle : public nsISVGAngle, >+ public nsSVGValue, >+ public nsISVGValueObserver, >+ public nsSupportsWeakReference As I understand the style guidelines, all of your 'public's should line up... >+protected: >+ friend nsresult NS_NewSVGAngle(nsISVGAngle** result, >+ float value, >+ PRUint16 unit); >+ ... and the 'f' in float should be under the 'n' in nsISVGAngle... >+ friend nsresult NS_NewSVGAngle(nsISVGAngle** result, >+ const nsAString &value); >+ ... extra space before const... >+ nsSVGAngle(float value, PRUint16 unit); >+ nsSVGAngle(); >+ virtual ~nsSVGAngle(); >+ >Index: content/svg/content/src/nsSVGAngle.h >=================================================================== >+ * >+ * Contributor(s): >+ * As above... >+ * Alternatively, the contents of this file may be used under the terms of > >+nsresult >+NS_NewSVGAngle(nsISVGAngle** result, >+ float value=0.0f, >+ PRUint16 unit=nsIDOMSVGAngle::SVG_ANGLETYPE_UNSPECIFIED); >+ Fix the tabs... >+nsresult >+NS_NewSVGAngle(nsISVGAngle** result, >+ const nsAString &value); And here... >+ >+#endif //__NS_SVGLENGTH_H__ >Index: content/svg/content/src/nsSVGAnimatedAngle.cpp >=================================================================== >RCS file: content/svg/content/src/nsSVGAnimatedAngle.cpp >diff -N content/svg/content/src/nsSVGAnimatedAngle.cpp >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ content/svg/content/src/nsSVGAnimatedAngle.cpp 15 Oct 2004 20:48:05 -0000 >+//////////////////////////////////////////////////////////////////////// >+// nsSVGAnimatedAngle >+ >+class nsSVGAnimatedAngle : public nsIDOMSVGAnimatedAngle, >+ public nsSVGValue, >+ public nsISVGValueObserver, >+ public nsSupportsWeakReference >+{ extra space under before public (x3)... >+protected: >+ friend nsresult NS_NewSVGAnimatedAngle(nsIDOMSVGAnimatedAngle** result, >+ nsIDOMSVGAngle* baseVal); extra space... >+ nsSVGAnimatedAngle(); >+ ~nsSVGAnimatedAngle(); >+/* readonly attribute nsIDOMSVGAngle baseVal; */ >+NS_IMETHODIMP >+nsSVGAnimatedAngle::GetBaseVal(nsIDOMSVGAngle * *aBaseVal) >+{ >+ *aBaseVal = mBaseVal; Should you protect against a aBaseVal == NULL?? >+ NS_ADDREF(*aBaseVal); >+ return NS_OK; >+} >+ >+/* readonly attribute nsIDOMSVGAngle animVal; */ >+NS_IMETHODIMP >+nsSVGAnimatedAngle::GetAnimVal(nsIDOMSVGAngle * *aAnimVal) >+{ >+ *aAnimVal = mBaseVal; Should you protect against a aAnimVal == NULL?? >+ NS_ADDREF(*aAnimVal); >+ return NS_OK; >+} >+ >+nsresult >+NS_NewSVGAnimatedAngle(nsIDOMSVGAnimatedAngle** aResult, >+ nsIDOMSVGAngle* baseVal) >+{ extra space... >+ *aResult = nsnull; >+ >Index: content/svg/content/src/nsSVGAnimatedAngle.h >=================================================================== >RCS file: content/svg/content/src/nsSVGAnimatedAngle.h >diff -N content/svg/content/src/nsSVGAnimatedAngle.h >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ content/svg/content/src/nsSVGAnimatedAngle.h 15 Oct 2004 20:48:05 -0000 >@@ -0,0 +1,46 @@ >+ >+nsresult NS_NewSVGAnimatedAngle(nsIDOMSVGAnimatedAngle** result, >+ nsIDOMSVGAngle* baseVal); >+ Spacing... >Index: content/svg/content/src/nsSVGElement.cpp$ >===================================================================$ >+// PresentationAttributes-Makers$ >+/* static */ const nsGenericElement::MappedAttributeEntry$ >+nsSVGElement::sMarkersMap[] = {$ >+ { &nsSVGAtoms::marker_end },$ >+ { &nsSVGAtoms::marker_mid },$ >+ { &nsSVGAtoms::marker_start },$ >+ { nsnull }$ >+};$ Why is marker not on this list? Does it get handled separately? >Index: content/svg/content/src/nsSVGElement.h >=================================================================== >RCS file: /cvsroot/mozilla/content/svg/content/src/nsSVGElement.h,v >Index: content/svg/content/src/nsSVGElementFactory.cpp >=================================================================== >RCS file: /cvsroot/mozilla/content/svg/content/src/nsSVGElementFactory.cpp,v >Index: content/svg/content/src/nsSVGLineElement.cpp >=================================================================== >Index: content/svg/content/src/nsSVGMarkerElement.cpp >=================================================================== >RCS file: content/svg/content/src/nsSVGMarkerElement.cpp >+typedef nsSVGGraphicElement nsSVGMarkerElementBase; >+ >+class nsSVGMarkerElement : public nsSVGMarkerElementBase, >+ public nsIDOMSVGMarkerElement, >+ public nsIDOMSVGFitToViewBox, >+ public nsSVGCoordCtxProvider Tabs.... >+{ >+protected: >+ friend nsresult NS_NewSVGMarkerElement(nsIContent **aResult, >+ nsINodeInfo *aNodeInfo); Fix tabs... >+ nsSVGMarkerElement(nsINodeInfo* aNodeInfo); >+ virtual ~nsSVGMarkerElement(); >+ nsresult Init(); >+ >+ >+ nsCOMPtr<nsIDOMSVGAnimatedRect> mViewBox; >+ nsCOMPtr<nsIDOMSVGAnimatedPreserveAspectRatio> mPreserveAspectRatio; >+ nsCOMPtr<nsIDOMSVGMatrix> mViewBoxToViewportTransform; Nit -- could you get this to line up with mViewBox? >+}; >+ >+NS_IMPL_NS_NEW_SVG_ELEMENT(Marker) >+ >+ // DOM property: orient$ >+ {$ >+ nsCOMPtr<nsISVGEnum> orient;$ >+ rv = NS_NewSVGEnum(getter_AddRefs(orient), SVG_MARKER_ORIENT_ANGLE, gOrientType);$ >+ NS_ENSURE_SUCCESS(rv,rv);$ >+ rv = NS_NewSVGAnimatedEnumeration(getter_AddRefs(mOrientType), orient);$ >+ NS_ENSURE_SUCCESS(rv,rv);$ >+ rv = AddMappedSVGValue(nsSVGAtoms::orient, mOrientType);$ >+ NS_ENSURE_SUCCESS(rv,rv);$ >+ }$ I'm confused... why do you have two orients? According to my reading of the spec, orient can either be 'auto' or an angle. Is there something else going on here that I'm missing?? >+ >+ // DOM property: orient >+ { >+ nsCOMPtr<nsISVGAngle> angle; >+ rv = NS_NewSVGAngle(getter_AddRefs(angle), 0.0f); >+ NS_ENSURE_SUCCESS(rv,rv); >+ rv = NS_NewSVGAnimatedAngle(getter_AddRefs(mOrientAngle), angle); >+ NS_ENSURE_SUCCESS(rv,rv); >+ // Can't map two values to the same attribute >+// rv = AddMappedSVGValue(nsSVGAtoms::orient, mOrientAngle); >+// NS_ENSURE_SUCCESS(rv,rv); >+ } >+ >+/* readonly attribute nsIDOMSVGAnimatedRect viewBox; */ >+NS_IMETHODIMP nsSVGMarkerElement::GetViewBox(nsIDOMSVGAnimatedRect * *aViewBox) >+{ >+ *aViewBox = mViewBox; Need to guard against a null aViewBox? >+ NS_ADDREF(*aViewBox); >+ return NS_OK; >+} >+ >+/* readonly attribute nsIDOMSVGAnimatedPreserveAspectRatio preserveAspectRatio; */ >+NS_IMETHODIMP >+nsSVGMarkerElement::GetPreserveAspectRatio(nsIDOMSVGAnimatedPreserveAspectRatio * *aPreserveAspectRatio) >+{ Need to guard against a null aPreserveAspectRatio? >+ *aPreserveAspectRatio = mPreserveAspectRatio; >+ NS_ADDREF(*aPreserveAspectRatio); >+ return NS_OK; >+} >+ >+//---------------------------------------------------------------------- >+// nsIDOMSVGMarkerElement methods >+ For some reason, indent spacing changed to 4 spaces from here on down.... >+/* readonly attribute nsIDOMSVGAnimatedLength refX; */ >+NS_IMETHODIMP nsSVGMarkerElement::GetRefX(nsIDOMSVGAnimatedLength * *aRefX) >+{ >+ *aRefX = mRefX; >+ NS_IF_ADDREF(*aRefX); >+ return NS_OK; >+} >+ >+/* readonly attribute nsIDOMSVGAnimatedLength refY; */ >+NS_IMETHODIMP nsSVGMarkerElement::GetRefY(nsIDOMSVGAnimatedLength * *aRefY) >+{ >+ *aRefY = mRefY; >+ NS_IF_ADDREF(*aRefY); >+ return NS_OK; >+} >+ >+/* readonly attribute nsIDOMSVGAnimatedEnumeration markerUnits; */ >+NS_IMETHODIMP nsSVGMarkerElement::GetMarkerUnits(nsIDOMSVGAnimatedEnumeration * *aMarkerUnits) >+{ >+ *aMarkerUnits = mMarkerUnits; >+ NS_IF_ADDREF(*aMarkerUnits); >+ return NS_OK; >+} >+ >+/* readonly attribute nsIDOMSVGAnimatedLength markerWidth; */ >+NS_IMETHODIMP nsSVGMarkerElement::GetMarkerWidth(nsIDOMSVGAnimatedLength * *aMarkerWidth) >+{ >+ *aMarkerWidth = mMarkerWidth; >+ NS_IF_ADDREF(*aMarkerWidth); >+ return NS_OK; >+} >+ >+/* readonly attribute nsIDOMSVGAnimatedLength markerHeight; */ >+NS_IMETHODIMP nsSVGMarkerElement::GetMarkerHeight(nsIDOMSVGAnimatedLength * *aMarkerHeight) >+{ >+ *aMarkerHeight = mMarkerHeight; >+ NS_IF_ADDREF(*aMarkerHeight); >+ return NS_OK; >+} >+ >+/* readonly attribute nsIDOMSVGAnimatedEnumeration orientType; */ >+NS_IMETHODIMP nsSVGMarkerElement::GetOrientType(nsIDOMSVGAnimatedEnumeration * *aOrientType) >+{ >+ *aOrientType = mOrientType; >+ NS_IF_ADDREF(*aOrientType); >+ return NS_OK; >+} >+ >+/* readonly attribute nsIDOMSVGAnimatedLength orientAngle; */ >+NS_IMETHODIMP nsSVGMarkerElement::GetOrientAngle(nsIDOMSVGAnimatedAngle * *aOrientAngle) >+{ >+ *aOrientAngle = mOrientAngle; >+ NS_IF_ADDREF(*aOrientAngle); >+ return NS_OK; >+} >+ >+/* void setOrientToAuto (); */ >+NS_IMETHODIMP nsSVGMarkerElement::SetOrientToAuto() >+{ >+ mOrientType->SetBaseVal(SVG_MARKER_ORIENT_AUTO); >+ return NS_OK; >+} >+ >+/* void setOrientToAngle (in nsIDOMSVGAngle angle); */ >+NS_IMETHODIMP nsSVGMarkerElement::SetOrientToAngle(nsIDOMSVGAngle *angle) >+{ >+ mOrientType->SetBaseVal(SVG_MARKER_ORIENT_ANGLE); >+ >+ nsIDOMSVGAngle *a; >+ mOrientAngle->GetBaseVal(&a); >+ float f; >+ angle->GetValue(&f); >+ a->SetValue(f); >+ return NS_OK; >+} >+ >+NS_IMETHODIMP >+nsSVGMarkerElement::GetMarkerTransform(float aStrokeWidth, >+ float aX, float aY, float aAngle, >+ nsIDOMSVGMatrix **_retval) >+{ Fix tabs... >+ float scale = 1.0; >+ PRUint16 val; >+ mMarkerUnits->GetAnimVal(&val); >+ if (val == SVG_MARKERUNITS_STROKEWIDTH) >+ scale = aStrokeWidth; Tab... >+ >+ mOrientType->GetAnimVal(&val); >+ if (val == SVG_MARKER_ORIENT_ANGLE) { >+ nsCOMPtr<nsIDOMSVGAngle> a; >+ mOrientAngle->GetAnimVal(getter_AddRefs(a)); >+ a->GetValue(&aAngle); Tabs... >+ } >+ >+// float aAngle = 45*M_PI/180.0; Should this still be here? >+ >+ nsCOMPtr<nsIDOMSVGMatrix> matrix; >+ nsSVGMatrix::Create(getter_AddRefs(matrix), >+ cos(aAngle) * scale, sin(aAngle) * scale, >+ -sin(aAngle) * scale, cos(aAngle) * scale, >+ aX, aY); Tabs.... >+ >+ *_retval = matrix; >+ NS_IF_ADDREF(*_retval); >+ return NS_OK; >+} >+ >+ >+/* nsIDOMSVGMatrix getViewboxToViewportTransform (); */ >+NS_IMETHODIMP >+nsSVGMarkerElement::GetViewboxToViewportTransform(nsIDOMSVGMatrix **_retval) >+{ Spacing still messed up.... >+ if (!mViewBoxToViewportTransform) { >+ float viewportWidth; >+ { >+ nsCOMPtr<nsIDOMSVGLength> l; >+ mMarkerWidth->GetAnimVal(getter_AddRefs(l)); >+ l->GetValue(&viewportWidth); >+ } >+ float viewportHeight; >+ { >+ nsCOMPtr<nsIDOMSVGLength> l; >+ mMarkerHeight->GetAnimVal(getter_AddRefs(l)); >+ l->GetValue(&viewportHeight); >+ } >+ >+ float viewboxX, viewboxY, viewboxWidth, viewboxHeight; >+ { >+ nsCOMPtr<nsIDOMSVGRect> vb; >+ mViewBox->GetAnimVal(getter_AddRefs(vb)); >+ NS_ASSERTION(vb, "could not get viewbox"); >+ vb->GetX(&viewboxX); >+ vb->GetY(&viewboxY); >+ vb->GetWidth(&viewboxWidth); >+ vb->GetHeight(&viewboxHeight); >+ } >+ if (viewboxWidth==0.0f || viewboxHeight==0.0f) { >+ NS_ERROR("XXX. We shouldn't get here. Viewbox width/height is set to 0. Need to disable display of element as per specs."); >+ viewboxWidth = 1.0f; >+ viewboxHeight = 1.0f; >+ } >+ >+ PRUint16 align, meetOrSlice; >+ { >+ nsCOMPtr<nsIDOMSVGPreserveAspectRatio> par; >+ mPreserveAspectRatio->GetAnimVal(getter_AddRefs(par)); >+ NS_ASSERTION(par, "could not get preserveAspectRatio"); >+ par->GetAlign(&align); >+ par->GetMeetOrSlice(&meetOrSlice); >+ } >+ >+ // default to the defaults >+ if (align == nsIDOMSVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_UNKNOWN) >+ align = nsIDOMSVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_XMIDYMID; >+ if (meetOrSlice == nsIDOMSVGPreserveAspectRatio::SVG_MEETORSLICE_UNKNOWN) >+ align = nsIDOMSVGPreserveAspectRatio::SVG_MEETORSLICE_MEET; >+ >+ float a, d, e, f; >+ a = viewportWidth/viewboxWidth; >+ d = viewportHeight/viewboxHeight; >+ e = 0.0f; >+ f = 0.0f; >+ >+ if (align != nsIDOMSVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_NONE && >+ a != d) { >+ if (meetOrSlice == nsIDOMSVGPreserveAspectRatio::SVG_MEETORSLICE_MEET && >+ a < d || >+ meetOrSlice == nsIDOMSVGPreserveAspectRatio::SVG_MEETORSLICE_SLICE && >+ d < a) { >+ d = a; >+ } >+ else if ( >+ meetOrSlice == nsIDOMSVGPreserveAspectRatio::SVG_MEETORSLICE_MEET && >+ d < a || >+ meetOrSlice == nsIDOMSVGPreserveAspectRatio::SVG_MEETORSLICE_SLICE && >+ a < d) { >+ a = d; >+ } >+ else NS_NOTREACHED("Unknown value for meetOrSlice"); >+ } >+ >+ if (viewboxX) e += -a * viewboxX; >+ if (viewboxY) f += -d * viewboxY; >+ >+ float refX; >+ { >+ nsCOMPtr<nsIDOMSVGLength> l; >+ mRefX->GetAnimVal(getter_AddRefs(l)); >+ l->GetValue(&refX); >+ } >+ >+ float refY; >+ { >+ nsCOMPtr<nsIDOMSVGLength> l; >+ mRefY->GetAnimVal(getter_AddRefs(l)); >+ l->GetValue(&refY); >+ } >+ >+ e -= refX * a; >+ f -= refY * d; >+ >+#ifdef DEBUG >+ printf("Marker Viewport=(0?,0?,%f,%f)\n", viewportWidth, viewportHeight); >+ printf("Marker Viewbox=(%f,%f,%f,%f)\n", viewboxX, viewboxY, viewboxWidth, viewboxHeight); >+ printf("Marker Viewbox->Viewport xform [a c e] = [%f, 0, %f]\n", a, e); >+ printf(" [b d f] = [ 0, %f, %f]\n", d, f); >+#endif >+ >+ nsSVGMatrix::Create(getter_AddRefs(mViewBoxToViewportTransform), >+ a, 0.0f, 0.0f, d, e, f); >+ } >+ >+ *_retval = mViewBoxToViewportTransform; >+ NS_IF_ADDREF(*_retval); >+ return NS_OK; >+} >+ ...more in next batch.....
Comment 12•20 years ago
|
||
Comment on attachment 162230 [details] [diff] [review] changes/new svg-only files (updated top tip) >+ >+ // Mozilla extension, not part of W3 SVG DOM: >+ [noscript] nsIDOMSVGMatrix getMarkerTransform(in float strokeWidth, in float x, in float y, in float angle); >+ [noscript] nsIDOMSVGMatrix getViewboxToViewportTransform( ); >+}; How picky are they about this? Are we going to run into trouble with sr on this?? >Index: layout/svg/base/src/nsSVGDefsFrame.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGDefsFrame.cpp,v >retrieving revision 1.5 >diff -u -8 -p -r1.5 nsSVGDefsFrame.cpp >--- layout/svg/base/src/nsSVGDefsFrame.cpp 9 Aug 2004 14:44:53 -0000 1.5 >+++ layout/svg/base/src/nsSVGDefsFrame.cpp 15 Oct 2004 20:48:05 -0000 >@@ -50,53 +50,49 @@ > //---------------------------------------------------------------------- > // Implementation > > nsresult > NS_NewSVGDefsFrame(nsIPresShell* aPresShell, nsIContent* aContent, nsIFrame** aNewFrame) > { > *aNewFrame = nsnull; > Can you explain why this needs to change for markers?? >- nsCOMPtr<nsIDOMSVGTransformable> transformable = do_QueryInterface(aContent); >- if (!transformable) { >-#ifdef DEBUG >- printf("warning: trying to construct an SVGDefsFrame for a content element that doesn't support the right interfaces\n"); >-#endif >- return NS_ERROR_FAILURE; >- } >- >Index: layout/svg/base/src/nsSVGMarkerFrame.cpp >=================================================================== >+NS_IMETHODIMP >+nsSVGMarkerFrame::QueryInterface(REFNSIID aIID, void** aInstancePtr) >+{ >+ if (nsnull == aInstancePtr) { >+ return NS_ERROR_NULL_POINTER; >+ } >+ if (aIID.Equals(nsSVGMarkerFrame::GetCID())) { >+ *aInstancePtr = (void*)(nsSVGMarkerFrame*)this; >+ NS_ADDREF_THIS(); >+ return NS_OK; >+ } >+ return (nsSVGDefsFrame::QueryInterface(aIID, aInstancePtr)); >+} Why do you need to implement this directly rather than inherit it?? >+nsresult >+NS_GetSVGMarkerFrame(nsSVGMarkerFrame **aResult, nsIURI *aURI, nsIContent *aContent) >+{ >+ nsresult rv; >+ *aResult = nsnull; >+ ... this seems to be the reason for the local CallQI. Why did you need to do this? Can you describe in a comment and elide the commented code? >+ >+// nsCOMPtr<nsSVGMarkerFrame> marker = do_QueryInterface(frame); >+ nsSVGMarkerFrame *marker; >+ CallQueryInterface(frame, &marker); >+ *aResult = marker; >+ return rv; >+} >+ >+ >+//---------------------------------------------------------------------- >Index: layout/svg/base/src/nsSVGMarkerFrame.h >=================================================================== >+ void PaintMark(nsISVGRendererCanvas *aCanvas, nsSVGPathGeometryFrame *aParent, >+ nsSVGMark *aMark, float aStrokeWidth); Bad tab! Go away tab! >+ NS_IMETHOD_(already_AddRefed<nsISVGRendererRegion>) >+ RegionMark(nsSVGPathGeometryFrame *aParent, >+ nsSVGMark *aMark, float aStrokeWidth); >+ >Index: layout/svg/base/src/nsSVGPathFrame.cpp >=================================================================== >+ >+void >+nsSVGPathFrame::GetMarkPoints(nsVoidArray *aMarks) { >+ PRUint32 count; >+ mSegments->GetNumberOfItems(&count); >+ nsCOMPtr<nsIDOMSVGPathSeg> segment; Why is this comment in front of this code? Is it not needed? >+// prevAngle = pathAngle; >+ } >+ break; >+ >Index: layout/svg/base/src/nsSVGPathGeometryFrame.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGPathGeometryFrame.cpp,v >Index: layout/svg/base/src/nsSVGPolygonFrame.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGPolygonFrame.cpp,v >retrieving revision 1.4 >diff -u -8 -p -r1.4 nsSVGPolygonFrame.cpp >--- layout/svg/base/src/nsSVGPolygonFrame.cpp 18 Apr 2004 14:30:34 -0000 1.4 >+++ layout/svg/base/src/nsSVGPolygonFrame.cpp 15 Oct 2004 20:48:05 -0000 >@@ -37,18 +37,24 @@ > * ***** END LICENSE BLOCK ***** */ > > #include "nsSVGPathGeometryFrame.h" > #include "nsIDOMSVGAnimatedPoints.h" > #include "nsIDOMSVGPointList.h" > #include "nsIDOMSVGPoint.h" > //#include "nsASVGPathBuilder.h" > #include "nsISVGRendererPathBuilder.h" >+#include "nsISVGMarkable.h" > >-class nsSVGPolygonFrame : public nsSVGPathGeometryFrame Should M_PI be included in nsISVGMarkable.h? >+#ifndef M_PI >+#define M_PI 3.14159265358979323846 >+#endif >+ >+class nsSVGPolygonFrame : public nsSVGPathGeometryFrame, >+ public nsISVGMarkable bisect is the same in each class that calls it. Should it be in a util module or something? >+static float >+bisect(float a1, float a2) { >+ if (a2 - a1 < M_PI) >+ return (a1+a2)/2; >+ else >+ return M_PI + (a1+a2)/2; >+} >+ Should this also be in a single module? >+void >+nsSVGPolygonFrame::GetMarkPoints(nsVoidArray *aMarks) { >+ >+ if (!mPoints) >+ return; >+ >+ PRUint32 count; >+ mPoints->GetNumberOfItems(&count); >+ if (count == 0) >+ return; >+ >+ float px = 0.0, py = 0.0, prevAngle, startAngle; >+ >+ nsCOMPtr<nsIDOMSVGPoint> point; >+ for (PRUint32 i = 0; i < count; ++i) { >+ mPoints->GetItem(i, getter_AddRefs(point)); >+ >+ float x, y; >+ point->GetX(&x); >+ point->GetY(&y); >+ >+ float angle = atan2(y-py, x-px); >+ if (i == 1) >+ startAngle = angle; >+ else if (i > 1) >+ ((nsSVGMark *)aMarks->ElementAt(aMarks->Count()-1))->angle = bisect(prevAngle, angle); >+ >+ nsSVGMark *mark; >+ mark = new nsSVGMark; >+ mark->x = x; >+ mark->y = y; >+ aMarks->AppendElement(mark); >+ >+ prevAngle = angle; >+ px = x; >+ py = y; >+ } >+ >+ float nx, ny, angle; >+ mPoints->GetItem(0, getter_AddRefs(point)); >+ point->GetX(&nx); >+ point->GetY(&ny); >+ angle = atan2(ny - py, nx - px); >+ >+ ((nsSVGMark *)aMarks->ElementAt(aMarks->Count()-1))->angle = bisect(prevAngle, angle); >+ ((nsSVGMark *)aMarks->ElementAt(0))->angle = bisect(angle, startAngle); >+} > Overall, looks pretty good. Lots of little nits and some questions that are probably my lack of understanding....
please don't implement QueryInterface manually, there's macros that can implement most variants of it, including onces that forward to a baseclass at the end.
Assignee | ||
Comment 14•20 years ago
|
||
;; This buffer is for notes you don't want to save, and for Lisp evaluation. ;; If you want to create a file, visit that file with C-x C-f, ;; then enter the text in that file's own buffer. (In reply to comment #11) > >+ * the Initial Developer. All Rights Reserved. > > Shouldn't you be the contributor? Expected practice is not to fill this in - reduntant with cvs log information. > >+//////////////////////////////////////////////////////////////////////// > >+// nsSVGAngle class > >+ > >+class nsSVGAngle : public nsISVGAngle, > >+ public nsSVGValue, > >+ public nsISVGValueObserver, > >+ public nsSupportsWeakReference > > As I understand the style guidelines, all of your 'public's should line up... In the future, please just say that whatever files need reindenting rather than listing all the nits. It'll save us both time. > >+nsSVGAnimatedAngle::GetBaseVal(nsIDOMSVGAngle * *aBaseVal) > >+nsSVGAnimatedAngle::GetAnimVal(nsIDOMSVGAngle * *aAnimVal) > > Should you protect against a a{Base,Anim}Val == NULL?? We shouldn't be getting into those accessors with null pointers. > >Index: content/svg/content/src/nsSVGElement.cpp$ > >===================================================================$ > >+// PresentationAttributes-Makers$ > >+/* static */ const nsGenericElement::MappedAttributeEntry$ > >+nsSVGElement::sMarkersMap[] = {$ > >+ { &nsSVGAtoms::marker_end },$ > >+ { &nsSVGAtoms::marker_mid },$ > >+ { &nsSVGAtoms::marker_start },$ > >+ { nsnull }$ > >+};$ > > Why is marker not on this list? Does it get handled separately? "marker" per the SVG specification is not an attribute, just a CSS property. > >+ // DOM property: orient$ > >+ // DOM property: orient > > I'm confused... why do you have two orients? According to my reading of the > spec, > orient can either be 'auto' or an angle. Is there something else going on here > that > I'm missing?? We can't map a mixed type directly, so I set up storage for both the enum and angle, and map the enum. The angle is updated in DidModifySVGObservable. I've added some comments to try to make this clear. > >+NS_IMETHODIMP nsSVGMarkerElement::GetViewBox(nsIDOMSVGAnimatedRect * *aViewBox) > >+nsSVGMarkerElement::GetPreserveAspectRatio(nsIDOMSVGAnimatedPreserveAspectRatio * *aPreserveAspectRatio) > > Need to guard against a null [foo]? Again, shouldn't happen.
Assignee | ||
Comment 15•20 years ago
|
||
(In reply to comment #12) > >+ // Mozilla extension, not part of W3 SVG DOM: > >+ [noscript] nsIDOMSVGMatrix getMarkerTransform(in float strokeWidth, in float x, in float y, in float angle); > >+ [noscript] nsIDOMSVGMatrix getViewboxToViewportTransform( ); > > How picky are they about this? Are we going to run into trouble with sr on > this?? They're not terribly fond of them, but [noscript] ones aren't as bad. If they have issues with it, we can change. > > nsresult > > NS_NewSVGDefsFrame(nsIPresShell* aPresShell, nsIContent* aContent, nsIFrame** aNewFrame) > > >- nsCOMPtr<nsIDOMSVGTransformable> transformable = do_QueryInterface(aContent); > >- if (!transformable) { > >-#ifdef DEBUG > >- printf("warning: trying to construct an SVGDefsFrame for a content element that doesn't support the right interfaces\n"); > >-#endif > >- return NS_ERROR_FAILURE; > >- } > >- > > Can you explain why this needs to change for markers?? I wanted to reuse the nsSVGDefsFrame code for the marker frame, but SVGMarker isn't SVGTransformable. > >Index: layout/svg/base/src/nsSVGMarkerFrame.cpp > >=================================================================== > > >+NS_IMETHODIMP > >+nsSVGMarkerFrame::QueryInterface(REFNSIID aIID, void** aInstancePtr) > > Why do you need to implement this directly rather than inherit it?? Because I'm implementing a queryable class without an nsIFoo and couldn't get any of the macro versions to work the way I wanted - "nsISupports is an ambiguous base" or query not working. If someone wants to point me at the right combination that will work I'll be happy to make the change. > Should M_PI be included in nsISVGMarkable.h? > > >+#ifndef M_PI > >+#define M_PI 3.14159265358979323846 > >+#endif > >+ > >+class nsSVGPolygonFrame : public nsSVGPathGeometryFrame, > >+ public nsISVGMarkable > > bisect is the same in each class that calls it. Should it be > in a util module or something? bisect moved to a static nsSVGMarkerFrame method.
Assignee | ||
Comment 16•20 years ago
|
||
Comment 17•20 years ago
|
||
Comment on attachment 162504 [details] [diff] [review] changes/new svg-only files (incorporating some comments) Minor comments only. You left the defines for M_PI, which aren't needed anymore, and you have a couple of left-over comments. It would probably also be good to provide a comment as to why you needed to implement CallQueryInterface yourself (unless someone has an alternative). >Index: content/svg/content/src/nsSVGMarkerElement.cpp >=================================================================== >RCS file: content/svg/content/src/nsSVGMarkerElement.cpp >diff -N content/svg/content/src/nsSVGMarkerElement.cpp >+ >+// float aAngle = 45*M_PI/180.0; Reason for this comment? >Index: layout/svg/base/src/nsSVGMarkerFrame.cpp >=================================================================== >RCS file: layout/svg/base/src/nsSVGMarkerFrame.cpp >diff -N layout/svg/base/src/nsSVGMarkerFrame.cpp >+ >+// nsCOMPtr<nsSVGMarkerFrame> marker = do_QueryInterface(frame); You should probably say why this comment is here... >+ nsSVGMarkerFrame *marker; >+ CallQueryInterface(frame, &marker); >+ *aResult = marker; >+ return rv; >+} >Index: layout/svg/base/src/nsSVGPathFrame.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGPathFrame.cpp,v >retrieving revision 1.20 >diff -u -8 -p -r1.20 nsSVGPathFrame.cpp >+#ifndef M_PI >+#define M_PI 3.14159265358979323846 >+#endif M_PI is not needed anymore, is it? >Index: layout/svg/base/src/nsSVGPolygonFrame.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGPolygonFrame.cpp,v >retrieving revision 1.4 >diff -u -8 -p -r1.4 nsSVGPolygonFrame.cpp >--- layout/svg/base/src/nsSVGPolygonFrame.cpp 18 Apr 2004 14:30:34 -0000 1.4 >+++ layout/svg/base/src/nsSVGPolygonFrame.cpp 18 Oct 2004 19:52:10 -0000 >@@ -37,18 +37,24 @@ > * ***** END LICENSE BLOCK ***** */ > > #include "nsSVGPathGeometryFrame.h" > #include "nsIDOMSVGAnimatedPoints.h" > #include "nsIDOMSVGPointList.h" > #include "nsIDOMSVGPoint.h" > //#include "nsASVGPathBuilder.h" > #include "nsISVGRendererPathBuilder.h" >+#include "nsISVGMarkable.h" > >-class nsSVGPolygonFrame : public nsSVGPathGeometryFrame >+#ifndef M_PI >+#define M_PI 3.14159265358979323846 >+#endif Again, M_PI is not needed anymore... >+ >+class nsSVGPolygonFrame : public nsSVGPathGeometryFrame, >+ public nsISVGMarkable >Index: layout/svg/base/src/nsSVGPolylineFrame.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGPolylineFrame.cpp,v >retrieving revision 1.4 >diff -u -8 -p -r1.4 nsSVGPolylineFrame.cpp >--- layout/svg/base/src/nsSVGPolylineFrame.cpp 18 Apr 2004 14:30:34 -0000 1.4 >+++ layout/svg/base/src/nsSVGPolylineFrame.cpp 18 Oct 2004 19:52:10 -0000 >@@ -37,18 +37,24 @@ > * ***** END LICENSE BLOCK ***** */ > > #include "nsSVGPathGeometryFrame.h" > #include "nsIDOMSVGAnimatedPoints.h" > #include "nsIDOMSVGPointList.h" > #include "nsIDOMSVGPoint.h" > //#include "nsASVGPathBuilder.h" > #include "nsISVGRendererPathBuilder.h" >+#include "nsISVGMarkable.h" > >-class nsSVGPolylineFrame : public nsSVGPathGeometryFrame >+#ifndef M_PI >+#define M_PI 3.14159265358979323846 >+#endif Not needed.. >+ >+class nsSVGPolylineFrame : public nsSVGPathGeometryFrame, >+ public nsISVGMarkable > {
Attachment #162504 -
Flags: review+
Attachment #162230 -
Attachment is obsolete: true
Attachment #162230 -
Flags: review?(scootermorris)
Assignee | ||
Comment 18•20 years ago
|
||
Attachment #162229 -
Flags: superreview?(dbaron)
It looks like attachment 162634 [details] [diff] [review] should have obsoleted attachment 162504 [details] [diff] [review].
Comment on attachment 162229 [details] [diff] [review] changes to shared mozilla files (updated to tip) >Index: content/shared/public/nsCSSKeywordList.h All the changes in this file are not needed. Skip them. >Index: content/shared/public/nsCSSPropList.h >+CSS_PROP_SVG(marker, marker, Marker, SVG, mMarker, eCSSType_Value, PR_FALSE, nsnull) This should be CSS_PROP_SHORTHAND, no? That probably requires a whole bunch of other changes in the patch. >Index: content/shared/public/nsStyleStruct.h >+ nsIURI *mMarker; // [inherited] Likewise, this doesn't belong. And how about using nsCOMPtr<nsIURI> for the others, and commenting that a null pointer means 'none'? It probably isn't absolutely necessary if you stop leaking them in clone, since the rules own them as well, but it's consistent with what we do elsewhere, and it actually may be necessary. >Index: content/base/src/nsRuleNode.cpp >+ // marker: url, inherit >+ if (eCSSUnit_URL == SVGData.mMarker.GetUnit()) { >+ svg->mMarker = SVGData.mMarker.GetURLValue(); >+ } else if (eCSSUnit_Inherit == SVGData.mMarker.GetUnit()) { >+ inherited = PR_TRUE; >+ svg->mMarker = parentSVG->mMarker; >+ } ditto. >+ // marker-end: url, inherit Looks like url, none, inherit per the spec. That needs to be implemented in addition to being fixed in the comments. It may work for some simple cases, but it won't work when the struct is copied from cached data in the rule tree than you need to override. Likewise for the other properties. nsCSSParser.cpp: >+ case eCSSProperty_marker: You need seriously different parsing code for marker, since it's a shorthand. >Index: content/html/style/src/nsCSSStruct.cpp >+ mMarker(aCopy.mMarker), Again. And I'm going to stop commenting on these now. >Index: content/shared/src/nsStyleStruct.cpp >+ mMarker = NULL; >+ mMarkerEnd = NULL; >+ mMarkerMid = NULL; >+ mMarkerStart = NULL; nsnull, please. >+ if (aSource.mMarkerEnd) >+ aSource.mMarkerEnd->Clone(&mMarkerEnd); >+ else >+ mMarkerEnd = NULL; >+ if (aSource.mMarkerMid) >+ aSource.mMarkerMid->Clone(&mMarkerMid); >+ else >+ mMarkerMid = NULL; >+ if (aSource.mMarkerStart) >+ aSource.mMarkerStart->Clone(&mMarkerStart); >+ else >+ mMarkerStart = NULL; Why not just refcount the same URI objects? We do that elsewhere in the style system. Nothing mutates them, so it's fine. Also, this is leaking the URIs, since URIs created other ways are weak pointers and these are strong.
Attachment #162229 -
Flags: superreview?(dbaron) → superreview-
(In reply to comment #20) > leaking them in clone er, in the copy constructor
Comment on attachment 162634 [details] [diff] [review] address last batch of comments >+nsSVGPathGeometryFrame::GetMarkerFrames(nsSVGMarkerFrame **markerStart, >+ aURI = ((const nsStyleSVG *)mStyleContext->GetStyleData(eStyleStruct_SVG))->mMarker; Even once you fix the fact what you're doing in this function is totally not the way to handle shorthand properties (since it gets the cascading all wrong -- see my comments on the other patch), this is the wrong way to use the computed style data API. The above line should be written as: aURI = GetStyleSVG()->mMarker; No casts. No mStyleContext reference from frames. No raw GetStyleData calls.
Also, is there a bug on making this work for URI references into other documents (with appropriate cross-domain checks, of course)?
Assignee | ||
Comment 24•20 years ago
|
||
Attachment #162229 -
Attachment is obsolete: true
Assignee | ||
Comment 25•20 years ago
|
||
Attachment #162504 -
Attachment is obsolete: true
Attachment #162634 -
Attachment is obsolete: true
Attachment #167243 -
Flags: superreview?(dbaron)
Comment on attachment 167243 [details] [diff] [review] changes to shared mozilla files (incorporating dbaron's comments) Index: content/html/style/src/nsCSSParser.cpp > #ifdef MOZ_SVG > case eCSSProperty_stroke_dasharray: > return ParseDasharray(aErrorCode); >+ case eCSSProperty_marker: >+ return ParseMarker(aErrorCode); > #endif Could you move this MOZ_SVG chunk before the "Strip out properties we use internally" section, please? >+PRBool CSSParserImpl::ParseMarker(nsresult& aErrorCode) >+{ >+ nsCSSValue marker; >+ if (ParseSingleValueProperty(aErrorCode, marker, eCSSProperty_marker_end)) { >+ if (ExpectEndProperty(aErrorCode, PR_TRUE)) { >+ AppendValue(eCSSProperty_marker_end, marker); >+ AppendValue(eCSSProperty_marker_mid, marker); >+ AppendValue(eCSSProperty_marker_start, marker); Judging from existing code, you might want to assign |aErrorCode = NS_OK| right here. I'm not sure if it's actually needed, but everyone else does it. >+ return PR_TRUE; >+ } >+ } >+ return PR_FALSE; >+} > #endif
And you should probably modify nsCSSDeclaration::GetValue and nsCSSDeclaration::ToString for the shorthand property as well (the latter is just to be nice; I'm not sure how the former is used, actually).
You should also change nsStyleContext::DumpRegressionData.
Comment on attachment 167243 [details] [diff] [review] changes to shared mozilla files (incorporating dbaron's comments) I'll be happy once those comments are addressed, but I probably should look at the revised patch, particularly the nsCSSDeclaration changes.
Attachment #167243 -
Flags: superreview?(dbaron) → superreview-
Assignee | ||
Comment 30•20 years ago
|
||
Attachment #167243 -
Attachment is obsolete: true
Attachment #167312 -
Flags: superreview?(dbaron)
Attachment #167312 -
Flags: superreview?(dbaron) → superreview+
Attachment #167312 -
Flags: review?(scootermorris)
Attachment #167244 -
Flags: review?(scootermorris)
Comment 31•20 years ago
|
||
Comment on attachment 167244 [details] [diff] [review] changes/new svg-only files (incorporating dbaron's comments) Nits only. Couple of comments that need some more explanation, and a bunch of DEBUG_tor's in nsSVGPathGeometryFrame.cpp >Index: content/svg/content/src/nsSVGAnimatedAngle.cpp >=================================================================== >RCS file: content/svg/content/src/nsSVGAnimatedAngle.cpp >diff -N content/svg/content/src/nsSVGAnimatedAngle.cpp >+//////////////////////////////////////////////////////////////////////// >+// Exported creation functions >+ >+nsresult >+NS_NewSVGAnimatedAngle(nsIDOMSVGAnimatedAngle** aResult, >+ nsIDOMSVGAngle* baseVal) >+{ >+ *aResult = nsnull; >+ >+ nsSVGAnimatedAngle* animatedLength = new nsSVGAnimatedAngle(); >+ if(!animatedLength) return NS_ERROR_OUT_OF_MEMORY; >+ NS_ADDREF(animatedLength); >+ Why are these lines commented out? You should at least add a comment as to why you are doing it the way you are as opposed to the commented lines... >+// nsCOMPtr<nsIDOMSVGLength> baseVal; >+// NS_NewSVGLength(getter_AddRefs(baseVal), context); >+ >+ animatedLength->Init(baseVal); >+ >+ *aResult = (nsIDOMSVGAnimatedAngle*) animatedLength; >+ >+ return NS_OK; >+} >Index: layout/svg/base/src/nsSVGLineFrame.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGLineFrame.cpp,v >retrieving revision 1.4 >diff -u -8 -p -r1.4 nsSVGLineFrame.cpp >--- layout/svg/base/src/nsSVGLineFrame.cpp 18 Apr 2004 14:30:34 -0000 1.4 >+++ layout/svg/base/src/nsSVGLineFrame.cpp 28 Nov 2004 04:34:46 -0000 Why is this commented out?? > > // const nsStyleSVG* nsSVGLineFrame::GetStyle() > // { > // // XXX TODO: strip out any fill color as per svg specs > // return nsSVGGraphicFrame::GetStyle(); > // } >Index: layout/svg/base/src/nsSVGPathGeometryFrame.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGPathGeometryFrame.cpp,v >retrieving revision 1.10 >diff -u -8 -p -r1.10 nsSVGPathGeometryFrame.cpp >--- layout/svg/base/src/nsSVGPathGeometryFrame.cpp 19 Nov 2004 22:52:13 -0000 1.10 >+++ layout/svg/base/src/nsSVGPathGeometryFrame.cpp 28 Nov 2004 04:34:46 -0000 >+// marker helper >+void >+nsSVGPathGeometryFrame::GetMarkerFrames(nsSVGMarkerFrame **markerStart, >+ nsSVGMarkerFrame **markerMid, >+ nsSVGMarkerFrame **markerEnd) >+{ >+ nsIURI *aURI; >+ >+ *markerStart = *markerMid = *markerEnd = NULL; >+ >+ aURI = GetStyleSVG()->mMarkerEnd; >+ if (aURI) { >+ NS_GetSVGMarkerFrame(markerEnd, aURI, mContent); Not sure you want to leave private debugs in the code... here and several other places in this module... >+#ifdef DEBUG_tor >+ nsCAutoString spec; >+ aURI->GetAsciiSpec(spec); >+ fprintf(stderr, "MARKER END %s %p\n", spec.get(), *markerEnd); >+#endif >+ } >+ >+ aURI = GetStyleSVG()->mMarkerMid; >+ if (aURI) { >+ NS_GetSVGMarkerFrame(markerMid, aURI, mContent);
Attachment #167244 -
Flags: review?(scootermorris) → review+
Comment 32•20 years ago
|
||
Comment on attachment 167312 [details] [diff] [review] changes to shared mozilla files (incorporating dbaron's comments) Looks good!
Attachment #167312 -
Flags: review?(scootermorris) → review+
Assignee | ||
Comment 33•20 years ago
|
||
Checked in with Scooter's comments addressed.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•