Simplify LayoutSelection::CalcSelection()

Simplify LayoutSelection::CalcSelection()

This CL changes
- Unify code whether selection is inside TextControl or not
- Split block cursor logic.

Then CalcSelection does
- return FrameSelection.VSinFlat if no block cursor
- if block cursor, extending selection with NextPositionOf(kGrapheme);

BUG=708453
Review-Url: https://codereview.chromium.org/2953743002
Cr-Commit-Position: refs/heads/master@{#482194}
Committed: https://chromium.googlesource.com/chromium/src/+/80471d8d1cbf8b08eaa893832f4d735765881a02

[yoichio, 2017-06-22 06:35:17]

The CQ bit was checked by yoichio@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 06:35:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 07:46:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 07:47:00]

Dry run: 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_...)

[yoichio, 2017-06-23 02:03:38]

Description was changed from

==========
Simplify CalcSelection

BUG=708453
==========

to

==========
Simplify CalcSelection

This CL changes
- Unify code whether selection is inside TextControl or not
- Split block cursor logic.

Then CalcSelection does
- return FrameSelection.VSinFlat if no block cursor
- if block cursor, extending selection with NextPositionOf(kGrapheme);

BUG=708453
==========

[yoichio, 2017-06-23 02:03:39]

yoichio@chromium.org changed reviewers:
+ yosin@chromium.org

[yoichio, 2017-06-23 02:04:00]

Bot failure looks not related this change.

[yoichio, 2017-06-23 02:04:32]

Description was changed from

==========
Simplify CalcSelection

This CL changes
- Unify code whether selection is inside TextControl or not
- Split block cursor logic.

Then CalcSelection does
- return FrameSelection.VSinFlat if no block cursor
- if block cursor, extending selection with NextPositionOf(kGrapheme);

BUG=708453
==========

to

==========
Simplify LayoutSelection::CalcSelection()

This CL changes
- Unify code whether selection is inside TextControl or not
- Split block cursor logic.

Then CalcSelection does
- return FrameSelection.VSinFlat if no block cursor
- if block cursor, extending selection with NextPositionOf(kGrapheme);

BUG=708453
==========

[yosin_UTC9, 2017-06-23 03:53:50]

https://codereview.chromium.org/2953743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2953743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:117: const bool
paint_block_cursor =
Could you move this expression into function?

https://codereview.chromium.org/2953743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:125: //
TODO(editing-dev): We should consider BIDI.
Could you explain more about this?
NextPositionOf() can work bidi, it returns logical next position.

[yoichio, 2017-06-23 04:26:03]

The CQ bit was checked by yoichio@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 04:26:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoichio, 2017-06-23 04:27:16]

https://codereview.chromium.org/2953743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2953743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:117: const bool
paint_block_cursor =
On 2017/06/23 03:53:50, yosin_UTC9 wrote:
> Could you move this expression into function?
I don't understand. This expression is used only here and 
 enough short to understand.

https://codereview.chromium.org/2953743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:125: //
TODO(editing-dev): We should consider BIDI.
On 2017/06/23 03:53:50, yosin_UTC9 wrote:
> Could you explain more about this?
> NextPositionOf() can work bidi, it returns logical next position.
Understood. This works already.

[yoichio, 2017-06-23 04:29:06]

On 2017/06/23 04:27:16, yoichio wrote:
>
https://codereview.chromium.org/2953743002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):
> 
>
https://codereview.chromium.org/2953743002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/LayoutSelection.cpp:117: const bool
> paint_block_cursor =
> On 2017/06/23 03:53:50, yosin_UTC9 wrote:
> > Could you move this expression into function?
> I don't understand. This expression is used only here and 
>  enough short to understand.
Plus, |const bool paint_block_cursor| describes well and 
even we need not to read the expression to understand CalcSelection code.

[commit-bot: I haz the power, 2017-06-23 04:41:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-23 04:41:29]

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...)

[yosin_UTC9, 2017-06-23 04:46:12]

https://codereview.chromium.org/2953743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2953743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:117: const bool
paint_block_cursor =
On 2017/06/23 at 04:27:15, yoichio wrote:
> On 2017/06/23 03:53:50, yosin_UTC9 wrote:
> > Could you move this expression into function?
> I don't understand. This expression is used only here and 
>  enough short to understand.

It is noisy for reading code focusing CalcSelection().
In that time, we don't care about details of ShouldShowBlockCursor().
Multiple lines of conditional expression is good candidate of introducing
function.
ShouldShowBlockCursor() could be written as

bool ShouldShowBlockCursor(const FrameSelection& frame_selection, const
VisibleSelectionInFlatTree& visible_selection) {
  if (!frame_selection.ShouldShowBLockCursor())
    return false;
  if (!visible_selection.IsCaret())
    return false;
  return !IsLogicalEndOfLine(CreateVisiblePosition(visible_selection.Start(),
visible_selection.Affinity()));
}

https://codereview.chromium.org/2953743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:119:
original_selection.GetSelectionType() == SelectionType::kCaretSelection &&
original_selection.IsCaret()

[yoichio, 2017-06-23 05:00:24]

The CQ bit was checked by yoichio@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 05:00:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoichio, 2017-06-23 05:01:01]

https://codereview.chromium.org/2953743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2953743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:117: const bool
paint_block_cursor =
On 2017/06/23 04:46:11, yosin_UTC9 wrote:
> On 2017/06/23 at 04:27:15, yoichio wrote:
> > On 2017/06/23 03:53:50, yosin_UTC9 wrote:
> > > Could you move this expression into function?
> > I don't understand. This expression is used only here and 
> >  enough short to understand.
> 
> It is noisy for reading code focusing CalcSelection().
> In that time, we don't care about details of ShouldShowBlockCursor().
> Multiple lines of conditional expression is good candidate of introducing
> function.
> ShouldShowBlockCursor() could be written as
> 
> bool ShouldShowBlockCursor(const FrameSelection& frame_selection, const
> VisibleSelectionInFlatTree& visible_selection) {
>   if (!frame_selection.ShouldShowBLockCursor())
>     return false;
>   if (!visible_selection.IsCaret())
>     return false;
>   return !IsLogicalEndOfLine(CreateVisiblePosition(visible_selection.Start(),
> visible_selection.Affinity()));
> }
> 

O.K. Done.

[yosin_UTC9, 2017-06-23 05:17:13]

lgtm

Thanks!

[commit-bot: I haz the power, 2017-06-23 05:19:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-23 05:19:51]

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...)

[yoichio, 2017-06-23 07:32:02]

The CQ bit was checked by yoichio@chromium.org

[commit-bot: I haz the power, 2017-06-23 07:32:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-23 07:46:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-23 07:46:38]

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...)

[yoichio, 2017-06-26 01:53:39]

The CQ bit was checked by yoichio@chromium.org

[commit-bot: I haz the power, 2017-06-26 01:53:51]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 04:07:46]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498442019958830,
"parent_rev": "4ce716d53ef2929a0515d2b12aea0b786b4024eb", "commit_rev":
"80471d8d1cbf8b08eaa893832f4d735765881a02"}

[commit-bot: I haz the power, 2017-06-26 04:07:57]

Description was changed from

==========
Simplify LayoutSelection::CalcSelection()

This CL changes
- Unify code whether selection is inside TextControl or not
- Split block cursor logic.

Then CalcSelection does
- return FrameSelection.VSinFlat if no block cursor
- if block cursor, extending selection with NextPositionOf(kGrapheme);

BUG=708453
==========

to

==========
Simplify LayoutSelection::CalcSelection()

This CL changes
- Unify code whether selection is inside TextControl or not
- Split block cursor logic.

Then CalcSelection does
- return FrameSelection.VSinFlat if no block cursor
- if block cursor, extending selection with NextPositionOf(kGrapheme);

BUG=708453

Review-Url: https://codereview.chromium.org/2953743002
Cr-Commit-Position: refs/heads/master@{#482194}
Committed:
https://chromium.googlesource.com/chromium/src/+/80471d8d1cbf8b08eaa893832f4d...
==========

[commit-bot: I haz the power, 2017-06-26 04:07:58]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/80471d8d1cbf8b08eaa893832f4d...