Closed
Bug 1264784
Opened 8 years ago
Closed 8 years ago
remove or devirtualize some nsIFrame methods
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
Details
Attachments
(3 files, 1 obsolete file)
5.55 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
6.25 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
1.41 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8741503 -
Flags: review?(dholbert)
Comment 2•8 years ago
|
||
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 3•8 years ago
|
||
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.
Comment 6•8 years ago
|
||
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.
Assignee | ||
Comment 7•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
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)
Assignee | ||
Comment 9•8 years ago
|
||
Nothing overrides these methods, and making them non-virtual reduces vtable sizes and improves code size and performance.
Attachment #8741865 -
Flags: review?(dholbert)
Assignee | ||
Updated•8 years ago
|
Attachment #8741503 -
Attachment is obsolete: true
Attachment #8741503 -
Flags: review?(dholbert)
Updated•8 years ago
|
Attachment #8741863 -
Flags: review?(dholbert) → review+
Updated•8 years ago
|
Attachment #8741864 -
Flags: review?(dholbert) → review+
Updated•8 years ago
|
Attachment #8741865 -
Flags: review?(dholbert) → review+
Updated•8 years ago
|
Summary: devirtualize some nsIFrame methods → remove or devirtualize some nsIFrame methods
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f9e86886f0f https://hg.mozilla.org/integration/mozilla-inbound/rev/380bf3456c3b https://hg.mozilla.org/integration/mozilla-inbound/rev/f5fbeeb2be69
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4f9e86886f0f https://hg.mozilla.org/mozilla-central/rev/380bf3456c3b https://hg.mozilla.org/mozilla-central/rev/f5fbeeb2be69
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•