384507 - nsContentSink.h contains static non-member functions and globals.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Johnny Stenback (:jst)]

Bug 340554 added:

static nsIAtom** const kDefaultAllowedTags [] ...
static nsIAtom** const kDefaultAllowedAttributes [] ...

and

static
PRBool IsAttrURI(nsIAtom *aName)
{
  ...
}

to nsContentSink.h. That means that every file that includes nsContentSink.h gets its own copy of those globals and that function. The linker *may* be smart enough to eliminate the duplicates, but we really don't want those there.

The function could be a non-static inline function, or a non-static function defined elsewhere, and the globals should also be elsewhere (read nsContentSink.cpp), and the header file should only have extern declarations of the above.

Robert, since you introduced this, I'm giving you this bug :)

Comment 1

[Robert Sayre]

Attachment: like so?

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 268836 [details] [diff] [review]
like so?

+extern nsIAtom** const kDefaultAllowedTags [74];
+extern nsIAtom** const kDefaultAllowedAttributes [76];

The size of the arrays shouldn't need to be declared here, I'd hate to have to keep those in sync with the actual lists. How about loosing the size declarations and the code that depends on it, and null terminating the lists instead?

r- based on that, but if that's not an option I could be swayed the other way :)

Comment 3

[Robert Sayre]

Attachment: null terminate

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 268860 [details] [diff] [review]
null terminate

+nsIAtom** const kDefaultAllowedTags [] = {
+  &nsGkAtoms::a,
+  &nsGkAtoms::abbr,
[...]
+  &nsGkAtoms::var,
+  '\0'
+};

Use nsnull instead of '\0' here, and in kDefaultAllowedAttributes.

r+sr=jst

Comment 5

[Robert Sayre]

Checked in. This did reduce codesize.