Bug 61011 - [Refactoring] attach() following detach() should be replaced with Node::forceReattach()
Summary: [Refactoring] attach() following detach() should be replaced with Node::force...
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:
Blocks:
 
Reported: 2011-05-17 18:18 PDT by Hajime Morrita
Modified: 2011-05-23 20:31 PDT (History)
2 users (show)

See Also:

Attachments
Patch (11.83 KB, patch)
2011-05-20 01:01 PDT, Hajime Morrita 		no flags Details | Formatted Diff | Diff
Patch (11.67 KB, patch)
2011-05-22 23:30 PDT, Hajime Morrita 		no flags Details | Formatted Diff | Diff
synced to HEAD (10.68 KB, patch)
2011-05-22 23:46 PDT, Hajime Morrita 		dglazkov: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2011-05-17 18:18:32 PDT 
We are going to have the method.
Comment 1 Dominic Cooney 2011-05-17 20:10:59 PDT 
Is there anything special about forceReattach compared to detach and attach? (eg detach and attach a bug because ...?, or just slow because ...?)
Comment 2 Hajime Morrita 2011-05-17 21:51:25 PDT 
(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.
Comment 3 Dimitri Glazkov (Google) 2011-05-18 07:38:51 PDT 
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.
Comment 4 Hajime Morrita 2011-05-20 01:01:06 PDT 
Created attachment 94184 [details]
Patch
Comment 5 Dimitri Glazkov (Google) 2011-05-20 09:12:46 PDT 
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?
Comment 6 Hajime Morrita 2011-05-22 23:30:25 PDT 
Created attachment 94373 [details]
Patch
Comment 7 Hajime Morrita 2011-05-22 23:32:26 PDT 
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.
Comment 8 Hajime Morrita 2011-05-22 23:46:58 PDT 
Created attachment 94375 [details]
synced to HEAD
Comment 9 Hajime Morrita 2011-05-23 20:31:56 PDT 
Committed r87123: <http://trac.webkit.org/changeset/87123>