Closed Bug 1400777 Opened 3 years ago Closed 3 years ago

Slim down nsElementTable.h

Categories

(Core :: DOM: HTML Parser, enhancement, P3)

56 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(5 files)

It has way more stuff exposed than necessary.
Priority: -- → P3
It's just a too-cute-by-half typedef for nsHTMLTag.
Attachment #8909554 - Flags: review?(mrbkap)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
The patch also removes some low-value comments.
Attachment #8909555 - Flags: review?(mrbkap)
This patch splits out most of nsHTMLElement into a new type HTMLElement within
nsElementTable.cpp. Only the static methods IsContainer() and IsBlock() need to
remain exposed via nsHTMLElement. The patch moves TestBits() into
nsElementTable.cpp as well.
Attachment #8909557 - Flags: review?(mrbkap)
This patch makes some style fixes and other minor improvements.
Attachment #8909558 - Flags: review?(mrbkap)
Attachment #8909554 - Flags: review?(mrbkap) → review+
Comment on attachment 8909555 [details] [diff] [review]
(part 2) - De-expose HTML group constants[]

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

::: parser/htmlparser/nsElementTable.cpp
@@ +13,5 @@
> +static const int kHTMLContent   = 0x0001; //  HEAD, (FRAMESET | BODY)
> +static const int kHeadContent   = 0x0002; //  Elements that *must* be in the head.
> +static const int kHeadMisc      = 0x0004; //  Elements that *can* be in the head.
> +
> +static const int kSpecial       = 0x0008; //  A,    IMG,  APPLET, OBJECT, FONT, BASEFONT, BR, SCRIPT, 

Nit: a couple of these lines came with trailing whitespace.
Attachment #8909555 - Flags: review?(mrbkap) → review+
Attachment #8909556 - Flags: review?(mrbkap) → review+
Comment on attachment 8909557 [details] [diff] [review]
(part 4) - Split nsHTMLElement

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

::: parser/htmlparser/nsElementTable.cpp
@@ +52,2 @@
>  
> +struct HTMLElement

Is it worth it to stick this in an anonymous namespace to make it "static"?
Attachment #8909557 - Flags: review?(mrbkap) → review+
Attachment #8909558 - Flags: review?(mrbkap) → review+
> > +struct HTMLElement
> 
> Is it worth it to stick this in an anonymous namespace to make it "static"?

What effect do anonymous namespace have on struct definitions? I tried to look this up but got inconclusive info.
Flags: needinfo?(mrbkap)
(In reply to Nicholas Nethercote [:njn] from comment #9)
> What effect do anonymous namespace have on struct definitions? I tried to
> look this up but got inconclusive info.

They have the effect of not exporting the name into the global namespace (so act as "static" for the struct name) ensuring no name clashes and one symbol less for the linker to care about [1].

[1] http://en.cppreference.com/w/cpp/language/namespace#Unnamed_namespaces
Flags: needinfo?(mrbkap)
You need to log in before you can comment on or make changes to this bug.