Get rid of flat tree version of createVisibleSelection() taking two VisiblePosition

Get rid of flat tree version of createVisibleSelection() taking two VisiblePosition

This patch gets rid of flat tree version of |createVisibleSelection()| taking
two |VisiblePosition| by replacing with |SelectionInDOMTree| version to reduce
number of overloads for improving code health.

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

Committed: https://crrev.com/91be04c48e4cc1ef958dc1ddbfc4cf34a8584842
Cr-Commit-Position: refs/heads/master@{#427031}

[yosin_UTC9, 2016-10-21 07:05:41]

Description was changed from

==========
2016-10-21T16:04:11

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

to

==========
Get rid of flat tree version of createVisibleSelection() taking two
VisiblePosition

This patch gets rid of flat tree version of |createVisibleSelection()| taking
to |VisiblePosition| by replacing with |SelectionInDOMTree| version to reduce 
number of overloads for improving code health.

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

[yosin_UTC9, 2016-10-21 07:06:00]

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

[commit-bot: I haz the power, 2016-10-21 07:06:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yosin_UTC9, 2016-10-21 08:20:59]

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

[yosin_UTC9, 2016-10-21 08:21:00]

PTAL

Not only core/editing but also 
 - core/svg/SVGTextContentElement.cpp
 - modules/accessibility/AXLayoutObject.cpp

[yosin_UTC9, 2016-10-21 08:45:00]

Description was changed from

==========
Get rid of flat tree version of createVisibleSelection() taking two
VisiblePosition

This patch gets rid of flat tree version of |createVisibleSelection()| taking
to |VisiblePosition| by replacing with |SelectionInDOMTree| version to reduce 
number of overloads for improving code health.

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

to

==========
Get rid of flat tree version of createVisibleSelection() taking two
VisiblePosition

This patch gets rid of flat tree version of |createVisibleSelection()| taking
two |VisiblePosition| by replacing with |SelectionInDOMTree| version to reduce 
number of overloads for improving code health.

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

[commit-bot: I haz the power, 2016-10-21 08:47:19]

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

[commit-bot: I haz the power, 2016-10-21 08:47:20]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2016-10-21 11:18:36]

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

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:948:
SelectionInDOMTree::Builder builder;
Let's use setBaseAndExtentDeprecated, since having a null VP here might indicate
something going wrong.

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:1448: if
(basePosition.isNotNull() && extentPosition.isNotNull()) {
Let's use setBaseAndExtentDeprecated, since having a null VP here might indicate
something going wrong.

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

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/GranularityStrategy.cpp:76:
.collapse(selection.visibleBase().toPositionWithAffinity())
We should use
collapse(selection.base()).setAffinity(selection.affinity()).extent(...)...
to reduce the usage of VisiblePosition.

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/GranularityStrategy.cpp:156:
.collapse(selection.visibleBase().toPositionWithAffinity())
We should use
collapse(selection.base()).setAffinity(selection.affinity()).extent(...)...
to reduce the usage of VisiblePosition.

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

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/ApplyBlockElementCommand.cpp:84:
builder.collapse(visibleStart.deepEquivalent());
builder.collapse(visibleStart.positionWithAffinity())?

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/ApplyBlockElementCommand.cpp:121:
.collapse(start.deepEquivalent())
builder.collapse(start.positionWithAffinity())?

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

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp:202:
if (newBase.isNotNull() && newExtent.isNotNull()) {
Let's use setBaseAndExtentDeprecated, as having a null VP here might indicate
something going wrong.

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

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp:286: if
(visibleStartOfSelection.isNotNull() &&
Let's use setBaseAndExtentDeprecated as having a null VP here might indicate
something going wrong.

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

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:137:
SelectionInDOMTree selectionFromBaseAndExtent(const VisiblePosition& base,
We should use setBaseAndExtentDeprecated. Though the call sites may pass null
|base| or |extent| (reached by some layout tests), this is undesired and should
be clearly indicated.

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:474:
selectionFromBaseAndExtent(wordStartOfPrevious, endOfWord(nextWord)));
We should use setBaseAndExtentDeprecated.

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp (right):

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:228:
SelectionInDOMTree::Builder builder;
How about just setBaseAndExtentDeprecated?

Since |start| and |end| are converted from offsets, there should be something
wrong if they are null. Let's use the deprecated function to indicate this.

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:238:
document().frame()->selection().setSelection(
We should use the FrameSelection::setSelection(SelectionInDOMTree) overload.

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:1955: if
(anchorVisiblePosition.isNotNull() && focusVisiblePosition.isNotNull()) {
No need for these branches as L1951 have ensured non-nullity for the two VPs.

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:1963:
frame->selection().setSelection(createVisibleSelection(builder.build()));
We should use the FrameSelection::setSelection(SelectionInDOMTree) overload.

[yosin_UTC9, 2016-10-24 04:36:40]

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

[commit-bot: I haz the power, 2016-10-24 04:36:45]

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, 2016-10-24 06:14:35]

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

[commit-bot: I haz the power, 2016-10-24 06:14:35]

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

[yosin_UTC9, 2016-10-24 06:19:45]

PTAL

Updated to follow review comments.

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

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:948:
SelectionInDOMTree::Builder builder;
On 2016/10/21 at 11:18:36, Xiaocheng wrote:
> Let's use setBaseAndExtentDeprecated, since having a null VP here might
indicate something going wrong.

Done

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:1448: if
(basePosition.isNotNull() && extentPosition.isNotNull()) {
On 2016/10/21 at 11:18:36, Xiaocheng wrote:
> Let's use setBaseAndExtentDeprecated, since having a null VP here might
indicate something going wrong.

Done

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

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/GranularityStrategy.cpp:76:
.collapse(selection.visibleBase().toPositionWithAffinity())
On 2016/10/21 at 11:18:36, Xiaocheng wrote:
> We should use
> collapse(selection.base()).setAffinity(selection.affinity()).extent(...)...
> to reduce the usage of VisiblePosition.

Good catch!
Done

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/GranularityStrategy.cpp:156:
.collapse(selection.visibleBase().toPositionWithAffinity())
On 2016/10/21 at 11:18:36, Xiaocheng wrote:
> We should use
> collapse(selection.base()).setAffinity(selection.affinity()).extent(...)...
> to reduce the usage of VisiblePosition.

Good catch!
Done.

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

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/ApplyBlockElementCommand.cpp:84:
builder.collapse(visibleStart.deepEquivalent());
On 2016/10/21 at 11:18:36, Xiaocheng wrote:
> builder.collapse(visibleStart.positionWithAffinity())?

Done

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/ApplyBlockElementCommand.cpp:121:
.collapse(start.deepEquivalent())
On 2016/10/21 at 11:18:36, Xiaocheng wrote:
> builder.collapse(start.positionWithAffinity())?

Done

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

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp:202:
if (newBase.isNotNull() && newExtent.isNotNull()) {
On 2016/10/21 at 11:18:36, Xiaocheng wrote:
> Let's use setBaseAndExtentDeprecated, as having a null VP here might indicate
something going wrong.

Done.

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

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp:286: if
(visibleStartOfSelection.isNotNull() &&
On 2016/10/21 at 11:18:36, Xiaocheng wrote:
> Let's use setBaseAndExtentDeprecated as having a null VP here might indicate
something going wrong.

Done

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

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:137:
SelectionInDOMTree selectionFromBaseAndExtent(const VisiblePosition& base,
On 2016/10/21 at 11:18:36, Xiaocheng wrote:
> We should use setBaseAndExtentDeprecated. Though the call sites may pass null
|base| or |extent| (reached by some layout tests), this is undesired and should
be clearly indicated.

Done.

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:474:
selectionFromBaseAndExtent(wordStartOfPrevious, endOfWord(nextWord)));
On 2016/10/21 at 11:18:36, Xiaocheng wrote:
> We should use setBaseAndExtentDeprecated.

Done

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp (right):

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:228:
SelectionInDOMTree::Builder builder;
On 2016/10/21 at 11:18:36, Xiaocheng wrote:
> How about just setBaseAndExtentDeprecated?
> 
> Since |start| and |end| are converted from offsets, there should be something
wrong if they are null. Let's use the deprecated function to indicate this.

Done.

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:238:
document().frame()->selection().setSelection(
On 2016/10/21 at 11:18:36, Xiaocheng wrote:
> We should use the FrameSelection::setSelection(SelectionInDOMTree) overload.

Good catch!

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:1955: if
(anchorVisiblePosition.isNotNull() && focusVisiblePosition.isNotNull()) {
On 2016/10/21 at 11:18:36, Xiaocheng wrote:
> No need for these branches as L1951 have ensured non-nullity for the two VPs.

I missed... :-<

https://codereview.chromium.org/2437873008/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:1963:
frame->selection().setSelection(createVisibleSelection(builder.build()));
On 2016/10/21 at 11:18:36, Xiaocheng wrote:
> We should use the FrameSelection::setSelection(SelectionInDOMTree) overload.

Good catch!
Done

[Xiaocheng, 2016-10-24 06:58:49]

One more comment as below.

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

https://codereview.chromium.org/2437873008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:1448:
.setBaseAndExtent(basePosition.deepEquivalent(),
Is it guaranteed to have non-null basePosition and extentPosition here?

[yosin_UTC9, 2016-10-24 07:39:29]

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

[commit-bot: I haz the power, 2016-10-24 07:39:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yosin_UTC9, 2016-10-24 07:40:17]

PTAL

Update to follow review comment.

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

https://codereview.chromium.org/2437873008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:1448:
.setBaseAndExtent(basePosition.deepEquivalent(),
On 2016/10/24 at 06:58:49, Xiaocheng wrote:
> Is it guaranteed to have non-null basePosition and extentPosition here?

Oops, No. 
We should use setBaseAndExtentDeprecated().

[Xiaocheng, 2016-10-24 08:30:22]

lgtm

[tkent, 2016-10-24 08:39:42]

lgtm.

The new code looks more complex, but I trust your decision.

[commit-bot: I haz the power, 2016-10-24 09:08:47]

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

[commit-bot: I haz the power, 2016-10-24 09:08:48]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2016-10-24 09:10:58]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2016-10-24 09:11:15]

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

[yosin_UTC9, 2016-10-24 09:16:25]

On 2016/10/24 at 08:39:42, tkent wrote:
> lgtm.
> 
> The new code looks more complex, but I trust your decision.

|createVisibleSelection()| is too simple to what actually it does,
e.g. it updates layout tree.

Also, createVisibleSelection(base, extent) hides program error,
e.g. passing base=null but extent=non-null

Using SelectionInDOMTree improves code health, we believe.

[commit-bot: I haz the power, 2016-10-24 09:25:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-24 09:27:51]

Description was changed from

==========
Get rid of flat tree version of createVisibleSelection() taking two
VisiblePosition

This patch gets rid of flat tree version of |createVisibleSelection()| taking
two |VisiblePosition| by replacing with |SelectionInDOMTree| version to reduce 
number of overloads for improving code health.

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

to

==========
Get rid of flat tree version of createVisibleSelection() taking two
VisiblePosition

This patch gets rid of flat tree version of |createVisibleSelection()| taking
two |VisiblePosition| by replacing with |SelectionInDOMTree| version to reduce
number of overloads for improving code health.

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

Committed: https://crrev.com/91be04c48e4cc1ef958dc1ddbfc4cf34a8584842
Cr-Commit-Position: refs/heads/master@{#427031}
==========

[commit-bot: I haz the power, 2016-10-24 09:27:52]

Patchset 3 (id:??) landed as
https://crrev.com/91be04c48e4cc1ef958dc1ddbfc4cf34a8584842
Cr-Commit-Position: refs/heads/master@{#427031}