Move setNonDirectionalSelectionIfNeeded() to SelectionController from FrameSelection

Move setNonDirectionalSelectionIfNeeded() to SelectionController from FrameSelection

This patch moves |setNonDirectionalSelectionIfNeeded()| to |SelectionController|
from |FrameSelection| since it is used only in |SelectionController| to simplify
|FrameSelection| for improving code health.

This patch also gets rid of |originalBase()| which had been used in
|setNonDirectionalSelectionIfNeeded()|, but it has never been used.

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

Committed: https://crrev.com/82db1ff7a020d127b961d81e4baa60bbfd5baf03
Cr-Commit-Position: refs/heads/master@{#427032}

[yosin_UTC9, 2016-10-20 07:45:32]

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

[commit-bot: I haz the power, 2016-10-20 07:45:42]

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-20 07:49:41]

Description was changed from

==========
2016-10-20T16:39:47

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

to

==========
Move setNonDirectionalSelectionIfNeeded() to SelectionController from
FrameSelection

This patch moves |setNonDirectionalSelectionIfNeeded()| to |SelectionController|
from |FrameSelection| since it is used only in |SelectionController| to simplify
|FrameSelection| for improving code health.

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

[yosin_UTC9, 2016-10-20 07:50:42]

The CQ bit was unchecked by yosin@chromium.org

[yosin_UTC9, 2016-10-20 08:22:10]

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

[commit-bot: I haz the power, 2016-10-20 08:22:25]

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-20 08:38:19]

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

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

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-20 09:05:39]

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

[commit-bot: I haz the power, 2016-10-20 09:05:50]

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-20 09:07:41]

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

[commit-bot: I haz the power, 2016-10-20 09:07:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[yosin_UTC9, 2016-10-20 09:13:21]

Patchset #2 (id:20001) has been deleted

[yosin_UTC9, 2016-10-20 09:20:14]

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

[commit-bot: I haz the power, 2016-10-20 09:20:23]

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-20 09:22:55]

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

[commit-bot: I haz the power, 2016-10-20 09:22:56]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yosin_UTC9, 2016-10-20 09:32:46]

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

[commit-bot: I haz the power, 2016-10-20 09:33:03]

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-20 09:34:33]

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

[commit-bot: I haz the power, 2016-10-20 09:34:34]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[yosin_UTC9, 2016-10-20 09:45:38]

Patchset #2 (id:40001) has been deleted

[yosin_UTC9, 2016-10-20 09:45:44]

Patchset #2 (id:60001) has been deleted

[yosin_UTC9, 2016-10-20 09:45:51]

Patchset #2 (id:80001) has been deleted

[yosin_UTC9, 2016-10-20 09:46:20]

Patchset #1 (id:1) has been deleted

[yosin_UTC9, 2016-10-21 02:35:19]

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

[commit-bot: I haz the power, 2016-10-21 02:35:50]

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-21 02:53:15]

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

[commit-bot: I haz the power, 2016-10-21 02:53:16]

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

[yosin_UTC9, 2016-10-21 04:35:13]

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

[commit-bot: I haz the power, 2016-10-21 04:35:28]

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-21 04:55:26]

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

[commit-bot: I haz the power, 2016-10-21 04:55:26]

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

[yosin_UTC9, 2016-10-21 05:43:15]

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

[commit-bot: I haz the power, 2016-10-21 05:43:31]

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-21 06:05:39]

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

[commit-bot: I haz the power, 2016-10-21 06:05:40]

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

[yosin_UTC9, 2016-10-21 06:44:43]

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

[commit-bot: I haz the power, 2016-10-21 06:44:53]

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:17:34]

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

[yosin_UTC9, 2016-10-21 08:17:35]

PTAL

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

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

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

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2016-10-24 03:53:11]

I'm mostly OK with the path. Just a few minor revisions needed.

https://codereview.chromium.org/2441573002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2441573002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:828:
m_frame->eventHandler().selectionController().documentDetached();
SelectionController::DocumentDetached doesn't seem to be quite relevant to
FrameSelection. I think we should move the call site somewhere else, say, to
Document::shutdown.

https://codereview.chromium.org/2441573002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.h (left):

https://codereview.chromium.org/2441573002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.h:308:
VisiblePositionTemplate<Strategy> originalBase() const;
Is it a drive-by removal of unused functions? If so, please mention in patch
description.

https://codereview.chromium.org/2441573002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionController.h (right):

https://codereview.chromium.org/2441573002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.h:43: : public
GarbageCollectedFinalized<SelectionController> {
What's the difference between GarbageCollected and GarbageCollectedFinalized?

https://codereview.chromium.org/2441573002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.h:120:
VisiblePositionInFlatTree m_originalBaseInFlatTree;
It's a bad pattern to store a VP on heap, as it's likely that the DOM changes
during the storage of the VP.

Can we add a TODO to eliminate it?

[yosin_UTC9, 2016-10-24 06:21:29]

Description was changed from

==========
Move setNonDirectionalSelectionIfNeeded() to SelectionController from
FrameSelection

This patch moves |setNonDirectionalSelectionIfNeeded()| to |SelectionController|
from |FrameSelection| since it is used only in |SelectionController| to simplify
|FrameSelection| for improving code health.

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

to

==========
Move setNonDirectionalSelectionIfNeeded() to SelectionController from
FrameSelection

This patch moves |setNonDirectionalSelectionIfNeeded()| to |SelectionController|
from |FrameSelection| since it is used only in |SelectionController| to simplify
|FrameSelection| for improving code health.

This patch also gets rid of |originalBase()| which had been used in
|setNonDirectionalSelectionIfNeeded()|, but it has never been used.

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

[yosin_UTC9, 2016-10-24 06:48:03]

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

[commit-bot: I haz the power, 2016-10-24 06:48:12]

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:12:11]

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

[yosin_UTC9, 2016-10-24 07:12:13]

PTAL

- Move SelectionController::documentDettached() call to Document
- Update comments

https://codereview.chromium.org/2441573002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2441573002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:828:
m_frame->eventHandler().selectionController().documentDetached();
On 2016/10/24 at 03:53:11, Xiaocheng wrote:
> SelectionController::DocumentDetached doesn't seem to be quite relevant to
FrameSelection. I think we should move the call site somewhere else, say, to
Document::shutdown.

Done.

https://codereview.chromium.org/2441573002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.h (left):

https://codereview.chromium.org/2441573002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.h:308:
VisiblePositionTemplate<Strategy> originalBase() const;
On 2016/10/24 at 03:53:11, Xiaocheng wrote:
> Is it a drive-by removal of unused functions? If so, please mention in patch
description.

Done

https://codereview.chromium.org/2441573002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionController.h (right):

https://codereview.chromium.org/2441573002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.h:43: : public
GarbageCollectedFinalized<SelectionController> {
On 2016/10/24 at 03:53:11, Xiaocheng wrote:
> What's the difference between GarbageCollected and GarbageCollectedFinalized?

Member variable |m_originalBaseInFlatTree| requires
|GarbageCollectedFinalized<T>|.

Detailed explanation found in "Finalization Error" section of
http://dev.chromium.org/developers/blink-gc-plugin-errors

https://codereview.chromium.org/2441573002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.h:120:
VisiblePositionInFlatTree m_originalBaseInFlatTree;
On 2016/10/24 at 03:53:11, Xiaocheng wrote:
> It's a bad pattern to store a VP on heap, as it's likely that the DOM changes
during the storage of the VP.
> 
> Can we add a TODO to eliminate it?

Done.

[Xiaocheng, 2016-10-24 07:21:18]

lgtm

[tkent, 2016-10-24 07:40:59]

https://codereview.chromium.org/2441573002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionController.h (right):

https://codereview.chromium.org/2441573002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.h:48:
DECLARE_VIRTUAL_TRACE();
Why do you make it virtual?

[commit-bot: I haz the power, 2016-10-24 08:01:56]

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

[commit-bot: I haz the power, 2016-10-24 08:01:57]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2016-10-24 08:03:39]

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

[commit-bot: I haz the power, 2016-10-24 08:03:57]

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 08:18:03]

PTAL

Use DEFINE_TRACE() in FrameSelection instead of DEFINE_VIRTUAL_TRACE()

https://codereview.chromium.org/2441573002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionController.h (right):

https://codereview.chromium.org/2441573002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.h:48:
DECLARE_VIRTUAL_TRACE();
On 2016/10/24 at 07:40:59, tkent wrote:
> Why do you make it virtual?

Just follow FrameSelection... Use |DEFINE_TRACE()| for |SelectionController|.
I make FrameSelection not use DEFINE_VIRTUAL_TRACE(),
http://crrev.com/2443883002

[tkent, 2016-10-24 08:20:11]

lgtm

[yosin_UTC9, 2016-10-24 08:21:23]

The CQ bit was unchecked by yosin@chromium.org

[yosin_UTC9, 2016-10-24 08:21:26]

The CQ bit was checked by yosin@chromium.org

[yosin_UTC9, 2016-10-24 08:21:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
xiaochengh@chromium.org
Link to the patchset: https://codereview.chromium.org/2441573002/#ps240001
(title: "2016-10-24T17:02:54")

[commit-bot: I haz the power, 2016-10-24 08:21:40]

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 09:37:51]

Committed patchset #8 (id:240001)

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

Description was changed from

==========
Move setNonDirectionalSelectionIfNeeded() to SelectionController from
FrameSelection

This patch moves |setNonDirectionalSelectionIfNeeded()| to |SelectionController|
from |FrameSelection| since it is used only in |SelectionController| to simplify
|FrameSelection| for improving code health.

This patch also gets rid of |originalBase()| which had been used in
|setNonDirectionalSelectionIfNeeded()|, but it has never been used.

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

to

==========
Move setNonDirectionalSelectionIfNeeded() to SelectionController from
FrameSelection

This patch moves |setNonDirectionalSelectionIfNeeded()| to |SelectionController|
from |FrameSelection| since it is used only in |SelectionController| to simplify
|FrameSelection| for improving code health.

This patch also gets rid of |originalBase()| which had been used in
|setNonDirectionalSelectionIfNeeded()|, but it has never been used.

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

Committed: https://crrev.com/82db1ff7a020d127b961d81e4baa60bbfd5baf03
Cr-Commit-Position: refs/heads/master@{#427032}
==========

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

Patchset 8 (id:??) landed as
https://crrev.com/82db1ff7a020d127b961d81e4baa60bbfd5baf03
Cr-Commit-Position: refs/heads/master@{#427032}