As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 655497 - Remove nsISVGGlyphFragmentLeaf
: Remove nsISVGGlyphFragmentLeaf
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Robert Longson
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: deCOM
  Show dependency treegraph
 
Reported: 2011-05-07 09:28 PDT by Robert Longson
Modified: 2011-05-08 02:22 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (23.52 KB, patch)
2011-05-07 09:32 PDT, Robert Longson
no flags Details | Diff | Splinter Review
updated patch (28.18 KB, patch)
2011-05-07 09:41 PDT, Robert Longson
no flags Details | Diff | Splinter Review
updated patch (28.84 KB, patch)
2011-05-07 10:08 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review

Description User image Robert Longson 2011-05-07 09:28:09 PDT
nsISVGGlyphFragment leaf is only implemented by nsSVGGlyphFrame. We can remove it and make lots of methods non-virtual.
Comment 1 User image Robert Longson 2011-05-07 09:32:43 PDT
Created attachment 530848 [details] [diff] [review]
patch
Comment 2 User image Daniel Holbert [:dholbert] 2011-05-07 09:37:25 PDT
Looks like nsISVGGlyphFragment.h has a documentation comment above the decls for GetEffecitve[XY|DxDy|Rotate].  We should probably preserve those, copying them over to nsSVGGlyphFrame.h, right?
Comment 3 User image Robert Longson 2011-05-07 09:38:21 PDT
Sure, and I should include the removal of nsISVGGlyphFragmentLeaf in the patch too!
Comment 4 User image Robert Longson 2011-05-07 09:41:33 PDT
Created attachment 530854 [details] [diff] [review]
updated patch

Copied the comments into nsSVGGlyphFrame.h
Comment 5 User image Daniel Holbert [:dholbert] 2011-05-07 09:57:28 PDT
Comment on attachment 530848 [details] [diff] [review]
patch

>diff --git a/layout/svg/base/src/nsSVGGlyphFrame.h b/layout/svg/base/src/nsSVGGlyphFrame.h
>   // nsSVGGeometryFrame methods
>   gfxMatrix GetCanvasTM();
> 
>-  // nsISVGGlyphFragmentLeaf interface:
[...]
>+  nsresult GetStartPositionOfChar(PRUint32 charnum, nsIDOMSVGPoint **_retval);
>+  nsresult GetEndPositionOfChar(PRUint32 charnum, nsIDOMSVGPoint **_retval);
>+  nsresult GetExtentOfChar(PRUint32 charnum, nsIDOMSVGRect **_retval);
>+  nsresult GetRotationOfChar(PRUint32 charnum, float *_retval);

Probably best to add something in place of "// nsISVGGlyphFragmentLeaf interface" there, or else everything below it implicitly falls under the heading of "// nsSVGGeometryFrame methods".

e.g. // My own methods, or // nsSVGGlyphFrame methods, or something.

>   // nsISVGGlyphFragmentNode interface:
>   // These do not use the global transform if NS_STATE_NONDISPLAY_CHILD

For logical organization, these inherited methods' declarations should probably move up top now, right?  So that we've got
 // methods from one iface
 ...
 // methods from another iface
 ...
 // my own methods

If that makes sense, maybe that move could happen in a second patch here that only moves these declarations without actually changing anything.

>+nsSVGTextContainerFrame::GetGlyphFrameAtCharNum(nsISVGGlyphFragmentNode* node,
>+                                                PRUint32 charnum,
>+                                                PRUint32 *offset)
> {
>-  nsISVGGlyphFragmentLeaf *fragment = node->GetFirstGlyphFragment();
>+  nsSVGGlyphFrame *fragment = node->GetFirstGlyphFrame();

Nit: in most places, your patch does s/fragment/frame/ on the naming of variables, but it looks like you might have missed this spot.
Comment 6 User image Daniel Holbert [:dholbert] 2011-05-07 10:02:08 PDT
Comment on attachment 530854 [details] [diff] [review]
updated patch

>@@ -450,10 +450,10 @@ nsSVGTextContainerFrame::BuildPositionLi
>-    } else if (leaf) {
>+    } else if (mContent->IsNodeOfType(nsINode::eTEXT)) {
>+      nsSVGGlyphFrame *leaf = static_cast<nsSVGGlyphFrame*>(kid);

Are we absolutely sure that there couldn't be any other eText type nodes here?

If so, add a NS_ABORT_IF_FALSE(kid->GetType() == nsGkAtoms::svgGlyphFrame
(and if not, maybe use GetType() in place of IsNodeOfType.)
Comment 7 User image Daniel Holbert [:dholbert] 2011-05-07 10:07:33 PDT
(In reply to comment #6)
> Are we absolutely sure that there couldn't be any other eText type nodes
> here?

(or rather, any other frames for eTEXT-type nodes)

I'm guessing we're sure, but just want to double-check (and get your feeling on how future-proof this assumption is), since you know this text code better than I do.
Comment 8 User image Robert Longson 2011-05-07 10:08:34 PDT
Created attachment 530857 [details] [diff] [review]
updated patch
Comment 9 User image Robert Longson 2011-05-07 10:11:33 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > Are we absolutely sure that there couldn't be any other eText type nodes
> > here?

Yes, with the code as it is at the moment. Even so there's no harm changing to check the frame directly so that's what I've done.
Comment 10 User image Daniel Holbert [:dholbert] 2011-05-07 10:17:18 PDT
Comment on attachment 530857 [details] [diff] [review]
updated patch

Looks great!
Comment 11 User image Dão Gottwald [:dao] 2011-05-08 02:22:39 PDT
http://hg.mozilla.org/mozilla-central/rev/23430875e861

Note You need to log in before you can comment on or make changes to this bug.