Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2840883002:
Check if fragment's block size fits into a layout opportunity. (Closed)
|
Created:
3 years, 8 months ago by Gleb Lanbin Modified:
3 years, 7 months ago CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-w3ctests_chromium.org, cbiesinger, chromium-reviews, dgrogan+ng_chromium.org, eae+blinkwatch, glebl+reviews_chromium.org, jchaffraix+rendering, leviw+renderwatch, ojan+watch_chromium.org, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck if fragment's block size fits into a layout opportunity.
Since we started using LayoutOpportunity iterator for positioning BFC
fragments, we can't longer rely on the assumption that
FindLayoutOpportunityForFragment will be used for floats only.
List of changes:
- Change FindLayoutOpportunityForFragment to check if fragment's block
size fits into a layout opportunity.
- Change NGInlineLayoutAlgorithm to avoid adding font's descent to
line_bottom if the text only includes object replacement characters
I need it to fix floats-wrap-top-below-002l-ref.xht.
BUG=635619
Review-Url: https://codereview.chromium.org/2840883002
Cr-Commit-Position: refs/heads/master@{#467875}
Committed: https://chromium.googlesource.com/chromium/src/+/f9d77f770956b1a2e948d54117d066a349680aa5
Patch Set 1 : git rebase-update #
Total comments: 1
Patch Set 2 : normal-flow/max-width-110.xht fails on mac(layoutng and legacy) #
Total comments: 1
Patch Set 3 : add a comment to line_box.Metrics().descent hack #Patch Set 4 : git rebase-update #Patch Set 5 : git rebase-update #
Messages
Total messages: 38 (31 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by glebl@chromium.org 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...
Patchset #1 (id:20001) has been deleted
glebl@chromium.org changed reviewers: + ikilpatrick@chromium.org, kojii@chromium.org
lgtm, but wait for koji. https://codereview.chromium.org/2840883002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc (right): https://codereview.chromium.org/2840883002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc:479: if (!Node()->Text().IsAllSpecialCharacters<IsObjectReplacementCharacter>()) can you add a comment here what this is doing?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by glebl@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/nit. https://codereview.chromium.org/2840883002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc (right): https://codereview.chromium.org/2840883002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc:479: if (!Node()->Text().IsAllSpecialCharacters<IsObjectReplacementCharacter>()) This doesn't look correct, but I admit ascent/descent for replaced elements needs more work. Can you leave a comment that this maybe wrong but needed for floats-wrap-top-below-002l-ref.xht, so that I can remember when I revisit?
On 2017/04/26 00:51:34, kojii wrote: > lgtm w/nit. > > https://codereview.chromium.org/2840883002/diff/60001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc > (right): > > https://codereview.chromium.org/2840883002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc:479: > if (!Node()->Text().IsAllSpecialCharacters<IsObjectReplacementCharacter>()) > This doesn't look correct, but I admit ascent/descent for replaced elements > needs more work. > > Can you leave a comment that this maybe wrong but needed for > floats-wrap-top-below-002l-ref.xht, so that I can remember when I revisit? done. it's not just one test. I think it helped to fix ~ 82 tests in total but I agree that it looks strange.
The CQ bit was checked by glebl@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by glebl@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm, hope I can figure this out in not to far future. Thank you.
The CQ bit was checked by glebl@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by glebl@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by glebl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org, kojii@chromium.org Link to the patchset: https://codereview.chromium.org/2840883002/#ps120001 (title: "git rebase-update")
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": 120001, "attempt_start_ts": 1493350181692260,
"parent_rev": "b68f094954f3a2173d54f748535c62528e1a6f35", "commit_rev":
"f9d77f770956b1a2e948d54117d066a349680aa5"}
Message was sent while issue was closed.
Description was changed from ========== Check if fragment's block size fits into a layout opportunity. Since we started using LayoutOpportunity iterator for positioning BFC fragments, we can't longer rely on the assumption that FindLayoutOpportunityForFragment will be used for floats only. List of changes: - Change FindLayoutOpportunityForFragment to check if fragment's block size fits into a layout opportunity. - Change NGInlineLayoutAlgorithm to avoid adding font's descent to line_bottom if the text only includes object replacement characters I need it to fix floats-wrap-top-below-002l-ref.xht. BUG=635619 ========== to ========== Check if fragment's block size fits into a layout opportunity. Since we started using LayoutOpportunity iterator for positioning BFC fragments, we can't longer rely on the assumption that FindLayoutOpportunityForFragment will be used for floats only. List of changes: - Change FindLayoutOpportunityForFragment to check if fragment's block size fits into a layout opportunity. - Change NGInlineLayoutAlgorithm to avoid adding font's descent to line_bottom if the text only includes object replacement characters I need it to fix floats-wrap-top-below-002l-ref.xht. BUG=635619 Review-Url: https://codereview.chromium.org/2840883002 Cr-Commit-Position: refs/heads/master@{#467875} Committed: https://chromium.googlesource.com/chromium/src/+/f9d77f770956b1a2e948d54117d0... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/f9d77f770956b1a2e948d54117d0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
