Bug 75290 – [Refactoring] Migration over TreeScopes should be done by its own class.

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

[Hajime Morrita]

Moving node over Document and/or TreeScope is complicated task involved by Node, Document and TreeScope.
Currently whole code is on Node.cpp and it makes the "migration/move" intention unclear.
It should be done by its own class.

This is preparation of Bug 59816, which needs a certain size of change related tree scope migration.

[Hajime Morrita]

Created attachment 120633 [details]
Patch

[Hajime Morrita]

Created attachment 120635 [details]
Rebased to ToT

[Ryosuke Niwa]

Comment on attachment 120635 [details]
Rebased to ToT

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

> Source/WebCore/dom/TreeScopeMigration.cpp:42
> +    if (needsMigration()) {

It seems oxymoron to ask this class whether we need to migrate the node or not given the class is called "migration".
i.e. it seems odd to instantiate this class when no "migration" is needed.

> Source/WebCore/dom/TreeScopeMigration.cpp:44
> +        m_departureDocument = migrationRoot->document();
> +        m_destinationDocument = destination->document();

Do we really need these member variables? It seems like we can just get away with putting these statements inside execute().

> Source/WebCore/dom/TreeScopeMigration.h:32
> +class TreeScopeMigration {

I'm not sure if "migration" is a descriptive name for this class. How about TreeScopeAdopter?

[Dominic Cooney]

Comment on attachment 120635 [details]
Rebased to ToT

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

> Source/WebCore/dom/TreeScopeMigration.cpp:84
> +    ASSERT(shouldMigrateOverDocument());

Why does migrateShadowOverDocument recurse into migrateOverDocument, but migrateOverDocument doesn’t migrate shadows?

> Source/WebCore/dom/TreeScopeMigration.h:46
> +    bool shouldMigrateOverDocument() const { return m_departureDocument != m_destinationDocument; }

I found this "migrate over document" a bit confusing. What about:

shouldMigrateOverDocument -> isMigratingToDifferentDocument
isMigratingToDocument -> isMigratingToDocumentTreeScope
migrateShadowOverDocument, migrateOverDocument -> migrateToDifferentDocument

> Source/WebCore/dom/TreeScopeMigration.h:48
> +    void migrateShadowOverDocument(ShadowRoot*) const;

Would this be simpler if you had a single method that tested node type to handle ShadowRoot differently? Because I am not sure this makes sense:

ShadowRoot* s = …;
migrateOverDocument(s);
migrateShadowOverDocument(s);

I guess it can happen if the "migration root" is a ShadowRoot. Is it intended?

[Gyuyoung Kim]

Comment on attachment 120635 [details]
Rebased to ToT

Attachment 120635 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11038504

[Hajime Morrita]

Created attachment 121214 [details]
Patch

[Hajime Morrita]

Ryosuke, Dominic, Thanks for reviewing at the year end! 
I updated the new year's patch.

(In reply to comment #3)
> (From update of attachment 120635 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120635&action=review
> 
> > Source/WebCore/dom/TreeScopeMigration.cpp:42
> > +    if (needsMigration()) {
> 
> It seems oxymoron to ask this class whether we need to migrate the node or not given the class is called "migration".
Sounds reasonable. tried different approach.

> > Source/WebCore/dom/TreeScopeMigration.cpp:44
> > +        m_departureDocument = migrationRoot->document();
> > +        m_destinationDocument = destination->document();
> 
> Do we really need these member variables? It seems like we can just get away with putting these statements inside execute().
Moved them to local variables

> 
> > Source/WebCore/dom/TreeScopeMigration.h:32
> > +class TreeScopeMigration {
> 
> I'm not sure if "migration" is a descriptive name for this class. How about TreeScopeAdopter?
This is a good point. I renamed it to TreeScopeAdopter and
Node::migrateTo() is now TreeScope::adoptIfNeeded().

(In reply to comment #4)
> (From update of attachment 120635 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120635&action=review
> 
> > Source/WebCore/dom/TreeScopeMigration.cpp:84
> > +    ASSERT(shouldMigrateOverDocument());
> 
> Why does migrateShadowOverDocument recurse into migrateOverDocument, but migrateOverDocument doesn’t migrate shadows?
Because migrateOverDocument is for shadowRoot, whose tree nodes keep the shadow root 
as their treescopes. We need to change tree scope of the light dom tree.

> 
> > Source/WebCore/dom/TreeScopeMigration.h:46
> > +    bool shouldMigrateOverDocument() const { return m_departureDocument != m_destinationDocument; }
> 
> I found this "migrate over document" a bit confusing. What about:
> 
> shouldMigrateOverDocument -> isMigratingToDifferentDocument
> isMigratingToDocument -> isMigratingToDocumentTreeScope
> migrateShadowOverDocument, migrateOverDocument -> migrateToDifferentDocument
Well, I agree that these namings are confusing. I tried different name.
Some member funcitons are removed due to other changes.

> 
> > Source/WebCore/dom/TreeScopeMigration.h:48
> > +    void migrateShadowOverDocument(ShadowRoot*) const;
> 
> Would this be simpler if you had a single method that tested node type to handle ShadowRoot differently? Because I am not sure this makes sense:
> 
> ShadowRoot* s = …;
> migrateOverDocument(s);
> migrateShadowOverDocument(s);
I guess you thought shadowRootFor() is a kind of dynamic cast but it isn't.
So type checking doesn't make sense here.

> 
> I guess it can happen if the "migration root" is a ShadowRoot. Is it intended?
Yes.

I found that the original naming was really confusing so I tried another names
which emphasizes the difference between tree traversals and per-node operations. 
Could you take another look?

[Hajime Morrita]

Created attachment 121217 [details]
Updated to ToT

[Dominic Cooney]

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

I think this is better. Some more comments inline.

> Source/WebCore/dom/TreeScope.cpp:148
> +    if (!adopter.isStayingSameScope())

What about changing this method to isChangingScope? It makes this test simpler to read.

Also the _adopter_ isn’t moving to a new scope or staying in the same scope. It is the nodes being processed that get moved. So adopter.isChangingScope or willChangeScope or something else might be slightly more precise?

> Source/WebCore/dom/TreeScopeAdopter.cpp:43
> +    ASSERT(isStayingSameScope());

The naming of these are confusing… moveTreeToNewScope asserts that the adoption isn’t moving to a new scope?

> Source/WebCore/dom/TreeScopeAdopter.cpp:51
> +    bool shouldMoveToNewDocument = departureDocument != destinationDocument;

What about isMovingToNewDocument or willMoveToNewDocument?

> Source/WebCore/dom/TreeScopeAdopter.cpp:58
> +        node->setTreeScope(destinationDocument == m_destination ? 0 : m_destination);

It would be really nice to replace access to document to be via treescope to simplify this. Would it be easier to make that change first?

[Hajime Morrita]

Created attachment 121218 [details]
Patch

[Hajime Morrita]

> > Source/WebCore/dom/TreeScope.cpp:148
> > +    if (!adopter.isStayingSameScope())
> 
> What about changing this method to isChangingScope? It makes this test simpler to read.
> 
> Also the _adopter_ isn’t moving to a new scope or staying in the same scope. It is the nodes being processed that get moved. So adopter.isChangingScope or willChangeScope or something else might be slightly more precise?
Renamed.

> 
> > Source/WebCore/dom/TreeScopeAdopter.cpp:43
> > +    ASSERT(isStayingSameScope());
> 
> The naming of these are confusing… moveTreeToNewScope asserts that the adoption isn’t moving to a new scope?
Ugh, it's not only confusing but actually wrong...

> 
> > Source/WebCore/dom/TreeScopeAdopter.cpp:51
> > +    bool shouldMoveToNewDocument = departureDocument != destinationDocument;
> 
> What about isMovingToNewDocument or willMoveToNewDocument?
Done.

> 
> > Source/WebCore/dom/TreeScopeAdopter.cpp:58
> > +        node->setTreeScope(destinationDocument == m_destination ? 0 : m_destination);
> 
> It would be really nice to replace access to document to be via treescope to simplify this. Would it be easier to make that change first?
TreeScope handling will be changed significantly in the coming patch. 
So I prefer to keep this as is to make the difference from original code obvious.

[Dominic Cooney]

Comment on attachment 121218 [details]
Patch

Looks good to me.

[Ryosuke Niwa]

Comment on attachment 121218 [details]
Patch

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

> Source/WebCore/dom/Node.cpp:432
> +    return hasRareData() && rareData()->nodeLists();

Obtaining rareData is very expensive according to my profiling results. We should avoid calling it many times.

> Source/WebCore/dom/TreeScopeAdopter.cpp:39
> +TreeScopeAdopter::TreeScopeAdopter(Node* migrationRoot, TreeScope* destination)
> +    : m_migrationRoot(migrationRoot)
> +    , m_destination(destination)
> +    , m_departure(migrationRoot->treeScope())
> +{
> +    ASSERT(destination);
> +}

We should probably make this constructor inline so that adoptIfNeeded doesn't need to create an extra function call for constructing TreeScopeAdopter when the adoption is not needed.

> Source/WebCore/dom/TreeScopeAdopter.cpp:57
> +        // Setting the new document tree scope will be handled implicitly
> +        // by setDocument() below.

This comment is out-dated, right?

> Source/WebCore/dom/TreeScopeAdopter.cpp:77
> +void TreeScopeAdopter::moveTreeToNewDocument(ShadowRoot* root, Document* departureDocument, Document* destinationDocument) const

Maybe we should rename this function to moveShadowTreeToNewDocument to make the context more explicit.

> Source/WebCore/dom/TreeScopeAdopter.h:54
> +    Node* m_migrationRoot;
> +    TreeScope* m_destination;
> +    TreeScope* m_departure;

I get confused by these variable names. Given that we no longer use the word migration, we should at least rename m_migrationRoot to something like m_subtreeToAdopt.

Also, we should probably rename m_destination and m_departure to m_newTreeScope and m_oldTreeScope; m_departure is particularly confusing since "departure" is a process and not a thing.

> Source/WebCore/dom/TreeScopeAdopter.h:60
> +inline ShadowRoot* TreeScopeAdopter::shadowRootFor(Node* node)
> +{
> +    return node->isElementNode() ? toElement(node)->shadowRoot() : 0;
> +}

Can this be moved into .cpp file?

[Hajime Morrita]

Created attachment 121230 [details]
Patch

[Hajime Morrita]

Comment on attachment 121218 [details]
Patch

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

Hi Ryosuke, thanks for the review! I updated it for the another round.

>> Source/WebCore/dom/TreeScopeAdopter.cpp:39
>> +}
> 
> We should probably make this constructor inline so that adoptIfNeeded doesn't need to create an extra function call for constructing TreeScopeAdopter when the adoption is not needed.

Sure. Moved to the header file.

>> Source/WebCore/dom/TreeScopeAdopter.cpp:57
>> +        // by setDocument() below.
> 
> This comment is out-dated, right?

Right. Removed.

>> Source/WebCore/dom/TreeScopeAdopter.h:54
>> +    TreeScope* m_departure;
> 
> I get confused by these variable names. Given that we no longer use the word migration, we should at least rename m_migrationRoot to something like m_subtreeToAdopt.
> 
> Also, we should probably rename m_destination and m_departure to m_newTreeScope and m_oldTreeScope; m_departure is particularly confusing since "departure" is a process and not a thing.

Renamed.

>> Source/WebCore/dom/TreeScopeAdopter.h:60
>> +}
> 
> Can this be moved into .cpp file?

Moved to .cpp file and turned it into a free floating static function.

[Hajime Morrita]

Created attachment 121231 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 121231 [details]
Patch

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

> Source/WebCore/dom/TreeScopeAdopter.cpp:72
> +void TreeScopeAdopter::moveTreeToNewDocument(Node* root, Document* oldDocument, Document* newDocument) const

No rename for this function?

> Source/WebCore/dom/TreeScopeAdopter.cpp:83
> +static bool didMoveToNewDocumentWasCalled;
> +static Document* oldDocumentDidMoveToNewDocumentWasCalledWith;

Please initialize these variables.

[Hajime Morrita]

(In reply to comment #17)
> (From update of attachment 121231 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121231&action=review
> 
> > Source/WebCore/dom/TreeScopeAdopter.cpp:72
> > +void TreeScopeAdopter::moveTreeToNewDocument(Node* root, Document* oldDocument, Document* newDocument) const
I noticed that this is not necessarily to be ShadowRoot specific so changed the signature instead. 

> 
> No rename for this function?
> 
> > Source/WebCore/dom/TreeScopeAdopter.cpp:83
> > +static bool didMoveToNewDocumentWasCalled;
> > +static Document* oldDocumentDidMoveToNewDocumentWasCalledWith;
> 
> Please initialize these variables.
Sure, will do before land.

Thanks for your midnight review!

[Hajime Morrita]

Created attachment 121244 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 121244 [details]
Patch for landing

Rejecting attachment 121244 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11128366

[Hajime Morrita]

Committed r104259: <http://trac.webkit.org/changeset/104259>

[Ryosuke Niwa]

Caused assertion failures on Qt: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Debug/r104269%20%2820128%29/results.html

[Ryosuke Niwa]

Reopen the bug since the patch was rolled out in http://trac.webkit.org/changeset/104271.

[Hajime Morrita]

Committed r104528: <http://trac.webkit.org/changeset/104528>