Introduce MarkSetSelectionState() in LayoutSelection::SetSelection()

Introduce  MarkSetSelectionState() in LayoutSelection::SetSelection()

This CL improves code readbility

BUG=708453
TEST=No change in behavior

Review-Url: https://codereview.chromium.org/2882963002
Cr-Commit-Position: refs/heads/master@{#472403}
Committed: https://chromium.googlesource.com/chromium/src/+/62b34f48da334b945960af53f8cbaf656907bf0c

[yoichio, 2017-05-15 07:46:45]

Description was changed from

==========
Introduce foobar() in LayoutSelection::SetSelection()

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

to

==========
Introduce  MarkSetSelectionState() in LayoutSelection::SetSelection()

This CL improves code readbility

BUG=708453
TEST=No change in behavior
==========

[yoichio, 2017-05-15 07:46:45]

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

[yosin_UTC9, 2017-05-15 08:30:05]

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

https://codereview.chromium.org/2882963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:189: static void
MarkSetSelectionState(LayoutObject* start,
nit: s/MarkSetSelectionState/MarkSelectionState/

No need to have "Set"

https://codereview.chromium.org/2882963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:192: // Update the
selection status of all objects between m_selectionStart and
nit: s/status/state/
nit: s/m_selectionStart/|start|/

https://codereview.chromium.org/2882963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:193: //
m_selectionEnd
nit: s/m_selecitonEnd/|end|/

This function doesn't refer |m_selectionStart| and |m_selectionEnd|

https://codereview.chromium.org/2882963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:196: } else {
nit: better to use else-if to shallow indentation

if (start && start == end)
  start->SetSel...
else if (start)
  start->...
else if (end)
  end->....

https://codereview.chromium.org/2882963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:203: LayoutObject* o
= start;
nit: s/o/runner/ or something
Please avoid to use one letter variable name.

https://codereview.chromium.org/2882963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:206: while (o && o !=
stop) {
nit: It is better to use for:

for (LayoutObject* runner = start; 
     start && start != stop; 
     runner = runner->NextInPreOrder() {
 ...
}

[yoichio, 2017-05-15 09:12:22]

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

https://codereview.chromium.org/2882963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:189: static void
MarkSetSelectionState(LayoutObject* start,
On 2017/05/15 08:30:05, yosin_UTC9 wrote:
> nit: s/MarkSetSelectionState/MarkSelectionState/
> 
> No need to have "Set"

Done.

https://codereview.chromium.org/2882963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:192: // Update the
selection status of all objects between m_selectionStart and
On 2017/05/15 08:30:04, yosin_UTC9 wrote:
> nit: s/status/state/
> nit: s/m_selectionStart/|start|/

Done.

https://codereview.chromium.org/2882963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:193: //
m_selectionEnd
On 2017/05/15 08:30:05, yosin_UTC9 wrote:
> nit: s/m_selecitonEnd/|end|/
> 
> This function doesn't refer |m_selectionStart| and |m_selectionEnd|

Done.

https://codereview.chromium.org/2882963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:196: } else {
On 2017/05/15 08:30:05, yosin_UTC9 wrote:
> nit: better to use else-if to shallow indentation
> 
> if (start && start == end)
>   start->SetSel...
> else if (start)
>   start->...
> else if (end)
>   end->....

Done.

https://codereview.chromium.org/2882963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:203: LayoutObject* o
= start;
On 2017/05/15 08:30:05, yosin_UTC9 wrote:
> nit: s/o/runner/ or something
> Please avoid to use one letter variable name.

Done.

https://codereview.chromium.org/2882963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:206: while (o && o !=
stop) {
On 2017/05/15 08:30:04, yosin_UTC9 wrote:
> nit: It is better to use for:
> 
> for (LayoutObject* runner = start; 
>      start && start != stop; 
>      runner = runner->NextInPreOrder() {
>  ...
> }

Done.

[yoichio, 2017-05-15 09:12:23]

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

[commit-bot: I haz the power, 2017-05-15 09:12:36]

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-05-15 10:23:40]

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

[commit-bot: I haz the power, 2017-05-15 10:23:40]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yosin_UTC9, 2017-05-16 01:38:14]

https://codereview.chromium.org/2882963002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2882963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:193: if (start &&
start == end)
Oops, sorry my comment was wrong, according to layout test failure.
This if-statement block should be:

if (start && start == end) {
  ...
} else {
  if (start)
   start->SetSelectionState(...)
  if (end)
   end->SetSelectionState(...)
}

When |start| is |nullptr|, also implies |end| is |nullptr|, this
function does't nothing.

I can't imagine |start && !end| or |!start && end| cases, could you
try?


So, we can write:
static void SetSelectionState(LayoutObject* start, ...) {
  if (!start) {
    DCHECK_EQ(end, nullptr);
    return;
  }

  DCHECK(end) << start;
  
  if (start == end) {
    start->StartSelectionIfNeeded(SelectionBoth);
    return;
  }

  start->SetSelectionStateIfNeeded(SelectionStart);
  end->SetSelectionStateIfNeeded(SelectionEnd);

  ...
}

https://codereview.chromium.org/2882963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:203: if (runner !=
start && runner != end && runner->CanBeSelectionLeaf())
nit: better to use early continue style.

if (runner == start || runner == end || !runner->CanBeSelectionLeaf())
  continue;
runner->SetSelectionStateIfNeeded(SelectionInSide)

If we initialize |runner| as |runner = runner->nextInPreorder()|, we don't
need to check |runner == start|.

BTW, it seems we can use |end| as loop condition |runner != end| instead of
|runner != stop|.

[yoichio, 2017-05-16 01:45:50]

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

[commit-bot: I haz the power, 2017-05-16 01:46:48]

Dry run: CQ is trying da patch.

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

[yoichio, 2017-05-16 01:48:07]

PTAL

https://codereview.chromium.org/2882963002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2882963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:193: if (start &&
start == end)
This code can be increased but I want this CL only to extract rather than detail
clean up.

https://codereview.chromium.org/2882963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:203: if (runner !=
start && runner != end && runner->CanBeSelectionLeaf())
On 2017/05/16 01:38:14, yosin_UTC9 wrote:
> nit: better to use early continue style.
> 
> if (runner == start || runner == end || !runner->CanBeSelectionLeaf())
>   continue;
> runner->SetSelectionStateIfNeeded(SelectionInSide)
> 
> If we initialize |runner| as |runner = runner->nextInPreorder()|, we don't
> need to check |runner == start|.
> 
> BTW, it seems we can use |end| as loop condition |runner != end| instead of
> |runner != stop|.

Acknowledged.

[yosin_UTC9, 2017-05-16 01:52:44]

It seems we have non-null |start| and |end|.

void LayoutSelection::SetSelection(
    LayoutObject* start,
    int start_pos,
    LayoutObject* end,
    int end_pos,
    SelectionPaintInvalidationMode block_paint_invalidation_mode) {
...
  if ((start && !end) || (end && !start))
    return;

[commit-bot: I haz the power, 2017-05-16 01:52:57]

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

[commit-bot: I haz the power, 2017-05-16 01:52:58]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[yosin_UTC9, 2017-05-16 01:53:55]

Note: trybots failures are not related this patch. It seems something is wrong
in trybots.

"analyze" step said:

ERROR at //third_party/WebKit/Source/modules/modules.gni:27:3: Source file not
found.
  target(target_type, target_name) {
  ^---------------------------------
The target:
  //third_party/WebKit/Source/modules/accessibility:accessibility
has a source file:
  //third_party/WebKit/Source/modules/accessibility/AXObjectImpl.cpp
which was not found.
___________________
ERROR at //third_party/WebKit/Source/modules/modules.gni:27:3: Source file not
found.
  target(target_type, target_name) {
  ^---------------------------------
The target:
  //third_party/WebKit/Source/modules/accessibility:accessibility
has a source file:
  //third_party/WebKit/Source/modules/accessibility/AXObjectImpl.h
which was not found.

[yoichio, 2017-05-16 01:56:31]

We only have non-null |start| and |end| or both null |start| and |end|.
Both-null case is only coming from cleaseSelection.
I think this code flow can be increased but it is after these code clean ups.

[yoichio, 2017-05-16 03:45:05]

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

[commit-bot: I haz the power, 2017-05-16 03:45:37]

Dry run: CQ is trying da patch.

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

[yosin_UTC9, 2017-05-16 04:14:21]

lgtm

[commit-bot: I haz the power, 2017-05-16 04:37:45]

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

[commit-bot: I haz the power, 2017-05-16 04:37:45]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yoichio, 2017-05-16 07:54:41]

The CQ bit was checked by yoichio@chromium.org

[commit-bot: I haz the power, 2017-05-16 07:55:02]

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-05-16 08:44:40]

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

[commit-bot: I haz the power, 2017-05-16 08:44:41]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yoichio, 2017-05-17 00:12:33]

The CQ bit was checked by yoichio@chromium.org

[commit-bot: I haz the power, 2017-05-17 00:14:19]

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-05-17 06:37:19]

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

[commit-bot: I haz the power, 2017-05-17 06:37:20]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yoichio, 2017-05-17 08:00:15]

The CQ bit was checked by yoichio@chromium.org

[commit-bot: I haz the power, 2017-05-17 08:00:52]

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-05-17 09:52:31]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495008015196880,
"parent_rev": "bd83809aa9c5069baa191633e9be405e7cbb498c", "commit_rev":
"62b34f48da334b945960af53f8cbaf656907bf0c"}

[commit-bot: I haz the power, 2017-05-17 09:52:43]

Description was changed from

==========
Introduce  MarkSetSelectionState() in LayoutSelection::SetSelection()

This CL improves code readbility

BUG=708453
TEST=No change in behavior
==========

to

==========
Introduce  MarkSetSelectionState() in LayoutSelection::SetSelection()

This CL improves code readbility

BUG=708453
TEST=No change in behavior

Review-Url: https://codereview.chromium.org/2882963002
Cr-Commit-Position: refs/heads/master@{#472403}
Committed:
https://chromium.googlesource.com/chromium/src/+/62b34f48da334b945960af53f8cb...
==========

[commit-bot: I haz the power, 2017-05-17 09:52:45]

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