1128356 - Add templates to eliminate boilerplate code for frame property destructors

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Virtually all frame property destructors either call |delete| or |Release()| to destroy the frame property value. We can get rid of a lot of boilerplate and make it easier to add new frame properties by adding some template functions that handle these common cases.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Add template functions for common frame property destructors

Here's the patch. The actual new template functions live in nsIFrame.h; the rest of the patch just updates everything to use them.

Comment 2

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

Comment on attachment 8557677 [details] [diff] [review]
Add template functions for common frame property destructors

Maybe the templates should be static members of nsIFrame, or, if not,
should be in the mozilla namespace?  I don't have a strong opinion on
this, I guess.

Did you check that the size of the compiled code is roughly the same?

(A few typedefs for some of the repeated types might also be nice,
especially if they are used consistently for all relevant uses.)

r=dbaron with that

Comment 3

[Seth Fowler [:seth] [:s2h]]

Thanks for the review!

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #2)
> Maybe the templates should be static members of nsIFrame, or, if not,
> should be in the mozilla namespace?  I don't have a strong opinion on
> this, I guess.

I originally had them as static members of nsIFrame, but this required that you write nsIFrame::DeleteValue or the like in many places, which I found too verbose. We can definitely put them in namespace mozilla, though.

> Did you check that the size of the compiled code is roughly the same?

It should be pretty much identical; there's a one-to-one mapping between the old manually written implementations and the instantiations of the new template-based ones.

Comment 4

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

(In reply to Seth Fowler [:seth] from comment #3)
> > Did you check that the size of the compiled code is roughly the same?
> 
> It should be pretty much identical; there's a one-to-one mapping between the
> old manually written implementations and the instantiations of the new
> template-based ones.

I think it's worth double-checking, just to make sure something strange didn't happen...

Comment 5

[Seth Fowler [:seth] [:s2h]]

OS X XUL size before the patch: 121,478,208 bytes.
OS X XUL size after the patch: 121,479,256 bytes.

A difference of 1KB. I'm pretty sure this is mostly the cost of the symbol names, since symbol names for templates are quite verbose:

> > nm -j obj/toolkit/library/XUL | ag '(Delete|Release)Value' | wc -c
> 1220

Looks about the right ballpark to me.

Comment 6

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] from comment #3)
> We can definitely put them in namespace mozilla, though.

Or not! Just tried this. It turns out that surprisingly many things in layout, including nsIFrame, are not in namespace mozilla. Since these templates are used most of the time in header files, where we can't use 'using', it'd make things significantly more verbose to put it there. Sigh. =(

Comment 7

[Seth Fowler [:seth] [:s2h]]

I went ahead and pushed:

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

It'd be nice to get more things in layout in namespace mozilla. Perhaps next time I find some time for quick refactoring bug I'll trying to move a big chunk of layout in there.

Comment 8

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=6200320&repo=mozilla-inbound

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: Add template functions for common frame property destructors

Sigh. Visual Studio 2013 and we still need to work around MSVC-only compilers bugs. Here's an updated version that doesn't anger MSVC.

Pushed again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/429f9e69e6db

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/429f9e69e6db