Chromium Code Reviews
Help | Chromium Project | Sign in
(1)

Issue 2513953002: [css-grid] Avoid double loop in positioned objects layout (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 1 week ago by Manuel Rego
Modified:
3 months, 1 week ago
CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, svillar, blink-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[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

Patch Set 1 #

Total comments: 4

Patch Set 2 : New version overriding layoutPositionedObjects() #

Messages

Total messages: 17 (5 generated)
Manuel Rego
This is a refactoring on positioned objects code. It avoid an unneeded extra loop for ...
5 months, 1 week ago (2016-11-18 15:34:56 UTC) #2
jfernandez
https://codereview.chromium.org/2513953002/diff/1/third_party/WebKit/Source/core/layout/LayoutBlock.h File third_party/WebKit/Source/core/layout/LayoutBlock.h (right): https://codereview.chromium.org/2513953002/diff/1/third_party/WebKit/Source/core/layout/LayoutBlock.h#newcode363 third_party/WebKit/Source/core/layout/LayoutBlock.h:363: virtual void layoutPositionedObject(LayoutBox*, I think it'd be better to ...
5 months, 1 week ago (2016-11-18 16:18:22 UTC) #3
Manuel Rego
Thanks for the review, uploaded new version of the patch. https://codereview.chromium.org/2513953002/diff/1/third_party/WebKit/Source/core/layout/LayoutBlock.h File third_party/WebKit/Source/core/layout/LayoutBlock.h (right): https://codereview.chromium.org/2513953002/diff/1/third_party/WebKit/Source/core/layout/LayoutBlock.h#newcode363 ...
5 months, 1 week ago (2016-11-18 16:39:28 UTC) #5
cbiesinger
So, I'd almost rather you didn't submit this. LayoutBlock has a lot of functions already ...
5 months, 1 week ago (2016-11-19 01:19:00 UTC) #6
Manuel Rego
On 2016/11/19 01:19:00, cbiesinger wrote: > So, I'd almost rather you didn't submit this. LayoutBlock ...
5 months ago (2016-11-25 12:33:48 UTC) #7
Manuel Rego
On 2016/11/19 01:19:00, cbiesinger wrote: > So, I'd almost rather you didn't submit this. LayoutBlock ...
5 months ago (2016-11-25 12:33:49 UTC) #8
cbiesinger
OK. Sounds good. Let me know if/when you want to proceed with this patch.
4 months, 3 weeks ago (2016-12-06 21:43:45 UTC) #9
Manuel Rego
Finally I had time to do progress on this. I managed to fix http://crbug.com/618996 and ...
3 months, 1 week ago (2017-01-18 13:27:53 UTC) #10
cbiesinger
ok, lgtm. I trust that the code you moved is identical to the previous code ...
3 months, 1 week ago (2017-01-19 21:55:40 UTC) #11
Manuel Rego
On 2017/01/19 21:55:40, cbiesinger wrote: > ok, lgtm. I trust that the code you moved ...
3 months, 1 week ago (2017-01-19 22:00:03 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2513953002/20001
3 months, 1 week ago (2017-01-19 22:00:43 UTC) #14
commit-bot: I haz the power
3 months, 1 week ago (2017-01-20 00:50:58 UTC) #17
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/81c3ee6670a520935f024398c8aa...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46