[css-grid] Avoid double loop in positioned objects layout

[css-grid] Avoid double loop in positioned objects layout

Currently we were doing 2 loops to layout positioned objects:
* First we calculate the extra offsets for each object.
* Then we call layoutPositionedObjects() which does a new loop.

This patch extracts the common logic for each positioned object
in a new method layoutPositionedObject().
Now in LayoutGrid::layoutPositionedObjects() we'll call
this new method and that way we'll be doing only 1 loop
over the positioned objects.

No new tests, no change of behavior.

BUG=273898
Review-Url: https://codereview.chromium.org/2513953002
Cr-Commit-Position: refs/heads/master@{#444897}
Committed: https://chromium.googlesource.com/chromium/src/+/81c3ee6670a520935f024398c8aa23c7e379ff58

[Manuel Rego, 2016-11-18 15:34:56]

rego@igalia.com changed reviewers:
+ cbiesinger@chromium.org, jfernandez@igalia.com, svillar@igalia.com

[Manuel Rego, 2016-11-18 15:34:56]

This is a refactoring on positioned objects code.
It avoid an unneeded extra loop for grid.

[jfernandez, 2016-11-18 16:18:22]

https://codereview.chromium.org/2513953002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlock.h (right):

https://codereview.chromium.org/2513953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.h:363: virtual void
layoutPositionedObject(LayoutBox*,
I think it'd be better to keep layoutPositionedObjects as virtual, so we can
re-implement it in the subclasses and call layoutPositionedObject when needed. I
understood from the rest of the patch that you don't want to change the behavior
of layoutPositionedObject, but doing some extra operations.

My suggestion tries to avoid calling n-times to the virtual function, so we can
do it just once an re-implement the loop using the new layoutPositionedObject
function, which contains the logic you want to execute.

https://codereview.chromium.org/2513953002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2513953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2485: void
LayoutGrid::layoutPositionedObject(LayoutBox* child,
Ditto.

https://codereview.chromium.org/2513953002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.h (right):

https://codereview.chromium.org/2513953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.h:186: void
layoutPositionedObject(
Better keep overriding the layoutPositionedObjects function, as explained
before.

[Manuel Rego, 2016-11-18 16:38:47]

Description was changed from

==========
[css-grid] Avoid double loop in positioned objects layout

Currently we were doing 2 loops to layout positioned objects:
* First we calculate the extra offsets for each object.
* Then we call layoutPositionedObjects() which does a new loop.

This patch extracts the common logic for each positioned object
in a new method layoutPositionedObject().
Now in LayoutGrid, we'll override this new method and that way
we'll be doing only 1 loop over the positioned objects.

No new tests, no change of behavior.

BUG=273898
==========

to

==========
[css-grid] Avoid double loop in positioned objects layout

Currently we were doing 2 loops to layout positioned objects:
* First we calculate the extra offsets for each object.
* Then we call layoutPositionedObjects() which does a new loop.

This patch extracts the common logic for each positioned object
in a new method layoutPositionedObject().
Now in LayoutGrid::layoutPositionedObjects() we'll call
this new method and that way we'll be doing only 1 loop
over the positioned objects.

No new tests, no change of behavior.

BUG=273898
==========

[Manuel Rego, 2016-11-18 16:39:28]

Thanks for the review, uploaded new version of the patch.

https://codereview.chromium.org/2513953002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlock.h (right):

https://codereview.chromium.org/2513953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.h:363: virtual void
layoutPositionedObject(LayoutBox*,
Good point, I'll keep the previous approach.

[cbiesinger, 2016-11-19 01:19:00]

So, I'd almost rather you didn't submit this. LayoutBlock has a lot of functions
already and it's hard to understand them all and their interactions. This adds
another one to it, and I'm not sure this is an optimization that matters in
practice. Do you have an example page where this optimization makes a
difference?

[Manuel Rego, 2016-11-25 12:33:48]

On 2016/11/19 01:19:00, cbiesinger wrote:
> So, I'd almost rather you didn't submit this. LayoutBlock has a lot of
functions
> already and it's hard to understand them all and their interactions. This adds
> another one to it, and I'm not sure this is an optimization that matters in
> practice. Do you have an example page where this optimization makes a
> difference?

This was just a first step of a refactoring with the goal
to fix this bug: http://crbug.com/618996
And then support alignment on positioned items too.

The plan was more or less:
* Get rid of extraInline|BlockOffset in LayoutBox:
  For that I need this patch first, then I can use local
  variables to store this information and use it after calling
  layoutPositionedObject().
* Set location manually for positioned items
  (like we do for regular items when they're aligned).

However, I didn't have time this week to work on this,
so I didn't manage to finish this refactoring.

We can hold on this patch and if I'm not successful with the plan
to fix #618996, then we might probably won't need this.

Thanks for the feedback.

[Manuel Rego, 2016-11-25 12:33:49]

On 2016/11/19 01:19:00, cbiesinger wrote:
> So, I'd almost rather you didn't submit this. LayoutBlock has a lot of
functions
> already and it's hard to understand them all and their interactions. This adds
> another one to it, and I'm not sure this is an optimization that matters in
> practice. Do you have an example page where this optimization makes a
> difference?

This was just a first step of a refactoring with the goal
to fix this bug: http://crbug.com/618996
And then support alignment on positioned items too.

The plan was more or less:
* Get rid of extraInline|BlockOffset in LayoutBox:
  For that I need this patch first, then I can use local
  variables to store this information and use it after calling
  layoutPositionedObject().
* Set location manually for positioned items
  (like we do for regular items when they're aligned).

However, I didn't have time this week to work on this,
so I didn't manage to finish this refactoring.

We can hold on this patch and if I'm not successful with the plan
to fix #618996, then we might probably won't need this.

Thanks for the feedback.

[cbiesinger, 2016-12-06 21:43:45]

OK. Sounds good. Let me know if/when you want to proceed with this patch.

[Manuel Rego, 2017-01-18 13:27:53]

Finally I had time to do progress on this.

I managed to fix http://crbug.com/618996
and get rid of extraInline|BlockOffset,
but I'd need this patch first, or do 3 loops.

The reason is that to layout a positioned item we need to do:
1) Place it on the grid (taking into account that "auto" lines
   refer to padding lines).
   Which defines its grid area (width and height)
2) Call layoutPositionedObject(), so it deals with all the
   offsets computations (e.g. "left: 10px; right: 20px;").
3) Place the item manually with setLogicalLocation(),
   like we do in layoutGridItems() in order to be able
   to align it properly.

Right now we've 1 loop to do 1) and another one to do 2)
which use the extraInline|BlockOffset from LayoutBox
(We didn't implement alignment yet, so no 3) so far).
If we use only 1 loop, we can first do 1) and
keep the offsets in local variables, then do 2) and
later use the offsets in 3).

So the code is going to be simpler if we have this patch.

@cbiesinger what do you think?

[cbiesinger, 2017-01-19 21:55:40]

ok, lgtm. I trust that the code you moved is identical to the previous code
(Rietveld is terrible at showing moves)

[Manuel Rego, 2017-01-19 22:00:03]

On 2017/01/19 21:55:40, cbiesinger wrote:
> ok, lgtm. I trust that the code you moved is identical to the previous code
> (Rietveld is terrible at showing moves)

Yes is the same code, but it's hard to see due to indentation.

[Manuel Rego, 2017-01-19 22:00:08]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2017-01-19 22:00:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 00:50:27]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484863208525310,
"parent_rev": "ede80b706285aa0052cf154d89c49f081a74b9a5", "commit_rev":
"81c3ee6670a520935f024398c8aa23c7e379ff58"}

[commit-bot: I haz the power, 2017-01-20 00:50:57]

Description was changed from

==========
[css-grid] Avoid double loop in positioned objects layout

Currently we were doing 2 loops to layout positioned objects:
* First we calculate the extra offsets for each object.
* Then we call layoutPositionedObjects() which does a new loop.

This patch extracts the common logic for each positioned object
in a new method layoutPositionedObject().
Now in LayoutGrid::layoutPositionedObjects() we'll call
this new method and that way we'll be doing only 1 loop
over the positioned objects.

No new tests, no change of behavior.

BUG=273898
==========

to

==========
[css-grid] Avoid double loop in positioned objects layout

Currently we were doing 2 loops to layout positioned objects:
* First we calculate the extra offsets for each object.
* Then we call layoutPositionedObjects() which does a new loop.

This patch extracts the common logic for each positioned object
in a new method layoutPositionedObject().
Now in LayoutGrid::layoutPositionedObjects() we'll call
this new method and that way we'll be doing only 1 loop
over the positioned objects.

No new tests, no change of behavior.

BUG=273898

Review-Url: https://codereview.chromium.org/2513953002
Cr-Commit-Position: refs/heads/master@{#444897}
Committed:
https://chromium.googlesource.com/chromium/src/+/81c3ee6670a520935f024398c8aa...
==========

[commit-bot: I haz the power, 2017-01-20 00:50:58]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/81c3ee6670a520935f024398c8aa...