MacViews: Select word under cursor on right click.

MacViews: Select word under cursor on right click.

This CL modifies SelectionController::OnMousePressed to ensure right clicking
outside the current selection, selects the word under the cursor on Mac. A new
constant PlatformStyle::kSelectWordOnRightClick is introduced to facilitate
this.

BUG=657556
TEST= On Mac, enable chrome://flags/#secondary-ui-md. Open Bookmark Bubble.
Enter some text in the name textfield. Ensure the word under the cursor is
selected on a right click outside the current selection.

Committed: https://crrev.com/e9f75eab14f8845b5f23d0d50286191454f9f90d
Cr-Commit-Position: refs/heads/master@{#440373}

[karandeepb, 2016-12-19 05:49:58]

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

[commit-bot: I haz the power, 2016-12-19 05:50:16]

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

[karandeepb, 2016-12-19 06:01:29]

Description was changed from

==========
MacViews: Select word under cursor on right click.
==========

to

==========
MacViews: Select word under cursor on right click.

This CL modifies SelectionController::OnMousePressed to ensure right clicking
outside the current selection, selects the word under the cursor on Mac. A new
constant PlatformStyle::kSelectWordOnRightClick is introduced to facilitate
this.

BUG=657556
TEST= On Mac, enable chrome://flags/#secondary-ui-md. Open Bookmark Bubble.
Enter some text in the name textfield. Ensure the word under the cursor is
selected on a right click outside the current selection.
==========

[karandeepb, 2016-12-19 06:02:02]

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

[karandeepb, 2016-12-19 06:02:07]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-12-19 06:02:18]

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

[karandeepb, 2016-12-19 06:10:38]

karandeepb@chromium.org changed reviewers:
+ tapted@chromium.org

[karandeepb, 2016-12-19 06:10:39]

PTAL Trent. Another Mac specific behavior I observed is that right clicking on
an unfocused text view, selects all the text. However, there are still some bugs
relating to focus and retaining selection, hence I am not implementing that for
now.

[tapted, 2016-12-19 06:49:16]

https://codereview.chromium.org/2580293003/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/2580293003/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:657: void PressMouseButton(int
mouse_button_flags, int extra_flags) {
int -> ui::EventFlags ?

https://codereview.chromium.org/2580293003/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:664: void
ReleaseMouseButton(int mouse_button_flags) {
int -> ui::EventFlags

https://codereview.chromium.org/2580293003/diff/20001/ui/views/selection_cont...
File ui/views/selection_controller.cc (right):

https://codereview.chromium.org/2580293003/diff/20001/ui/views/selection_cont...
ui/views/selection_controller.cc:78: SelectWord(event.location());
There's another NSTextField behaviour related to this that we don't seem to get
right. That is: When right-clicking a text field when another control in the
window has focus, we should select _all_ the text. Fixing that may influence the
control flow we want here as well (and the names we choose e.g. maybe
kAlterSelectionOnRightClick )

https://codereview.chromium.org/2580293003/diff/20001/ui/views/selection_cont...
File ui/views/selection_controller.h (right):

https://codereview.chromium.org/2580293003/diff/20001/ui/views/selection_cont...
ui/views/selection_controller.h:56: // Selects word at the given |point|.
nit: word -> the word

[commit-bot: I haz the power, 2016-12-19 06:54:24]

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

[commit-bot: I haz the power, 2016-12-19 06:54:25]

Dry run: This issue passed the CQ dry run.

[karandeepb, 2016-12-19 07:05:43]

PTAL Trent.

https://codereview.chromium.org/2580293003/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/2580293003/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:657: void PressMouseButton(int
mouse_button_flags, int extra_flags) {
On 2016/12/19 06:49:16, tapted wrote:
> int -> ui::EventFlags ?

Done.

https://codereview.chromium.org/2580293003/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:664: void
ReleaseMouseButton(int mouse_button_flags) {
On 2016/12/19 06:49:16, tapted wrote:
> int -> ui::EventFlags

Done.

https://codereview.chromium.org/2580293003/diff/20001/ui/views/selection_cont...
File ui/views/selection_controller.cc (right):

https://codereview.chromium.org/2580293003/diff/20001/ui/views/selection_cont...
ui/views/selection_controller.cc:78: SelectWord(event.location());
On 2016/12/19 06:49:16, tapted wrote:
> There's another NSTextField behaviour related to this that we don't seem to
get
> right. That is: When right-clicking a text field when another control in the
> window has focus, we should select _all_ the text. Fixing that may influence
the
> control flow we want here as well (and the names we choose e.g. maybe
> kAlterSelectionOnRightClick )

Yeah had pointed this out in the comment with the previous patchset. As you
said, this'll probably require changing the control flow, maybe even adding a
RequestFocus method to the SelectionControllerDelegate. If it's ok, I think it
can be implemented separately.

https://codereview.chromium.org/2580293003/diff/20001/ui/views/selection_cont...
File ui/views/selection_controller.h (right):

https://codereview.chromium.org/2580293003/diff/20001/ui/views/selection_cont...
ui/views/selection_controller.h:56: // Selects word at the given |point|.
On 2016/12/19 06:49:16, tapted wrote:
> nit: word -> the word

Done.

[tapted, 2016-12-20 00:12:42]

https://codereview.chromium.org/2580293003/diff/20001/ui/views/selection_cont...
File ui/views/selection_controller.cc (right):

https://codereview.chromium.org/2580293003/diff/20001/ui/views/selection_cont...
ui/views/selection_controller.cc:78: SelectWord(event.location());
On 2016/12/19 07:05:43, karandeepb wrote:
> On 2016/12/19 06:49:16, tapted wrote:
> > There's another NSTextField behaviour related to this that we don't seem to
> get
> > right. That is: When right-clicking a text field when another control in the
> > window has focus, we should select _all_ the text. Fixing that may influence
> the
> > control flow we want here as well (and the names we choose e.g. maybe
> > kAlterSelectionOnRightClick )
> 
> Yeah had pointed this out in the comment with the previous patchset. As you
> said, this'll probably require changing the control flow, maybe even adding a
> RequestFocus method to the SelectionControllerDelegate. If it's ok, I think it
> can be implemented separately.

I couldn't find this comment - can you send me a link?

If we're not incorporating it now, there should be a TODO() here to note that
this is incomplete, with a link to the bug about selecting the whole text when
it doesn't have focus.

[karandeepb, 2016-12-21 14:20:03]

PTAL Trent.

https://codereview.chromium.org/2580293003/diff/20001/ui/views/selection_cont...
File ui/views/selection_controller.cc (right):

https://codereview.chromium.org/2580293003/diff/20001/ui/views/selection_cont...
ui/views/selection_controller.cc:78: SelectWord(event.location());
On 2016/12/20 00:12:42, tapted wrote:
> On 2016/12/19 07:05:43, karandeepb wrote:
> > On 2016/12/19 06:49:16, tapted wrote:
> > > There's another NSTextField behaviour related to this that we don't seem
to
> > get
> > > right. That is: When right-clicking a text field when another control in
the
> > > window has focus, we should select _all_ the text. Fixing that may
influence
> > the
> > > control flow we want here as well (and the names we choose e.g. maybe
> > > kAlterSelectionOnRightClick )
> > 
> > Yeah had pointed this out in the comment with the previous patchset. As you
> > said, this'll probably require changing the control flow, maybe even adding
a
> > RequestFocus method to the SelectionControllerDelegate. If it's ok, I think
it
> > can be implemented separately.
> 
> I couldn't find this comment - can you send me a link?

I was referring to the comment with the initial PTAL message. Copying it here - 
"PTAL Trent. Another Mac specific behavior I observed is that right clicking on
an unfocused text view, selects all the text. However, there are still some bugs
relating to focus and retaining selection, hence I am not implementing that for
now."

> If we're not incorporating it now, there should be a TODO() here to note that
> this is incomplete, with a link to the bug about selecting the whole text when
> it doesn't have focus.
Done. Still have to think how to incorporate this behavior since currently focus
is requested by the SelectionController clients.

[tapted, 2016-12-21 23:00:27]

lgtm

[tapted, 2016-12-22 05:34:47]

still lgtm but one more nit (sorry) then I think this is good to land.

https://codereview.chromium.org/2580293003/diff/60001/ui/views/selection_cont...
File ui/views/selection_controller.cc (right):

https://codereview.chromium.org/2580293003/diff/60001/ui/views/selection_cont...
ui/views/selection_controller.cc:101: void SelectionController::SelectWord(const
gfx::Point& point) {
nit: reorder to match header (between TrackMouseClicks/GetRenderText)

[karandeepb, 2016-12-22 08:51:25]

https://codereview.chromium.org/2580293003/diff/60001/ui/views/selection_cont...
File ui/views/selection_controller.cc (right):

https://codereview.chromium.org/2580293003/diff/60001/ui/views/selection_cont...
ui/views/selection_controller.cc:101: void SelectionController::SelectWord(const
gfx::Point& point) {
On 2016/12/22 05:34:46, tapted (OOO until 2017-01-03) wrote:
> nit: reorder to match header (between TrackMouseClicks/GetRenderText)

Done.

[karandeepb, 2016-12-22 08:51:33]

The CQ bit was checked by karandeepb@chromium.org

[karandeepb, 2016-12-22 08:51:33]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/2580293003/#ps80001
(title: "Reorder function.")

[commit-bot: I haz the power, 2016-12-22 08:51:43]

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-12-22 09:46:36]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482396693197200,
"parent_rev": "cc5dedd2146ba20b3d5a9c41342b7bb611fdd90c", "commit_rev":
"0615a140ba128d5fd00903972c65c3e33a22cac5"}

[commit-bot: I haz the power, 2016-12-22 09:47:02]

Description was changed from

==========
MacViews: Select word under cursor on right click.

This CL modifies SelectionController::OnMousePressed to ensure right clicking
outside the current selection, selects the word under the cursor on Mac. A new
constant PlatformStyle::kSelectWordOnRightClick is introduced to facilitate
this.

BUG=657556
TEST= On Mac, enable chrome://flags/#secondary-ui-md. Open Bookmark Bubble.
Enter some text in the name textfield. Ensure the word under the cursor is
selected on a right click outside the current selection.
==========

to

==========
MacViews: Select word under cursor on right click.

This CL modifies SelectionController::OnMousePressed to ensure right clicking
outside the current selection, selects the word under the cursor on Mac. A new
constant PlatformStyle::kSelectWordOnRightClick is introduced to facilitate
this.

BUG=657556
TEST= On Mac, enable chrome://flags/#secondary-ui-md. Open Bookmark Bubble.
Enter some text in the name textfield. Ensure the word under the cursor is
selected on a right click outside the current selection.

Review-Url: https://codereview.chromium.org/2580293003
==========

[commit-bot: I haz the power, 2016-12-22 09:47:04]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-12-22 09:50:32]

Description was changed from

==========
MacViews: Select word under cursor on right click.

This CL modifies SelectionController::OnMousePressed to ensure right clicking
outside the current selection, selects the word under the cursor on Mac. A new
constant PlatformStyle::kSelectWordOnRightClick is introduced to facilitate
this.

BUG=657556
TEST= On Mac, enable chrome://flags/#secondary-ui-md. Open Bookmark Bubble.
Enter some text in the name textfield. Ensure the word under the cursor is
selected on a right click outside the current selection.

Review-Url: https://codereview.chromium.org/2580293003
==========

to

==========
MacViews: Select word under cursor on right click.

This CL modifies SelectionController::OnMousePressed to ensure right clicking
outside the current selection, selects the word under the cursor on Mac. A new
constant PlatformStyle::kSelectWordOnRightClick is introduced to facilitate
this.

BUG=657556
TEST= On Mac, enable chrome://flags/#secondary-ui-md. Open Bookmark Bubble.
Enter some text in the name textfield. Ensure the word under the cursor is
selected on a right click outside the current selection.

Committed: https://crrev.com/e9f75eab14f8845b5f23d0d50286191454f9f90d
Cr-Commit-Position: refs/heads/master@{#440373}
==========

[commit-bot: I haz the power, 2016-12-22 09:50:33]

Patchset 4 (id:??) landed as
https://crrev.com/e9f75eab14f8845b5f23d0d50286191454f9f90d
Cr-Commit-Position: refs/heads/master@{#440373}