Remove potentially tree-corrupting methods from CXFA_LayoutItem.
SetParent(), SetNextSibling() and SetFirstChild() all leave the
nodes of the tree in an inconsistent state and rely on the callers
to make futher adjustments to restore consistency. Use other
methods instead.
- Tidy InsertChild() method.
- Remove a no-op cast and a needless null assignment.
Change-Id: If1a2a3b052daa38638a99f69e4aeb9d42878de18
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/53391
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
diff --git a/xfa/fxfa/layout/cxfa_itemlayoutprocessor.cpp b/xfa/fxfa/layout/cxfa_itemlayoutprocessor.cpp
index a6ddfc8..2387b57 100644
--- a/xfa/fxfa/layout/cxfa_itemlayoutprocessor.cpp
+++ b/xfa/fxfa/layout/cxfa_itemlayoutprocessor.cpp
@@ -722,22 +722,21 @@
}
}
} else {
- pSecondLayoutItem->SetParent(pLayoutItem->GetParent());
- pSecondLayoutItem->SetNextSibling(pLayoutItem->GetNextSibling());
- pLayoutItem->SetNextSibling(pSecondLayoutItem);
+ if (pSecondLayoutItem->GetParent())
+ pSecondLayoutItem->GetParent()->RemoveChild(pSecondLayoutItem);
+ pLayoutItem->GetParent()->InsertChild(pLayoutItem, pSecondLayoutItem);
}
- CXFA_ContentLayoutItem* pChildren =
- ToContentLayoutItem(pLayoutItem->GetFirstChild());
- pLayoutItem->SetFirstChild(nullptr);
+ std::vector<CXFA_ContentLayoutItem*> children;
+ while (auto* pFirst = ToContentLayoutItem(pLayoutItem->GetFirstChild())) {
+ children.push_back(pFirst);
+ pLayoutItem->RemoveChild(pFirst);
+ }
float lHeightForKeep = 0;
float fAddMarginHeight = 0;
std::vector<CXFA_ContentLayoutItem*> keepLayoutItems;
- for (CXFA_ContentLayoutItem *pChildItem = pChildren, *pChildNext = nullptr;
- pChildItem; pChildItem = pChildNext) {
- pChildNext = ToContentLayoutItem(pChildItem->GetNextSibling());
- pChildItem->SetNextSibling(nullptr);
+ for (auto* pChildItem : children) {
if (fSplitPos <= fCurTopMargin + pChildItem->m_sPos.y + fCurBottomMargin +
kXFALayoutPrecision) {
if (!ExistContainerKeep(pChildItem->GetFormNode(), true)) {
@@ -798,7 +797,8 @@
CXFA_ContentLayoutItem* pLayoutItem = m_pLayoutItem;
if (pLayoutItem) {
m_pLayoutItem = ToContentLayoutItem(pLayoutItem->GetNextSibling());
- pLayoutItem->SetNextSibling(nullptr);
+ if (pLayoutItem->GetParent())
+ pLayoutItem->GetParent()->RemoveChild(pLayoutItem);
}
if (m_nCurChildNodeStage != Stage::kDone || !m_pOldLayoutItem)
@@ -1588,28 +1588,23 @@
}
}
- if (ToContentLayoutItem(m_pLayoutItem->GetFirstChild()) == pLastChild) {
- m_pLayoutItem->SetFirstChild(nullptr);
- } else {
- for (CXFA_LayoutItem* pLayoutNext = m_pLayoutItem->GetFirstChild();
- pLayoutNext; pLayoutNext = pLayoutNext->GetNextSibling()) {
- if (ToContentLayoutItem(pLayoutNext->GetNextSibling()) ==
- pLastChild) {
- pLayoutNext->SetNextSibling(nullptr);
- break;
- }
+ // TODO(tsepez): avoid looping if we can prove it is in the list.
+ for (CXFA_LayoutItem* pLayoutNext = m_pLayoutItem->GetFirstChild();
+ pLayoutNext; pLayoutNext = pLayoutNext->GetNextSibling()) {
+ if (ToContentLayoutItem(pLayoutNext) == pLastChild) {
+ m_pLayoutItem->RemoveChild(pLastChild);
+ break;
}
}
- CXFA_ContentLayoutItem* pLayoutNextTemp = ToContentLayoutItem(pLastChild);
+ CXFA_ContentLayoutItem* pLayoutNextTemp = pLastChild;
while (pLayoutNextTemp) {
- pLayoutNextTemp->SetParent(nullptr);
CXFA_ContentLayoutItem* pSaveLayoutNext =
ToContentLayoutItem(pLayoutNextTemp->GetNextSibling());
- pLayoutNextTemp->SetNextSibling(nullptr);
+ if (pLayoutNextTemp->GetParent())
+ pLayoutNextTemp->GetParent()->RemoveChild(pLayoutNextTemp);
pLayoutNextTemp = pSaveLayoutNext;
}
- pLastChild = nullptr;
}
while (m_pCurChildNode) {
diff --git a/xfa/fxfa/layout/cxfa_layoutitem.cpp b/xfa/fxfa/layout/cxfa_layoutitem.cpp
index 3dc65ce..543e99d 100644
--- a/xfa/fxfa/layout/cxfa_layoutitem.cpp
+++ b/xfa/fxfa/layout/cxfa_layoutitem.cpp
@@ -21,7 +21,7 @@
CXFA_LayoutProcessor* pDocLayout = pDocument->GetLayoutProcessor();
while (pNode) {
CXFA_LayoutItem* pNext = pNode->GetNextSibling();
- pNode->SetParent(nullptr);
+ pLayoutItem->RemoveChild(pNode);
pNotify->OnLayoutItemRemoving(pDocLayout, pNode);
XFA_ReleaseLayoutItem(pNode);
pNode = pNext;
@@ -103,14 +103,11 @@
CXFA_LayoutItem* pChildItem) {
if (pBeforeItem->m_pParent != this)
return;
- if (pChildItem->m_pParent)
- pChildItem->m_pParent = nullptr;
+ ASSERT(!pChildItem->m_pParent);
pChildItem->m_pParent = this;
-
- CXFA_LayoutItem* pExistingChildItem = pBeforeItem->m_pNextSibling;
+ pChildItem->m_pNextSibling = pBeforeItem->m_pNextSibling;
pBeforeItem->m_pNextSibling = pChildItem;
- pChildItem->m_pNextSibling = pExistingChildItem;
}
void CXFA_LayoutItem::RemoveChild(CXFA_LayoutItem* pChildItem) {
diff --git a/xfa/fxfa/layout/cxfa_layoutitem.h b/xfa/fxfa/layout/cxfa_layoutitem.h
index 3dd6754..c983668 100644
--- a/xfa/fxfa/layout/cxfa_layoutitem.h
+++ b/xfa/fxfa/layout/cxfa_layoutitem.h
@@ -32,11 +32,6 @@
CXFA_Node* GetFormNode() const { return m_pFormNode.Get(); }
void SetFormNode(CXFA_Node* pNode) { m_pFormNode = pNode; }
- // TODO(tsepez) replace these calls with AddChild() etc.
- void SetParent(CXFA_LayoutItem* pParent) { m_pParent = pParent; }
- void SetFirstChild(CXFA_LayoutItem* pChild) { m_pFirstChild = pChild; }
- void SetNextSibling(CXFA_LayoutItem* pSibling) { m_pNextSibling = pSibling; }
-
void AddChild(CXFA_LayoutItem* pChildItem);
void AddHeadChild(CXFA_LayoutItem* pChildItem);
void RemoveChild(CXFA_LayoutItem* pChildItem);
diff --git a/xfa/fxfa/layout/cxfa_layoutpagemgr.cpp b/xfa/fxfa/layout/cxfa_layoutpagemgr.cpp
index 917e748..63743ce 100644
--- a/xfa/fxfa/layout/cxfa_layoutpagemgr.cpp
+++ b/xfa/fxfa/layout/cxfa_layoutpagemgr.cpp
@@ -158,7 +158,7 @@
}
}
-void ReorderLayoutItemToTail(CXFA_ViewLayoutItem* pLayoutItem) {
+void ReorderLayoutItemToTail(CXFA_LayoutItem* pLayoutItem) {
CXFA_ViewLayoutItem* pParentLayoutItem =
ToViewLayoutItem(pLayoutItem->GetParent());
if (!pParentLayoutItem)
@@ -168,7 +168,7 @@
pParentLayoutItem->AddChild(pLayoutItem);
}
-void RemoveLayoutItem(CXFA_ViewLayoutItem* pLayoutItem) {
+void RemoveLayoutItem(CXFA_LayoutItem* pLayoutItem) {
CXFA_ViewLayoutItem* pParentLayoutItem =
ToViewLayoutItem(pLayoutItem->GetParent());
if (!pParentLayoutItem)
@@ -389,9 +389,7 @@
ASSERT(m_pTemplatePageSetRoot);
if (m_pPageSetLayoutItemRoot) {
- m_pPageSetLayoutItemRoot->SetParent(nullptr);
- m_pPageSetLayoutItemRoot->SetFirstChild(nullptr);
- m_pPageSetLayoutItemRoot->SetNextSibling(nullptr);
+ RemoveLayoutItem(m_pPageSetLayoutItemRoot);
m_pPageSetLayoutItemRoot->SetFormNode(m_pTemplatePageSetRoot);
} else {
m_pPageSetLayoutItemRoot =
@@ -649,7 +647,7 @@
while (pPrePageSet->GetNextSibling()) {
pPrePageSet = pPrePageSet->GetNextSibling()->AsViewLayoutItem();
}
- pPrePageSet->SetNextSibling(pPageSetLayoutItem);
+ pPrePageSet->GetParent()->InsertChild(pPrePageSet, pPageSetLayoutItem);
m_pPageSetCurRoot = pPageSetLayoutItem;
} else {
pParentPageSetLayout->AddChild(pPageSetLayoutItem);
@@ -1644,9 +1642,7 @@
if (pCurLayoutItem->GetFirstChild())
SaveLayoutItem(pCurLayoutItem);
- pCurLayoutItem->SetParent(nullptr);
- pCurLayoutItem->SetNextSibling(nullptr);
- pCurLayoutItem->SetFirstChild(nullptr);
+ RemoveLayoutItem(pCurLayoutItem);
if (!pCurLayoutItem->IsContentLayoutItem() &&
pCurLayoutItem->GetFormNode()->GetElementType() !=
XFA_Element::PageArea) {
@@ -1941,7 +1937,7 @@
CXFA_LayoutItem* pNode = pLayoutItem->GetFirstChild();
while (pNode) {
CXFA_LayoutItem* pNext = pNode->GetNextSibling();
- pNode->SetParent(nullptr);
+ pLayoutItem->RemoveChild(pNode);
XFA_ReleaseLayoutItem_NoPageArea(pNode);
pNode = pNext;
}
