Closed Bug 1264784 Opened 8 years ago Closed 8 years ago

remove or devirtualize some nsIFrame methods

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(3 files, 1 obsolete file)

Having virtual methods in a base class that nobody overrides is
moderately expensive in binary size, both from the storage required in
vtables and the extra code to call virtual methods.  And nsIFrame has a
lot of subclasses, which makes us pay the storage costs over and over
and over again.  Making them non-virtual won't be an enormous space
improvement, but every little bit helps.
Attachment #8741503 - Flags: review?(dholbert)
Comment on attachment 8741503 [details] [diff] [review]
devirtualize some nsIFrame methods

>+++ b/layout/generic/nsIFrame.h
>   nsView* GetView() const;
>-  virtual nsView* GetViewExternal() const;
>+  nsView* GetViewExternal() const;
[...]
>   nsIFrame* GetAncestorWithView() const;
>-  virtual nsIFrame* GetAncestorWithViewExternal() const;
>+  nsIFrame* GetAncestorWithViewExternal() const;
[...]
>   nsPoint GetOffsetTo(const nsIFrame* aOther) const;
>-  virtual nsPoint GetOffsetToExternal(const nsIFrame* aOther) const;
>+  nsPoint GetOffsetToExternal(const nsIFrame* aOther) const;


In all of these "Foo" / "FooExternal" pairs here (which make up the first ~half of this patch), I believe the FooExternal() ones are *explicitly* required to be virtual, so they can be called from external libraries, or something like that. I forget the details. But I do recall that their virtualness is a necessary part of their usefulness.

BUT, I'm not actually sure these FooExternal functions are needed at all anymore, now that we don't support binary add-ons (which maybe is what used them? I'm not sure.)  If that's the case, we should get rid of them, rather than devirtualizing them.  That should be done separately, so regardless, they should be stripped out of this patch.  (and maybe removed in a separate patch, though I'd check with dbaron to see if he thinks they're removable)
Comment on attachment 8741503 [details] [diff] [review]
devirtualize some nsIFrame methods

Review of attachment 8741503 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsIFrame.h
@@ +2929,5 @@
>  
>    nsresult Redraw(nsBoxLayoutState& aState);
>    virtual nsresult RelayoutChildAtOrdinal(nsIFrame* aChild)=0;
>    // XXX take this out after we've branched
> +  bool GetMouseThrough() const { return false; }

This function should just be removed, per its XXX comment. There are zero calls to it, AFAICT.

(There is a function by the same name in nsDisplayList, which has one call -- but that's distinct from this function.)
If the *External methods are no longer used outside of libxul, we should just remove them.  If we have callers inside of libxul, we should make them use the non-External versions.

(We also used to have a way of automatically calling the right one based on an #ifdef that was defined for everything in libxul... but that seems gone.)

Also, the one that isn't a *External method says "XXX take this out after we've branched"!
I guess nsIFrame.h now has this at the top:

#ifndef MOZILLA_INTERNAL_API
#error This header/class should only be used within Mozilla code. It should not be used by extensions.
#endif

which I think  says that we should be able to remove all the *External methods.
Cool, OK. I'd suggest splitting this bug into two parts:

* one patch to completely remove a bunch of now-useless virtual methods -- the *External() stuff and GetMouseThrough().

* another patch to de-virtualize the GetNearestWidget() methods (as in your original patch here)

And I think that leaves us in a good state w.r.t. all of the methods your original patch touched.
The latter functions just call through to the former functions, and
we're going to remove the latter functions in a separate, subsequent
patch.
Attachment #8741863 - Flags: review?(dholbert)
There are no callers of these methods in mozilla-central, and having
them in the vtable of nsIFrame (and its 100+ subclasses!) consumes
needless space.
Attachment #8741864 - Flags: review?(dholbert)
Nothing overrides these methods, and making them non-virtual reduces
vtable sizes and improves code size and performance.
Attachment #8741865 - Flags: review?(dholbert)
Attachment #8741503 - Attachment is obsolete: true
Attachment #8741503 - Flags: review?(dholbert)
Attachment #8741863 - Flags: review?(dholbert) → review+
Attachment #8741864 - Flags: review?(dholbert) → review+
Attachment #8741865 - Flags: review?(dholbert) → review+
Summary: devirtualize some nsIFrame methods → remove or devirtualize some nsIFrame methods
You need to log in before you can comment on or make changes to this bug.