357808 - make nsSVGMarkerFrame::PaintMark and nsSVGMarkerFrame::RegionMark more robust

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

These functions could use a helper class to set up and tear down attributes.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 243334 [details] [diff] [review]
patch

I wish mMarkerParent had a different name. Currently it suggests it points to the nsSVGMarkerFrame's parent in the frame tree, not the frame of the element that is being marked. Perhaps mMarkedFrame or mReferencingFrame would be better.

>+nsSVGMarkerFrame::nsSVGAutoMarkerHelperContext::nsSVGAutoMarkerHelperContext(
>+    nsSVGMarkerFrame *aFrame,
>+    nsSVGPathGeometryFrame *aParent,
>+    const nsSVGMark *aMark, float aStrokeWidth)
>+      : mFrame(aFrame)
>+{
>+  mFrame->mMarkerParent = aParent;
>+  mFrame->mInUse = PR_TRUE;

Great.

>+  mFrame->mStrokeWidth = aStrokeWidth;
>+  mFrame->mX = aMark->x;
>+  mFrame->mY = aMark->y;
>+  mFrame->mAngle = aMark->angle;

Not great IMHO. When classes have a well defined purpose and they stick to doing that task and only that task, code is easy to read, understand and remember. When unrelated code starts to creep in I find it starts to get harder to understand and recall how things work. I think in this case maintaining a tidy, well defined architecture should trump the tiny code size saving and these four lines should go back where they were.


>+  nsSVGMarkerElement *marker = NS_STATIC_CAST(nsSVGMarkerElement*, mFrame->GetContent());
>+
>+  nsRefPtr<nsSVGCoordCtxProvider> ctx;
>+  ctx =

Nit: just make this a single statement by replacing the semicolon with " =" and indenting the nsSVGUtils::GetCoordContextProvider call on the next line by an extra two spaces.


>+  // A helper class to deal with temporary coord contexts.
>+  // It destroys the context when it goes out of scope.
>+  class nsSVGAutoMarkerHelperContext

I think the comment and class name could be more helpful. IMHO this classes primary purpose should be to make the reference loop protection code much harder to accidentally break. It's mostly about reference loops, not coordinate contexts. That said, it should also set the referencing frame and coordinate context I think. I'll think on this some more and see if I can come up with a name I'd be happy with. Suggestions?

Comment 3

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #2)
> (From update of attachment 243334 [details] [diff] [review] [edit])
> I wish mMarkerParent had a different name. Currently it suggests it points to
> the nsSVGMarkerFrame's parent in the frame tree, not the frame of the element
> that is being marked. Perhaps mMarkedFrame or mReferencingFrame would be
> better.

Changed to mReferencedFrame c.f. nsSVGUtils which has a getReferencedFrame function.

> > >+  mFrame->mStrokeWidth = aStrokeWidth;
> >+  mFrame->mX = aMark->x;
> >+  mFrame->mY = aMark->y;
> >+  mFrame->mAngle = aMark->angle;
> 
> Not great IMHO. When classes have a well defined purpose and they stick to
> doing that task and only that task, code is easy to read, understand and
> remember. When unrelated code starts to creep in I find it starts to get harder
> to understand and recall how things work. I think in this case maintaining a
> tidy, well defined architecture should trump the tiny code size saving and
> these four lines should go back where they were.

Done.

> 
> 
> >+  nsSVGMarkerElement *marker = NS_STATIC_CAST(nsSVGMarkerElement*, mFrame->GetContent());
> >+
> >+  nsRefPtr<nsSVGCoordCtxProvider> ctx;
> >+  ctx =
> 
> Nit: just make this a single statement by replacing the semicolon with " =" and
> indenting the nsSVGUtils::GetCoordContextProvider call on the next line by an
> extra two spaces.

Done.

> 
> 
> >+  // A helper class to deal with temporary coord contexts.
> >+  // It destroys the context when it goes out of scope.
> >+  class nsSVGAutoMarkerHelperContext
> 
> I think the comment and class name could be more helpful. IMHO this classes
> primary purpose should be to make the reference loop protection code much
> harder to accidentally break. It's mostly about reference loops, not coordinate
> contexts. That said, it should also set the referencing frame and coordinate
> context I think. I'll think on this some more and see if I can come up with a
> name I'd be happy with. Suggestions?
> 

comment updated to indicate that the context is cleared rather than destroyed. It has a similar name to the nsSVGGlyphFrame inner class. I've no better naming ideas myself.

Comment 4

[Robert Longson [:longsonr]]

Attachment: update patch

previous patch had problems with compilation of nsSVGMarkerElement.

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 243655 [details] [diff] [review]
update patch

> Changed to mReferencedFrame c.f. nsSVGUtils which has a getReferencedFrame
> function.

Heh, thanks, but the _marker_ frame is the referenced frame. The other one is the referenc_ing_ frame. :-)

> nsresult
> nsSVGMarkerFrame::PaintMark(nsISVGRendererCanvas *aCanvas,
>-                            nsSVGPathGeometryFrame *aParent,
>-                            nsSVGMark *aMark, float aStrokeWidth)
>+                            nsSVGPathGeometryFrame *aReferencedFrame,
>+                            const nsSVGMark *aMark, float aStrokeWidth)
> {
>   // If the flag is set when we get here, it means this marker frame
>   // has already been used painting the current mark, and the document
>   // has a marker reference loop.
>   if (mInUse)
>     return NS_OK;

Can you put the helper here, right after the mInUse check rather than further down please?

>+
>+void
>+nsSVGMarkerFrame::SetParentCoordCtxProvider(nsSVGCoordCtxProvider *aContext)
>+{
>+  nsSVGMarkerElement *marker = NS_STATIC_CAST(nsSVGMarkerElement*, mContent);
>+  marker->SetParentCoordCtxProvider(aContext);
>+}

Adding a function for two lines of code (one of which is simply a cast and optimized away) and only calling it in two places isn't worth it I think. Perhaps just replace the calling points with something along the lines of:

  NS_STATIC_CAST(nsSVGMarkerElement*, mFrame.GetContent())->
    SetParentCoordCtxProvider(ctx);

?

>+nsSVGMarkerFrame::nsSVGAutoMarkerHelperContext::nsSVGAutoMarkerHelperContext(
>+    nsSVGMarkerFrame *aFrame,
>+    nsSVGPathGeometryFrame *aReferencedFrame)
>+      : mFrame(aFrame)
>+{
>+  mFrame->mReferencedFrame = aReferencedFrame;
>+  mFrame->mInUse = PR_TRUE;

Nit: can you make the mInUse the first line? I'm paranoid about emphasizing the ref loop protection. :-)

As for the name, I definitely think we should loose the "ns" and "SVG" parts for this nested class. What do you think of ?

Comment 6

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #5)
> (From update of attachment 243655 [details] [diff] [review] [edit])
> > Changed to mReferencedFrame c.f. nsSVGUtils which has a getReferencedFrame
> > function.
> 
> Heh, thanks, but the _marker_ frame is the referenced frame. The other one is
> the referenc_ing_ frame. :-)

I went with your original suggestion of mMarkedFrame.

> 
> > nsresult
> > nsSVGMarkerFrame::PaintMark(nsISVGRendererCanvas *aCanvas,
> >-                            nsSVGPathGeometryFrame *aParent,
> >-                            nsSVGMark *aMark, float aStrokeWidth)
> >+                            nsSVGPathGeometryFrame *aReferencedFrame,
> >+                            const nsSVGMark *aMark, float aStrokeWidth)
> > {
> >   // If the flag is set when we get here, it means this marker frame
> >   // has already been used painting the current mark, and the document
> >   // has a marker reference loop.
> >   if (mInUse)
> >     return NS_OK;
> 
> Can you put the helper here, right after the mInUse check rather than further
> down please?

Done.

> 
> >+
> >+void
> >+nsSVGMarkerFrame::SetParentCoordCtxProvider(nsSVGCoordCtxProvider *aContext)
> >+{
> >+  nsSVGMarkerElement *marker = NS_STATIC_CAST(nsSVGMarkerElement*, mContent);
> >+  marker->SetParentCoordCtxProvider(aContext);
> >+}
> 
> Adding a function for two lines of code (one of which is simply a cast and
> optimized away) and only calling it in two places isn't worth it I think.
> Perhaps just replace the calling points with something along the lines of:
> 
>   NS_STATIC_CAST(nsSVGMarkerElement*, mFrame.GetContent())->
>     SetParentCoordCtxProvider(ctx);
> 
> ?
> 

I can't do that. The reason for this method is that SetParentCoordCtxProvider is a protected member of nsSVGMarkerElement. nsSVGMarkerFrame gets access to it as it is declared to be a friend of nsSVGMarkerElement. Inner classes of nsSVGMarkerFrame do not count as friends of nsSVGMarkerElement so the inner class will not compile if I move the code to the calling points. I don't want to make the inner class a friend of nsSVGMarkerElement either as a) it's an implementation detail of nsSVGMarkerFrame and b) I'd have to #include nsSVGMarkerFrame.h in nsSVGMarkerElement to get nsSVGMarkerElement.cpp to compile and content should not depend on frames IMHO.

> >+nsSVGMarkerFrame::nsSVGAutoMarkerHelperContext::nsSVGAutoMarkerHelperContext(
> >+    nsSVGMarkerFrame *aFrame,
> >+    nsSVGPathGeometryFrame *aReferencedFrame)
> >+      : mFrame(aFrame)
> >+{
> >+  mFrame->mReferencedFrame = aReferencedFrame;
> >+  mFrame->mInUse = PR_TRUE;
> 
> Nit: can you make the mInUse the first line? I'm paranoid about emphasizing the
> ref loop protection. :-)

Done.

> 
> As for the name, I definitely think we should loose the "ns" and "SVG" parts
> for this nested class. What do you think of ?
> 

Done. I should raise a follow-up to do the same thing to the inner class of nsSVGGlyphFrame.

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 243889 [details] [diff] [review]
address review comments

(In reply to comment #6)
> I can't do that. The reason for this...

Ah, I see.

> As for the name, I definitely think we should loose the "ns" and "SVG" parts
> for this nested class. What do you think of ?

Oops, I meant to suggest a name at that point. I'm not at all in love with the "Context" in the name, since I think it puts the focus on the wrong thing. Perhaps "AutoMarkerReferencer"?

Comments on the current patch:

>+  nsCOMPtr<nsIDOMSVGMatrix> markedTM;

Sounds like the TM is marked to me. Would prefer markedFrameTM, but okay.

>+  // A helper class to deal with temporary coord contexts.
>+  // It clears the context when it goes out of scope.

Again, I'd like the emphasis to be on the breaking of evil reference loops. Perhaps:

  // A helper class to allow us to paint markers safely. The helper
  // automatically sets and clears the mInUse flag on the marker frame (to
  // prevent nasty reference loops) as well as the reference to the marked
  // frame and its coordinate context. It's easy to mess up or break these
  // references, so this helper makes the code far more robust.

>+  class AutoMarkerHelperContext
>+  {
>+  public:
>+    AutoMarkerHelperContext(nsSVGMarkerFrame *aFrame,
>+                            nsSVGPathGeometryFrame *aMarkedFrame);
>+    ~AutoMarkerHelperContext();
>+  private:
>+    nsSVGMarkerFrame *mFrame;
>+  };

Comment 8

[Jonathan Watt [:jwatt]]

Actually that last sentence would be better like this I think: "It's easy to mess this up and break things, so this helper makes the code far more robust."

Comment 9

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #7)

> Oops, I meant to suggest a name at that point. I'm not at all in love with the
> "Context" in the name, since I think it puts the focus on the wrong thing.
> Perhaps "AutoMarkerReferencer"?

Done.

> 
> Comments on the current patch:
> 
> >+  nsCOMPtr<nsIDOMSVGMatrix> markedTM;
> 
> Sounds like the TM is marked to me. Would prefer markedFrameTM, but okay.

Left as is. None of the other TM variables have frame in them.

> Again, I'd like the emphasis to be on the breaking of evil reference loops.
> Perhaps:
> 
>   // A helper class to allow us to paint markers safely. The helper
>   // automatically sets and clears the mInUse flag on the marker frame (to
>   // prevent nasty reference loops) as well as the reference to the marked
>   // frame and its coordinate context. It's easy to mess up or break these
>   // references, so this helper makes the code far more robust.
> 

Done including other change too.

Comment 10

[tor]

Comment on attachment 246808 [details] [diff] [review]
address review comments

>+  class AutoMarkerReferencer
>+  {
>+  public:
>+    AutoMarkerReferencer(nsSVGMarkerFrame *aFrame,
>+                         nsSVGPathGeometryFrame *aMarkedFrame);
>+    ~AutoMarkerReferencer();
>+  private:
>+    nsSVGMarkerFrame *mFrame;
>+  };
>+
>+  // nsSVGMarkerFrame methods:
>+  void SetParentCoordCtxProvider(nsSVGCoordCtxProvider *aContext);

Why not make this a method of the referencer helper class?  It appears to be the only user of this, and already contains the necessary information in mFrame.

Comment 11

[Robert Longson [:longsonr]]

Tor,

(In reply to comment #10)
> >+  // nsSVGMarkerFrame methods:
> >+  void SetParentCoordCtxProvider(nsSVGCoordCtxProvider *aContext);
> 
> Why not make this a method of the referencer helper class?  It appears to be
> the only user of this, and already contains the necessary information in
> mFrame.
> 

Jonathan made the same observation earlier. Check out the middle bit of comment 6 for an explanation of the problem. 

Comment 12

[tor]

(In reply to comment #11)
> Jonathan made the same observation earlier. Check out the middle bit of comment
> 6 for an explanation of the problem. 

On gcc-4.1.1, the inner class seems to inherit the permission of the parent, so I can do this without adding any more friends or the like.

void
nsSVGMarkerFrame::AutoMarkerReferencer::SetParentCoordCtxProvider(nsSVGCoordCtxProvider *aContext)
{
  nsSVGMarkerElement *marker = NS_STATIC_CAST(nsSVGMarkerElement*,
                                              mFrame->GetContent());
  marker->SetParentCoordCtxProvider(aContext);
}

Comment 13

[tor]

Comment on attachment 246808 [details] [diff] [review]
address review comments

As I read Stroustrup, this appears to be a bug in gcc.

Comment 14

[Robert Longson [:longsonr]]

checked in.