Introduce CollectSelectedMap() in LayoutSelection::SetSelection()

Introduce CollectSelectedMap() in LayoutSelection::SetSelection()

This CL introduces local static CollectSelectedMap() from big LayoutSelection::SetSelection()
 and shares it to create |{new,old}_selected_map|.

TEST=No change in behavior.
BUG=708453
Review-Url: https://codereview.chromium.org/2874153002
Cr-Commit-Position: refs/heads/master@{#471247}
Committed: https://chromium.googlesource.com/chromium/src/+/18e6b4998b080ffe6c303c85a3498dab8f55f622

[yoichio, 2017-05-11 07:50:47]

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

[commit-bot: I haz the power, 2017-05-11 07:51:16]

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-11 08:27:24]

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

[commit-bot: I haz the power, 2017-05-11 10:45:43]

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

[commit-bot: I haz the power, 2017-05-11 10:45:44]

Dry run: This issue passed the CQ dry run.

[yoichio, 2017-05-12 01:55:54]

PTAL

[yosin_UTC9, 2017-05-12 02:09:45]

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

https://codereview.chromium.org/2874153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:141: swap(first,
other.first);
nit: s/swap/std::swap/
or
|first = std::move(other.first)|

to avoid temporary variable in |std::swap()|, it is better to use |std::moved()|

https://codereview.chromium.org/2874153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:142: swap(second,
other.second);
nit: s/swap/std::swap/
second = std::move(other.second)

to avoid temporary variable in |std::swap()|, it is better to use |std::moved()|

https://codereview.chromium.org/2874153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:144:
SelectedObjectMap first;
nit: s/first/object_map/ or another instead of |first|.
Because |first| doesn't mean anything.

https://codereview.chromium.org/2874153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:145: SelectedBlockMap
second;
nit: s/second/block_map/ or another instead of |second|.
Because |second| doesn't mean anything.

https://codereview.chromium.org/2874153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:159: LayoutObject* os
= selection_start;
nit: s/os/runner/ or another meaning full name.

https://codereview.chromium.org/2874153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:173: LayoutBlock* cb
= os->ContainingBlock();
nit: s/cb/containing_block/

https://codereview.chromium.org/2874153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:247: LayoutObject*
stop = LayoutObjectAfterPosition(end, end_pos);
Can we make |LayoutObject* stop|?

[yoichio, 2017-05-12 04:37:40]

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

https://codereview.chromium.org/2874153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:141: swap(first,
other.first);
On 2017/05/12 02:09:45, yosin_UTC9 wrote:
> nit: s/swap/std::swap/
> or
> |first = std::move(other.first)|
> 
> to avoid temporary variable in |std::swap()|, it is better to use
|std::moved()|

Done.

https://codereview.chromium.org/2874153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:142: swap(second,
other.second);
On 2017/05/12 02:09:45, yosin_UTC9 wrote:
> nit: s/swap/std::swap/
> second = std::move(other.second)
> 
> to avoid temporary variable in |std::swap()|, it is better to use
|std::moved()|

Done.

https://codereview.chromium.org/2874153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:144:
SelectedObjectMap first;
On 2017/05/12 02:09:45, yosin_UTC9 wrote:
> nit: s/first/object_map/ or another instead of |first|.
> Because |first| doesn't mean anything.

Done.

https://codereview.chromium.org/2874153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:145: SelectedBlockMap
second;
On 2017/05/12 02:09:45, yosin_UTC9 wrote:
> nit: s/second/block_map/ or another instead of |second|.
> Because |second| doesn't mean anything.

Done.

https://codereview.chromium.org/2874153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:159: LayoutObject* os
= selection_start;
On 2017/05/12 02:09:45, yosin_UTC9 wrote:
> nit: s/os/runner/ or another meaning full name.

Done.

https://codereview.chromium.org/2874153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:173: LayoutBlock* cb
= os->ContainingBlock();
On 2017/05/12 02:09:45, yosin_UTC9 wrote:
> nit: s/cb/containing_block/

Done.

https://codereview.chromium.org/2874153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:247: LayoutObject*
stop = LayoutObjectAfterPosition(end, end_pos);
On 2017/05/12 02:09:45, yosin_UTC9 wrote:
> Can we make |LayoutObject* stop|?

Done.

[yosin_UTC9, 2017-05-12 05:16:12]

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

https://codereview.chromium.org/2874153002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:139: SelectedMap() {}
nit: s/{}/= default/

https://codereview.chromium.org/2874153002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:144:
SelectedObjectMap object_map;
Please move member variable declaration before ctor's.

e.g. From "Node.h"

 struct AttachContext {
    STACK_ALLOCATED();
    ComputedStyle* resolved_style = nullptr;
    bool performing_reattach = false;
    bool clear_invalidation = false;

    AttachContext() {}
  };

https://codereview.chromium.org/2874153002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:157:
LayoutSelection::kPaintInvalidationNewXOROld) {
Since |CollectSelectedMap()| is used in two places, 
please avoid to use default parameter for ease of reading
e.g. when we see a call site using default parameter, we
may not know |CollectSelectedMap()| does XOR.

https://codereview.chromium.org/2874153002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:159: LayoutObject*
iter = selection_start;
nit: s/iter/runner/ or s/iter/layout_object/
Blink doesn't prefer abbreviation

[yoichio, 2017-05-12 05:46:43]

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

https://codereview.chromium.org/2874153002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:139: SelectedMap() {}
On 2017/05/12 05:16:11, yosin_UTC9 wrote:
> nit: s/{}/= default/

Done.

https://codereview.chromium.org/2874153002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:144:
SelectedObjectMap object_map;
On 2017/05/12 05:16:11, yosin_UTC9 wrote:
> Please move member variable declaration before ctor's.
> 
> e.g. From "Node.h"
> 
>  struct AttachContext {
>     STACK_ALLOCATED();
>     ComputedStyle* resolved_style = nullptr;
>     bool performing_reattach = false;
>     bool clear_invalidation = false;
> 
>     AttachContext() {}
>   };

Done.

https://codereview.chromium.org/2874153002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:157:
LayoutSelection::kPaintInvalidationNewXOROld) {
On 2017/05/12 05:16:11, yosin_UTC9 wrote:
> Since |CollectSelectedMap()| is used in two places, 
> please avoid to use default parameter for ease of reading
> e.g. when we see a call site using default parameter, we
> may not know |CollectSelectedMap()| does XOR.

Done.

https://codereview.chromium.org/2874153002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:159: LayoutObject*
iter = selection_start;
On 2017/05/12 05:16:11, yosin_UTC9 wrote:
> nit: s/iter/runner/ or s/iter/layout_object/
> Blink doesn't prefer abbreviation

Done.

[yoichio, 2017-05-12 05:46:54]

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

[commit-bot: I haz the power, 2017-05-12 05:47:18]

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-12 06:00:35]

lgtm

This code still have room of improvement on readability...

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

https://codereview.chromium.org/2874153002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:163: while
(continue_exploring) {
Next patch suggestion: 
Could you change this to use for-statement and use early-continue style?

We should rewrite GetNextOrPrevLayoutObjectBasedOnDirection() to return
|nullptr|
where it set |continue_exploring| to false.

for (LayoutObject* runner = selection_start;
     runner && runner != stop;
     runner = GetNextOrPrevLayoutObjectBasedOn(runner, stop,
exploaring_backwards)) {
  if (...)
    continue;
  if (...)
   continue;
  while (...)
   ...
}

It seems better to introduce Iterator class to hold |continue_exploring| and
|explorer_backwards|.

https://codereview.chromium.org/2874153002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:225: for
(SelectedObjectMap::iterator i = old_selected_map.object_map.begin();
I hope following patch replaces this and others to range-for.

[commit-bot: I haz the power, 2017-05-12 06:46:20]

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

[commit-bot: I haz the power, 2017-05-12 06:46:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoichio, 2017-05-12 06:58:42]

The CQ bit was checked by yoichio@chromium.org

[commit-bot: I haz the power, 2017-05-12 06:59: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-12 08:04:14]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494572322833140,
"parent_rev": "7795bff0b81a0cd0a8f840357545f591d0cdf6b0", "commit_rev":
"18e6b4998b080ffe6c303c85a3498dab8f55f622"}

[commit-bot: I haz the power, 2017-05-12 08:04:31]

Description was changed from

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

This CL introduces local static CollectSelectedMap() from big
LayoutSelection::SetSelection()
 and shares it to create |{new,old}_selected_map|.

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

to

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

This CL introduces local static CollectSelectedMap() from big
LayoutSelection::SetSelection()
 and shares it to create |{new,old}_selected_map|.

TEST=No change in behavior.
BUG=708453

Review-Url: https://codereview.chromium.org/2874153002
Cr-Commit-Position: refs/heads/master@{#471247}
Committed:
https://chromium.googlesource.com/chromium/src/+/18e6b4998b080ffe6c303c85a349...
==========

[commit-bot: I haz the power, 2017-05-12 08:04:32]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/18e6b4998b080ffe6c303c85a349...