Closed
Bug 349879
Opened 18 years ago
Closed 18 years ago
nsSVGGlyphFrame.cpp needs code consolidation
Categories
(Core :: SVG, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: tor, Assigned: longsonr)
Details
Attachments
(2 files, 2 obsolete files)
|
(deleted),
patch
|
tor
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
|
(deleted),
patch
|
tor
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
|
Assignee | |
Updated•18 years ago
|
Assignee: general → longsonr
|
|
Assignee | |
Comment 1•18 years ago
|
||
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.
Attachment #235407 -
Flags: review?(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.
|
|
Assignee | |
Comment 3•18 years ago
|
||
(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
Attachment #235407 -
Attachment is obsolete: true
Attachment #235907 -
Flags: review?(tor)
Attachment #235407 -
Flags: review?(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.
|
|
Assignee | |
Comment 5•18 years ago
|
||
(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.
Attachment #235907 -
Attachment is obsolete: true
Attachment #236385 -
Flags: review?(tor)
Attachment #235907 -
Flags: review?(tor)
Attachment #236385 -
Flags: review?(tor) → review+
|
|
Assignee | |
Updated•18 years ago
|
Attachment #236385 -
Flags: superreview?(roc)
Comment on attachment 236385 [details] [diff] [review]
address additional review comments
great!
Attachment #236385 -
Flags: superreview?(roc) → superreview+
|
|
Assignee | |
Comment 7•18 years ago
|
||
checked in
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
|
||
Comment 8•18 years ago
|
||
Creature does not seem to like this for some reason. See http://tinderbox.mozilla.org/SeaMonkey/
|
||
Comment 9•18 years ago
|
||
patch backed out, it turned creature red.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
|
|
||
Comment 10•18 years ago
|
||
|
|
Assignee | |
Comment 11•18 years ago
|
||
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
Attachment #237722 -
Flags: superreview?(roc)
Attachment #237722 -
Flags: review?(tor)
Attachment #237722 -
Flags: review?(tor) → review+
Attachment #237722 -
Flags: superreview?(roc) → superreview+
|
|
Assignee | |
Comment 12•18 years ago
|
||
vc6 version checked in
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•