Bug 73800 – [Refactoring] Accessing Node::m_document should be minimized.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

It should be indirected by Node::document() because Node::document() is going to be a bit nontrivial.

[Hajime Morrita]

Created attachment 117851 [details]
Patch

[Kent Tamura]

Comment on attachment 117851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117851&action=review

> Source/WebCore/ChangeLog:12
> +        [Refactoring] Accessing Node::m_document should be minimized.
> +        https://bugs.webkit.org/show_bug.cgi?id=73800
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests. No behavioral change.
> +
> +        Replaced m_document reference with the document() accessor
> +        or temporaril variables.
> +

Please write a reason of the change.

[WebKit Review Bot]

Comment on attachment 117851 [details]
Patch

Attachment 117851 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10688615

New failing tests:
svg/custom/linking-uri-01-b.svg

[Hajime Morrita]

(In reply to comment #2)
> > +
> 
> Please write a reason of the change.
Hi Kent-san, thanks for quick review!
I'll add some background explanation before landing.

[Hajime Morrita]

Committed r101983: <http://trac.webkit.org/changeset/101983>

[Iain Merrick]

I think this is causing a crash in debug builds.

~Node now calls Node::document(), which includes the following:

ASSERT(m_document || (nodeType() == DOCUMENT_TYPE_NODE && !inDocument()));

Node::nodeType() is pure virtual and there cannot be called in a destructor (the subclass has already been destroyed).

Chromium crashes on startup with a "pure virtual method called" error. I assume this affects all WebKit ports. I'll verify with a debug build of DumpRenderTree.

[Iain Merrick]

Aha, already rolled out in http://trac.webkit.org/changeset/101995

[Csaba Osztrogonác]

(In reply to comment #7)
> Aha, already rolled out in http://trac.webkit.org/changeset/101995

It was rolled out because it broke all tests on GTK and on Qt in debug mode.
The exact reason can be found in comment #6.

[Csaba Osztrogonác]

Comment on attachment 117851 [details]
Patch

removing flags from rolled out patch

[Alexey Proskuryakov]

> Node::document() is going to be a bit nontrivial

What is the plan for Node::document()? It directly maps to DOM concept, so changing it could introduce a lot of complexity.

[Hajime Morrita]

I'm totally sorry about the last change. It was a disaster...

By the way,

> What is the plan for Node::document()? It directly maps to DOM concept, so changing it could introduce a lot of complexity.

Basically, this series of change is adding an indirection for Node::document() call.

This is preparation for Bug 59816,
where I'm going to turn m_document into m_treeScope pointing TreeScope* instance,
which is a superclass of Document and ShadowRoot.

After the change, Node::document() will be something like:
  return m_treeScope->enclosingDocument();

TreeScope::m_enclosingDocument will point itself when the real class of the instance is Document.
return its containing Document.

Currently all elements in shadow have ElementRareData because they need ElementRareData::m_treeScope.
This change eliminates ElementRareData::m_treeScope, 
so these nodes no longer need to have  its rare data and can save memory allocation for that.

[Hajime Morrita]

Created attachment 117993 [details]
A fixed version

[Kent Tamura]

Comment on attachment 117993 [details]
A fixed version

ok.
Please watch the buildbots after landing the patch.

[WebKit Review Bot]

Comment on attachment 117993 [details]
A fixed version

Clearing flags on attachment: 117993

Committed r102119: <http://trac.webkit.org/changeset/102119>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.