Bug 61011 – [Refactoring] attach() following detach() should be replaced with Node::forceReattach()

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

[Hajime Morrita]

We are going to have the method.

[Dominic Cooney]

Is there anything special about forceReattach compared to detach and attach? (eg detach and attach a bug because ...?, or just slow because ...?)

[Hajime Morrita]

(In reply to comment #1)
> Is there anything special about forceReattach compared to detach and attach? (eg detach and attach a bug because ...?, or just slow because ...?)
Nothing special, it's just a shorthand.

[Dimitri Glazkov (Google)]

Also, it's something we should work to replace every instance of detach()/attach() (anti)pattern with something smarter or gentler. Rounding all occurrences into one place seems like a good start.

[Hajime Morrita]

Created attachment 94184 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 94184 [details]
Patch

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

Thank you for taking on this!

> Source/WebCore/ChangeLog:9
> +        - Replaced detach() following attach() with reattach()

Great name!

> Source/WebCore/dom/CharacterData.cpp:185
> +        reattach();

This will never hit unless the node is already attached, so the value of ReattachType is irrelevant here.

> Source/WebCore/dom/Node.h:454
> +    void reattach(ReattachType = ReattachIfAttached);

Looking at the callsites, I think ReattachAlways should be the default.

> Source/WebCore/dom/Node.h:746
> +    if (!attached()) {
> +        if (type == ReattachAlways)
> +            attach();

This seems wrong. Why are we attempting to attach here?

[Hajime Morrita]

Created attachment 94373 [details]
Patch

[Hajime Morrita]

Comment on attachment 94184 [details]
Patch

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

Hi Dimitri, thank you for the first round!  Here is update.

>> Source/WebCore/dom/CharacterData.cpp:185
>> +        reattach();
> 
> This will never hit unless the node is already attached, so the value of ReattachType is irrelevant here.

I splited reattach() and reattachIfAttached() an use reattach() here.

>> Source/WebCore/dom/Node.h:454
>> +    void reattach(ReattachType = ReattachIfAttached);
> 
> Looking at the callsites, I think ReattachAlways should be the default.

Now there are reattach() and reattachIfAttached().

>> Source/WebCore/dom/Node.h:746
>> +            attach();
> 
> This seems wrong. Why are we attempting to attach here?

Makes sense. I clarified this by splitting the function into two.

[Hajime Morrita]

Created attachment 94375 [details]
synced to HEAD

[Hajime Morrita]

Committed r87123: <http://trac.webkit.org/changeset/87123>