1260090 - Remove nsBlockFrameSuper as a nsContainerFrame alias

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Many nsBlockFrame methods call its parent method by nsContainerFrame prefix [1]. I feel nsBlockFrameSuper might no longer valid everywhere. 

[1] https://dxr.mozilla.org/mozilla-central/rev/63be002b4a803df1122823841ef7633b7561d873/layout/generic/nsBlockFrame.cpp#547,554

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: MozReview Request: Bug 1260090 - Remove nsBlockFrameSuper as nsContainerFrame alias. r=dholbert

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

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why not go the other direction, and use nsBlockFrameSuper more reliably?

I generally like having this typedef when reading code, because it makes it easier to tell at a glance that the code is calling the correct base class method rather than skipping an intermediate class.  Then again, maybe it's an odd convention that we should get rid of.

Comment 3

[Daniel Holbert [:dholbert]]

I know some reviewer (mats perhaps?) has advocated against the "FooSuper" typedef convention, in reviews of my patches in the past.  I'm inclined to agree that its usefulness doesn't actually merit the extra abstraction & reduced greppability that it introduces.

Examples of reduced greppability:
 - If you want to find all explicit calls to nsContainerFrame::Foo(), you have to also search for ns[EverySubclassFrame]Super::Foo() as well
 - If you run into a nsBlockFrameSuper::Bar() call and grep for that, you'll find no definition. (You have to either just know what the parent class is up-front, or do a two-part grep to discover what the superclass is and then search for its function-impl.)

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8735378 [details]
MozReview Request: Bug 1260090 - Remove nsBlockFrameSuper as nsContainerFrame alias. r=dholbert

https://reviewboard.mozilla.org/r/42761/#review39217

I'll mark this r=me, with one nit (below), though I'll tag it for feedback from dbaron to be sure he's all right with this in general.

::: layout/generic/nsBlockFrame.cpp:2919
(Diff revision 1)
>  nsBlockFrame::AttributeChanged(int32_t         aNameSpaceID,
>                                 nsIAtom*        aAttribute,
>                                 int32_t         aModType)
>  {
> -  nsresult rv = nsBlockFrameSuper::AttributeChanged(aNameSpaceID,
> +  nsresult rv = nsContainerFrame::AttributeChanged(aNameSpaceID,
>                                                      aAttribute, aModType);

Please fix indendation in this contextual line (remove 1 space to keep it aligned with the previous line, which you're changing).

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8735378 [details]
MozReview Request: Bug 1260090 - Remove nsBlockFrameSuper as nsContainerFrame alias. r=dholbert

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

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

https://reviewboard.mozilla.org/r/42761/#review39217

> Please fix indendation in this contextual line (remove 1 space to keep it aligned with the previous line, which you're changing).

Fixed.

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> I know some reviewer (mats perhaps?) has advocated against the "FooSuper"
> typedef convention, in reviews of my patches in the past.  I'm inclined to
> agree that its usefulness doesn't actually merit the extra abstraction &
> reduced greppability that it introduces.

FWIW, I had removed the typedef of the parent class of nsTextFrame in bug 1251519 Part 1. Here's the mats comment about it. https://bugzilla.mozilla.org/show_bug.cgi?id=1251519#c9

I agree with him that someone might accidentally broke the conversion when calling parent methods unless he/she is aware of the typedef of "FooSuper".

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Personally, I still prefer using the form FrameSuper, and make it just behave like Java's keyword "super"...

Extra abstraction is a non-issue. This abstraction does improve the convenience to change a super class of a frame, if all people follow the convention to use this alias. Using the concrete class name everywhere makes doing that error prone.

The reduction of greppability doesn't really convince me. Virtual function is hard to grep by its nature, while callsites of non-virtual functions can easily be found via any reasonably modern static analysis tool, including our DXR and various IDEs.

Comment 9

[Mats Palmgren (inactive)]

 (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #8)
> This abstraction does improve the
> convenience to change a super class of a frame,

That happens extremely rarely in my experience, like once in
a decade or so (except maybe for trivial classes but those
rarely have explicit base class calls).

> if all people follow the convention to use this alias.

They won't.  Errare humanum est.

> Using the concrete class name everywhere makes
> doing that error prone.

I think you got this backwards.  If we adopt this convention then
everyone will assume they can just change the typedef to use
a different base class and everything will just work.  This is
a bad assumption.

> The reduction of greppability doesn't really convince me.

It *should* convince you because that's a very common use case, unlike
changing the base class.  Also, as someone who has refactored Frame
class methods a few times I can tell you that these typedefs made that
harder.

I'm against "FooSuper" typedefs because:

1. it's adds cognitive load compared to simply using the base class name
2. it makes reading the code harder, e.g. if you see FooSuper::Bar() and
   want to see the code for it, then you have to first figure out what
   FooSuper is to know which .cpp file to load
3. everyone, including reviewers, needs to be constantly aware of this
   convention to prevent introducing uses of concrete base class names
   (and missing one might cause an exploitable bug in the future)
4. it makes grepping harder, and this is a very very common use case

The idea that it makes changing base class "convenient" is a mistake
because, to be safe, you need to search for existing uses of the old
base class name anyway, because inevitably someone failed on point 3
above (this has happened).  It's a *good* thing that you (and the
reviewer) will look at each call site because it makes it less likely
that such dangerous errors slip through.

And again, but it's worth repeating, changing the base class for
a Frame class is extremely rare.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Examples of reduced greppability:
>  - If you want to find all explicit calls to nsContainerFrame::Foo(), you
> have to also search for ns[EverySubclassFrame]Super::Foo() as well

I don't think this point matters.  You have to search for [EverySubclassFrame]::Foo() as well, even without the Super convention, since there may be subclasses that don't define Foo().  For example, given:
  nsBlockFrame derived from nsContainerFrame derived from nsSplittableFrame derived from nsFrame

If nsBlockFrame were to override, say, GetPrevInFlow, a call in nsBlockFrame::GetPrevInFlow to call the base class method should be written as nsContainerFrame::GetPrevInFlow, even though that method doesn't actually exist, and it's really a call to nsSplittableFrame::GetPrevInFlow.

That, I think, brings up the actual worry, which is that in such a situation, somebody might be tempted to write nsSplittableFrame::GetPrevInFlow because that's the thing that exists.  This would be problematic if somebody later introduced nsContainerFrame::GetPrevInFlow.

This pattern exists today in nsBlockFrame::GetLogicalBaseline, 

>  - If you run into a nsBlockFrameSuper::Bar() call and grep for that, you'll
> find no definition. (You have to either just know what the parent class is
> up-front, or do a two-part grep to discover what the superclass is and then
> search for its function-impl.)

This isn't, in general, the result of the *Super convention, for the same reason as my previous point.  The real definition might be further up.



I think the real points are about consistency, both within the project, and with other C++ projects.  I think this convention is unusual, and that points towards getting rid of it.  So I'm ok with removing it -- but we should remove it throughout layout, and not just from this one class.  So I'm ok with the change if you continue the cleanup to remove the convention from other classes in layout.

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Filed bug 1264837 for removing the convention from all the class in layout.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6677278a282

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c6677278a282