Select All should show handles if they were already present

Select All should show handles if they were already present

If the touch handles were present when the select all was executed then the newly
selected text should also have touch handles.

BUG=682128

[amaralp, 2017-01-19 00:23:23]

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

[commit-bot: I haz the power, 2017-01-19 00:24:17]

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-19 00:27:08]

Description was changed from

==========
Select All should show handles if they were already present

BUG=682128
==========

to

==========
Select All should show handles if they were already present

If the touch handles were present when the select all was executed then the
newly select text should also have touch handles.

BUG=682128
==========

[amaralp, 2017-01-19 00:27:09]

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

[amaralp, 2017-01-19 00:29:08]

Description was changed from

==========
Select All should show handles if they were already present

If the touch handles were present when the select all was executed then the
newly select text should also have touch handles.

BUG=682128
==========

to

==========
Select All should show handles if they were already present

If the touch handles were present when the select all was executed then the
newly
select text should also have touch handles.

BUG=682128
==========

[commit-bot: I haz the power, 2017-01-19 02:17:08]

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

[commit-bot: I haz the power, 2017-01-19 02:17:08]

Dry run: This issue passed the CQ dry run.

[amaralp, 2017-01-19 02:56:52]

Description was changed from

==========
Select All should show handles if they were already present

If the touch handles were present when the select all was executed then the
newly
select text should also have touch handles.

BUG=682128
==========

to

==========
Select All should show handles if they were already present

If the touch handles were present when the select all was executed then the
newly
selected text should also have touch handles.

BUG=682128
==========

[amaralp, 2017-01-19 05:54:22]

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

[commit-bot: I haz the power, 2017-01-19 05:54:46]

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-19 07:30:53]

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

[commit-bot: I haz the power, 2017-01-19 07:30:54]

Dry run: This issue passed the CQ dry run.

[amaralp, 2017-01-19 07:33:10]

PTAL

[yosin_UTC9, 2017-01-19 08:31:03]

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

https://codereview.chromium.org/2642913003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:916:
SetSelectionOptions options = CloseTyping | ClearTypingStyle;
I prefer

const SetSelectionOptions options = isHandleVisible()
  ? CloseTypeing | ClearTyping | HandleVisible
  : CloseTypeing | ClearTyping;

https://codereview.chromium.org/2642913003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:917: if
(isHandleVisible())
When do we not show handle if |isHandleVisbile()|?
Can we have a option |HandleVisibleIfHahdleVisible| or another better name?
Checking |isHandleVisible()| and turn on option is very error prone.

[yosin_UTC9, 2017-01-19 09:36:52]

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

https://codereview.chromium.org/2642913003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:917: if
(isHandleVisible())
On 2017/01/19 at 08:31:02, Yosi_UTC9 wrote:
> When do we not show handle if |isHandleVisbile()|?
> Can we have a option |HandleVisibleIfHahdleVisible| or another better name?
> Checking |isHandleVisible()| and turn on option is very error prone.

[1] is another example of error prone regarding handle visibility.

[1] https://codereview.chromium.org/2370663002

[amaralp, 2017-01-19 22:28:13]

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

[commit-bot: I haz the power, 2017-01-19 22:29:05]

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-19 22:44:46]

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

[commit-bot: I haz the power, 2017-01-19 22:44:47]

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

[amaralp, 2017-01-19 22:46:06]

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

[commit-bot: I haz the power, 2017-01-19 22:46:57]

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-20 01:10:21]

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

[commit-bot: I haz the power, 2017-01-20 01:10:22]

Dry run: This issue passed the CQ dry run.

[amaralp, 2017-01-20 01:22:09]

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

https://codereview.chromium.org/2642913003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:916:
SetSelectionOptions options = CloseTyping | ClearTypingStyle;
On 2017/01/19 at 08:31:03, Yosi_UTC9 wrote:
> I prefer
> 
> const SetSelectionOptions options = isHandleVisible()
>   ? CloseTypeing | ClearTyping | HandleVisible
>   : CloseTypeing | ClearTyping;

This has been removed.

https://codereview.chromium.org/2642913003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:917: if
(isHandleVisible())
On 2017/01/19 at 08:31:02, Yosi_UTC9 wrote:
> When do we not show handle if |isHandleVisbile()|?
A few cases I can think of:
  1) Javascript selection
  2) Tapping to clear a handle
  3) Typing should clear the handle.

> Can we have a option |HandleVisibleIfHahdleVisible| or another better name?
Yep, |PreserveHandle| has been added.

> Checking |isHandleVisible()| and turn on option is very error prone.
Not sure what you mean by this. Have my new changes addressed this?

I think this change would also make changwan@'s CL simpler.

[Changwan Ryu, 2017-01-20 01:28:35]

On 2017/01/20 01:22:09, amaralp wrote:
>
https://codereview.chromium.org/2642913003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):
> 
>
https://codereview.chromium.org/2642913003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/FrameSelection.cpp:916:
> SetSelectionOptions options = CloseTyping | ClearTypingStyle;
> On 2017/01/19 at 08:31:03, Yosi_UTC9 wrote:
> > I prefer
> > 
> > const SetSelectionOptions options = isHandleVisible()
> >   ? CloseTypeing | ClearTyping | HandleVisible
> >   : CloseTypeing | ClearTyping;
> 
> This has been removed.
> 
>
https://codereview.chromium.org/2642913003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/FrameSelection.cpp:917: if
> (isHandleVisible())
> On 2017/01/19 at 08:31:02, Yosi_UTC9 wrote:
> > When do we not show handle if |isHandleVisbile()|?
> A few cases I can think of:
>   1) Javascript selection
>   2) Tapping to clear a handle
>   3) Typing should clear the handle.
> 
> > Can we have a option |HandleVisibleIfHahdleVisible| or another better name?
> Yep, |PreserveHandle| has been added.
> 
> > Checking |isHandleVisible()| and turn on option is very error prone.
> Not sure what you mean by this. Have my new changes addressed this?
> 
> I think this change would also make changwan@'s CL simpler.

Unfortunately, no, because of the reasons I explained in my CL comment. :'(

[yosin_UTC9, 2017-01-20 04:50:37]

On 2017/01/20 at 01:22:09, amaralp wrote:
>
https://codereview.chromium.org/2642913003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):
> 
>
https://codereview.chromium.org/2642913003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/FrameSelection.cpp:916:
SetSelectionOptions options = CloseTyping | ClearTypingStyle;
> On 2017/01/19 at 08:31:03, Yosi_UTC9 wrote:
> > I prefer
> > 
> > const SetSelectionOptions options = isHandleVisible()
> >   ? CloseTypeing | ClearTyping | HandleVisible
> >   : CloseTypeing | ClearTyping;
> 
> This has been removed.
> 
>
https://codereview.chromium.org/2642913003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/FrameSelection.cpp:917: if
(isHandleVisible())
> On 2017/01/19 at 08:31:02, Yosi_UTC9 wrote:
> > When do we not show handle if |isHandleVisbile()|?
> A few cases I can think of:
>   1) Javascript selection
>   2) Tapping to clear a handle
>   3) Typing should clear the handle.
> 
> > Can we have a option |HandleVisibleIfHahdleVisible| or another better name?
> Yep, |PreserveHandle| has been added.
> 
> > Checking |isHandleVisible()| and turn on option is very error prone.
> Not sure what you mean by this. Have my new changes addressed this?
> 
> I think this change would also make changwan@'s CL simpler.

Let's use SelectionTemplate to pass handle visibility parameter instead of using
FrameSelection::setSelectionOption
as discussed in [1].

It seems using setSelectionOptions make our code to be complex and adding more
flags makes more complex.
Also, as changwang@ said addition option doesn't work other cases.

So, let's move handle visibility to SelectionTemplate, then work this patch and
changwang@'s patch in parallel.

Once we've done to move handle visibility to SelectionTemplate, we can get rid
of  HandleVisibility enum class
to utilize SelectionTemplate. This is another story but we can make our code
simpler.





[1] http://crbug.com/633281 Refactor Selection Handle Visibility