Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1441443003:
[Line Layout API] Convert LayoutBlockFlowLine to use new line layout API (Closed)
|
Created:
5 years, 1 month ago by pilgrim_google Modified:
5 years, 1 month ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Line Layout API] Convert LayoutBlockFlowLine to use new line layout API
There are just a few calls and one comparison. No functional changes.
This CL is not dependent on any other pending CLs.
BUG=499321
Committed: https://crrev.com/3a4f6915366d65af0decaa4b413f21ab318a46df
Cr-Commit-Position: refs/heads/master@{#360467}
Patch Set 1 #Patch Set 2 : fix crashes by checking whether line layout item is text before making a text out of it #
Total comments: 1
Patch Set 3 : promote selectionState() to LineLayoutItem, simplify caller #
Messages
Total messages: 31 (11 generated)
Description was changed from ========== [Line Layout API] Convert LayoutBlockFlowLine to use new line layout API There are just a few calls and one comparison. No functional changes. BUG=499321 ========== to ========== [Line Layout API] Convert LayoutBlockFlowLine to use new line layout API There are just a few calls and one comparison. No functional changes. This CL is not dependent on any other pending CLs. BUG=499321 ==========
pilgrim@chromium.org changed reviewers: + eae@chromium.org, jsbell@chromium.org, leviw@chromium.org, ojan@chromium.org
The CQ bit was checked by leviw@chromium.org
lgtm Easy one!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441443003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441443003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pilgrim@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441443003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441443003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by pilgrim@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441443003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441443003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pilgrim@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441443003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441443003/1
I'm beginning to suspect that this patch is actually broken. Can't imagine why, though.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Please re-review.
https://codereview.chromium.org/1441443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1441443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:286: if (!rootHasSelectedChildren && box->lineLayoutItem().isText() && LineLayoutText(box->lineLayoutItem()).selectionState() != SelectionNone) selectionState exists for non-Text objects. Why do we now only check it for Text?
On 2015/11/18 at 18:41:41, leviw wrote: > https://codereview.chromium.org/1441443003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp (right): > > https://codereview.chromium.org/1441443003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:286: if (!rootHasSelectedChildren && box->lineLayoutItem().isText() && LineLayoutText(box->lineLayoutItem()).selectionState() != SelectionNone) > selectionState exists for non-Text objects. Why do we now only check it for Text? My mistake, perhaps. In the API, selectionState() is only defined on LineLayoutText. Shall I move it up to LineLayoutItem and fix this caller?
On 2015/11/18 at 19:26:15, pilgrim wrote: > On 2015/11/18 at 18:41:41, leviw wrote: > > https://codereview.chromium.org/1441443003/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp (right): > > > > https://codereview.chromium.org/1441443003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:286: if (!rootHasSelectedChildren && box->lineLayoutItem().isText() && LineLayoutText(box->lineLayoutItem()).selectionState() != SelectionNone) > > selectionState exists for non-Text objects. Why do we now only check it for Text? > > My mistake, perhaps. In the API, selectionState() is only defined on LineLayoutText. Shall I move it up to LineLayoutItem and fix this caller? That would seem like the best course forward to me.
On 2015/11/18 at 22:17:28, leviw wrote: > On 2015/11/18 at 19:26:15, pilgrim wrote: > > On 2015/11/18 at 18:41:41, leviw wrote: > > > https://codereview.chromium.org/1441443003/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp (right): > > > > > > https://codereview.chromium.org/1441443003/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:286: if (!rootHasSelectedChildren && box->lineLayoutItem().isText() && LineLayoutText(box->lineLayoutItem()).selectionState() != SelectionNone) > > > selectionState exists for non-Text objects. Why do we now only check it for Text? > > > > My mistake, perhaps. In the API, selectionState() is only defined on LineLayoutText. Shall I move it up to LineLayoutItem and fix this caller? > > That would seem like the best course forward to me. Done in Patch 3.
The CQ bit was checked by leviw@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441443003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441443003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3a4f6915366d65af0decaa4b413f21ab318a46df Cr-Commit-Position: refs/heads/master@{#360467} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
