Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2913773002:
[WIP][b:eae_mywip_paint] Paint Selection NG.
|
Created:
3 years, 6 months ago by yoichio Modified:
3 years, 5 months ago CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, cbiesinger, chromium-reviews, dgrogan+ng_chromium.org, eae+blinkwatch, glebl+reviews_chromium.org, jchaffraix+rendering, lchoi+reviews_chromium.org, leviw+renderwatch, ojan+watch_chromium.org, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPaint Selection NG.
Design doc:
https://goo.gl/xzbb6E
This patch depends on Emil's inline painter patch:
https://codereview.chromium.org/2842983002
and Xiaochengs's offset mapping:
https://codereview.chromium.org/2943573002/
LayoutSelection::SelectionStartEnd()
- It returns NG Offset if needs.
NGTextFragmentPainter.cpp:
SelectionStartEnd() returns offsets of NGInlineNodeData.text_content_ to be painted
as selection.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Patch Set 1 : Basic #Patch Set 2 : + WhiteSpaceCollapsing #Patch Set 3 : update #Patch Set 4 : rebase #Patch Set 5 : update #Patch Set 6 : update #Patch Set 7 : update #
Total comments: 23
Patch Set 8 : tmp #
Total comments: 8
Messages
Total messages: 38 (25 generated)
Description was changed from ========== [WIP] Create offset map through InlineItemsBuilder::Append BUG= ========== to ========== [WIP] Create offset map through InlineItemsBuilder::Append BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ==========
The CQ bit was checked by yoichio@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: linux_chromium_tsan_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 yoichio@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.
Description was changed from ========== [WIP] Create offset map through InlineItemsBuilder::Append BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ========== to ========== [WIP] Create offset map through InlineItemsBuilder::Append BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ==========
Description was changed from ========== [WIP] Create offset map through InlineItemsBuilder::Append BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ========== to ========== [WIP] Create offset map through InlineItemsBuilder::Append BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== [WIP] Create offset map through InlineItemsBuilder::Append BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [WIP] Create offset map through InlineItemsBuilder::Append BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by yoichio@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== [WIP] Create offset map through InlineItemsBuilder::Append BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Paint Selection NG. Design doc: https://goo.gl/fACpS6 Points: LayoutSelection.h::SelectionPaintRange - Add start/end Node and assigned FlatTreeSelection.start/end into them at LayoutSelection::Commit() before painting. LayoutSelection::SelectionStartEnd() - It returns NG Offset if needs. Current mapping algorithm is bad because create full map on stack each time. It should be better(see the doc). NGInlineItemsBuilder: Add Vector<int>* offset_mapping_. This logs offset shrinking around whitespace collapsing. NGTextFragmentPainter.cpp: SelectionStartEnd() returns offsets of NGInlineNodeData.text_content_ to be painted as selection. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
yoichio@chromium.org changed reviewers: + eae@chromium.org, kojii@chromium.org, xiaochengh@chromium.org, yosin@chromium.org
Description was changed from ========== Paint Selection NG. Design doc: https://goo.gl/fACpS6 Points: LayoutSelection.h::SelectionPaintRange - Add start/end Node and assigned FlatTreeSelection.start/end into them at LayoutSelection::Commit() before painting. LayoutSelection::SelectionStartEnd() - It returns NG Offset if needs. Current mapping algorithm is bad because create full map on stack each time. It should be better(see the doc). NGInlineItemsBuilder: Add Vector<int>* offset_mapping_. This logs offset shrinking around whitespace collapsing. NGTextFragmentPainter.cpp: SelectionStartEnd() returns offsets of NGInlineNodeData.text_content_ to be painted as selection. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Paint Selection NG. Design doc: https://goo.gl/fACpS6 This patch depends on Emil's inline painter patch: https://codereview.chromium.org/2842983002 Points: LayoutSelection.h::SelectionPaintRange - Add start/end Node and assigned FlatTreeSelection.start/end into them at LayoutSelection::Commit() before painting. LayoutSelection::SelectionStartEnd() - It returns NG Offset if needs. Current mapping algorithm is bad because create full map on stack each time. It should be better(see the doc). NGInlineItemsBuilder: Add Vector<int>* offset_mapping_. This logs offset shrinking around whitespace collapsing. NGTextFragmentPainter.cpp: SelectionStartEnd() returns offsets of NGInlineNodeData.text_content_ to be painted as selection. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:358: const Position& start = selection_in_dom.ComputeStartPosition(); Why don't you use flat tree version? https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:413: return CalcSelectionNG(frame_selection); So, for painting selection, we need to have NG version of: - VisibleSelection - MostBackwardCaretPosition() - MostForwardCaretPosition()
https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:350: static SelectionPaintRange* CalcSelectionNG( If this function traverse flat tree and works on legacy tree, it is nice and we can replace current implementation.
https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:297: return offset; // Legay. Return DOM offset; nit: s/Legay/Legacy/ https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:360: LayoutObject* const start_layout_object = |start_layout_object| can be nullptr. So, we need to find boundary layout object from start to end. https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:362: LayoutObject* const end_layout_object = end.AnchorNode()->GetLayoutObject(); |end_layout_object_| can be nullptr. So, we need to find boundary layout from end to start.
It seems memory inefficient to store the mapping value for every offset... I'll upload another WIP patch implementing the APIs defined in https://goo.gl/CJbxky. I hope that's going to capture all cases and reduce duplicate work.
https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:285: runner = runner->ContainingBlock()) { why ContainingBlock(), not Parent()? https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:558: NGTextOffsetMap::Builder::Builder() : offset_map_(new NGTextOffsetMap) {} so this part will be removed and not for review, correct? https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.cc (right): https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.cc:189: if (offset_mapping_) { Changes in this file is not for review, correct? https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.h (right): https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.h:37: : items_(items), offset_mapping_(offset_mapping) {} this isn't for review, correct? https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/NGBoxFragmentPainter.cpp (right): https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/NGBoxFragmentPainter.cpp:47: if (DrawingRecorder::UseCachedDrawingIfPossible( Can explain what this is? https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp (right): https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp:79: static std::pair<int, int> SelectionStartEnd( Can add explanation what this function does and what the return is? Shouldn't |int| be |unsigned|? Layout always use unsigned for offset, I'm not sure about paint though. https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp:86: return {0, text_fragment->Text().length()}; why it computes min/max to text_fragment->Start/EndOffset() but this part does not? https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp:88: const std::pair<int, int> startend_in_ngblockflow = I can't understand how this works...so you get two |int|s from FrameView. What are they, and why can you subtract text_fragment->StartOffset() from it? Can you explain? https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp:95: 0, startend_in_ngblockflow.first - text_fragment->StartOffset()); This might not work, because StartOffset() is unsigned, no? https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp:160: selection_style.fill_color = Color(1.0f, 0.0f, 0.0f, 1.0f); this is temporary, correct? Can you add TODO comment?
Description was changed from ========== Paint Selection NG. Design doc: https://goo.gl/fACpS6 This patch depends on Emil's inline painter patch: https://codereview.chromium.org/2842983002 Points: LayoutSelection.h::SelectionPaintRange - Add start/end Node and assigned FlatTreeSelection.start/end into them at LayoutSelection::Commit() before painting. LayoutSelection::SelectionStartEnd() - It returns NG Offset if needs. Current mapping algorithm is bad because create full map on stack each time. It should be better(see the doc). NGInlineItemsBuilder: Add Vector<int>* offset_mapping_. This logs offset shrinking around whitespace collapsing. NGTextFragmentPainter.cpp: SelectionStartEnd() returns offsets of NGInlineNodeData.text_content_ to be painted as selection. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Paint Selection NG. Design doc: https://goo.gl/fACpS6 This patch depends on Emil's inline painter patch: https://codereview.chromium.org/2842983002 and Xiaochengs's offset mapping: https://codereview.chromium.org/2943573002/ Out of review: - editing/BUILD.gn - editing/LayoutSelectionTest.cpp - layout/ng/inline/ng_inline_items_builder.* - layout/ng/inline/ng_inline_node_data You should ignore LayoutSelection::NGTextOffsetMap and its test. This is temporary solution and we use Xiaochengs's mapping API. Points: LayoutSelection.h::SelectionPaintRange - Add start/end Node and assigned FlatTreeSelection.start/end into them at LayoutSelection::Commit() before painting. LayoutSelection::SelectionStartEnd() - It returns NG Offset if needs. NGTextFragmentPainter.cpp: SelectionStartEnd() returns offsets of NGInlineNodeData.text_content_ to be painted as selection. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Patchset #8 (id:140001) has been deleted
Patchset #8 (id:160001) has been deleted
Patchset #8 (id:180001) has been deleted
Description was changed from ========== Paint Selection NG. Design doc: https://goo.gl/fACpS6 This patch depends on Emil's inline painter patch: https://codereview.chromium.org/2842983002 and Xiaochengs's offset mapping: https://codereview.chromium.org/2943573002/ Out of review: - editing/BUILD.gn - editing/LayoutSelectionTest.cpp - layout/ng/inline/ng_inline_items_builder.* - layout/ng/inline/ng_inline_node_data You should ignore LayoutSelection::NGTextOffsetMap and its test. This is temporary solution and we use Xiaochengs's mapping API. Points: LayoutSelection.h::SelectionPaintRange - Add start/end Node and assigned FlatTreeSelection.start/end into them at LayoutSelection::Commit() before painting. LayoutSelection::SelectionStartEnd() - It returns NG Offset if needs. NGTextFragmentPainter.cpp: SelectionStartEnd() returns offsets of NGInlineNodeData.text_content_ to be painted as selection. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Paint Selection NG. Design doc: https://goo.gl/fACpS6 This patch depends on Emil's inline painter patch: https://codereview.chromium.org/2842983002 and Xiaochengs's offset mapping: https://codereview.chromium.org/2943573002/ LayoutSelection::SelectionStartEnd() - It returns NG Offset if needs. NGTextFragmentPainter.cpp: SelectionStartEnd() returns offsets of NGInlineNodeData.text_content_ to be painted as selection. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Because it seems to me that NGTextPainter fails to paint partially and we don't have selection background painting yet and this patch also depends on Xiaocheng's another patch, it is bit hard to make this patch forward. WDYT, everyone? Design doc first? https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:285: runner = runner->ContainingBlock()) { On 2017/06/16 11:25:56, kojii wrote: > why ContainingBlock(), not Parent()? Perhaps its my misunderstanding. If LayoutBlockFlow is not only LayoutBlock, it should be |Parent()|. https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:350: static SelectionPaintRange* CalcSelectionNG( On 2017/06/15 09:51:27, yosin_UTC9 wrote: > If this function traverse flat tree and works on legacy tree, it is nice and we > can replace current implementation. I will rewrite w/o most.*CaretPosition() functions. https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp (right): https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp:79: static std::pair<int, int> SelectionStartEnd( On 2017/06/16 11:25:57, kojii wrote: > Can add explanation what this function does and what the return is? > > Shouldn't |int| be |unsigned|? Layout always use unsigned for offset, I'm not > sure about paint though. This returns 0-origin offset range in NGPhysicalTextFragment to be painted as selection. Yes, this should be unsigned but |selection_start| in NGTextFragmentPainter::Paint is defined as |int|. I guess because legacy InlineTextBoxPainter defines selection range as int pair, where it uses (-1,-1) as invalid range, and Emil just follows it. https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp:86: return {0, text_fragment->Text().length()}; On 2017/06/16 11:25:57, kojii wrote: > why it computes min/max to text_fragment->Start/EndOffset() but this part does > not? Ah, it should be {text_fragment->StartOffst(), text_fragment->EndOffset()} What is 'it'? Could you explain more? https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp:88: const std::pair<int, int> startend_in_ngblockflow = On 2017/06/16 11:25:57, kojii wrote: > I can't understand how this works...so you get two |int|s from FrameView. What > are they, and why can you subtract text_fragment->StartOffset() from it? Can you > explain? They are index in NGInlineNodeData.text_content_, but NGTextPainter.paint(start, end) demands offset in NGPhysixalTextFragment. https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp:160: selection_style.fill_color = Color(1.0f, 0.0f, 0.0f, 1.0f); On 2017/06/16 11:25:57, kojii wrote: > this is temporary, correct? Can you add TODO comment? Yes, just for testing. Remove from reviewing.
FYI, I removed unrelated code.
https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp (right): https://codereview.chromium.org/2913773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp:88: const std::pair<int, int> startend_in_ngblockflow = On 2017/06/21 at 08:16:43, yoichio wrote: > On 2017/06/16 11:25:57, kojii wrote: > > I can't understand how this works...so you get two |int|s from FrameView. What > > are they, and why can you subtract text_fragment->StartOffset() from it? Can you > > explain? > They are index in NGInlineNodeData.text_content_, but NGTextPainter.paint(start, end) demands > offset in NGPhysixalTextFragment. But you get data from Frame, without passing text_fragment. How could it find the relevant NGInlineNodeData? https://codereview.chromium.org/2913773002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2913773002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:297: runner = runner->ContainingBlock()) { Parent()
In general, more comments are appreciated. Hard to know what functions are doing, or what variables are, esp. when reading other area's code. and what is the goal of this CL? Keep rebasing and land after Xiaocheng and Emil's CL landed? It looks like it's using different API than what Xiaocheng is working on?
On 2017/06/21 18:15:37, kojii wrote: > In general, more comments are appreciated. Hard to know what functions are > doing, or what variables are, esp. when reading other area's code. > > and what is the goal of this CL? Keep rebasing and land after Xiaocheng and > Emil's CL landed? I don't think landing this CL itself because serious review on a CL depending on unshipped CLs is not efficient. I want to show new strategy how we paint selection. It looks like it's using different API than what Xiaocheng is > working on? I'm using my WIP mapping to debug but I would use Xiaocheng's. I wrote new doc about this CL w/o mapping: https://goo.gl/xzbb6E kojii@, yosin@, xiaochengh@, eae@, Please take a look.
Description was changed from ========== Paint Selection NG. Design doc: https://goo.gl/fACpS6 This patch depends on Emil's inline painter patch: https://codereview.chromium.org/2842983002 and Xiaochengs's offset mapping: https://codereview.chromium.org/2943573002/ LayoutSelection::SelectionStartEnd() - It returns NG Offset if needs. NGTextFragmentPainter.cpp: SelectionStartEnd() returns offsets of NGInlineNodeData.text_content_ to be painted as selection. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Paint Selection NG. Design doc: https://goo.gl/xzbb6E This patch depends on Emil's inline painter patch: https://codereview.chromium.org/2842983002 and Xiaochengs's offset mapping: https://codereview.chromium.org/2943573002/ LayoutSelection::SelectionStartEnd() - It returns NG Offset if needs. NGTextFragmentPainter.cpp: SelectionStartEnd() returns offsets of NGInlineNodeData.text_content_ to be painted as selection. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Would you mind setting up an offline doc/CL review mtg?
Update mtg memo. https://codereview.chromium.org/2913773002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2913773002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:355: static SelectionPaintRange CalcSelectionNG( Its strange that we have 3 code pass for legacy, ng-ng, ng-legacy. We should unify them to legacy and ng. https://codereview.chromium.org/2913773002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:376: if (runner->IsText() && runner->GetNode()->IsTextNode()) { LayoutText will gone. https://codereview.chromium.org/2913773002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:385: } br is LayouObject.isText(). https://codereview.chromium.org/2913773002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/LayoutSelection.h (right): https://codereview.chromium.org/2913773002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/LayoutSelection.h:81: int EndOffset() const; Having LayoutNGBlockFlow, textcontent offset and item_index. Having item_index to get InlineItem and its Layoutobject in O(1). https://codereview.chromium.org/2913773002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp (right): https://codereview.chromium.org/2913773002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp:79: static std::pair<int, int> SelectionStartEnd( Much comment is appreciate https://codereview.chromium.org/2913773002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp:86: return {0, text_fragment->Text().length()}; Rerturn painting full; define lewngth; https://codereview.chromium.org/2913773002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/NGTextFragmentPainter.cpp:93: .LayoutSelectionStartEnd(); Describe much more. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
