Skip to content

Commit

Permalink
Oilpanize blink::ListGrid::GridCell
Browse files Browse the repository at this point in the history 
Changed GridCell to on-heap object by inheriting garbage-collected
GridLinkedListNodeBase and replacing wtf::DoublyLinkedList with
blink::GridLinkedList.

Correspondingly, manual memory management code was removed.

Bug: 1306747
Change-Id: I52c6925e80728d1e706d3bf93be526cfea388aec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3561203
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Commit-Queue: Chika Okuda <okudachika@google.com>
Cr-Commit-Position: refs/heads/main@{#988407}
  • Loading branch information
Chika Okuda authored and Chromium LUCI CQ committed Apr 4, 2022
1 parent 6ccb554 commit ce3d461
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 64 deletions.
60 changes: 23 additions & 37 deletions third_party/blink/renderer/core/layout/grid.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ Grid::GridIterator::GridIterator(GridTrackSizingDirection direction,

ListGrid::GridCell* ListGrid::GridTrack::Find(wtf_size_t index) const {
auto orthogonal_axis = OrthogonalDirection(direction_);
for (auto* cell = cells_.Head(); cell;
for (GridCell* cell = cells_->Head(); cell;
cell = cell->NextInDirection(direction_)) {
wtf_size_t cell_index = cell->Index(orthogonal_axis);
if (cell_index == index)
Expand All @@ -154,11 +154,11 @@ static int ComparePositions(wtf_size_t first, wtf_size_t second) {
return first < second ? -1 : (first != second);
}

DoublyLinkedList<ListGrid::GridCell>::AddResult ListGrid::GridTrack::Insert(
GridLinkedList<ListGrid::GridCell>::AddResult ListGrid::GridTrack::Insert(
GridCell* cell) {
cell->SetTraversalMode(direction_);

return cells_.Insert(
return cells_->Insert(
cell, [this](ListGrid::GridCell* first, ListGrid::GridCell* second) {
// This is ugly but we need to do this in order the
// DoublyLinkedList::Insert() algorithm to work at that code
Expand All @@ -171,7 +171,7 @@ DoublyLinkedList<ListGrid::GridCell>::AddResult ListGrid::GridTrack::Insert(
});
}

DoublyLinkedList<ListGrid::GridCell>::AddResult ListGrid::GridTrack::Insert(
GridLinkedList<ListGrid::GridCell>::AddResult ListGrid::GridTrack::Insert(
LayoutBox& item,
const GridSpan& span) {
auto compare_cells = [this](ListGrid::GridCell* first,
Expand All @@ -186,9 +186,9 @@ DoublyLinkedList<ListGrid::GridCell>::AddResult ListGrid::GridTrack::Insert(
wtf_size_t col_index = direction_ == kForColumns ? Index() : span.StartLine();
wtf_size_t row_index = direction_ == kForColumns ? span.StartLine() : Index();

auto result = cells_.Insert(
base::WrapUnique(new GridCell(row_index, col_index)), compare_cells);
auto* cell = result.node;
auto result = cells_->Insert(
MakeGarbageCollected<GridCell>(row_index, col_index), compare_cells);
GridCell* cell = result.node;
for (auto index : span) {
cell->AppendItem(item);

Expand All @@ -203,55 +203,37 @@ DoublyLinkedList<ListGrid::GridCell>::AddResult ListGrid::GridTrack::Insert(
direction_ == kForColumns ? Index() : index + 1;
wtf_size_t next_row_index =
direction_ == kForColumns ? index + 1 : Index();
auto next_cell =
base::WrapUnique(new GridCell(next_row_index, next_col_index));
if (InsertAfter(next_cell.get(), cell).is_new_entry)
next_cell.release();
GridCell* next_cell =
MakeGarbageCollected<GridCell>(next_row_index, next_col_index);
InsertAfter(next_cell, cell);
}
cell = cell->Next();
}
return result;
}

DoublyLinkedList<ListGrid::GridCell>::AddResult
ListGrid::GridTrack::InsertAfter(GridCell* cell, GridCell* insertion_point) {
GridLinkedList<ListGrid::GridCell>::AddResult ListGrid::GridTrack::InsertAfter(
GridCell* cell,
GridCell* insertion_point) {
insertion_point->SetTraversalMode(direction_);
cell->SetTraversalMode(direction_);
if (auto* next = insertion_point->Next()) {
if (GridCell* next = insertion_point->Next()) {
if (next == cell)
return {cell, false};
// We need to set the traversal mode for the next cell as we're
// going to insert in between and we need to properly update next_
// and prev_ pointers.
next->SetTraversalMode(direction_);
}
return cells_.InsertAfter(cell, insertion_point);
}

ListGrid::GridTrack::~GridTrack() {
// We destroy cells just when disposing columns as we don't want to
// double free them.
// TODO(svillar): we need to eventually get rid of this different
// destructors depending on the axis.
if (direction_ == kForRows) {
cells_.Clear();
return;
}

while (!cells_.IsEmpty()) {
cells_.Head()->SetTraversalMode(kForColumns);
if (cells_.Head()->Next())
cells_.Head()->Next()->SetTraversalMode(kForColumns);
delete cells_.RemoveHead();
}
return cells_->InsertAfter(cell, insertion_point);
}

const GridItemList& ListGrid::Cell(wtf_size_t row_index,
wtf_size_t column_index) const {
DEFINE_STATIC_LOCAL(const GridItemList, empty_vector, ());
for (auto* row = rows_.Head(); row; row = row->Next()) {
if (row->Index() == row_index) {
auto* cell = row->Find(column_index);
GridCell* cell = row->Find(column_index);
return cell ? cell->Items() : empty_vector;
}
if (row->Index() > row_index)
Expand Down Expand Up @@ -353,13 +335,17 @@ void ListGrid::GridCell::SetTraversalMode(GridTrackSizingDirection direction) {
if (direction == direction_)
return;
direction_ = direction;
std::swap(next_, next_ortho_);
std::swap(prev_, prev_ortho_);
GridCell* next = Next();
SetNext(next_ortho_);
next_ortho_ = next;
GridCell* prev = Prev();
SetPrev(prev_ortho_);
prev_ortho_ = prev;
}

ListGrid::GridCell* ListGrid::GridCell::NextInDirection(
GridTrackSizingDirection direction) const {
return direction_ == direction ? next_ : next_ortho_;
return direction_ == direction ? Next() : next_ortho_.Get();
}

std::unique_ptr<Grid::GridIterator> ListGrid::CreateIterator(
Expand Down
41 changes: 21 additions & 20 deletions third_party/blink/renderer/core/layout/grid.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@

#include "base/dcheck_is_on.h"
#include "third_party/blink/renderer/core/core_export.h"
#include "third_party/blink/renderer/core/layout/grid_linked_list.h"
#include "third_party/blink/renderer/core/layout/order_iterator.h"
#include "third_party/blink/renderer/core/style/grid_area.h"
#include "third_party/blink/renderer/core/style/grid_positions_resolver.h"
#include "third_party/blink/renderer/platform/heap/persistent.h"
#include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
#include "third_party/blink/renderer/platform/wtf/doubly_linked_list.h"
#include "third_party/blink/renderer/platform/wtf/linked_hash_set.h"
Expand All @@ -29,7 +31,6 @@ struct OrderedTrackIndexSetHashTraits : public HashTraits<wtf_size_t> {
}
};

// TODO(svillar): Perhaps we should use references here.
typedef Vector<UntracedMember<LayoutBox>, 1> GridItemList;
typedef LinkedHashSet<wtf_size_t, OrderedTrackIndexSetHashTraits>
OrderedTrackIndexSet;
Expand Down Expand Up @@ -175,10 +176,7 @@ class CORE_EXPORT ListGrid final : public Grid {
// only created for those cells which do have items inside. Each
// GridCell will be part of two different DLL, one representing the
// column and another one representing the row.
class GridCell final : public DoublyLinkedListNode<GridCell> {
USING_FAST_MALLOC(GridCell);
friend class WTF::DoublyLinkedListNode<GridCell>;

class GridCell final : public GridLinkedListNodeBase<GridCell> {
public:
GridCell(wtf_size_t row, wtf_size_t column) : row_(row), column_(column) {}

Expand All @@ -190,6 +188,12 @@ class CORE_EXPORT ListGrid final : public Grid {

const GridItemList& Items() const { return items_; }

void Trace(Visitor* visitor) const final {
visitor->Trace(prev_ortho_);
visitor->Trace(next_ortho_);
GridLinkedListNodeBase<GridCell>::Trace(visitor);
}

// DoublyLinkedListNode classes must provide a next_ and prev_
// pointers to the DoublyLinkedList class so that it could perform
// the list operations. In the case of GridCells we need them to
Expand All @@ -213,10 +217,8 @@ class CORE_EXPORT ListGrid final : public Grid {
GridCell* NextInDirection(GridTrackSizingDirection) const;

private:
GridCell* prev_{nullptr};
GridCell* next_{nullptr};
GridCell* prev_ortho_{nullptr};
GridCell* next_ortho_{nullptr};
Member<GridCell> prev_ortho_;
Member<GridCell> next_ortho_;

GridTrackSizingDirection direction_{kForColumns};
GridItemList items_;
Expand All @@ -239,22 +241,21 @@ class CORE_EXPORT ListGrid final : public Grid {

public:
GridTrack(wtf_size_t index, GridTrackSizingDirection direction)
: index_(index), direction_(direction) {}
: cells_(MakeGarbageCollected<GridLinkedList<GridCell>>()),
index_(index),
direction_(direction) {}

wtf_size_t Index() const { return index_; }
DoublyLinkedList<GridCell>::AddResult Insert(GridCell*);
DoublyLinkedList<GridCell>::AddResult InsertAfter(
GridCell* cell,
GridCell* insertion_point);
DoublyLinkedList<GridCell>::AddResult Insert(LayoutBox&, const GridSpan&);
GridLinkedList<GridCell>::AddResult Insert(GridCell*);
GridLinkedList<GridCell>::AddResult InsertAfter(GridCell* cell,
GridCell* insertion_point);
GridLinkedList<GridCell>::AddResult Insert(LayoutBox&, const GridSpan&);
GridCell* Find(wtf_size_t cell_index) const;

const DoublyLinkedList<GridCell>& Cells() const { return cells_; }

~GridTrack();
const GridLinkedList<GridCell>& Cells() const { return *cells_; }

private:
DoublyLinkedList<GridCell> cells_;
Persistent<GridLinkedList<GridCell>> cells_;
wtf_size_t index_;
GridTrackSizingDirection direction_;

Expand Down Expand Up @@ -308,7 +309,7 @@ class ListGridIterator final : public Grid::GridIterator {

private:
const ListGrid& grid_;
ListGrid::GridCell* cell_node_{nullptr};
Persistent<ListGrid::GridCell> cell_node_;
};

} // namespace blink
Expand Down
13 changes: 10 additions & 3 deletions third_party/blink/renderer/core/layout/grid_linked_list.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@

namespace blink {

template <typename NodeType>
class GridLinkedList;

// A class defining the type of node in the GridLinkedList should inherit
// GridLinkedListNodeBase. This will give previous and next pointer for the
// node, as well as apply garbage collection to all nodes.
Expand All @@ -62,17 +65,21 @@ class GridLinkedListNodeBase
: public GarbageCollected<GridLinkedListNodeBase<NodeType>> {
public:
GridLinkedListNodeBase() = default;
virtual ~GridLinkedListNodeBase() = default;

NodeType* Prev() const { return prev_; }
NodeType* Next() const { return next_; }
void SetPrev(NodeType* prev) { prev_ = prev; }
void SetNext(NodeType* next) { next_ = next; }

void Trace(Visitor* visitor) const {
virtual void Trace(Visitor* visitor) const {
visitor->Trace(prev_);
visitor->Trace(next_);
}

protected:
friend class GridLinkedList<NodeType>;
void SetPrev(NodeType* prev) { prev_ = prev; }
void SetNext(NodeType* next) { next_ = next; }

private:
Member<NodeType> prev_;
Member<NodeType> next_;
Expand Down
4 changes: 3 additions & 1 deletion third_party/blink/renderer/core/layout/grid_linked_list_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ namespace {
class IntNode : public GridLinkedListNodeBase<IntNode> {
public:
explicit IntNode(int value) : value_(value) {}
~IntNode() { destructor_calls.fetch_add(1, std::memory_order_relaxed); }
~IntNode() override {
destructor_calls.fetch_add(1, std::memory_order_relaxed);
}

int Value() const { return value_; }

Expand Down
6 changes: 3 additions & 3 deletions third_party/blink/renderer/core/layout/grid_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,13 +340,13 @@ TEST_F(GridTest, CellInsert) {
return;

auto track = base::WrapUnique(new ListGrid::GridTrack(0, kForColumns));
auto* cell = new ListGrid::GridCell(0, 0);
ListGrid::GridCell* cell = MakeGarbageCollected<ListGrid::GridCell>(0, 0);

auto result = track->Insert(cell);
EXPECT_TRUE(result.is_new_entry);
EXPECT_EQ(cell, result.node);

auto* cell2 = new ListGrid::GridCell(1, 0);
ListGrid::GridCell* cell2 = MakeGarbageCollected<ListGrid::GridCell>(1, 0);
result = track->Insert(cell2);
EXPECT_TRUE(result.is_new_entry);
EXPECT_EQ(cell2, result.node);
Expand All @@ -355,7 +355,7 @@ TEST_F(GridTest, CellInsert) {
EXPECT_FALSE(result.is_new_entry);
EXPECT_EQ(cell2, result.node);

auto* cell3 = new ListGrid::GridCell(2, 0);
ListGrid::GridCell* cell3 = MakeGarbageCollected<ListGrid::GridCell>(2, 0);
result = track->InsertAfter(cell3, cell2);
EXPECT_TRUE(result.is_new_entry);
EXPECT_EQ(cell3, result.node);
Expand Down

0 comments on commit ce3d461

Please sign in to comment.