Bug 75290

Created attachment 120633 [details] Patch

Created attachment 120635 [details] Rebased to ToT

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?

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?

Comment on attachment 120635 [details] Rebased to ToT Attachment 120635 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11038504

Created attachment 121214 [details] Patch

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?

Created attachment 121217 [details] Updated to ToT

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?

Created attachment 121218 [details] Patch

> > 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.

Comment on attachment 121218 [details] Patch Looks good to me.

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?

Created attachment 121230 [details] Patch

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.

Created attachment 121231 [details] Patch

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.

(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!

Created attachment 121244 [details] Patch for landing

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

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

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

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

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