1388953 - Rename variable name of nsSVGDisplayableFrame type from svg to displayable

Status: RESOLVED WONTFIX

(Core :: SVG, enhancement, P3)

Description

[u459114]

Mostly, the varaiable name of nsSVGDisplayableFrame* is svg, for exp:
nsSVGDisplayableFrame* svg = do_QueryFrame(aFrame);

Rename it as displayable to improve readability

Comment 1

[u459114]

Attachment: Bug 1388953 - Rename variable name of nsSVGDisplayableFrame type to displayable.

Only try to improve code readibility.

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

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 8895838 [details]
Bug 1388953 - Rename variable name of nsSVGDisplayableFrame type to displayable.

https://reviewboard.mozilla.org/r/167126/#review174262

::: dom/svg/SVGTransformableElement.cpp:184
(Diff revision 1)
>  
>    if (!frame || (frame->GetStateBits() & NS_FRAME_IS_NONDISPLAY)) {
>      rv.Throw(NS_ERROR_FAILURE);
>      return nullptr;
>    }
> -  nsSVGDisplayableFrame* svgframe = do_QueryFrame(frame);
> +  nsSVGDisplayableFrame* displayable = do_QueryFrame(frame);

We don't actually need a variable here. Can you change this code to do what we do here:

https://dxr.mozilla.org/mozilla-central/rev/b95b1638db48fc3d450b95b98da6bcd2f9326d2f/layout/svg/nsSVGIntegrationUtils.cpp#1274

::: layout/painting/nsDisplayList.cpp:8546
(Diff revision 1)
>  bool nsDisplaySVGEffects::ValidateSVGFrame()
>  {
>    const nsIContent* content = mFrame->GetContent();
>    bool hasSVGLayout = (mFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT);
>    if (hasSVGLayout) {
> -    nsSVGDisplayableFrame* svgFrame = do_QueryFrame(mFrame);
> +    nsSVGDisplayableFrame* displayable = do_QueryFrame(mFrame);

Same here.

::: layout/svg/nsSVGClipPathFrame.cpp:53
(Diff revision 1)
>    // Restore current transform after applying clip path:
>    gfxContextMatrixAutoSaveRestore autoRestore(&aContext);
>  
>    RefPtr<Path> clipPath;
>  
> -  nsSVGDisplayableFrame* singleClipPathChild = nullptr;
> +  nsSVGDisplayableFrame* displayable = nullptr;

I don't want to change the name here. What we care about is whether the clipPath contains a single, displayable SVG element. One half of that (the fact that it's displayable) is explicit the type name. The other half (that it is the sole instance of such a frame in the clipPath) is equally important to make clear though, and your name change obscures that whereas the current name makes it clear.

::: layout/svg/nsSVGClipPathFrame.cpp:199
(Diff revision 1)
>  nsSVGClipPathFrame::PaintFrameIntoMask(nsIFrame *aFrame,
>                                         nsIFrame* aClippedFrame,
>                                         gfxContext& aTarget,
>                                         const gfxMatrix& aMatrix)
>  {
> -  nsSVGDisplayableFrame* frame = do_QueryFrame(aFrame);
> +  nsSVGDisplayableFrame* displayable = do_QueryFrame(aFrame);

There are lots of different types of "displayable"s in the Mozilla code so this name change makes the code a bit less clear in my opinion. (The name "displayable" is too generic.)

Besides that, the issue with this code isn't that it's not clear that the frame is a nsSVGDisplayableFrame, the issue is that it's not clear that aFrame is a child frame of the clipPath.

What I'd rather see here is aFrame renamed to aClipPathChildFrame and 'frame' renamed to clipPathChildFrame.

::: layout/svg/nsSVGClipPathFrame.cpp:320
(Diff revision 1)
>      return false;
>    }
>  
>    for (nsIFrame* kid = mFrames.FirstChild(); kid;
>         kid = kid->GetNextSibling()) {
> -    nsSVGDisplayableFrame* SVGFrame = do_QueryFrame(kid);
> +    nsSVGDisplayableFrame* displayable = do_QueryFrame(kid);

Again "displayable" is too generic, and actually the important thing is that this is a child frame. "displayableChild" would make me happier.

::: layout/svg/nsSVGClipPathFrame.cpp:341
(Diff revision 1)
>  
>    return false;
>  }
>  
>  bool
> -nsSVGClipPathFrame::IsTrivial(nsSVGDisplayableFrame **aSingleChild)
> +nsSVGClipPathFrame::IsTrivial(nsSVGDisplayableFrame **aDisplayable)

Again, I think we should leave this variable name alone.

::: layout/svg/nsSVGClipPathFrame.cpp:355
(Diff revision 1)
>  
> -  nsSVGDisplayableFrame* foundChild = nullptr;
> +  nsSVGDisplayableFrame* foundDisplayable = nullptr;
>  
>    for (nsIFrame* kid = mFrames.FirstChild(); kid;
>         kid = kid->GetNextSibling()) {
> -    nsSVGDisplayableFrame* svgChild = do_QueryFrame(kid);
> +    nsSVGDisplayableFrame* displayable = do_QueryFrame(kid);

"displayableChild"

::: layout/svg/nsSVGClipPathFrame.cpp:481
(Diff revision 1)
>    SVGBBox unionBBox, tmpBBox;
>    for (; node; node = node->GetNextSibling()) {
>      nsIFrame *frame =
>        static_cast<nsSVGElement*>(node)->GetPrimaryFrame();
>      if (frame) {
> -      nsSVGDisplayableFrame* svg = do_QueryFrame(frame);
> +      nsSVGDisplayableFrame* displayable = do_QueryFrame(frame);

Again, "displayable" too generic. You could call this "displayableSibling" or something, but I don't think we gain much from this change. I think it would be better to just drop it.

::: layout/svg/nsSVGContainerFrame.cpp:181
(Diff revision 1)
>          (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN |
>           NS_FRAME_IS_NONDISPLAY))) {
>      for (nsIFrame* kid = firstNewFrame; kid != nextFrame;
>           kid = kid->GetNextSibling()) {
> -      nsSVGDisplayableFrame* SVGFrame = do_QueryFrame(kid);
> -      if (SVGFrame) {
> +      nsSVGDisplayableFrame* displayable = do_QueryFrame(kid);
> +      if (displayable) {

displayableChild

::: layout/svg/nsSVGContainerFrame.cpp:345
(Diff revision 1)
>  
>    nsOverflowAreas overflowRects;
>  
>    for (nsIFrame* kid = mFrames.FirstChild(); kid;
>         kid = kid->GetNextSibling()) {
> -    nsSVGDisplayableFrame* SVGFrame = do_QueryFrame(kid);
> +    nsSVGDisplayableFrame* displayable = do_QueryFrame(kid);

displayableChild

::: layout/svg/nsSVGContainerFrame.cpp:418
(Diff revision 1)
>    SVGBBox bboxUnion;
>  
>    nsIFrame* kid = mFrames.FirstChild();
>    while (kid) {
>      nsIContent *content = kid->GetContent();
> -    nsSVGDisplayableFrame* svgKid = do_QueryFrame(kid);
> +    nsSVGDisplayableFrame* displayable = do_QueryFrame(kid);

displayableChild

::: layout/svg/nsSVGIntegrationUtils.cpp:626
(Diff revision 1)
>  #endif
>  
>    bool hasSVGLayout = (aFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT);
>    if (hasSVGLayout) {
>  #ifdef DEBUG
> -    nsSVGDisplayableFrame* svgFrame = do_QueryFrame(aFrame);
> +    nsSVGDisplayableFrame* displayable = do_QueryFrame(aFrame);

displayableFrame, maybe? (If you think it's worth actually changing this.)

::: layout/svg/nsSVGMarkerFrame.cpp:142
(Diff revision 1)
>      nsSVGUtils::SetClipRect(&aContext, markTM, clipRect);
>    }
>  
>  
>    nsIFrame* kid = GetAnonymousChildFrame(this);
> -  nsSVGDisplayableFrame* SVGFrame = do_QueryFrame(kid);
> +  nsSVGDisplayableFrame* displayable = do_QueryFrame(kid);

displayableChild

::: layout/svg/nsSVGMarkerFrame.cpp:184
(Diff revision 1)
>    mMarkerTM = content->GetMarkerTransform(aStrokeWidth, aMark);
>    Matrix viewBoxTM = content->GetViewBoxTransform();
>  
>    Matrix tm = viewBoxTM * mMarkerTM * aToBBoxUserspace;
>  
> -  nsSVGDisplayableFrame* child = do_QueryFrame(GetAnonymousChildFrame(this));
> +  nsSVGDisplayableFrame* displayable =

I don't think we should change this.

::: layout/svg/nsSVGMaskFrame.cpp:115
(Diff revision 1)
>                         aParams.toUserSpace;
>  
>    for (nsIFrame* kid = mFrames.FirstChild(); kid;
>         kid = kid->GetNextSibling()) {
>      // The CTM of each frame referencing us can be different
> -    nsSVGDisplayableFrame* SVGFrame = do_QueryFrame(kid);
> +    nsSVGDisplayableFrame* displayable = do_QueryFrame(kid);

displayableChild

::: layout/svg/nsSVGPatternFrame.cpp:384
(Diff revision 1)
>    if (!(patternWithChildren->GetStateBits() & NS_FRAME_DRAWING_AS_PAINTSERVER)) {
>      AutoSetRestorePaintServerState paintServer(patternWithChildren);
>      for (nsIFrame* kid = firstKid; kid;
>           kid = kid->GetNextSibling()) {
>        // The CTM of each frame referencing us can be different
> -      nsSVGDisplayableFrame* SVGFrame = do_QueryFrame(kid);
> +      nsSVGDisplayableFrame* displayable = do_QueryFrame(kid);

displayableChild

::: layout/svg/nsSVGSwitchFrame.cpp:131
(Diff revision 1)
>                 "If display lists are enabled, only hit-testing of non-display "
>                 "SVG should take this code path");
>  
>    nsIFrame *kid = GetActiveChildFrame();
> -  nsSVGDisplayableFrame* svgFrame = do_QueryFrame(kid);
> -  if (svgFrame) {
> +  nsSVGDisplayableFrame* displayable = do_QueryFrame(kid);
> +  if (displayable) {

displayableChild

::: layout/svg/nsSVGSwitchFrame.cpp:183
(Diff revision 1)
>    }
>  
>    nsOverflowAreas overflowRects;
>  
>    nsIFrame *child = GetActiveChildFrame();
> -  nsSVGDisplayableFrame* svgChild = do_QueryFrame(child);
> +  nsSVGDisplayableFrame* displayable = do_QueryFrame(child);

displayableChild

::: layout/svg/nsSVGSwitchFrame.cpp:215
(Diff revision 1)
>  SVGBBox
>  nsSVGSwitchFrame::GetBBoxContribution(const Matrix &aToBBoxUserspace,
>                                        uint32_t aFlags)
>  {
>    nsIFrame* kid = GetActiveChildFrame();
> -  nsSVGDisplayableFrame* svgKid = do_QueryFrame(kid);
> +  nsSVGDisplayableFrame* displayable = do_QueryFrame(kid);

displayableChild

::: layout/svg/nsSVGUtils.cpp:364
(Diff revision 1)
>  
>  nsIFrame*
>  nsSVGUtils::GetOuterSVGFrameAndCoveredRegion(nsIFrame* aFrame, nsRect* aRect)
>  {
> -  nsSVGDisplayableFrame* svg = do_QueryFrame(aFrame);
> -  if (!svg)
> +  nsSVGDisplayableFrame* displayable = do_QueryFrame(aFrame);
> +  if (!displayable)

I'd call this displayableSVGFrame.

::: layout/svg/nsSVGUtils.cpp:418
(Diff revision 1)
>  }
>  
>  gfxMatrix
>  nsSVGUtils::GetUserToCanvasTM(nsIFrame *aFrame)
>  {
> -  nsSVGDisplayableFrame* svgFrame = do_QueryFrame(aFrame);
> +  nsSVGDisplayableFrame* displayable = do_QueryFrame(aFrame);

Same.

::: layout/svg/nsSVGUtils.cpp:435
(Diff revision 1)
>  
>  void
>  nsSVGUtils::NotifyChildrenOfSVGChange(nsIFrame *aFrame, uint32_t aFlags)
>  {
>    for (nsIFrame* kid : aFrame->PrincipalChildList()) {
> -    nsSVGDisplayableFrame* SVGFrame = do_QueryFrame(kid);
> +    nsSVGDisplayableFrame* displayable = do_QueryFrame(kid);

Back to displayableChild.

::: layout/svg/nsSVGUtils.cpp:461
(Diff revision 1)
>    virtual void Paint(gfxContext& aContext, nsIFrame *aTarget,
>                             const gfxMatrix& aTransform,
>                             const nsIntRect* aDirtyRect,
>                             imgDrawingParams& aImgParams) override
>    {
> -    nsSVGDisplayableFrame* svgFrame = do_QueryFrame(aTarget);
> +    nsSVGDisplayableFrame* displayable = do_QueryFrame(aTarget);

displayableFrame

::: layout/svg/nsSVGUtils.cpp:671
(Diff revision 1)
>                 (aFrame->GetStateBits() & NS_FRAME_IS_NONDISPLAY) ||
>                 aFrame->PresContext()->IsGlyph(),
>                 "If display lists are enabled, only painting of non-display "
>                 "SVG should take this code path");
>  
> -  nsSVGDisplayableFrame* svgFrame = do_QueryFrame(aFrame);
> +  nsSVGDisplayableFrame* displayable = do_QueryFrame(aFrame);

Same.

::: layout/svg/nsSVGUtils.cpp:931
(Diff revision 1)
>    // the topmost frame that intersects the point; then we can just return it.
>    nsIFrame* result = nullptr;
>    for (nsIFrame* current = aFrame->PrincipalChildList().LastChild();
>         current;
>         current = current->GetPrevSibling()) {
> -    nsSVGDisplayableFrame* SVGFrame = do_QueryFrame(current);
> +    nsSVGDisplayableFrame* displayable = do_QueryFrame(current);

displayableChild

::: layout/svg/nsSVGUtils.cpp:1835
(Diff revision 1)
>  
>  void
>  nsSVGUtils::PaintSVGGlyph(Element* aElement, gfxContext* aContext)
>  {
>    nsIFrame* frame = aElement->GetPrimaryFrame();
> -  nsSVGDisplayableFrame* svgFrame = do_QueryFrame(frame);
> +  nsSVGDisplayableFrame* displayable = do_QueryFrame(frame);

displayableFrame I think.