Removing HandleVisible enum from FrameSelection

Removing HandleVisible enum from FrameSelection

Removing the |HandleVisible| value from |SetSelectionOption|

BUG=633281
Review-Url: https://codereview.chromium.org/2647283006
Cr-Commit-Position: refs/heads/master@{#448686}
Committed: https://chromium.googlesource.com/chromium/src/+/cfc62226b8b375088327fd872c621e90f36f4d7f

[amaralp, 2017-01-24 20:37:15]

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

[commit-bot: I haz the power, 2017-01-24 20:37:53]

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-01-24 20:55:44]

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

[commit-bot: I haz the power, 2017-01-24 20:55:47]

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

[amaralp, 2017-01-25 03:44:15]

Description was changed from

==========
Removing HandleVisible enum

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

to

==========
Removing HandleVisible enum from FrameSelection

Removing the |HandleVisible| value from |SetSelectionOption|

BUG=633281
==========

[amaralp, 2017-01-25 03:44:15]

amaralp@chromium.org changed reviewers:
+ changwan@chromium.org, yosin@chromium.org

[amaralp, 2017-01-25 03:46:36]

I haven't had time to investigate why some tests are failing, but is this what
you were looking for yosin@?

[yosin_UTC9, 2017-01-25 04:01:09]

Let's keep |HandleVisible| enum until we make
|FrameSeelction::setSelectionAlgorithm<T>()| to take |SelectionTemplate<T>|.

BTW, This patch doesn't getting rid of |enum class HandleVisible|.

For test failures, it seem bots hiccup. Let's see results of another attempt.

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

https://codereview.chromium.org/2647283006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:1361:
setSelection(newSelection, isHandleVisible(),
Once we use |HandleVisibility| as parameter, we can write |m_handleVisibility|

https://codereview.chromium.org/2647283006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:1384:
setSelection(newSelection, isHandleVisible(), CloseTyping | ClearTypingStyle,
Once we use |HandleVisibility| as parameter, we can write |m_handleVisibility|

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

https://codereview.chromium.org/2647283006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.h:138: bool
isHandleVisible = false,
Since we still have |HandleVisibility|. Can we use it instead of |bool|
parameter?

https://codereview.chromium.org/2647283006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/SelectionTemplate.h (right):

https://codereview.chromium.org/2647283006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/SelectionTemplate.h:69: Builder&
setIsHandleVisible(bool);
Could you move this change to another patch with test cases in
"SelectionTemplateTest.cpp"?

BTW, it seems changwang@ also does same change. Please coordinate the work.

[Changwan Ryu, 2017-01-25 09:29:47]

https://codereview.chromium.org/2647283006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/SelectionTemplate.h (right):

https://codereview.chromium.org/2647283006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/SelectionTemplate.h:69: Builder&
setIsHandleVisible(bool);
On 2017/01/25 04:01:09, Yosi_UTC9 wrote:
> Could you move this change to another patch with test cases in
> "SelectionTemplateTest.cpp"?
> 
> BTW, it seems changwang@ also does same change. Please coordinate the work.

amaralp@, could you work on this as well? I'll wait for your CLs to land first.

[amaralp, 2017-01-25 23:16:23]

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

[commit-bot: I haz the power, 2017-01-25 23:17:05]

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

[amaralp, 2017-01-26 00:24:15]

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

[commit-bot: I haz the power, 2017-01-26 00:25:19]

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-01-26 01:04:19]

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

[commit-bot: I haz the power, 2017-01-26 01:04:19]

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

[amaralp, 2017-01-27 01:43:09]

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

[commit-bot: I haz the power, 2017-01-27 01:44:06]

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-01-27 03:27:28]

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

[commit-bot: I haz the power, 2017-01-27 03:27:28]

Dry run: This issue passed the CQ dry run.

[amaralp, 2017-01-27 22:50:03]

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

[commit-bot: I haz the power, 2017-01-27 22:51:06]

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-01-27 23:07:33]

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

[commit-bot: I haz the power, 2017-01-27 23:07:34]

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

[amaralp, 2017-01-27 23:34:29]

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

[commit-bot: I haz the power, 2017-01-27 23:35:08]

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

[amaralp, 2017-01-27 23:59:30]

Patchset #7 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-01-28 00:22:01]

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

[commit-bot: I haz the power, 2017-01-28 00:22:01]

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

[amaralp, 2017-01-28 01:36:27]

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

[commit-bot: I haz the power, 2017-01-28 01:37:04]

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-01-28 05:01:36]

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

[commit-bot: I haz the power, 2017-01-28 05:01:37]

Dry run: This issue passed the CQ dry run.

[amaralp, 2017-01-30 00:27:39]

PTAL, note that this CL is dependent on crrev.com/2664443002

[yosin_UTC9, 2017-02-06 05:33:28]

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

https://codereview.chromium.org/2647283006/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:623:
setSelection(newSelection, HandleVisibility::NotVisible, DoNotSetFocus);
To reduce code change, can we introduce:

FrameSelection::setSelection(const VisibleSelection& visibleSeleciton,
SetSelectionOptions options) {
  setSelection(visibleSelection, HandleVisibilit::NotVisible, options);
}

[amaralp, 2017-02-07 01:41:54]

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

[commit-bot: I haz the power, 2017-02-07 01:42:39]

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-07 02:34:26]

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

[commit-bot: I haz the power, 2017-02-07 02:34:26]

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

[amaralp, 2017-02-07 08:07:40]

On 2017/02/06 at 05:33:28, yosin wrote:
>
https://codereview.chromium.org/2647283006/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):
> 
>
https://codereview.chromium.org/2647283006/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/FrameSelection.cpp:623:
setSelection(newSelection, HandleVisibility::NotVisible, DoNotSetFocus);
> To reduce code change, can we introduce:
> 
> FrameSelection::setSelection(const VisibleSelection& visibleSeleciton,
SetSelectionOptions options) {
>   setSelection(visibleSelection, HandleVisibilit::NotVisible, options);
> }

Done.

[yosin_UTC9, 2017-02-07 09:00:33]

lgtm

Sorry for delay. m(_ _)m

[amaralp, 2017-02-07 19:00:49]

The CQ bit was checked by amaralp@chromium.org

[commit-bot: I haz the power, 2017-02-07 19:01:27]

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-07 19:09:12]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1486494049395850,
"parent_rev": "66ec149425f5fe673280522e6e6f6d837c72dd08", "commit_rev":
"cfc62226b8b375088327fd872c621e90f36f4d7f"}

[commit-bot: I haz the power, 2017-02-07 19:09:49]

Description was changed from

==========
Removing HandleVisible enum from FrameSelection

Removing the |HandleVisible| value from |SetSelectionOption|

BUG=633281
==========

to

==========
Removing HandleVisible enum from FrameSelection

Removing the |HandleVisible| value from |SetSelectionOption|

BUG=633281

Review-Url: https://codereview.chromium.org/2647283006
Cr-Commit-Position: refs/heads/master@{#448686}
Committed:
https://chromium.googlesource.com/chromium/src/+/cfc62226b8b375088327fd872c62...
==========

[commit-bot: I haz the power, 2017-02-07 19:09:51]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/cfc62226b8b375088327fd872c62...