Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(22)

Issue 944073006: Remove the concept of staticly positioned absolutes. (Closed)

Created:
5 years, 10 months ago by ojan
Modified:
5 years, 10 months ago
CC:
ojan, abarth-chromium, esprehn, mojo-reviews_chromium.org, qsr+mojo_chromium.org
Base URL:
git@github.com:domokit/mojo.git@position
Target Ref:
refs/heads/master
Project:
mojo
Visibility:
Public.

Description

Remove the concept of staticly positioned absolutes. On the web, if you set position:absolute, but not top/right/bottom/left, then the absolute goes where it would have gone if it wasn't positioned. The use-cases for this are slim and it introduces a lot of complexity to the engine. Also changes behavior in the presence of direction:rtl. On the web, direction:rtl and top/left:auto would sometimes set right:0. Instead we always position at 0,0 if the opposing values are auto. This removes the code for this positioning and allows simplifying a bunch of dirty bit handling code since we don't need to setNeedsLayout if lines move around or wrap differently. The test cases did change their output, but the new positioning all looks correct to me. Committed: https://chromium.googlesource.com/external/mojo/+/dfc6cda892ebde7a1d1bd42760d87625abff0c0d

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -229 lines) Patch
M sky/engine/core/rendering/RenderBlock.cpp View 1 chunk +0 lines, -9 lines 0 comments Download
M sky/engine/core/rendering/RenderBlockFlow.h View 2 chunks +0 lines, -5 lines 0 comments Download
M sky/engine/core/rendering/RenderBlockFlow.cpp View 3 chunks +0 lines, -29 lines 0 comments Download
M sky/engine/core/rendering/RenderBox.cpp View 8 chunks +8 lines, -75 lines 0 comments Download
M sky/engine/core/rendering/RenderFlexibleBox.h View 1 chunk +0 lines, -1 line 0 comments Download
M sky/engine/core/rendering/RenderFlexibleBox.cpp View 4 chunks +3 lines, -28 lines 0 comments Download
M sky/engine/core/rendering/RenderLayer.h View 2 chunks +0 lines, -10 lines 0 comments Download
M sky/engine/core/rendering/RenderLayer.cpp View 1 chunk +0 lines, -2 lines 0 comments Download
M sky/engine/core/rendering/RenderParagraph.cpp View 1 chunk +0 lines, -3 lines 0 comments Download
M sky/engine/core/rendering/line/BreakingContextInlineHeaders.h View 4 chunks +2 lines, -48 lines 0 comments Download
M sky/engine/core/rendering/line/LineBreaker.cpp View 1 chunk +4 lines, -6 lines 1 comment Download
M sky/engine/core/rendering/style/RenderStyle.h View 1 chunk +0 lines, -2 lines 0 comments Download
M sky/tests/layout/position-absolute-expected.txt View 1 chunk +6 lines, -6 lines 0 comments Download
M sky/tests/layout/position-absolute-pixels-expected.sky View 1 chunk +1 line, -1 line 0 comments Download
M sky/tests/lowlevel/layers.sky View 1 chunk +3 lines, -3 lines 0 comments Download
M sky/tests/lowlevel/layers-expected.sky View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 5 (2 generated)
abarth-chromium
rslgtm
5 years, 10 months ago (2015-02-21 04:16:04 UTC) #2
ojan
Committed patchset #1 (id:1) manually as dfc6cda892ebde7a1d1bd42760d87625abff0c0d.
5 years, 10 months ago (2015-02-21 04:27:05 UTC) #3
ojan
5 years, 10 months ago (2015-02-21 04:29:19 UTC) #5
Message was sent while issue was closed.
I landed this. Elliott, please take a look at this and just sanity check. I can
undo if you disagree or spot some code you think I screwed up.

https://codereview.chromium.org/944073006/diff/1/sky/engine/core/rendering/li...
File sky/engine/core/rendering/line/LineBreaker.cpp (right):

https://codereview.chromium.org/944073006/diff/1/sky/engine/core/rendering/li...
sky/engine/core/rendering/line/LineBreaker.cpp:36: &&
object->style()->isOriginalDisplayInlineType()) {
I maintained behavior here, but I'm not really sure what this code does. I
expect we want to remove it from Sky.

Powered by Google App Engine
This is Rietveld 408576698