Bug 82701 - insertedIntoDocument and insertedIntoTree should be unified
Summary: insertedIntoDocument and insertedIntoTree should be unified
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on: 84154
Blocks: 83268 79668 82681 83269
  Show dependency treegraph
 
Reported: 2012-03-30 00:30 PDT by Hajime Morrita
Modified: 2012-04-21 17:46 PDT (History)
12 users (show)

See Also:


Attachments
Patch (120.69 KB, patch)
2012-04-05 04:44 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (145.25 KB, patch)
2012-04-12 01:58 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.22 MB, application/zip)
2012-04-12 03:21 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-04 (6.40 MB, application/zip)
2012-04-12 04:26 PDT, WebKit Review Bot
no flags Details
Updated with a test failure fix (148.80 KB, patch)
2012-04-12 05:05 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (136.45 KB, patch)
2012-04-13 01:33 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (136.27 KB, patch)
2012-04-15 21:02 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (137.41 KB, patch)
2012-04-16 01:36 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (140.25 KB, patch)
2012-04-16 22:10 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch for landing (140.48 KB, patch)
2012-04-16 22:40 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2012-03-30 00:30:28 PDT
These two are both virtual. and merging these into one will help speedup.
This also will give a cleaner hook to hook tree insertion, which I need for Bug 82681.
Comment 1 Adam Klein 2012-04-04 14:52:52 PDT
Sounds reasonable to me. But note that you probably don't want to merge removedFromTree/removedFromDocument, even though that seems more symmetric (see https://bugs.webkit.org/show_bug.cgi?id=80628 for some of the problems with removedFromDocument).
Comment 2 Hajime Morrita 2012-04-05 04:44:47 PDT
Created attachment 135805 [details]
Patch
Comment 3 Dimitri Glazkov (Google) 2012-04-05 08:46:07 PDT
Comment on attachment 135805 [details]
Patch

The proliferation of *Context pattern is now starting to alarm me (even though I think I first plopped it in with EventContext). However, I agree that this is not bad at all. The deep/not-deep distinction was never clear and this makes it a lot more intuitive.
Comment 4 Hajime Morrita 2012-04-05 18:05:11 PDT
Thanks for your feedback Dimitri!

> (From update of attachment 135805 [details])
> The proliferation of *Context pattern is now starting to alarm me (even though I think I first plopped it in with EventContext). However, I agree that this is not bad at all. The deep/not-deep distinction was never clear and this makes it a lot more intuitive.
Yeah, "Context" seems like "Manager" in certain level, which give little color for the class...
The tree traversal code could be stayed in ContainerNode.cpp or could be part of
ContainerNodeAlgorithms.h.
Comment 5 Dimitri Glazkov (Google) 2012-04-05 20:00:33 PDT
(In reply to comment #4)
> Thanks for your feedback Dimitri!
> 
> > (From update of attachment 135805 [details] [details])
> > The proliferation of *Context pattern is now starting to alarm me (even though I think I first plopped it in with EventContext). However, I agree that this is not bad at all. The deep/not-deep distinction was never clear and this makes it a lot more intuitive.
> Yeah, "Context" seems like "Manager" in certain level, which give little color for the class...
> The tree traversal code could be stayed in ContainerNode.cpp or could be part of
> ContainerNodeAlgorithms.h.

I am fine either way.
Comment 6 Ryosuke Niwa 2012-04-05 20:22:54 PDT
(In reply to comment #4)
> Thanks for your feedback Dimitri!
> 
> > (From update of attachment 135805 [details] [details])
> > The proliferation of *Context pattern is now starting to alarm me (even though I think I first plopped it in with EventContext). However, I agree that this is not bad at all. The deep/not-deep distinction was never clear and this makes it a lot more intuitive.
> Yeah, "Context" seems like "Manager" in certain level, which give little color for the class...
> The tree traversal code could be stayed in ContainerNode.cpp or could be part of
> ContainerNodeAlgorithms.h.

Putting it in ContainerNodeAlgorithms makes most sense.
Comment 7 Hajime Morrita 2012-04-12 01:58:53 PDT
Created attachment 136854 [details]
Patch
Comment 8 Ryosuke Niwa 2012-04-12 02:28:01 PDT
Comment on attachment 136854 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        getting together them into one, to gain some speedup. Such

Nit: combining them together? merging them into one function?

> Source/WebCore/ChangeLog:12
> +        unification also could clarify the semantics of the hook.

Nit: also is redundant here.

> Source/WebCore/ChangeLog:14
> +        This change makes it happen by:

"it" here is ambiguous. I think it's probably better to say "This patch makes the following changes" instead.

> Source/WebCore/ChangeLog:37
> +        There is possible situation where a node need to hook both before
> +        and after its children is nofitied.  It was represented naturally
> +        with previous traversal-by-ContainerNode pattern, but is no longer
> +        simple with this new external traversal. To support this scenario,

I don't fully understand these two paragraphs. What are you referring to by "hook"?

> Source/WebCore/ChangeLog:41
> +        how it works. In fast, that is the only instance where this
> +        mechanism is employed.

Is it possible to get rid of setNeedsNotificationAfterChildrenNotified by refactoring the form element code?

> Source/WebCore/ChangeLog:43
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the description.

> Source/WebCore/dom/ContainerNode.h:84
>      // -----------------------------------------------------------------------------
>      // Notification of document structure changes (see Node.h for more notification methods)
>  

I'm not sure if this comment is helpful now given you've removed two out of the three methods we had.

> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:34
> +namespace NodeInsertionPrivate {

Do we really want to introduce new namespaces for these functions?

> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:36
> +void traverseChildrenForDocument(ContainerNode* node)

Instead of hiding these functions inside a namespace and giving a generic name like traverseChildrenForDocument, can we just add them to WebCore with much more descriptive function names (e.g. notifyDescendentsInsertedIntoDocument)?

> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:45
> +            traverseForDocument(children[i].get());

notifyInsertedIntoDocument?

> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:78
> +namespace NodeRemovalPrivate {
> +
> +void traverseChildrenForDocument(ContainerNode* node)

Ditto about namespace & function name.

> Source/WebCore/dom/ContainerNodeAlgorithms.h:184
> +namespace NodeInsertionPrivate {
> +
> +void traverseChildrenForDocument(ContainerNode*);
> +void traverseChildrenForTree(ContainerNode*);
> +
> +inline void traverseForDocument(Node* node)
> +{
> +    NodeInsertionContext context(NodeMovingContext::MoveTypeDocument);
> +    RefPtr<Node> protect(node);
> +    node->insertedInto(context);
> +    if (node->isContainerNode())
> +        traverseChildrenForDocument(toContainerNode(node));
> +    if (context.needsNotificationAfterChildrenNotified())
> +        node->notifiedInsertedInto(context);
> +}
> +
> +inline void traverseForTree(ContainerNode* node)
> +{
> +    NodeInsertionContext context(NodeMovingContext::MoveTypeTree);
> +    forbidEventDispatch();
> +    node->insertedInto(context);
> +    traverseChildrenForTree(node);
> +    if (context.needsNotificationAfterChildrenNotified())
> +        node->notifiedInsertedInto(context);
> +    allowEventDispatch();
> +}

Do we really need to keep these functions inline? We'll end up calling functions anyways, right?
Can't we just move these functions to ContainerNodeAlgorithms.cpp ?

> Source/WebCore/dom/ContainerNodeAlgorithms.h:192
> +namespace NodeRemovalPrivate {
> +
> +void traverseChildrenForDocument(ContainerNode*);
> +void traverseChildrenForTree(ContainerNode*);
> +

These proliferations of namespaces and prototypes don't look great to me.

> Source/WebCore/dom/NodeMovingContext.h:41
> +    bool isAcrossDocument() const { return m_type == MoveTypeDocument; }

I'm not sure if "across document" is an accurate description here. It's more like moving out from and into a document...

> Source/WebCore/dom/NodeMovingContext.h:66
> +class NodeInsertionContext : public NodeMovingContext {
> +public:
> +    NodeInsertionContext(MoveType type)
> +        : NodeMovingContext(type)
> +        , m_needsNotificationAfterChildrenNotified(false)
> +    { }
> +
> +    void setNeedsNotificationAfterChildrenNotified() { m_needsNotificationAfterChildrenNotified = true; }
> +    bool needsNotificationAfterChildrenNotified() const { return m_needsNotificationAfterChildrenNotified; }
> +
> +private:
> +    bool m_needsNotificationAfterChildrenNotified;
> +};
> +
> +class NodeRemovalContext : public NodeMovingContext {
> +public:
> +    NodeRemovalContext(MoveType type)
> +        : NodeMovingContext(type)
> +    { }
> +};

Do we really need 3 classes for this? I feel like the only reason we do is because of setNeedsNotificationAfterChildrenNotified and we can just use a bit flag otherwise.
Comment 9 WebKit Review Bot 2012-04-12 03:21:34 PDT
Comment on attachment 136854 [details]
Patch

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

New failing tests:
tables/mozilla/bugs/bug4527.html
fast/frames/no-frame-borders.html
Comment 10 WebKit Review Bot 2012-04-12 03:21:40 PDT
Created attachment 136865 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 11 WebKit Review Bot 2012-04-12 04:26:26 PDT
Comment on attachment 136854 [details]
Patch

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

New failing tests:
tables/mozilla/bugs/bug4527.html
fast/frames/no-frame-borders.html
Comment 12 WebKit Review Bot 2012-04-12 04:26:32 PDT
Created attachment 136875 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 13 Hajime Morrita 2012-04-12 05:05:40 PDT
Created attachment 136879 [details]
Updated with a test failure fix
Comment 14 Hajime Morrita 2012-04-12 05:06:54 PDT
Comment on attachment 136854 [details]
Patch

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

Hi Ryosuke, thanks for taking a look! I updated the patch.

>> Source/WebCore/ChangeLog:11
>> +        getting together them into one, to gain some speedup. Such
> 
> Nit: combining them together? merging them into one function?

Fixed.

>> Source/WebCore/ChangeLog:12
>> +        unification also could clarify the semantics of the hook.
> 
> Nit: also is redundant here.

Removed

>> Source/WebCore/ChangeLog:14
>> +        This change makes it happen by:
> 
> "it" here is ambiguous. I think it's probably better to say "This patch makes the following changes" instead.

clarified.

>> Source/WebCore/ChangeLog:37
>> +        simple with this new external traversal. To support this scenario,
> 
> I don't fully understand these two paragraphs. What are you referring to by "hook"?

Clarified.

>> Source/WebCore/ChangeLog:41
>> +        mechanism is employed.
> 
> Is it possible to get rid of setNeedsNotificationAfterChildrenNotified by refactoring the form element code?

Possibly. But it isn't trivial change. Could be attacked by a separate change.

>> Source/WebCore/ChangeLog:43
>> +        Reviewed by NOBODY (OOPS!).
> 
> This line should appear before the description.

Oops.

>> Source/WebCore/dom/ContainerNode.h:84
>>  
> 
> I'm not sure if this comment is helpful now given you've removed two out of the three methods we had.

Moved and reworded the comment to Node.h

>> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:36
>> +void traverseChildrenForDocument(ContainerNode* node)
> 
> Instead of hiding these functions inside a namespace and giving a generic name like traverseChildrenForDocument, can we just add them to WebCore with much more descriptive function names (e.g. notifyDescendentsInsertedIntoDocument)?

What I'd intended was that the private functions are just an implementation detail and shouldn't be used.
I put these class instead to clarify the intention.

>> Source/WebCore/dom/ContainerNodeAlgorithms.h:184
>> +}
> 
> Do we really need to keep these functions inline? We'll end up calling functions anyways, right?
> Can't we just move these functions to ContainerNodeAlgorithms.cpp ?

That prevents compiler to make them actually inline because these function has recursion. 
Also, the functions are not called when the node is a Text object.

>> Source/WebCore/dom/ContainerNodeAlgorithms.h:192
>> +
> 
> These proliferations of namespaces and prototypes don't look great to me.

Turned them to classes.

>> Source/WebCore/dom/NodeMovingContext.h:41
>> +    bool isAcrossDocument() const { return m_type == MoveTypeDocument; }
> 
> I'm not sure if "across document" is an accurate description here. It's more like moving out from and into a document...

Good point. Will move them to subclasses to give appropriate names.

>> Source/WebCore/dom/NodeMovingContext.h:66
>> +};
> 
> Do we really need 3 classes for this? I feel like the only reason we do is because of setNeedsNotificationAfterChildrenNotified and we can just use a bit flag otherwise.

I'm planning to merge willRemove() later where m_originalParent will be added to NodeRemovalContext.
Comment 15 Hajime Morrita 2012-04-13 01:33:53 PDT
Created attachment 137060 [details]
Patch
Comment 16 Hajime Morrita 2012-04-13 01:37:38 PDT
Ryosuke, Could you take a look? This patch addressed your points at #webkit.
Comment 17 Hajime Morrita 2012-04-15 21:02:49 PDT
Created attachment 137272 [details]
Patch
Comment 18 Ryosuke Niwa 2012-04-15 23:58:54 PDT
Comment on attachment 137272 [details]
Patch

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

The patch looks much better. I think we want one more iteration of name tweaking.

> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:34
> +void ChildNodeInsertionHelper::traverseChildrenForDocument(ContainerNode* node)

Can we call this class ChildNodeInsertionNotifier instead? "helper" sounds too generic to me. Also, it's probably better to call this function notifyDescendentsInsertedIntoDocument to be more explicit.

> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:56
> +void ChildNodeInsertionHelper::traverseChildrenForTree(ContainerNode* node)

Ditto about class & function names.

> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:72
> +void ChildNodeRemovalHelper::traverseChildrenForDocument(ContainerNode* node)

Ditto.

> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:97
> +void ChildNodeRemovalHelper::traverseChildrenForTree(ContainerNode* node)

Ditto.

> Source/WebCore/dom/Node.h:511
> +    enum InsertionResult {
> +        InsertionDone,
> +        InsertionNeedsNotification
> +    };

I think we need to rename these flags so that the semantics is clearer.
When is InsertionNeedsNotification returned and why?

> Source/WebCore/dom/Node.h:518
> +    // This first type of nofification comes from NodeMovingContext::MoveTypeDocument.

This comment is out of date, right?

> Source/WebCore/dom/Node.h:523
> +    // In that case, the notification type is NodeMovingContext::MoveTypeTree.

Ditto.
Comment 19 Hajime Morrita 2012-04-16 01:36:55 PDT
Created attachment 137300 [details]
Patch
Comment 20 Hajime Morrita 2012-04-16 01:45:43 PDT
(In reply to comment #18)
> (From update of attachment 137272 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137272&action=review
> 
> The patch looks much better. I think we want one more iteration of name tweaking.
> 
> > Source/WebCore/dom/ContainerNodeAlgorithms.cpp:34
> > +void ChildNodeInsertionHelper::traverseChildrenForDocument(ContainerNode* node)
> 
> Can we call this class ChildNodeInsertionNotifier instead? "helper" sounds too generic to me. Also, it's probably better to call this function notifyDescendentsInsertedIntoDocument to be more explicit.
Right.renamed.

> > Source/WebCore/dom/Node.h:511
> > +    enum InsertionResult {
> > +        InsertionDone,
> > +        InsertionNeedsNotification
> > +    };
> 
> I think we need to rename these flags so that the semantics is clearer.
> When is InsertionNeedsNotification returned and why?
A few element implementation needs this for their own reasons.
But It should clarify "when" indeed. I renamed it to more descriptive name.

> 
> > Source/WebCore/dom/Node.h:518
> > +    // This first type of nofification comes from NodeMovingContext::MoveTypeDocument.
> 
> This comment is out of date, right?
> 
Oops. Rewrote the comment.
Comment 21 Dimitri Glazkov (Google) 2012-04-16 08:44:06 PDT
Comment on attachment 137300 [details]
Patch

Oh wow. This patch looks so much better than what I looked at a week ago.
Comment 22 Hajime Morrita 2012-04-16 16:45:28 PDT
(In reply to comment #21)
> (From update of attachment 137300 [details])
> Oh wow. This patch looks so much better than what I looked at a week ago.
Yeah, you should thank Ryosuke ;-)
Comment 23 Ryosuke Niwa 2012-04-16 21:04:31 PDT
Comment on attachment 137300 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        unification could clarify the semantics of the hook.

I would get rid of "of the hook" and replace it by "as well".

> Source/WebCore/dom/ContainerNode.cpp:408
> +    ChildNodeRemovalNotifier(this).notifyRemoved(child.get());

Given that the class is called ChildNodeRemovalNotifier, we can probably just call this method "notify".

> Source/WebCore/dom/ContainerNode.cpp:593
>      // FIXME: Why doesn't this use notifyChildInserted(newChild) instead?

Maybe we should update this comment?

> Source/WebCore/dom/Element.cpp:892
> +    if (containsFullScreenElement() && parentElement() && !parentElement()->containsFullScreenElement())
> +        setContainsFullScreenElementOnAncestorsCrossingFrameBoundaries(true);

This looks like horrifyingly inefficient :(  Note that this code was just moved from Element::insertedIntoTree so it's not morrita's fault.

> Source/WebCore/dom/Node.h:522
> +    enum InsertionResult {

Instead of calling it "result" can it name it to something like ShouldBlah? Because it isn't really a result. It's more of a decision/command as to something should happen or not.

> Source/WebCore/html/FormAssociatedElement.cpp:63
> +void FormAssociatedElement::insertedInto(Node* rootParent)

I don't think "rootParent" is semantically clear. How about something like insertionPointContainer?

> Source/WebCore/html/FormAssociatedElement.cpp:74
> +static inline Node* findRoot(Node* n)

Can we rename this function to highestAncestor since that's what this function is doing?
Also, please rename n to node.
Comment 24 Ryosuke Niwa 2012-04-16 21:10:40 PDT
Comment on attachment 137300 [details]
Patch

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

> Source/WebCore/html/HTMLStyleElement.cpp:-173
> -    HTMLElement::removedFromDocument();
> -    StyleElement::removedFromDocument(document(), this);

It seems like we used to call StyleElement::removedFromDocument after HTMLElement::removedFromDocument?
Comment 25 Hajime Morrita 2012-04-16 22:10:47 PDT
Created attachment 137479 [details]
Patch
Comment 26 Hajime Morrita 2012-04-16 22:11:48 PDT
(In reply to comment #23)
> (From update of attachment 137300 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137300&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        unification could clarify the semantics of the hook.
> 
> I would get rid of "of the hook" and replace it by "as well".
Fixed. Sounds more like English... 

> 
> > Source/WebCore/dom/ContainerNode.cpp:408
> > +    ChildNodeRemovalNotifier(this).notifyRemoved(child.get());
> 
> Given that the class is called ChildNodeRemovalNotifier, we can probably just call this method "notify".
Renamed.

> 
> > Source/WebCore/dom/ContainerNode.cpp:593
> >      // FIXME: Why doesn't this use notifyChildInserted(newChild) instead?
> 
> Maybe we should update this comment?
True. updated to align the method name.

> 
> > Source/WebCore/dom/Element.cpp:892
> > +    if (containsFullScreenElement() && parentElement() && !parentElement()->containsFullScreenElement())
> > +        setContainsFullScreenElementOnAncestorsCrossingFrameBoundaries(true);
> 
> This looks like horrifyingly inefficient :(  Note that this code was just moved from Element::insertedIntoTree so it's not morrita's fault.

So I'd keep this for homework for someone else...

> 
> > Source/WebCore/dom/Node.h:522
> > +    enum InsertionResult {
> 
> Instead of calling it "result" can it name it to something like ShouldBlah? Because it isn't really a result. It's more of a decision/command as to something should happen or not.
Hmm. How about InsertionNotificationRequest? I tried this and looks reasonable.

> 
> > Source/WebCore/html/FormAssociatedElement.cpp:63
> > +void FormAssociatedElement::insertedInto(Node* rootParent)
> 
> I don't think "rootParent" is semantically clear. How about something like insertionPointContainer?
Renamed to insertionPoint - it's necessarily a container.
> 
> > Source/WebCore/html/FormAssociatedElement.cpp:74
> > +static inline Node* findRoot(Node* n)
> 
> Can we rename this function to highestAncestor since that's what this function is doing?
> Also, please rename n to node.
Done.


(In reply to comment #24)
> (From update of attachment 137300 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137300&action=review
> 
> > Source/WebCore/html/HTMLStyleElement.cpp:-173
> > -    HTMLElement::removedFromDocument();
> > -    StyleElement::removedFromDocument(document(), this);
> 
> It seems like we used to call StyleElement::removedFromDocument after HTMLElement::removedFromDocument?
Ah, yes. It looks. Aligned to existing code.
Comment 27 Ryosuke Niwa 2012-04-16 22:18:26 PDT
Comment on attachment 137479 [details]
Patch

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

> Source/WebCore/dom/ContainerNodeAlgorithms.h:198
> +    ASSERT(m_insertionPoint->inDocument());
> +    RefPtr<Node> protect(node);
> +    Node::InsertionNotificationRequest request = node->insertedInto(m_insertionPoint);
> +    if (node->isContainerNode())
> +        notifyDescendantInsertedIntoDocument(toContainerNode(node));
> +    if (request == Node::InsertionShouldCallDidNotifyDescendantInseretions)
> +        node->didNotifyDescendantInseretions(m_insertionPoint);

Maybe some blank lines would improve the readability?

> Source/WebCore/dom/ContainerNodeAlgorithms.h:209
> +    ASSERT(!m_insertionPoint->inDocument());
> +    forbidEventDispatch();
> +    Node::InsertionNotificationRequest request = node->insertedInto(m_insertionPoint);
> +    notifyDescendantInsertedIntoTree(node);
> +    if (request == Node::InsertionShouldCallDidNotifyDescendantInseretions)
> +        node->didNotifyDescendantInseretions(m_insertionPoint);
> +    allowEventDispatch();

Ditto.
Comment 28 Hajime Morrita 2012-04-16 22:40:42 PDT
Created attachment 137480 [details]
Patch for landing
Comment 29 WebKit Review Bot 2012-04-16 23:43:01 PDT
Comment on attachment 137480 [details]
Patch for landing

Clearing flags on attachment: 137480

Committed r114351: <http://trac.webkit.org/changeset/114351>
Comment 30 WebKit Review Bot 2012-04-16 23:43:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Csaba Osztrogonác 2012-04-17 05:04:19 PDT
Reopen, because it made 5 tests assert on Mac and on Qt:
- http://build.webkit.org/results/Lion%20Debug%20%28Tests%29/r114351%20%285606%29/results.html
- http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Debug/r114351%20%2822316%29/results.html

crash log for DumpRenderTree (pid 11595):
STDOUT: <empty>
STDERR: ASSERTION FAILED: !HashTranslator::equal(KeyTraits::emptyValue(), key)
STDERR: ../../../../Source/WTF/wtf/HashTable.h(487) : void WTF::HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::checkKey(const T&) [with HashTranslator = WTF::HashMapTranslator<WTF::PairHashTraits<WTF::HashTraits<WTF::RefPtr<WebCore::Node> >, WTF::HashTraits<int> >, WTF::PtrHash<WTF::RefPtr<WebCore::Node> > >, T = WebCore::Node*, Key = WTF::RefPtr<WebCore::Node>, Value = std::pair<WTF::RefPtr<WebCore::Node>, int>, Extractor = WTF::PairFirstExtractor<std::pair<WTF::RefPtr<WebCore::Node>, int> >, HashFunctions = WTF::PtrHash<WTF::RefPtr<WebCore::Node> >, Traits = WTF::PairHashTraits<WTF::HashTraits<WTF::RefPtr<WebCore::Node> >, WTF::HashTraits<int> >, KeyTraits = WTF::HashTraits<WTF::RefPtr<WebCore::Node> >]
STDERR: 1   0x7f230938a257 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(void WTF::HashTable<WTF::RefPtr<WebCore::Node>, std::pair<WTF::RefPtr<WebCore::Node>, int>, WTF::PairFirstExtractor<std::pair<WTF::RefPtr<WebCore::Node>, int> >, WTF::PtrHash<WTF::RefPtr<WebCore::Node> >, WTF::PairHashTraits<WTF::HashTraits<WTF::RefPtr<WebCore::Node> >, WTF::HashTraits<int> >, WTF::HashTraits<WTF::RefPtr<WebCore::Node> > >::checkKey<WTF::HashMapTranslator<WTF::PairHashTraits<WTF::HashTraits<WTF::RefPtr<WebCore::Node> >, WTF::HashTraits<int> >, WTF::PtrHash<WTF::RefPtr<WebCore::Node> > >, WebCore::Node*>(WebCore::Node* const&)+0x89) [0x7f230938a257]
STDERR: 2   0x7f2309386ce7 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(std::pair<WTF::RefPtr<WebCore::Node>, int>* WTF::HashTable<WTF::RefPtr<WebCore::Node>, std::pair<WTF::RefPtr<WebCore::Node>, int>, WTF::PairFirstExtractor<std::pair<WTF::RefPtr<WebCore::Node>, int> >, WTF::PtrHash<WTF::RefPtr<WebCore::Node> >, WTF::PairHashTraits<WTF::HashTraits<WTF::RefPtr<WebCore::Node> >, WTF::HashTraits<int> >, WTF::HashTraits<WTF::RefPtr<WebCore::Node> > >::lookup<WTF::HashMapTranslator<WTF::PairHashTraits<WTF::HashTraits<WTF::RefPtr<WebCore::Node> >, WTF::HashTraits<int> >, WTF::PtrHash<WTF::RefPtr<WebCore::Node> > >, WebCore::Node*>(WebCore::Node* const&)+0x23) [0x7f2309386ce7]
STDERR: 3   0x7f230938338f /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WTF::HashMap<WTF::RefPtr<WebCore::Node>, int, WTF::PtrHash<WTF::RefPtr<WebCore::Node> >, WTF::HashTraits<WTF::RefPtr<WebCore::Node> >, WTF::HashTraits<int> >::inlineGet(WebCore::Node*) const+0x23) [0x7f230938338f]
STDERR: 4   0x7f230937fdd3 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WTF::HashMap<WTF::RefPtr<WebCore::Node>, int, WTF::PtrHash<WTF::RefPtr<WebCore::Node> >, WTF::HashTraits<WTF::RefPtr<WebCore::Node> >, WTF::HashTraits<int> >::get(WebCore::Node*) const+0x23) [0x7f230937fdd3]
STDERR: 5   0x7f230937db73 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::InspectorDOMAgent::didInsertDOMNode(WebCore::Node*)+0x69) [0x7f230937db73]
STDERR: 6   0x7f23093a1258 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::InspectorInstrumentation::didInsertDOMNodeImpl(WebCore::InstrumentingAgents*, WebCore::Node*)+0x3a) [0x7f23093a1258]
STDERR: 7   0x7f2308fe169a /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::InspectorInstrumentation::didInsertDOMNode(WebCore::Document*, WebCore::Node*)+0x46) [0x7f2308fe169a]
STDERR: 8   0x7f2308fe19ba /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::ChildNodeInsertionNotifier::notify(WebCore::Node*)+0x72) [0x7f2308fe19ba]
STDERR: 9   0x7f23090dbc1f /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::ShadowTree::addShadowRoot(WebCore::Element*, WTF::PassRefPtr<WebCore::ShadowRoot>, int&)+0x127) [0x7f23090dbc1f]
STDERR: 10  0x7f23090daf81 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::ShadowRoot::create(WebCore::Element*, WebCore::ShadowRoot::ShadowRootCreationPurpose, int&)+0x15f) [0x7f23090daf81]
STDERR: 11  0x7f230924aca8 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLInputElement::createShadowSubtree()+0x9c) [0x7f230924aca8]
STDERR: 12  0x7f230924abbc /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLInputElement::create(WebCore::QualifiedName const&, WebCore::Document*, WebCore::HTMLFormElement*, bool)+0xd0) [0x7f230924abbc]
STDERR: 13  0x7f230a180284 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(+0x341f284) [0x7f230a180284]
STDERR: 14  0x7f230a182339 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLElementFactory::createHTMLElement(WebCore::QualifiedName const&, WebCore::Document*, WebCore::HTMLFormElement*, bool)+0xab) [0x7f230a182339]
STDERR: 15  0x7f23092b944a /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLConstructionSite::createHTMLElement(WebCore::AtomicHTMLToken&)+0x7e) [0x7f23092b944a]
STDERR: 16  0x7f23092b8755 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLConstructionSite::insertSelfClosingHTMLElement(WebCore::AtomicHTMLToken&)+0x7b) [0x7f23092b8755]
STDERR: 17  0x7f23092e31ba /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLTreeBuilder::processStartTagForInBody(WebCore::AtomicHTMLToken&)+0x140a) [0x7f23092e31ba]
STDERR: 18  0x7f23092e4cd3 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLTreeBuilder::processStartTag(WebCore::AtomicHTMLToken&)+0x82d) [0x7f23092e4cd3]
STDERR: 19  0x7f23092e05c2 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLTreeBuilder::processToken(WebCore::AtomicHTMLToken&)+0xc8) [0x7f23092e05c2]
STDERR: 20  0x7f23092e03fe /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken(WebCore::AtomicHTMLToken&)+0x50) [0x7f23092e03fe]
STDERR: 21  0x7f23092e02f8 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLTreeBuilder::constructTreeFromToken(WebCore::HTMLToken&)+0x66) [0x7f23092e02f8]
STDERR: 22  0x7f23092bf023 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)+0x327) [0x7f23092bf023]
STDERR: 23  0x7f23092be8e9 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode)+0xb1) [0x7f23092be8e9]
STDERR: 24  0x7f23092bfce6 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution()+0xe0) [0x7f23092bfce6]
STDERR: 25  0x7f23092c002d /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource*)+0x1c7) [0x7f23092c002d]
STDERR: 26  0x7f23094493fd /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::CachedResource::checkNotify()+0x6b) [0x7f23094493fd]
STDERR: 27  0x7f2309453677 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::CachedScript::data(WTF::PassRefPtr<WebCore::SharedBuffer>, bool)+0x9d) [0x7f2309453677]
STDERR: 28  0x7f23094bdbc8 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::SubresourceLoader::didFinishLoading(double)+0x244) [0x7f23094bdbc8]
STDERR: 29  0x7f23094b26a9 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double)+0x33) [0x7f23094b26a9]
STDERR: 30  0x7f23099379e9 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::QNetworkReplyHandler::finish()+0x1d3) [0x7f23099379e9]
STDERR: 31  0x7f230993598e /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::QNetworkReplyHandlerCallQueue::flush()+0x9c) [0x7f230993598e]


Could you check what happened?
Comment 32 Csaba Osztrogonác 2012-04-18 03:21:12 PDT
Fixed by http://trac.webkit.org/changeset/114388
Comment 33 Hajime Morrita 2012-04-18 08:41:43 PDT
Thanks for the fix, Ossy.
Comment 34 Csaba Osztrogonác 2012-04-18 08:46:00 PDT
(In reply to comment #33)
> Thanks for the fix, Ossy.

It wasn't me, apavlov was the bug fixer. ;-) I only 
noticed that he fixed it without reference to this bug.
Comment 35 Hajime Morrita 2012-04-18 13:54:02 PDT
(In reply to comment #34)
> It wasn't me, apavlov was the bug fixer. ;-) I only 
> noticed that he fixed it without reference to this bug.
Thanks for letting me know then ;-)