Make SelecitonModifier class not to use VisibleSelection::setBase() and setExtent()

Make SelecitonModifier class not to use VisibleSelection::setBase() and setExtent()

This patch makes |SelecitonModifier| class not to use |Position| version of
|setBase()| and |setExtent()| of |VisibleSelection| as a preparation of
making |VisibleSelection| immutable for improving code health.

The concept of this patch is creating new |VisibleSelection| from
|m_selection| and copying back to |m_selection| instead of modifying
|m_selection| by using |setBase()| and |setExtent()|.

This is done by decomposing |willModify()| into |isBaseFirst()|,
returns true if |SelecitonModifier| uses selection start as base of new
selection, and |prepareForExtend()| to return new |VisibleSelection| from
start and end positions of passed |VisibleSelection|.

Since |setBase()| and |setExtent()| call |VisibleSelection:::validate()|,
the original code calls |validate()| twice, but this patch calls once.

BUG=660320
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2708823003
Cr-Commit-Position: refs/heads/master@{#451921}
Committed: https://chromium.googlesource.com/chromium/src/+/453d5882079b36fa2ab5b2240b9bcad81428eaa8

[yosin_UTC9, 2017-02-21 05:44:48]

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

[commit-bot: I haz the power, 2017-02-21 05:45:02]

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-02-21 07:21:56]

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

[commit-bot: I haz the power, 2017-02-21 07:21:57]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[yosin_UTC9, 2017-02-21 07:55:27]

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

[commit-bot: I haz the power, 2017-02-21 07:55:48]

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-02-21 08:20:59]

Description was changed from

==========
2017-02-21T14:41:51

2017-02-21T14:35:44

BUG=
==========

to

==========
2017-02-21T14:41:51

2017-02-21T14:35:44

BUG=660320
==========

[yosin_UTC9, 2017-02-21 08:40:44]

Description was changed from

==========
2017-02-21T14:41:51

2017-02-21T14:35:44

BUG=660320
==========

to

==========
Make SelecitonModifier class not to use VisibleSelection::setBase() and
setExtent()

This patch makes |SelecitonModifier| class not to use |Position| version of
|setBase()| and |setExtent()| of |VisibleSelection| as a preparation of
making |VisibleSelection| immutable for improving code health.

The concept of this patch is creating new |VisibleSelection| from
|m_selection| and copying back to |m_selection| instead of modifying
|m_selection| by using |setBase()| and |setExtent()|.

This is done by decomposing |willModify()| into |isBaseFirst()|,
returns true if |SelecitonModifier| uses selection start as base of new
selection, and |prepareForExtend()| to return new |VisibleSelection| from
start and end positions of passed |VisibleSelection|.

Since |setBase()| and |setExtent()| call |VisibleSelection:::validate()|,
the original code calls |validate()| twice, but this patch calls once.

BUG=660320
TEST=n/a; no behavior changes
==========

[yosin_UTC9, 2017-02-21 09:15:32]

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

[yosin_UTC9, 2017-02-21 09:15:32]

PTAL

[commit-bot: I haz the power, 2017-02-21 10:27:18]

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

[commit-bot: I haz the power, 2017-02-21 10:27:19]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[yosin_UTC9, 2017-02-21 23:24:56]

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

[yosin_UTC9, 2017-02-21 23:24:56]

+xiaochengh@, it seems he is working now. ;-)

[tkent, 2017-02-21 23:34:19]

tkent@chromium.org changed reviewers:
- xiaochengh@chromium.org

[tkent, 2017-02-21 23:34:20]

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

https://codereview.chromium.org/2708823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionModifier.cpp:60: static
TextDirection directionOfEnclosingBlockOf(const Position& position) {
What's the benefit of this function?

https://codereview.chromium.org/2708823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionModifier.cpp:85: TextDirection
SelectionModifier::directionOfSelection() const {
What's the benefit of this function?

https://codereview.chromium.org/2708823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionModifier.cpp:112: static
SelectionInDOMTree prepareForExtend(
nit: prepareForExtend ->prepareForExtent, prepareForExtension, or
prepareToExtend

However, I feel |prepare| isn't a proper word for this function. 
createExtendingSelectionInDOMTree() or something?

[tkent, 2017-02-21 23:34:44]

tkent@chromium.org changed reviewers:
+ xiaochengh@chromium.org

[Xiaocheng, 2017-02-22 01:06:20]

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

https://codereview.chromium.org/2708823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionModifier.cpp:85: TextDirection
SelectionModifier::directionOfSelection() const {
On 2017/02/21 at 23:34:20, tkent wrote:
> What's the benefit of this function?

This function seems necessary as it's called by
SelectionModifier::ModifyMovingLeft/Right.

[tkent, 2017-02-22 01:10:20]

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

https://codereview.chromium.org/2708823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionModifier.cpp:85: TextDirection
SelectionModifier::directionOfSelection() const {
On 2017/02/22 at 01:06:19, Xiaocheng wrote:
> On 2017/02/21 at 23:34:20, tkent wrote:
> > What's the benefit of this function?
> 
> This function seems necessary as it's called by
SelectionModifier::ModifyMovingLeft/Right.

Oh, right.  This is an existing function, and this CL updates the
implementation.  I was confused that this was a new function.

[yosin_UTC9, 2017-02-22 05:00:38]

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

[commit-bot: I haz the power, 2017-02-22 05:03:00]

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-02-22 05:11:05]

PTAL

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

https://codereview.chromium.org/2708823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionModifier.cpp:60: static
TextDirection directionOfEnclosingBlockOf(const Position& position) {
On 2017/02/21 at 23:34:20, tkent wrote:
> What's the benefit of this function?

Oops, this function is useless.
it may be introduced during rewriting to try not using "blink::".

Removing...

https://codereview.chromium.org/2708823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionModifier.cpp:85: TextDirection
SelectionModifier::directionOfSelection() const {
On 2017/02/22 at 01:10:20, tkent wrote:
> On 2017/02/22 at 01:06:19, Xiaocheng wrote:
> > On 2017/02/21 at 23:34:20, tkent wrote:
> > > What's the benefit of this function?
> > 
> > This function seems necessary as it's called by
SelectionModifier::ModifyMovingLeft/Right.
> 
> Oh, right.  This is an existing function, and this CL updates the
implementation.  I was confused that this was a new function.

Thanks xiaochengh@ for explanation.

https://codereview.chromium.org/2708823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionModifier.cpp:112: static
SelectionInDOMTree prepareForExtend(
On 2017/02/21 at 23:34:20, tkent wrote:
> nit: prepareForExtend ->prepareForExtent, prepareForExtension, or
prepareToExtend
> 
> However, I feel |prepare| isn't a proper word for this function. 
createExtendingSelectionInDOMTree() or something?

Done. Rename to |prepareToExtendSeelction()|

[tkent, 2017-02-22 05:20:26]

lgtm

[yosin_UTC9, 2017-02-22 05:44:40]

The CQ bit was unchecked by yosin@chromium.org

[yosin_UTC9, 2017-02-22 05:44:43]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-02-22 05:45:00]

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-02-22 08:11:19]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487742283552670,
"parent_rev": "8529c47b368722b13a67f29bb1f0e1b772e1f0c3", "commit_rev":
"453d5882079b36fa2ab5b2240b9bcad81428eaa8"}

[commit-bot: I haz the power, 2017-02-22 08:11:54]

Description was changed from

==========
Make SelecitonModifier class not to use VisibleSelection::setBase() and
setExtent()

This patch makes |SelecitonModifier| class not to use |Position| version of
|setBase()| and |setExtent()| of |VisibleSelection| as a preparation of
making |VisibleSelection| immutable for improving code health.

The concept of this patch is creating new |VisibleSelection| from
|m_selection| and copying back to |m_selection| instead of modifying
|m_selection| by using |setBase()| and |setExtent()|.

This is done by decomposing |willModify()| into |isBaseFirst()|,
returns true if |SelecitonModifier| uses selection start as base of new
selection, and |prepareForExtend()| to return new |VisibleSelection| from
start and end positions of passed |VisibleSelection|.

Since |setBase()| and |setExtent()| call |VisibleSelection:::validate()|,
the original code calls |validate()| twice, but this patch calls once.

BUG=660320
TEST=n/a; no behavior changes
==========

to

==========
Make SelecitonModifier class not to use VisibleSelection::setBase() and
setExtent()

This patch makes |SelecitonModifier| class not to use |Position| version of
|setBase()| and |setExtent()| of |VisibleSelection| as a preparation of
making |VisibleSelection| immutable for improving code health.

The concept of this patch is creating new |VisibleSelection| from
|m_selection| and copying back to |m_selection| instead of modifying
|m_selection| by using |setBase()| and |setExtent()|.

This is done by decomposing |willModify()| into |isBaseFirst()|,
returns true if |SelecitonModifier| uses selection start as base of new
selection, and |prepareForExtend()| to return new |VisibleSelection| from
start and end positions of passed |VisibleSelection|.

Since |setBase()| and |setExtent()| call |VisibleSelection:::validate()|,
the original code calls |validate()| twice, but this patch calls once.

BUG=660320
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2708823003
Cr-Commit-Position: refs/heads/master@{#451921}
Committed:
https://chromium.googlesource.com/chromium/src/+/453d5882079b36fa2ab5b2240b9b...
==========

[commit-bot: I haz the power, 2017-02-22 08:11:56]

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