Bug 73800

Summary: [Refactoring] Accessing Node::m_document should be minimized.
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: DOMAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, dglazkov, dominicc, husky, ossy, rolandsteiner, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 73827    
Bug Blocks: 59816    
Attachments:
Description Flags
Patch
none
A fixed version none

Description Hajime Morrita 2011-12-04 22:54:33 PST
It should be indirected by Node::document() because Node::document() is going to be a bit nontrivial.
Comment 1 Hajime Morrita 2011-12-05 00:27:38 PST
Created attachment 117851 [details]
Patch
Comment 2 Kent Tamura 2011-12-05 01:20:42 PST
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 3 WebKit Review Bot 2011-12-05 01:27:10 PST
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
Comment 4 Hajime Morrita 2011-12-05 03:00:36 PST
(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.
Comment 5 Hajime Morrita 2011-12-05 03:15:59 PST
Committed r101983: <http://trac.webkit.org/changeset/101983>
Comment 6 Iain Merrick 2011-12-05 07:02:18 PST
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.
Comment 7 Iain Merrick 2011-12-05 07:09:55 PST
Aha, already rolled out in http://trac.webkit.org/changeset/101995
Comment 8 Csaba Osztrogon√°c 2011-12-05 07:28:11 PST
(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 9 Csaba Osztrogon√°c 2011-12-05 07:28:29 PST
Comment on attachment 117851 [details]
Patch

removing flags from rolled out patch
Comment 10 Alexey Proskuryakov 2011-12-05 10:26:20 PST
> 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.
Comment 11 Hajime Morrita 2011-12-05 22:10:41 PST
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.
Comment 12 Hajime Morrita 2011-12-05 23:02:29 PST
Created attachment 117993 [details]
A fixed version
Comment 13 Kent Tamura 2011-12-05 23:25:33 PST
Comment on attachment 117993 [details]
A fixed version

ok.
Please watch the buildbots after landing the patch.
Comment 14 WebKit Review Bot 2011-12-06 03:31:01 PST
Comment on attachment 117993 [details]
A fixed version

Clearing flags on attachment: 117993

Committed r102119: <http://trac.webkit.org/changeset/102119>
Comment 15 WebKit Review Bot 2011-12-06 03:31:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Lucas Forschler 2019-02-06 09:02:59 PST
Mass moving XML DOM bugs to the "DOM" Component.