New issue
Advanced search Search tips

Issue 687184 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocking:
issue 79180



Sign in to add a comment

[css-grid] Move the track sizing algorithm to its own class

Project Member Reported by svil...@igalia.com, Jan 31 2017

Issue description

The LayoutGrid class is becoming too big and unmanageable (>3.6k LoC). One of the reasons why it's so big is because it contains all the track sizing algorithm code. Although it looks like it naturally belong to the LayoutGrid object the truth is that it creates some difficulties:

1- requires lots of methods that are used just by the algorithm
2- requires a temporary data structure (GridSizingData) to store the results of the different steps of the algorithm
3- this data structure has to be passed to tons of methods
4- the indefinite/definite behaviour is mixed in method implementations so it's difficult to track the code path for each one

The proposal is the following:
1- Move Grid class out of LayoutGrid
2- Move the track sizing algorithm code to a new class GridTrackSizingAlgorithm
3- Move all the GridSizingData attributes to private members of GridTrackSizingAlgorithm and do not pass it as argument anymore
4- Use a strategy pattern to configure the behaviour of the algorithm for the definite size and indefinite size cases

Once the proposal is implemented, all the track sizing algorithm code will be isolated in a single file. The caller would configure the algorithm (with some setup() method) passing some parameters like the axis (row|columns), the available space, the free space.... After that a run() method will perform the track sizing, and that's it. The LayoutAlgorithm will later use some public API to retrieve the required info.

The track sizing algorithm will delegate any task with different behaviours for indefinite/definite sizes to another class called GridTrackSizingAlgorithmStrategy. That abstract class will have two children: on for definite sizes and another one for indefinite ones. The GridTrackSizingAlgorithm will create the strategy (depending on the values passed to setup()) which will be totally transparent to the client.
 

Comment 1 by svil...@igalia.com, Jan 31 2017

Components: Blink>Layout>Grid
Status: Assigned (was: Untriaged)

Comment 2 by r...@igalia.com, Jan 31 2017

Blocking: 79180
Cc: jfernan...@igalia.com r...@igalia.com
Labels: OS-All
Owner: svil...@igalia.com
Status: Started (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4cb3f97bf30b086d273330d03c92fcc8b6ea7070

commit 4cb3f97bf30b086d273330d03c92fcc8b6ea7070
Author: svillar <svillar@igalia.com>
Date: Wed Feb 01 12:07:16 2017

[css-grid] Move the track sizing algorithm to its own class

This is about moving the track sizing algorithm code out of LayoutGrid to
a new class GridTrackSizingAlgorithm, making the LayoutGrid more compact and
easy to use. A side effect of this patch is the removal of GridSizingData as
it is no longer needed. All the data structures in that class were
transferred to GridTrackSizingAlgorithm as attribute members.

The algorithm execution starts with the call to run(). It's mandatory to call
setup() before any call to run() in order to properly configure the behaviour
of the algorithm. You can call setup() & run() multiple times for a single layout
operation (normally twice, one for columns and another one for rows). The
algorithm uses an state machine to verify that the client issues the calls in
the proper order (i.e. first columns and then rows). After finishing the layout,
the client should call reset() to allow the algorithm to perform cleanups and
to prepare itself for another round of calls.

In order to implement the different behaviours of the algorithm depending on
whether the available size is definite or not, a strategy pattern was
implemented in the GridTrackSizingAlgorithmStrategy class. It has two
subclasses, one for definite sizes and another one for indefinite ones.

BUG= 687184 

Review-Url: https://codereview.chromium.org/2654533003
Cr-Commit-Position: refs/heads/master@{#447489}

[modify] https://crrev.com/4cb3f97bf30b086d273330d03c92fcc8b6ea7070/third_party/WebKit/Source/core/layout/BUILD.gn
[add] https://crrev.com/4cb3f97bf30b086d273330d03c92fcc8b6ea7070/third_party/WebKit/Source/core/layout/Grid.cpp
[add] https://crrev.com/4cb3f97bf30b086d273330d03c92fcc8b6ea7070/third_party/WebKit/Source/core/layout/Grid.h
[add] https://crrev.com/4cb3f97bf30b086d273330d03c92fcc8b6ea7070/third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp
[add] https://crrev.com/4cb3f97bf30b086d273330d03c92fcc8b6ea7070/third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h
[modify] https://crrev.com/4cb3f97bf30b086d273330d03c92fcc8b6ea7070/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/4cb3f97bf30b086d273330d03c92fcc8b6ea7070/third_party/WebKit/Source/core/layout/LayoutGrid.h

Comment 4 by svil...@igalia.com, Feb 15 2017

Status: Fixed (was: Started)
This was already done.

Sign in to add a comment