It should be indirected by Node::document() because Node::document() is going to be a bit nontrivial.
Created attachment 117851 [details] Patch
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.
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
(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.
Committed r101983: <http://trac.webkit.org/changeset/101983>
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.
Aha, already rolled out in http://trac.webkit.org/changeset/101995
(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.
Comment on attachment 117851 [details] Patch removing flags from rolled out patch
> 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.
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.
Created attachment 117993 [details] A fixed version
Comment on attachment 117993 [details] A fixed version ok. Please watch the buildbots after landing the patch.
Comment on attachment 117993 [details] A fixed version Clearing flags on attachment: 117993 Committed r102119: <http://trac.webkit.org/changeset/102119>
All reviewed patches have been landed. Closing bug.
Mass moving XML DOM bugs to the "DOM" Component.