1349462 - Rename nsIFrame::IsSVGText() to nsSVGUtils::IsInSVGTextSubtree()

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[u459114]

nsIFrame::IsSVGText() return true if this object is an SVG text object or a descendant of an SVG test object. Function name is kind of misleading.

Comment 1

[Robert Longson [:longsonr]]

Seems fine as is is to me FWIW, have you checked with cam?

Comment 2

[u459114]

(In reply to Robert Longson from comment #1)
> Seems fine as is is to me FWIW, have you checked with cam?

Yes, I discussed with him several weeks ago.

Comment 3

[Cameron McCormack (:heycam)]

I think the current name does kind of sound like "is this an SVGTextFrame", rather than "is this a frame that implements SVG text", which would include the descendant nsBlockFrames/nsInlineFrames.

Since the SVGTextFrame itself also has this bit set, I don't think "IsDescendantOfSVGText" is quite right.  (If anything, it would need to be "IsSVGTextOrDescendant".)

Maybe "IsInSVGTextSubtree" (to mirror things like IsInNativeAnonymousSubtree)?

Comment 4

[Cameron McCormack (:heycam)]

An alternative would be just to document the IsSVGText method in nsIFrame.h.  (Also it looks like the comment above the NS_FRAME_IS_SVG_TEXT definition in nsFrameStateBits.h is wrong, since it also says "descendant" when it should include the SVGTextFrame itself, too.)

Comment 5

[u459114]

How about we move this util function to nsSVGUtil? Define an SVG specific function in nsIFrame is a little bit weird.
For example:
bool nsSVGUtil::IsInSVGTextSubtree(nsIFrame* aFrame)
{
  return aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT;
}
bool nsSVGUtil::HasSVGLayout(nsIFrame* aFrame)
{
  return aFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT;
}

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Cameron McCormack (:heycam) from comment #3)
> Maybe "IsInSVGTextSubtree"

Gets my vote.

I'd also have a slight preference for putting these on nsSVGUtils.

Comment 7

[Cameron McCormack (:heycam)]

FWIW there are other access methods on nsIFrame that just check state bits (e.g. IsGeneratedContentFrame, IsXULHorizontal, IsAbsoluteContainer), so it seems fine to me to stay there.  But I don't mind if it's on nsSVGUtils either.

Comment 8

[u459114]

IsGeneratedContentFrame and IsAbsoluteContainer is ok for me. If we have nsXULUtil, I would like to move IsXULHorizontal there.

Comment 9

[u459114]

Attachment: Bug 1349462 - Part 1. Rename IsSVGText as IsInSVGTextSubtree.

Review commit: https://reviewboard.mozilla.org/r/123006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123006/

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8850409 [details]
Bug 1349462 - Part 1. Rename IsSVGText as IsInSVGTextSubtree.

https://reviewboard.mozilla.org/r/123006/#review125270

::: layout/svg/nsSVGUtils.h:613
(Diff revision 1)
> +  static bool
> +  IsInSVGTextSubtree(const nsIFrame* aFrame) {
> +    return aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT;
> +  }

Nit: one line for |static bool IsInSVGTextSubtree(...) {|.  (The few functions above here have the wrong style, for declarations inside a class.)

Also, please add a comment describing what this does (i.e., returns true if the frame is an SVGTextFrame or one of its descendants).

Comment 11

[u459114]

Attachment: Bug 1349462 - Part 2. one line for static function.

Review commit: https://reviewboard.mozilla.org/r/123038/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123038/

Comment 12

[u459114]

Comment on attachment 8850409 [details]
Bug 1349462 - Part 1. Rename IsSVGText as IsInSVGTextSubtree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123006/diff/1-2/

Comment 13

[u459114]

Comment on attachment 8850445 [details]
Bug 1349462 - Part 2. one line for static function.

https://reviewboard.mozilla.org/r/123038/#review125298

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 8850445 [details]
Bug 1349462 - Part 2. one line for static function.

https://reviewboard.mozilla.org/r/123038/#review125294

::: layout/svg/nsSVGUtils.h:367
(Diff revision 1)
> -  GetClipRectForFrame(nsIFrame *aFrame,
> -                      float aX, float aY, float aWidth, float aHeight);
> +                                     float aX, float aY, float aWidth,
> +                                     float aHeight);

Since you're having to wrap aHeight here, I'd put aWidth on the same line, but maybe that's just me. ;)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9222bab527d
Part 1. Rename IsSVGText as IsInSVGTextSubtree. r=heycam
https://hg.mozilla.org/integration/autoland/rev/821005b8d9fa
Part 2. one line for static function. r=jwatt

Comment 16

[Jonathan Watt [:jwatt]]

(In reply to Pulsebot from comment #15)
> https://hg.mozilla.org/integration/autoland/rev/821005b8d9fa
> Part 2. one line for static function. r=jwatt

Hmm, I forgot to review the commit message. It's useful to think of commit messages from the point of view of someone looking at *all* commits landing in the tree and asking yourself if this message is useful for them. Something like "Reformat nsSVGUtils' methods to conform to the style guidelines" would have been much more useful I think. :) Sorry for not catching that.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9222bab527d
https://hg.mozilla.org/mozilla-central/rev/821005b8d9fa