349879 - nsSVGGlyphFrame.cpp needs code consolidation

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

See https://bugzilla.mozilla.org/show_bug.cgi?id=347245#c4 .

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Here's my take on the consolidation. Should improve performance a little too by passing in the text to GetCharacterPosition, reducing the number of calls to GetCharacterData.

Comment 2

[tor]

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

> nsSVGGlyphFrame::GetCharacterData(nsAString & aCharacterData)
>+  } else {
>+    nsAString::iterator start, end;
>+    characterData.BeginWriting(start);
>+    characterData.EndWriting(end);
>+    while (start != end) {
>+      if (NS_IsAsciiWhitespace(*start))
>+        *start = ' ';
>+      ++start;
>+    }

Unrelated whitespace handling change - please remove from this patch.

>Index: renderer/src/cairo/nsSVGCairoGlyphGeometry.h
>Index: renderer/src/cairo/nsSVGRendererCairo.cpp

Whoops, forgot those two files in my patch, didn't I?  Move these changes to a different bug/patch.

Comment 3

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #2)
> (From update of attachment 235407 [details] [diff] [review] [edit])
> > nsSVGGlyphFrame::GetCharacterData(nsAString & aCharacterData)
> >+  } else {
> >+    nsAString::iterator start, end;
> >+    characterData.BeginWriting(start);
> >+    characterData.EndWriting(end);
> >+    while (start != end) {
> >+      if (NS_IsAsciiWhitespace(*start))
> >+        *start = ' ';
> >+      ++start;
> >+    }
> 
> Unrelated whitespace handling change - please remove from this patch.

Done.

> 
> >Index: renderer/src/cairo/nsSVGCairoGlyphGeometry.h
> >Index: renderer/src/cairo/nsSVGRendererCairo.cpp
> 
> Whoops, forgot those two files in my patch, didn't I?  Move these changes to a
> different bug/patch.
> 

Submitted as bug 350581

Comment 4

[tor]

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

> nsresult
> nsSVGGlyphFrame::GetCharacterPosition(cairo_t *ctx,
>+                                      nsAString &aText,
>                                       nsSVGCharacterPosition **aCharacterPosition)
> {
>   *aCharacterPosition = nsnull;
>+
>+  PRUint32 strLength = aText.Length();
>+  if (strLength == 0)
>+    return NS_OK;

Use IsEmpty().

> class nsSVGGlyphFrame : public nsSVGGlyphFrameBase,
>                         public nsISVGGlyphFragmentLeaf, // : nsISVGGlyphFragmentNode
>                         public nsISVGChildFrame
> {
> protected:
>   friend nsIFrame*
>   NS_NewSVGGlyphFrame(nsIPresShell* aPresShell, nsIContent* aContent,
>                       nsIFrame* parentFrame, nsStyleContext* aContext);
>   nsSVGGlyphFrame(nsStyleContext* aContext);
> 
>+  friend class nsSVGAutoGlyphHelperContext;

Instead of making it a friend, you could make it a nested class.

Comment 5

[Robert Longson [:longsonr]]

Attachment: address additional review comments

(In reply to comment #4)
> (From update of attachment 235907 [details] [diff] [review] [edit])
> > nsresult
> > nsSVGGlyphFrame::GetCharacterPosition(cairo_t *ctx,
> >+                                      nsAString &aText,
> >                                       nsSVGCharacterPosition **aCharacterPosition)
> > {
> >   *aCharacterPosition = nsnull;
> >+
> >+  PRUint32 strLength = aText.Length();
> >+  if (strLength == 0)
> >+    return NS_OK;
> 
> Use IsEmpty().

That would mean an additional virtual call. As this is a belt and braces test I've changed it to an NS_ASSERTION using IsEmpty.

> 
> > class nsSVGGlyphFrame : public nsSVGGlyphFrameBase,
> >                         public nsISVGGlyphFragmentLeaf, // : nsISVGGlyphFragmentNode
> >                         public nsISVGChildFrame
> > {
> > protected:
> >   friend nsIFrame*
> >   NS_NewSVGGlyphFrame(nsIPresShell* aPresShell, nsIContent* aContent,
> >                       nsIFrame* parentFrame, nsStyleContext* aContext);
> >   nsSVGGlyphFrame(nsStyleContext* aContext);
> > 
> >+  friend class nsSVGAutoGlyphHelperContext;
> 
> Instead of making it a friend, you could make it a nested class.
> 

Done. I've also made the arguments of some functions const where possible.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 236385 [details] [diff] [review]
address additional review comments

great!

Comment 7

[Robert Longson [:longsonr]]

checked in

Comment 8

[Bill Gianopoulos [:WG9s]]

Creature does not seem to like this for some reason.  See http://tinderbox.mozilla.org/SeaMonkey/

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

patch backed out, it turned creature red.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1157768640.11487.gz for the log

Comment 11

[Robert Longson [:longsonr]]

Attachment: attempt to fix VC6 issues

I'm sorry I caused such problems. 

This patch differs from the previous one only in that it makes the nsSVGAutoGlyphHelperContext a friend of nsSVGGlyphFrame as per http://www.mozilla.org/hacking/portable-cpp.html#inner_classes

Comment 12

[Robert Longson [:longsonr]]

vc6 version checked in