[refactor] - Rename and Move WebWidget::CaretOrSelectionRange to WebInputMethodController::GetSelec…

[refactor] - Rename and Move WebWidget::CaretOrSelectionRange to WebInputMethodController::GetSelectionOffsets

Reasons for move:
1- This is a frame concept given (the two nontrivial calls are from
   render_frame_impl.cc and frame_input_handler_impl.cc).

2- It cleans up WebViewImpl/WebFrameWidgetImpl (and helps with the split)

3- The logic is simply calling the corresponding method on
   InputMethodController of the focused LocalFrame.

4- Jumping from RenderFrameImpl to RenderWidget for getting selection is futile
   and expecting a focused frame is incorrect. Note that
   GestureManager::HandleGestureLongPress first passes the hit test result to
   SelectionController and then to MouseEventManager where the former generates
   selection updates and the latter updates frame focus. Therefore, right now,
   long pressing text on an unfocused OOPIF will lead to an empty selection
   update which is incorrect.

Reason for rename:
1- The method calls GetSelectionOffsets on InputMethodController (core) and it
   makes sense to make the wrapper variant have the same name.

  This CL also adds a small change to TextSelectionControllerClientChildFrame
to use selection range instead of selection bounds (now that the update is
fixed after the refactor).

BUG=629721, 723790
Review-Url: https://codereview.chromium.org/2910223003
Cr-Commit-Position: refs/heads/master@{#488035}
Committed: https://chromium.googlesource.com/chromium/src/+/d503ac6073fad15a95d4eb5d9f1343a315c7a9d7

[EhsanK, 2017-05-30 22:19:31]

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

[commit-bot: I haz the power, 2017-05-30 22:19:59]

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-05-30 22:42:36]

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

[commit-bot: I haz the power, 2017-05-30 22:42:37]

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

[EhsanK, 2017-05-31 01:30:23]

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

[commit-bot: I haz the power, 2017-05-31 01:31:20]

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-05-31 03:14:16]

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

[commit-bot: I haz the power, 2017-05-31 03:14:18]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2017-05-31 13:03:21]

ekaramad@chromium.org changed reviewers:
+ dcheng@chromium.org

[EhsanK, 2017-05-31 13:03:22]

PTAL.

[EhsanK, 2017-06-19 21:37:45]

On 2017/05/31 13:03:22, EhsanK wrote:
> PTAL.

Hello Daniel,

Friendly ping on this CL. Thanks!

[dcheng, 2017-06-20 09:05:47]

Sorry for the long review latency.

https://codereview.chromium.org/2910223003/diff/40001/content/browser/rendere...
File
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.cc
(right):

https://codereview.chromium.org/2910223003/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.cc:130:
rwhv_->GetSelectionRange(&selection_range) && !selection_range.is_empty();
Can you help me understand what the difference between selection_start /
selection_end_ and RWHV::GetSelectionRange() here is?

[EhsanK, 2017-07-10 15:38:28]

ekaramad@chromium.org changed reviewers:
+ wjmaclean@chromium.org

[EhsanK, 2017-07-10 15:38:30]

Thanks Daniel and sorry for the even longer delay (I missed the question).

PTAL.

https://codereview.chromium.org/2910223003/diff/40001/content/browser/rendere...
File
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.cc
(right):

https://codereview.chromium.org/2910223003/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.cc:130:
rwhv_->GetSelectionRange(&selection_range) && !selection_range.is_empty();
On 2017/06/20 09:05:47, dcheng wrote:
> Can you help me understand what the difference between selection_start /
> selection_end_ and RWHV::GetSelectionRange() here is?

They come from two different IPCs. 
  * |selection_start_| and |selection_end_|: gfx::SelectionBounds which I
beleive represents the whereabouts of the highlight text. This does not include
any text.

  * GetSelectionRange: This is based on text selection and does not include any
geometry data. Essentially, when FOO is highlighted/selected in a page, we send
a substring from page including FOO (preferrably 100 chars before and after it)
through FrameHostMsg_SelectionChanged IPC. This IPC includes the following data:
  1- text: which is the whole text (could be as much as 200 + length(FOO))
  2- range: the selection range in the text (indices are wrt the beginning of
the page)
  3- offset: from the beginning of the page.

So basically, GetSelectionRange(&range) should return a range with nonzero
length when we have highlighted text.

+wjmaclean@ for more input.

[wjmaclean, 2017-07-10 18:36:07]

https://codereview.chromium.org/2910223003/diff/40001/content/browser/rendere...
File
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.cc
(right):

https://codereview.chromium.org/2910223003/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.cc:130:
rwhv_->GetSelectionRange(&selection_range) && !selection_range.is_empty();
On 2017/07/10 15:38:29, EhsanK wrote:
> On 2017/06/20 09:05:47, dcheng wrote:
> > Can you help me understand what the difference between selection_start /
> > selection_end_ and RWHV::GetSelectionRange() here is?
> 
> They come from two different IPCs. 
>   * |selection_start_| and |selection_end_|: gfx::SelectionBounds which I
> beleive represents the whereabouts of the highlight text. This does not
include
> any text.
> 
>   * GetSelectionRange: This is based on text selection and does not include
any
> geometry data. Essentially, when FOO is highlighted/selected in a page, we
send
> a substring from page including FOO (preferrably 100 chars before and after
it)
> through FrameHostMsg_SelectionChanged IPC. This IPC includes the following
data:
>   1- text: which is the whole text (could be as much as 200 + length(FOO))
>   2- range: the selection range in the text (indices are wrt the beginning of
> the page)
>   3- offset: from the beginning of the page.
> 
> So basically, GetSelectionRange(&range) should return a range with nonzero
> length when we have highlighted text.
> 
> +wjmaclean@ for more input.

If you compare the pre- vs. post-patch code here, and compare with the
equivalent code in TouchSelectionControllerClientAura, ekaramad@'s change brings
the code in line with what the top-level Aura-client is doing. I would have
written the child client the same way, except there's an issue where the
information for GetSelectionRange() in a child has different timing w.r.t. the
mainframe's version (and it's a different IPC from the OnSelectionUpdate one).

I guess Im sort of repeating what ekaramad@ has already said.

[dcheng, 2017-07-17 19:07:38]

lgtm

[EhsanK, 2017-07-18 11:47:41]

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

[commit-bot: I haz the power, 2017-07-18 11:47:48]

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-07-18 11:50:18]

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

[commit-bot: I haz the power, 2017-07-18 11:50:19]

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

[EhsanK, 2017-07-18 11:50:26]

ekaramad@chromium.org changed reviewers:
+ alexmos@chromium.org, dtapuska@chromium.org

[EhsanK, 2017-07-18 11:50:26]

dtapuska@chromium.org: Please review changes in 
*/input/
alexmos@chromium.org: Please review changes in 
content/

Thanks!

[EhsanK, 2017-07-18 14:22:28]

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

[commit-bot: I haz the power, 2017-07-18 14:22:39]

Dry run: CQ is trying da patch.

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

[alexmos, 2017-07-18 16:29:59]

content/ LGTM

[commit-bot: I haz the power, 2017-07-18 18:25:35]

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

[commit-bot: I haz the power, 2017-07-18 18:25:37]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2017-07-19 20:45:20]

On 2017/07/18 16:29:59, alexmos wrote:
> content/ LGTM

lgtm

[EhsanK, 2017-07-19 20:52:05]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2017-07-19 20:52:05]

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

[commit-bot: I haz the power, 2017-07-19 20:52:18]

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-07-19 23:26:15]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1500497525259640,
"parent_rev": "f289680e58fc82c89bc01f2e8c2f370915552865", "commit_rev":
"b2c7cb1cb13c67f1df8c669c4a1a11ddd9c2412a"}

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

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1500497525259640,
"parent_rev": "5c6dd63b557508e679aa4fc700812f2df6854ea4", "commit_rev":
"d503ac6073fad15a95d4eb5d9f1343a315c7a9d7"}

[commit-bot: I haz the power, 2017-07-19 23:26:53]

Description was changed from

==========
[refactor] - Rename and Move WebWidget::CaretOrSelectionRange to
WebInputMethodController::GetSelectionOffsets

Reasons for move:
1- This is a frame concept given (the two nontrivial calls are from
   render_frame_impl.cc and frame_input_handler_impl.cc).

2- It cleans up WebViewImpl/WebFrameWidgetImpl (and helps with the split)

3- The logic is simply calling the corresponding method on
   InputMethodController of the focused LocalFrame.

4- Jumping from RenderFrameImpl to RenderWidget for getting selection is futile
   and expecting a focused frame is incorrect. Note that
   GestureManager::HandleGestureLongPress first passes the hit test result to
   SelectionController and then to MouseEventManager where the former generates
   selection updates and the latter updates frame focus. Therefore, right now,
   long pressing text on an unfocused OOPIF will lead to an empty selection
   update which is incorrect.

Reason for rename:
1- The method calls GetSelectionOffsets on InputMethodController (core) and it
   makes sense to make the wrapper variant have the same name.

  This CL also adds a small change to TextSelectionControllerClientChildFrame
to use selection range instead of selection bounds (now that the update is
fixed after the refactor).

BUG=629721, 723790
==========

to

==========
[refactor] - Rename and Move WebWidget::CaretOrSelectionRange to
WebInputMethodController::GetSelectionOffsets

Reasons for move:
1- This is a frame concept given (the two nontrivial calls are from
   render_frame_impl.cc and frame_input_handler_impl.cc).

2- It cleans up WebViewImpl/WebFrameWidgetImpl (and helps with the split)

3- The logic is simply calling the corresponding method on
   InputMethodController of the focused LocalFrame.

4- Jumping from RenderFrameImpl to RenderWidget for getting selection is futile
   and expecting a focused frame is incorrect. Note that
   GestureManager::HandleGestureLongPress first passes the hit test result to
   SelectionController and then to MouseEventManager where the former generates
   selection updates and the latter updates frame focus. Therefore, right now,
   long pressing text on an unfocused OOPIF will lead to an empty selection
   update which is incorrect.

Reason for rename:
1- The method calls GetSelectionOffsets on InputMethodController (core) and it
   makes sense to make the wrapper variant have the same name.

  This CL also adds a small change to TextSelectionControllerClientChildFrame
to use selection range instead of selection bounds (now that the update is
fixed after the refactor).

BUG=629721, 723790

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

[commit-bot: I haz the power, 2017-07-19 23:26:55]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/d503ac6073fad15a95d4eb5d9f13...