get rid of a bunch of static constructors in dom/ and content/

RESOLVED FIXED in mozilla24

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 756232 [details] [diff] [review]
get rid of a number of static constructors in content/ and dom/

unfortunately constexpr doesn't fix the svg stuff because register an
at exit  handler is required for the hash tables, and for the vtable for
the singletons but I didn't look very closely.
Attachment #756232 - Flags: review?(bugs)
(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> Created attachment 756232 [details] [diff] [review]
> get rid of a number of static constructors in content/ and dom/
> 
> unfortunately constexpr (...)

And constexpr is C++11.
(Assignee)

Comment 3

5 years ago
(In reply to Mike Hommey (high latency until June 25) [:glandium] from comment #2)
> (In reply to Trevor Saunders (:tbsaunde) from comment #1)
> > Created attachment 756232 [details] [diff] [review]
> > get rid of a number of static constructors in content/ and dom/
> > 
> > unfortunately constexpr (...)
> 
> And constexpr is C++11.

true, but what official builds do we do that don't compile this code in c++11 mode?
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> true, but what official builds do we do that don't compile this code in
> c++11 mode?

What the word "this code" refers? The attached patch doesn't use constexpr.
(Assignee)

Comment 5

5 years ago
(In reply to Masatoshi Kimura [:emk] from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > true, but what official builds do we do that don't compile this code in
> > c++11 mode?
> 
> What the word "this code" refers? The attached patch doesn't use constexpr.

content/ and dom/
The ChildIterator bits of this patch snuck in from the xbl stuff mrbkap/enn/wchen are doing, right?
And the XBLChildrenElement bits.
(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> (In reply to Masatoshi Kimura [:emk] from comment #4)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > > true, but what official builds do we do that don't compile this code in
> > > c++11 mode?
> > 
> > What the word "this code" refers? The attached patch doesn't use constexpr.
> 
> content/ and dom/

I'm not sure why it has to do with constexpr.
(Assignee)

Comment 9

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #7)
> And the XBLChildrenElement bits.

right,  I'd applied  that patch a while ago git reset --hard to get rid of it, but that didn't get rid of untracked files, then blindly git add content/ to commit this patch but not some other stuff in that tree.


(In reply to Masatoshi Kimura [:emk] from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > (In reply to Masatoshi Kimura [:emk] from comment #4)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > > > true, but what official builds do we do that don't compile this code in
> > > > c++11 mode?
> > > 
> > > What the word "this code" refers? The attached patch doesn't use constexpr.
> > 
> > content/ and dom/
> 
> I'm not sure why it has to do with constexpr.

only because constexpr sometimes is enough to convince gcc to not be stupid and create static initializers where they aren't actually needed.
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> true, but what official builds do we do that don't compile this code in
> c++11 mode?

We still support building on Linux distros with gcc 4.4, and they don't build in C++11. We should keep it that way until (and including) 24.
(Assignee)

Comment 11

5 years ago
(In reply to Mike Hommey (high latency until June 25) [:glandium] from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > true, but what official builds do we do that don't compile this code in
> > c++11 mode?
> 
> We still support building on Linux distros with gcc 4.4, and they don't
> build in C++11. We should keep it that way until (and including) 24.

oh, by constexpr I really meant MOZ_CONSTEXPR so we'd still support  compiling without constexpr but you'd get a bunch of static initializers you wouldn't otherwise.
Comment on attachment 756232 [details] [diff] [review]
get rid of a number of static constructors in content/ and dom/

Any chance to get a patch which is actually for the current m-c or m-i
Attachment #756232 - Flags: review?(bugs)
(Assignee)

Comment 13

5 years ago
Created attachment 756586 [details] [diff] [review]
bug 877886 - get rid of a number of static constructors in content/ and dom/

rm'd the extra files and rebased to tip inbound
Attachment #756586 - Flags: review?(bugs)
Comment on attachment 756586 [details] [diff] [review]
bug 877886 - get rid of a number of static constructors in content/ and dom/


> static float kDefaultFontSize = 10.0;
>-static NS_NAMED_LITERAL_STRING(kDefaultFontName, "sans-serif");
>-static NS_NAMED_LITERAL_STRING(kDefaultFontStyle, "10px sans-serif");

Could you move also kDefaultFontSize to the method where it is used.
Or perhaps #define DEFAULT_FONT_NAME and DEFAULT_FONT_STYLE here and use them
in ::GetCurrentFontStyle. I think I'd prefer the latter approach.
>   // Singleton for nsSMILValue objects to hold onto.
>-  static nsSMILFloatType sSingleton;
>+  static nsSMILFloatType&
>+    Singleton()
>+    {
>+      static nsSMILFloatType sSingleton;
>+      return sSingleton;
>+    }
Fix indentation.

Couldn't you return pointer, not reference in each these Singletons.
There wouldn't be need to & the return value.


>-static nsSVGAttrTearoffTable<SVGAnimatedLengthList, DOMSVGAnimatedLengthList>
>-  sSVGAnimatedLengthListTearoffTable;
>+static inline
>+nsSVGAttrTearoffTable<SVGAnimatedLengthList, DOMSVGAnimatedLengthList>&
>+TearoffTable()

>+nsSVGAttrTearoffTable<SVGAnimatedNumberList, DOMSVGAnimatedNumberList>&
>+TearoffTable()

I'd prefer to not have TearOffTable() in several compilation units.
Could you name them more precisely. 


>+nsSVGAttrTearoffTable<void, DOMSVGPathSegList>&
>+TearoffTable()
Ditto

>+nsSVGAttrTearoffTable<void, DOMSVGPointList>&
>+TearoffTable()
ditto

>+static inline
>+nsSVGAttrTearoffTable<SVGStringList, DOMSVGStringList>&
>+TearoffTable()
ditto

>-  static SVGIntegerPairSMILType sSingleton;
>+  static SVGIntegerPairSMILType&
>+  Singleton()
>+  {
>+    static SVGIntegerPairSMILType sSingleton;
>+  return sSingleton;
missing 2 spaces before return.


>+static nsSVGAttrTearoffTable<SVGTransform, SVGMatrix>&
>+TearoffTable()
rename TearoffTable() here too

>+nsSVGAttrTearoffTable<nsSVGString, nsSVGString::DOMAnimatedString>&
>+TearoffTable()
and here

>+++ b/content/xbl/src/nsXBLChildrenElement.cpp
This is not about this bug
Attachment #756586 - Flags: review?(bugs) → review+
(Assignee)

Comment 15

5 years ago
(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 756586 [details] [diff] [review]
> bug 877886 - get rid of a number of static constructors in content/ and dom/
> 
> 
> > static float kDefaultFontSize = 10.0;
> >-static NS_NAMED_LITERAL_STRING(kDefaultFontName, "sans-serif");
> >-static NS_NAMED_LITERAL_STRING(kDefaultFontStyle, "10px sans-serif");
> 
> Could you move also kDefaultFontSize to the method where it is used.
> Or perhaps #define DEFAULT_FONT_NAME and DEFAULT_FONT_STYLE here and use them
> in ::GetCurrentFontStyle. I think I'd prefer the latter approach.

yeah, macros might be the right approach here

> >   // Singleton for nsSMILValue objects to hold onto.
> >-  static nsSMILFloatType sSingleton;
> >+  static nsSMILFloatType&
> >+    Singleton()
> >+    {
> >+      static nsSMILFloatType sSingleton;
> >+      return sSingleton;
> >+    }
> Fix indentation.
> 
> Couldn't you return pointer, not reference in each these Singletons.
> There wouldn't be need to & the return value.

sgtm, I didn't realize people always just wanted the address until the patch was written.

> 
> >-static nsSVGAttrTearoffTable<SVGAnimatedLengthList, DOMSVGAnimatedLengthList>
> >-  sSVGAnimatedLengthListTearoffTable;
> >+static inline
> >+nsSVGAttrTearoffTable<SVGAnimatedLengthList, DOMSVGAnimatedLengthList>&
> >+TearoffTable()
> 
> >+nsSVGAttrTearoffTable<SVGAnimatedNumberList, DOMSVGAnimatedNumberList>&
> >+TearoffTable()
> 
> I'd prefer to not have TearOffTable() in several compilation units.
> Could you name them more precisely. 

how does SVGWhateverTearoffTable() sound?

> >+++ b/content/xbl/src/nsXBLChildrenElement.cpp
> This is not about this bug

err, sorry I forget to rm that as well.
(Assignee)

Comment 16

5 years ago
Created attachment 758188 [details] [diff] [review]
bug 877886 - get rid of a number of static constructors in content/ and dom/

probably can't hurt to check I did fix everything you wanted improved and didn't add anything bad.
Attachment #758188 - Flags: review?(bugs)
(Assignee)

Comment 17

5 years ago
Created attachment 758709 [details] [diff] [review]
bug 877886 - get rid of a number of static constructors in content/ and dom/

I forgot to fix up the canvas thing before.  I went for moving kDefaultFontSize
instead of using #defines because a define that expands to
NS_NAMED_LITERAL_STRING doesn't really work in multiple places and I'm not sure
if we care about how how fast that function is.
Attachment #756232 - Attachment is obsolete: true
Attachment #756586 - Attachment is obsolete: true
Attachment #758188 - Attachment is obsolete: true
Attachment #758188 - Flags: review?(bugs)
Attachment #758709 - Flags: review?(bugs)
Comment on attachment 758709 [details] [diff] [review]
bug 877886 - get rid of a number of static constructors in content/ and dom/

Looks like there are still few Singleton methods returning &, but * should work just fine. With that, r=me
Attachment #758709 - Flags: review?(bugs) → review+
(Assignee)

Comment 20

5 years ago
somehow my local builds didn't catch a couple more calls to Singleton() that need updated
(Assignee)

Comment 21

5 years ago
Comment on attachment 758709 [details] [diff] [review]
bug 877886 - get rid of a number of static constructors in content/ and dom/

> static void
> NSResultToNameAndMessage(nsresult aNSResult,
>                          const char** aName,
>                          const char** aMessage,
>                          uint16_t* aCode)
> {
>   *aName = nullptr;
>   *aMessage = nullptr;
>   *aCode = 0;
>-  ResultStruct* result_struct = gDOMErrorMsgMap;
> 
>-  while (result_struct->mName) {
>-    if (aNSResult == result_struct->mNSResult) {
>-      *aName = result_struct->mName;
>-      *aMessage = result_struct->mMessage;
>-      *aCode = result_struct->mCode;
>+#define DOM4_MSG_DEF(name, message, nsresult) {(nsresult), name, #name, message},
>+#define DOM_MSG_DEF(val, message) {(val), NS_ERROR_GET_CODE(val), #val, message},
>+
>+  static const struct ResultStruct
>+  {
>+    nsresult mNSResult;
>+    uint16_t mCode;
>+    const char* mName;
>+    const char* mMessage;
>+  } sDOMErrorMsgMap[] = {
>+#include "domerr.msg"
>+  };

this actually needs to stay out side the function because gcc 4.4 is dumb, ok?
https://hg.mozilla.org/mozilla-central/rev/9e310bf47f50
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.