1459034 - devirtualize some functions in nsIFrame

Status: RESOLVED INCOMPLETE

(Core :: Layout, enhancement, P3)

Description

[Nathan Froyd [:froydnj]]

This saves a small amount of vtable space and a decent-ish amount of text size,
because subclasses no longer need forwarding thunks.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - devirtualize nsIFrame::IsFocusable

nsPluginFrame is the lone overrider of this method, but its
implementation duplicates work done by the primary implementation, and
calls through to the primary implementation anyway.  There's no need for
this method to be virtual.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - devirtualize nsIFrame::GetContentOffsetsFromPointExternal

The default implementation of this function just forwards to
GetContentOffsetsFromPoint, and there are no subclasses that override
this function.  I believe the "External" prefix hearkens back to a day
when bits of layout lived in a separate library from everything else,
but that day is long gone.  Both reasons are sufficient motivation to
devirtualize this function.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - devirtualize nsIFrame::GetSelectionController

nsFrame provides the lone implementation of this method, and there are
no other overrides.  We might as well move its implementation to
nsIFrame and devirtualize it.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 4 - devirtualize nsIFrame::GetLogicalUsed{Margin,Border,Padding}

Nothing overrides these default implementations, and the underlying
GetUsed* methods they call are already virtual.  So a subclass needing
to override the GetLogicalUsed* variants seems rather unlikely.

Comment 5

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

Comment on attachment 8973024 [details] [diff] [review]
part 1 - devirtualize nsIFrame::IsFocusable

I think the commit message should be clearer that you're *removing* the nsPluginFrame override because the override is no longer needed.

The code looks good, though.

(dholbert's away, and since I looked, I may as well review this)

Comment 6

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

Comment on attachment 8973025 [details] [diff] [review]
part 2 - devirtualize nsIFrame::GetContentOffsetsFromPointExternal

Yeah, this probably dated back to when accessibility was in a different library from layout.

Comment 7

[Mats Palmgren (inactive)]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> Nothing overrides these default implementations, and the underlying
> GetUsed* methods they call are already virtual.  So a subclass needing
> to override the GetLogicalUsed* variants seems rather unlikely.

IIRC, the idea was that the classes that override the GetUsed*
methods would be changed to instead override GetLogicalUsed*
so that we could minimize the use of, or remove, the physical
variants eventually.  I suspect that these days the logical
variants are used more than the physical ones.

There are also some cases where we first compute the result in
logical coordinates, then convert it to physical, and then back to
logical in GetLogicalUsed*.  e.g.
https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/layout/tables/nsTableFrame.cpp#2866

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Mats Palmgren (:mats) from comment #7)
> (In reply to Nathan Froyd [:froydnj] from comment #4)
> > Nothing overrides these default implementations, and the underlying
> > GetUsed* methods they call are already virtual.  So a subclass needing
> > to override the GetLogicalUsed* variants seems rather unlikely.
> 
> IIRC, the idea was that the classes that override the GetUsed*
> methods would be changed to instead override GetLogicalUsed*
> so that we could minimize the use of, or remove, the physical
> variants eventually.  I suspect that these days the logical
> variants are used more than the physical ones.
> 
> There are also some cases where we first compute the result in
> logical coordinates, then convert it to physical, and then back to
> logical in GetLogicalUsed*.  e.g.
> https://searchfox.org/mozilla-central/rev/
> c0d81882c7941c4ff13a50603e37095cdab0d1ea/layout/tables/nsTableFrame.cpp#2866

Thanks for the explanation here; I am woefully unfamiliar with layout code, and the history here is helpful.

For avoidance of doubt, are you saying:

a) The patch should not be landed;
b) The patch can be landed, but somebody will probably undo its effects later as part of a (step-wise) refactoring; or
c) The patch is a good idea, but should really be accomplished by removing the physical variants and making the overrides work with logical coordinates instead, and should not be landed in its current state.

?

Comment 9

[Mats Palmgren (inactive)]

Sorry for the confusion, please land it.

I suspect somebody will probably switch the roles of GetLogicalUsed*/GetUsed*
eventually though, in terms of which should be virtual and overridable.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:froydnj, could you have a look please?