inline WebCore::Node::Node constructor more safely

inline WebCore::Node::Node constructor more safely

Recently, some toolchains started giving link errors due to WebCore::Node::Node
being undefined in libwebkit.so.  I suspect this is due to incomplete inlining,
since ContainerNode.h does not include Document.h (where the inlining was
defined).

This fix changes the Document argument to the Node constructor to a TreeScope,
which is the sole purpose of this argument, and since Node isn't a TreeScope
there's no circular dependency to worry about forward declarations.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151791

[Mostyn Bramley-Moore, 2013-06-04 21:49:29]

Adding Eric for review since he commented on the patch that introduced this
inlining:
https://bugs.webkit.org/show_bug.cgi?id=38682

And Dimitri since he was the last person to review changes to Node.h (and I'm
not sure how to find more specific reviewers for blink yet).

Any opinions on this?  Would it be preferable to just de-inline this
constructor?  The style guide suggests that this would be preferable, but I'm
not sure if there are performance implications in this case.
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in....

[adamk, 2013-06-04 23:08:08]

I'm not familiar with the build issues involved, but this seems like a fine
change to me; it's certainly less surprising for Node's constructor to live in
Node.h.

And I think it's best to keep the Node constructor inline unless there's some
reason to make it out-of-line.

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/CharacterData.h
File Source/core/dom/CharacterData.h (right):

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/CharacterData...
Source/core/dom/CharacterData.h:27: #include "core/dom/TreeScope.h"
No need for an #include here

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/CharacterData...
Source/core/dom/CharacterData.h:54: CharacterData(TreeScope* tree_scope, const
String& text, ConstructionType type)
tree_scope -> treeScope

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/ContainerNode.h
File Source/core/dom/ContainerNode.h (right):

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/ContainerNode...
Source/core/dom/ContainerNode.h:29: #include "core/dom/TreeScope.h"
No need for an #include here

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/ContainerNode...
Source/core/dom/ContainerNode.h:188: inline
ContainerNode::ContainerNode(TreeScope* tree_scope, ConstructionType type)
tree_scope -> treeScope

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/Node.h
File Source/core/dom/Node.h (right):

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/Node.h#newcod...
Source/core/dom/Node.h:754: Node(TreeScope *tree_scope, ConstructionType type)
Style: the '*' should be next to the type, and the name of the argument should
be treeScope (variables and methods are camelCased).

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/Node.h#newcod...
Source/core/dom/Node.h:763: m_treeScope = TreeScope::noDocumentInstance();
I think you'll want a TreeScope.h #include in Node.h due to this and the next
line.

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/Notation.cpp
File Source/core/dom/Notation.cpp (right):

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/Notation.cpp#...
Source/core/dom/Notation.cpp:26: Notation::Notation(TreeScope* tree_scope, const
String& name, const String& publicId, const String& systemId)
tree_scope -> treeScope

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/Notation.h
File Source/core/dom/Notation.h (right):

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/Notation.h#ne...
Source/core/dom/Notation.h:26: #include "core/dom/TreeScope.h"
You shouldn't need an #include here

[esprehn, 2013-06-04 23:24:00]

I had always intended to make this change after I switched to m_treeScope,
thanks for the patch! :)

I believe it was made inline so when calling the super constructor in
ContainerNode.h it was "faster". I don't know how much that matters tbh, I'd
make this change and then make it out of line and see if the PLT or perf tests
regress.

This patch is fine if you fix the style violations (* placement and camel
casing).

[Mostyn Bramley-Moore, 2013-06-04 23:46:01]

Style fixes should all be in patchset 2.

On 2013/06/04 23:24:00, esprehn wrote:
> I believe it was made inline so when calling the super constructor in
> ContainerNode.h it was "faster". I don't know how much that matters tbh, I'd
> make this change and then make it out of line and see if the PLT or perf tests
> regress.

I'll see if I can try that later in the week (after this lands).

[Mostyn Bramley-Moore, 2013-06-04 23:46:07]

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/CharacterData.h
File Source/core/dom/CharacterData.h (right):

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/CharacterData...
Source/core/dom/CharacterData.h:27: #include "core/dom/TreeScope.h"
On 2013/06/04 23:08:08, adamk wrote:
> No need for an #include here

Done.

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/CharacterData...
Source/core/dom/CharacterData.h:54: CharacterData(TreeScope* tree_scope, const
String& text, ConstructionType type)
On 2013/06/04 23:08:08, adamk wrote:
> tree_scope -> treeScope

Done.

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/ContainerNode.h
File Source/core/dom/ContainerNode.h (right):

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/ContainerNode...
Source/core/dom/ContainerNode.h:29: #include "core/dom/TreeScope.h"
On 2013/06/04 23:08:08, adamk wrote:
> No need for an #include here

Done.

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/ContainerNode...
Source/core/dom/ContainerNode.h:188: inline
ContainerNode::ContainerNode(TreeScope* tree_scope, ConstructionType type)
On 2013/06/04 23:08:08, adamk wrote:
> tree_scope -> treeScope

Done.

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/Node.h
File Source/core/dom/Node.h (right):

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/Node.h#newcod...
Source/core/dom/Node.h:754: Node(TreeScope *tree_scope, ConstructionType type)
On 2013/06/04 23:08:08, adamk wrote:
> Style: the '*' should be next to the type, and the name of the argument should
> be treeScope (variables and methods are camelCased).

Done.

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/Node.h#newcod...
Source/core/dom/Node.h:763: m_treeScope = TreeScope::noDocumentInstance();
On 2013/06/04 23:08:08, adamk wrote:
> I think you'll want a TreeScope.h #include in Node.h due to this and the next
> line.

TreeScope.h is already included on line 32.

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/Notation.cpp
File Source/core/dom/Notation.cpp (right):

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/Notation.cpp#...
Source/core/dom/Notation.cpp:26: Notation::Notation(TreeScope* tree_scope, const
String& name, const String& publicId, const String& systemId)
On 2013/06/04 23:08:08, adamk wrote:
> tree_scope -> treeScope

Done.

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/Notation.h
File Source/core/dom/Notation.h (right):

https://codereview.chromium.org/16379002/diff/1/Source/core/dom/Notation.h#ne...
Source/core/dom/Notation.h:26: #include "core/dom/TreeScope.h"
On 2013/06/04 23:08:08, adamk wrote:
> You shouldn't need an #include here

Done.

[adamk, 2013-06-05 00:03:07]

lgtm

[commit-bot: I haz the power, 2013-06-05 00:03:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/16379002/8001

[eseidel, 2013-06-05 00:29:11]

lgtm

https://codereview.chromium.org/16379002/diff/8001/Source/core/dom/Text.h
File Source/core/dom/Text.h (right):

https://codereview.chromium.org/16379002/diff/8001/Source/core/dom/Text.h#new...
Source/core/dom/Text.h:61: Text(TreeScope* tree_scope, const String& data,
ConstructionType type)
treeScope

[commit-bot: I haz the power, 2013-06-05 01:27:16]

Change committed as 151791

[esprehn, 2013-06-05 05:28:22]

On 2013/06/05 01:27:16, I haz the power (commit-bot) wrote:
> Change committed as 151791

Hmm, so this landed with the style violation in that one argument. It'd be nice
if you could fix that in a follow up. Also I'm curious why the style check
didn't prevent the patch from landing, it used to check these things? :(

[Mostyn Bramley-Moore, 2013-06-05 05:38:46]

Cleanup in https://codereview.chromium.org/15813017/

Should the style check false positive be reported somewhere?  (Was it even run?)