Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2511283003:
Set the inline position of floats a bit later. (Closed)
|
Created:
4 years, 1 month ago by mstensho (USE GERRIT) Modified:
4 years ago Reviewers:
szager1 CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet the inline position of floats a bit later.
No need to do it so early, since nobody cares about its position at this point.
This means that there's also no need to update it after having been pushed down
by pagination. As long as we set it before positioning subsequent floats or
other types of content, we're good.
Also store margins as local variables. No huge gain, apart from prettier code
with fewer breaks.
No behavior changes intended.
Committed: https://crrev.com/73b64e1b7ea5f8d962e3538564e49c3c92bdff46
Cr-Commit-Position: refs/heads/master@{#434388}
Patch Set 1 #Patch Set 2 : Rebase master. No conflicts here. #Messages
Total messages: 43 (23 generated)
The CQ bit was checked by mstensho@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mstensho@opera.com changed reviewers: + szager@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping?
lgtm
The CQ bit was checked by mstensho@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mstensho@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mstensho@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mstensho@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by mstensho@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by mstensho@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mstensho@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:
While running git apply --index -p1;
error: patch failed:
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:3684
error: third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp: patch does
not apply
Patch: third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
Index: third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
b/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
index
1b64042e1b0b87a92d3e41a6e0e1298a907d381f..4fd15ddbf52edec421766681501ec08afea2cf9a
100644
--- a/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
@@ -3668,9 +3668,6 @@ LayoutUnit LayoutBlockFlow::positionAndLayoutFloat(
// FIXME Investigate if this can be removed. crbug.com/370006
child.setMayNeedPaintInvalidation();
- LayoutUnit childLogicalLeftMargin = style()->isLeftToRightDirection()
- ? marginStartForChild(child)
- : marginEndForChild(child);
logicalTopMarginEdge = std::max(
logicalTopMarginEdge, lowestFloatLogicalBottom(child.style()->clear()));
@@ -3684,16 +3681,13 @@ LayoutUnit LayoutBlockFlow::positionAndLayoutFloat(
}
}
+ LayoutUnit marginBefore = marginBeforeForChild(child);
+ LayoutUnit marginAfter = marginAfterForChild(child);
LayoutPoint floatLogicalLocation =
computeLogicalLocationForFloat(floatingObject, logicalTopMarginEdge);
logicalTopMarginEdge = floatLogicalLocation.y();
- setLogicalLeftForFloat(floatingObject, floatLogicalLocation.x());
-
- setLogicalLeftForChild(child,
- floatLogicalLocation.x() + childLogicalLeftMargin);
- setLogicalTopForChild(child,
- logicalTopMarginEdge + marginBeforeForChild(child));
+ setLogicalTopForChild(child, logicalTopMarginEdge + marginBefore);
SubtreeLayoutScope layoutScope(child);
if (isPaginated && !child.needsLayout())
@@ -3708,12 +3702,7 @@ LayoutUnit LayoutBlockFlow::positionAndLayoutFloat(
floatLogicalLocation = computeLogicalLocationForFloat(
floatingObject, newLogicalTopMarginEdge);
logicalTopMarginEdge = floatLogicalLocation.y();
- setLogicalLeftForFloat(floatingObject, floatLogicalLocation.x());
-
- setLogicalLeftForChild(child,
- floatLogicalLocation.x() +
childLogicalLeftMargin);
- setLogicalTopForChild(child,
- logicalTopMarginEdge +
marginBeforeForChild(child));
+ setLogicalTopForChild(child, logicalTopMarginEdge + marginBefore);
if (child.isLayoutBlock())
child.setChildNeedsLayout(MarkOnlyThis);
@@ -3721,11 +3710,15 @@ LayoutUnit LayoutBlockFlow::positionAndLayoutFloat(
}
}
+ LayoutUnit childLogicalLeftMargin = style()->isLeftToRightDirection()
+ ? marginStartForChild(child)
+ : marginEndForChild(child);
+ setLogicalLeftForChild(child,
+ floatLogicalLocation.x() + childLogicalLeftMargin);
+ setLogicalLeftForFloat(floatingObject, floatLogicalLocation.x());
setLogicalTopForFloat(floatingObject, logicalTopMarginEdge);
-
setLogicalHeightForFloat(floatingObject, logicalHeightForChild(child) +
- marginBeforeForChild(child) +
- marginAfterForChild(child));
+ marginBefore + marginAfter);
if (ShapeOutsideInfo* shapeOutside = child.shapeOutsideInfo())
shapeOutside->setReferenceBoxLogicalSize(logicalSizeForChild(child));
The CQ bit was checked by mstensho@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from szager@chromium.org Link to the patchset: https://codereview.chromium.org/2511283003/#ps20001 (title: "Rebase master. No conflicts here.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480016665846850,
"parent_rev": "dcbfabf467c9d7f78d75e8b01bb089625c9691f2", "commit_rev":
"ed9d35714ef7bff9fcae35d4c9d6ee0569c21f78"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Set the inline position of floats a bit later. No need to do it so early, since nobody cares about its position at this point. This means that there's also no need to update it after having been pushed down by pagination. As long as we set it before positioning subsequent floats or other types of content, we're good. Also store margins as local variables. No huge gain, apart from prettier code with fewer breaks. No behavior changes intended. ========== to ========== Set the inline position of floats a bit later. No need to do it so early, since nobody cares about its position at this point. This means that there's also no need to update it after having been pushed down by pagination. As long as we set it before positioning subsequent floats or other types of content, we're good. Also store margins as local variables. No huge gain, apart from prettier code with fewer breaks. No behavior changes intended. Committed: https://crrev.com/73b64e1b7ea5f8d962e3538564e49c3c92bdff46 Cr-Commit-Position: refs/heads/master@{#434388} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/73b64e1b7ea5f8d962e3538564e49c3c92bdff46 Cr-Commit-Position: refs/heads/master@{#434388} |
