Reland: Implement Aura side of unified touch text selection for contents

Reland: Implement Aura side of unified touch text selection for contents

This is the first part of unified touch text selection for Aura which
implements it using the new ui::TouchSelectionController for contents.

Selection updates are now coming from swap-compositor-frame IPC message
instead of selection-bounds-changed IPC message.

This CL also replaces TouchEditableImplAura (owned by
WebContentsViewAura) with ui::TouchSelectionController (owned by
RenderWidgetHostViewAura) for which a client implementation is provided
in TouchSelectionControllerClientAura.

Note that at the moment we are unable to remove selection-bounds-changed
IPC message completely as it is also used by IME and the new path is not
yet working properly inside <webview> (which is being worked on).

Next part would be using ui::TouchSelectionController to implement
unified touch text selection for Views textfields.

Relanding after the blink issue for composited selection in remote
frames got fixed: r200194.

COLLABORATOR=mfomitchev
BUG=399721
Committed: https://crrev.com/2a5e3624e4d0b78bfe5f5a3d40f5a98730291056
Cr-Commit-Position: refs/heads/master@{#341134}

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

[mfomitchev, 2014-11-04 22:55:49]

mfomitchev@chromium.org changed reviewers:
+ mfomitchev@chromium.org

[jdduke (slow), 2014-11-06 16:17:44]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2014-11-06 16:17:45]

Nice!

https://codereview.chromium.org/698253004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_aura.cc:938: NOTREACHED()
<< "Selection bounds should be routed through the compositor.";
Yay! I'm still curious how the frame metadata will work with, say, iframes where
the selection is in an iframe. Is that something you've had a chance to test?

[mohsen, 2014-11-07 17:36:07]

On 2014/11/06 16:17:45, jdduke wrote:
> Nice!
> 
>
https://codereview.chromium.org/698253004/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/698253004/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/render_widget_host_view_aura.cc:938:
NOTREACHED()
> << "Selection bounds should be routed through the compositor.";
> Yay! I'm still curious how the frame metadata will work with, say, iframes
where
> the selection is in an iframe. Is that something you've had a chance to test?

I haven't looked into this. Can you explain what might go wrong with iframes?

[jdduke (slow), 2014-11-07 17:43:48]

On 2014/11/07 17:36:07, mohsen wrote:
> On 2014/11/06 16:17:45, jdduke wrote:
> > Nice!
> > 
> >
>
https://codereview.chromium.org/698253004/diff/1/content/browser/renderer_hos...
> > File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> > 
> >
>
https://codereview.chromium.org/698253004/diff/1/content/browser/renderer_hos...
> > content/browser/renderer_host/render_widget_host_view_aura.cc:938:
> NOTREACHED()
> > << "Selection bounds should be routed through the compositor.";
> > Yay! I'm still curious how the frame metadata will work with, say, iframes
> where
> > the selection is in an iframe. Is that something you've had a chance to
test?
> 
> I haven't looked into this. Can you explain what might go wrong with iframes?

I would assume that each iframe might get its own frame metadata? Or is that
only for out-of-process iframes? In which case, the question is how does the
frame metadata bubble to the browser, and will selection regions in iframes be
properly recognized?

[jdduke (slow), 2014-11-13 15:18:56]

https://codereview.chromium.org/698253004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_selection_controller.cc (right):

https://codereview.chromium.org/698253004/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_selection_controller.cc:49:
HideAndDisallowShowingAutomatically(); // XXX: needed?
Probably not =/

https://codereview.chromium.org/698253004/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_selection_controller.cc:318: if
(!activate_selection_automatically_) // XXX: needed?
Yes, else the handles would be shown for programmatic selection.

[mfomitchev, 2014-11-26 22:02:44]

https://codereview.chromium.org/698253004/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2470: void
RenderWidgetHostViewAura::InternalSetBounds(const gfx::Rect& rect) {
Don't we need to notify the touch selection controller here somehow given that
we used to notify touch_editing_client_?

[mohsen, 2014-12-02 17:21:00]

Patchset #4 (id:60001) has been deleted

[mohsen, 2014-12-15 04:35:50]

Patchset #6 (id:120001) has been deleted

[mohsen, 2014-12-30 17:47:11]

Patchset #8 (id:180001) has been deleted

[jdduke (slow), 2015-01-28 16:35:32]

At a high level, this seems more than reasonable, though I can't really speak to
the Aura-specific changes.

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle.h (right):

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
ui/touch_selection/touch_handle.h:98: friend class
TouchSelectionControllerAuraTest;
Hmm, this friend declaration is a shame, I guess the alternative is having the
client return a drawable with some shared state?

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle_drawable_aura.cc (right):

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:32: handle_image =
&ui::ResourceBundle::GetSharedInstance().GetImageNamed(
How expensive are these calls? If they're relatively cheap, we can just query
them on-demand, as we shouldn't be fetching them super often.

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:217: void
TouchSelectionController::TryActivateSelection() {
How exactly does this differ from |AllowShowingFromCurrentSelection|? Can we
merge the two in some fashion?

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller_aura.cc (right):

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.cc:33: true));
The side-effetcs of |true| are a little unclear unless you go look at the
declaration, can you give this a temporary named variable, e.g., |bool
show_on_tap_for_empty_editable = true|?

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.cc:53: if
(controller_->start() == start && controller_->end() == end)
Don't love this early exit, one alternative would be returning a bool from
|controller_->OnSelectionBoundsUpdated| if the bounds have changed.

[mohsen, 2015-02-12 23:06:53]

Patchset #19 (id:420001) has been deleted

[mohsen, 2015-02-12 23:09:23]

Patchset #19 (id:440001) has been deleted

[mohsen, 2015-02-13 19:29:10]

Patchset #21 (id:500001) has been deleted

[mohsen, 2015-02-20 20:29:51]

Patchset #22 (id:540001) has been deleted

[mohsen, 2015-02-22 23:16:45]

Patchset #23 (id:580001) has been deleted

[mohsen, 2015-02-22 23:23:07]

mohsen@chromium.org changed reviewers:
+ sadrul@chromium.org

[mohsen, 2015-02-22 23:23:09]

This patch is finally ready for review. Note that it depends on
http://crrev.com/937773003.

jdduke@, sadrul@: can you please take a look...

(I've also replied to some old comments)

https://codereview.chromium.org/698253004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_selection_controller.cc (right):

https://codereview.chromium.org/698253004/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_selection_controller.cc:49:
HideAndDisallowShowingAutomatically(); // XXX: needed?
On 2014/11/13 15:18:55, jdduke wrote:
> Probably not =/

Removed.

https://codereview.chromium.org/698253004/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2470: void
RenderWidgetHostViewAura::InternalSetBounds(const gfx::Rect& rect) {
On 2014/11/26 22:02:44, mfomitchev wrote:
> Don't we need to notify the touch selection controller here somehow given that
> we used to notify touch_editing_client_?

In WebContentsViewAura, we're handling a bunch of window movements. I believe
that will cover cases we need to handle for touch selection.

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle.h (right):

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
ui/touch_selection/touch_handle.h:98: friend class
TouchSelectionControllerAuraTest;
On 2015/01/28 16:35:32, jdduke wrote:
> Hmm, this friend declaration is a shame, I guess the alternative is having the
> client return a drawable with some shared state?

Fixed, please take another look.

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle_drawable_aura.cc (right):

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:32: handle_image =
&ui::ResourceBundle::GetSharedInstance().GetImageNamed(
On 2015/01/28 16:35:32, jdduke wrote:
> How expensive are these calls? If they're relatively cheap, we can just query
> them on-demand, as we shouldn't be fetching them super often.

Yeah, apparently ResourceBundle has its own caching and we also need to load
handle images only when handle's orientation changes or quick menu is shown,
which are not happening very often. Removed the caching.

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:217: void
TouchSelectionController::TryActivateSelection() {
On 2015/01/28 16:35:32, jdduke wrote:
> How exactly does this differ from |AllowShowingFromCurrentSelection|? Can we
> merge the two in some fashion?

Oh, I missed |AllowShowingFromCurrentSelection()| completely. It does exactly
what I want. Removed this one.

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller_aura.cc (right):

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.cc:33: true));
On 2015/01/28 16:35:32, jdduke wrote:
> The side-effetcs of |true| are a little unclear unless you go look at the
> declaration, can you give this a temporary named variable, e.g., |bool
> show_on_tap_for_empty_editable = true|?

Done.

https://codereview.chromium.org/698253004/diff/400001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.cc:53: if
(controller_->start() == start && controller_->end() == end)
On 2015/01/28 16:35:32, jdduke wrote:
> Don't love this early exit, one alternative would be returning a bool from
> |controller_->OnSelectionBoundsUpdated| if the bounds have changed.

Done.

[jdduke (slow), 2015-02-24 16:53:47]

Have you verified that this works properly with plugin-based text selection?
Particularly as it pertains to the selection bounds being routed from the
renderer (see
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

https://codereview.chromium.org/698253004/diff/600001/content/browser/rendere...
File content/browser/renderer_host/ui_touch_selection_helper.h (right):

https://codereview.chromium.org/698253004/diff/600001/content/browser/rendere...
content/browser/renderer_host/ui_touch_selection_helper.h:12: namespace ui {
Let's go ahead and include the necessary ui::SelectionBound header, as we're
returning by value.

Also, can we move this file to renderer_host/input?

[mohsen, 2015-02-25 23:52:19]

jdduke@: As for plugin-based text selection, I've tried the patch with Secure
Shell[1] and Vim[2] extensions which I believe are NaCl based extensions and it
seems to be working fine.

sadrul@: Do you know any other similar cases that we should be careful not to
break touch selection on?

[1]
https://chrome.google.com/webstore/detail/secure-shell/pnhechapfaindjhompbnfl...
[2]
https://chrome.google.com/webstore/detail/vim/dhhoacdlegcbdglbfnhgnlchpkdlofkb

https://codereview.chromium.org/698253004/diff/600001/content/browser/rendere...
File content/browser/renderer_host/ui_touch_selection_helper.h (right):

https://codereview.chromium.org/698253004/diff/600001/content/browser/rendere...
content/browser/renderer_host/ui_touch_selection_helper.h:12: namespace ui {
On 2015/02/24 16:53:47, jdduke wrote:
> Let's go ahead and include the necessary ui::SelectionBound header, as we're
> returning by value.
> 
> Also, can we move this file to renderer_host/input?

Sure. Done.

[jdduke (slow), 2015-02-27 18:36:46]

At a high-level this looks good. The changes to touch_selection_controller.* are
fine, but I may not be the best reviewer for Aura-specific changes. Happy to
rubberstamp approval when ready.

https://codereview.chromium.org/698253004/diff/660001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller_aura.cc (right):

https://codereview.chromium.org/698253004/diff/660001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.cc:95: case
ET_GESTURE_LONG_PRESS:
Who is most familiar with the previous version of this code? Can we get them to
review it?

[mohsen, 2015-02-27 18:50:41]

https://codereview.chromium.org/698253004/diff/660001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller_aura.cc (right):

https://codereview.chromium.org/698253004/diff/660001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.cc:95: case
ET_GESTURE_LONG_PRESS:
On 2015/02/27 18:36:46, jdduke wrote:
> Who is most familiar with the previous version of this code? Can we get them
to
> review it?

Previously, mostly sadrul@ and sky@ were reviewing codes for touch selection on
aura. sadrul@ is already in the reviewers.

[sadrul, 2015-03-05 12:37:03]

I haven't gone through the entire CL yet, but I have left some
comments/questions.

https://codereview.chromium.org/698253004/diff/720001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/720001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2021: return;
Have you considered making the selection controller a pre-target event handler
so you don't have to manually feed it events from here?

https://codereview.chromium.org/698253004/diff/720001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/698253004/diff/720001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:315:
ui::TouchSelectionControllerAuraClient* selection_controller_client);
scoped_ptr<> so ownership transfer is clear.

https://codereview.chromium.org/698253004/diff/720001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/720001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1129:
view->InitSelectionController(selection_controller_client);
Why can't RWHVA itself create this?

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle_drawable_aura.cc (right):

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:45: default:
Remove default, explicitly list UNDEFINED

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:57: // through handles to the
underlying window.
Can you set Window::set_ignore_events() instead?

Why ignore events though? Shouldn't the user be able to drag the handle to
change selection?

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:148: default:
case UNDEFINED: instead of default:

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle_drawable_aura.h (right):

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.h:15: public
aura_extra::ImageWindowDelegate {
Instead of making TouchHandleDrawableAura a subclass of ImageWindowDelegate, can
TouchHandleDrawableAura have an instance of ImageWindowDelegate instead?
Something like:

  TouchHandleDrawableAura : TouchHandleDrawable {
    ImageWindowDelegate* window_delegate_;
    scoped_ptr<aura::Window> window_;
  }

As long as |window_| is alive, |window_delegate_| is guaranteed to be alive as
well, and so you can SetImage() etc. on it as you need to, and you don't need
the OnWindowDestroyed() override either.

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller_aura.cc (right):

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.cc:33: end.y() - origin.y());
I don't think this is correct if there's rotation involved?

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.cc:196:
aura::Env::GetInstance()->AddPreTargetHandler(this);
Whoa. Why is this necessary?

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.cc:213:
aura::Env::GetInstance()->AddPreTargetHandler(this);
ditto

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller_aura.h (right):

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.h:41: class
UI_TOUCH_SELECTION_EXPORT TouchSelectionControllerAura
Calling this TouchSelectionControllerAura when it is not a
TouchSelectionController can be confusing. (e.g. RenderWidgetHostViewAura is-a
RenderWidgetHostView, NativeWidgetAura is-a NativeWidget, etc.)

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.h:55: void
HandleTouchEvent(TouchEvent* event);
Why have these two separate, instead of overriding the equivalents from
EventHandler?

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.h:102: }
Can you move these to TestApi pattern?

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.h:113: bool
immediate_quick_menu_for_testing_;
Doesn't look like this is ever initialized?

Also, move to test-api?

https://codereview.chromium.org/698253004/diff/720001/ui/views/touchui/touch_...
File ui/views/touchui/touch_selection_menu_runner_views.cc (right):

https://codereview.chromium.org/698253004/diff/720001/ui/views/touchui/touch_...
ui/views/touchui/touch_selection_menu_runner_views.cc:63: DCHECK(menu == menu_);
// or nullptr?
?

https://codereview.chromium.org/698253004/diff/720001/ui/views/touchui/touch_...
File ui/views/touchui/touch_selection_menu_runner_views.h (right):

https://codereview.chromium.org/698253004/diff/720001/ui/views/touchui/touch_...
ui/views/touchui/touch_selection_menu_runner_views.h:20:
~TouchSelectionMenuRunnerViews() override;
indenting looks off

[sadrul, 2015-03-05 12:38:36]

> sadrul@: Do you know any other similar cases that we should be careful not to
> break touch selection on?

You should check that this works correctly with the <webview> tag (and also keep
an eye on how it works with OOPIF).

[mfomitchev, 2015-03-05 16:07:54]

https://codereview.chromium.org/698253004/diff/720001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/720001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1129:
view->InitSelectionController(selection_controller_client);
Because TouchSelectionControllerAuraClientImpl is in web_contents/

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle_drawable_aura.cc (right):

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:57: // through handles to the
underlying window.
So this is a bit awkward because at this point we are trying to share the
targeting code with Android. The targeting flow for Android is as follows:
RenderWidgetHostViewAndroid::OnTouchEvent ->
TouchSelectionController::WillHandleTouchEvent -> TouchHandle::SetOrientation
The last method in the chain is where the targeting logic is actually
implemented. Android doesn't have Windows, WindowTargeters, etc, so this is what
it does. So we are trying to use a similar flow Aura. So we don't do any
targeting in the handle's window - we let it fall through to RWHVA, then to
TouchSelectionControllerAura::HandleTouchEvent, and then - the same flow as on
Android.

Now, I think we could've just used a Layer instead of |window_| for the handle
drawable, but for native Aura selection we actually will want the targeting to
be done by the WindowTargeter rather than TouchHandle. Perhaps at that point we
should also switch the content targeting over to use the same mechanism - it's
probably more important for Aura handles to be consistent across native/web than
for handles to be consistent from Aura to Clank.

[sadrul, 2015-03-05 16:56:23]

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle_drawable_aura.cc (right):

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:57: // through handles to the
underlying window.
On 2015/03/05 16:07:54, mfomitchev wrote:
> So this is a bit awkward because at this point we are trying to share the
> targeting code with Android. The targeting flow for Android is as follows:
> RenderWidgetHostViewAndroid::OnTouchEvent ->
> TouchSelectionController::WillHandleTouchEvent -> TouchHandle::SetOrientation
> The last method in the chain is where the targeting logic is actually
> implemented. Android doesn't have Windows, WindowTargeters, etc, so this is
what
> it does. So we are trying to use a similar flow Aura. So we don't do any
> targeting in the handle's window - we let it fall through to RWHVA, then to
> TouchSelectionControllerAura::HandleTouchEvent, and then - the same flow as on
> Android.

We shouldn't do this in a roundabout way to fit into the same flow as Android.
Being consistent with the rest of aura is as important as being consistent with
Android code. It would be better to do any necessary refactor here so that the
default window-targeting can be used here.

> 
> Now, I think we could've just used a Layer instead of |window_| for the handle
> drawable, but for native Aura selection we actually will want the targeting to
> be done by the WindowTargeter rather than TouchHandle. Perhaps at that point
we
> should also switch the content targeting over to use the same mechanism - it's
> probably more important for Aura handles to be consistent across native/web
than
> for handles to be consistent from Aura to Clank. 

I am not sure what you mean by 'native/web'; do you mean native == views (e.g.
omnibox) and web == content (i.e. content/RWHVA)?

[mfomitchev, 2015-03-05 18:03:24]

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle_drawable_aura.cc (right):

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:57: // through handles to the
underlying window.
On 2015/03/05 16:56:22, sadrul wrote:
> On 2015/03/05 16:07:54, mfomitchev wrote:
> > So this is a bit awkward because at this point we are trying to share the
> > targeting code with Android. The targeting flow for Android is as follows:
> > RenderWidgetHostViewAndroid::OnTouchEvent ->
> > TouchSelectionController::WillHandleTouchEvent ->
TouchHandle::SetOrientation
> > The last method in the chain is where the targeting logic is actually
> > implemented. Android doesn't have Windows, WindowTargeters, etc, so this is
> what
> > it does. So we are trying to use a similar flow Aura. So we don't do any
> > targeting in the handle's window - we let it fall through to RWHVA, then to
> > TouchSelectionControllerAura::HandleTouchEvent, and then - the same flow as
on
> > Android.
> 
> We shouldn't do this in a roundabout way to fit into the same flow as Android.
> Being consistent with the rest of aura is as important as being consistent
with
> Android code. It would be better to do any necessary refactor here so that the
> default window-targeting can be used here.
>
> > Now, I think we could've just used a Layer instead of |window_| for the
handle
> > drawable, but for native Aura selection we actually will want the targeting
to
> > be done by the WindowTargeter rather than TouchHandle. Perhaps at that point
> we
> > should also switch the content targeting over to use the same mechanism -
it's
> > probably more important for Aura handles to be consistent across native/web
> than
> > for handles to be consistent from Aura to Clank. 
> 
> I am not sure what you mean by 'native/web'; do you mean native == views (e.g.
> omnibox) and web == content (i.e. content/RWHVA)?

Yes, that's what I meant. Thus we'd have to switch to using window-based
targeting for content/RWHVA for Aura eventually in any case. So I agree we might
as well do it now.

[mohsen, 2015-03-06 22:40:39]

Patchset #30 (id:740001) has been deleted

[mohsen, 2015-03-06 23:10:09]

https://codereview.chromium.org/698253004/diff/720001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/720001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2021: return;
On 2015/03/05 12:37:03, sadrul wrote:
> Have you considered making the selection controller a pre-target event handler
> so you don't have to manually feed it events from here?

Since selection controller was already a pre-target handler for Env, if I had
set it as a pre-target handler for RWHVA, too, I wouldn't be able to check if an
OnXXXEvent is called due to Env or RWHVA. I fixed this by creating a separate
event handler class for Env (TouchSelectionControllerAura::EnvPreTargetHandler)
and making selection controller pre-target handler for RWHVA's window.

https://codereview.chromium.org/698253004/diff/720001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/698253004/diff/720001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:315:
ui::TouchSelectionControllerAuraClient* selection_controller_client);
On 2015/03/05 12:37:03, sadrul wrote:
> scoped_ptr<> so ownership transfer is clear.

Done.

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle_drawable_aura.cc (right):

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:45: default:
On 2015/03/05 12:37:03, sadrul wrote:
> Remove default, explicitly list UNDEFINED

Done.

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:57: // through handles to the
underlying window.
On 2015/03/05 18:03:24, mfomitchev wrote:
> On 2015/03/05 16:56:22, sadrul wrote:
> > On 2015/03/05 16:07:54, mfomitchev wrote:
> > > So this is a bit awkward because at this point we are trying to share the
> > > targeting code with Android. The targeting flow for Android is as follows:
> > > RenderWidgetHostViewAndroid::OnTouchEvent ->
> > > TouchSelectionController::WillHandleTouchEvent ->
> TouchHandle::SetOrientation
> > > The last method in the chain is where the targeting logic is actually
> > > implemented. Android doesn't have Windows, WindowTargeters, etc, so this
is
> > what
> > > it does. So we are trying to use a similar flow Aura. So we don't do any
> > > targeting in the handle's window - we let it fall through to RWHVA, then
to
> > > TouchSelectionControllerAura::HandleTouchEvent, and then - the same flow
as
> on
> > > Android.
> > 
> > We shouldn't do this in a roundabout way to fit into the same flow as
Android.
> > Being consistent with the rest of aura is as important as being consistent
> with
> > Android code. It would be better to do any necessary refactor here so that
the
> > default window-targeting can be used here.
> >
> > > Now, I think we could've just used a Layer instead of |window_| for the
> handle
> > > drawable, but for native Aura selection we actually will want the
targeting
> to
> > > be done by the WindowTargeter rather than TouchHandle. Perhaps at that
point
> > we
> > > should also switch the content targeting over to use the same mechanism -
> it's
> > > probably more important for Aura handles to be consistent across
native/web
> > than
> > > for handles to be consistent from Aura to Clank. 
> > 
> > I am not sure what you mean by 'native/web'; do you mean native == views
(e.g.
> > omnibox) and web == content (i.e. content/RWHVA)?
> 
> Yes, that's what I meant. Thus we'd have to switch to using window-based
> targeting for content/RWHVA for Aura eventually in any case. So I agree we
might
> as well do it now.

This is how ui::TouchSelectionController is implemented and I tried to change it
as little as possible for this patch. It gets the touch events and has its own
handle targeting and even gesture recognition logic. Probably, jdduke@ can
explain better why this was needed. Our plan is to remove the pass-through
targeter when adding support for Views textfields. But, even in that case the
handle would receive events and just forward them to selection controller for
targeting/gesture recognition (see ui::TouchHandleDrawableAura and
ui::TouchHandleDrawableAuraDelegate sections in the design doc).

(For now, I replaced targeter with set_ignore_events())

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:148: default:
On 2015/03/05 12:37:03, sadrul wrote:
> case UNDEFINED: instead of default:

Done.

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle_drawable_aura.h (right):

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.h:15: public
aura_extra::ImageWindowDelegate {
On 2015/03/05 12:37:03, sadrul wrote:
> Instead of making TouchHandleDrawableAura a subclass of ImageWindowDelegate,
can
> TouchHandleDrawableAura have an instance of ImageWindowDelegate instead?
> Something like:
> 
>   TouchHandleDrawableAura : TouchHandleDrawable {
>     ImageWindowDelegate* window_delegate_;
>     scoped_ptr<aura::Window> window_;
>   }
> 
> As long as |window_| is alive, |window_delegate_| is guaranteed to be alive as
> well, and so you can SetImage() etc. on it as you need to, and you don't need
> the OnWindowDestroyed() override either.

Yeah. Done.

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller_aura.cc (right):

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.cc:33: end.y() - origin.y());
On 2015/03/05 12:37:03, sadrul wrote:
> I don't think this is correct if there's rotation involved?

I've just copied an existing code (from RWHVAura::ConvertRectToScreen). There is
also a somewhat similar code in ash/screen_util.h which doesn't seem to handle
rotation. Do we need to handle rotation here? Probably, ConvertPointToScreen()
which is used here should handle rotation?

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.cc:196:
aura::Env::GetInstance()->AddPreTargetHandler(this);
On 2015/03/05 12:37:03, sadrul wrote:
> Whoa. Why is this necessary?

This was needed as we might have multiple selection-shown's ending in one
selection-cleared. So, at this point "this" might have been already added to
pre-target handlers list. The above code is to ensure that we don't end up
having multiple copies of "this" in the list. Now that I've created a separate
pre-target event handler class for Env, this is not an issue anymore.

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller_aura.h (right):

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.h:41: class
UI_TOUCH_SELECTION_EXPORT TouchSelectionControllerAura
On 2015/03/05 12:37:03, sadrul wrote:
> Calling this TouchSelectionControllerAura when it is not a
> TouchSelectionController can be confusing. (e.g. RenderWidgetHostViewAura is-a
> RenderWidgetHostView, NativeWidgetAura is-a NativeWidget, etc.)

Yeah. Any suggestions? AuraTouchSelectionController?!

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.h:55: void
HandleTouchEvent(TouchEvent* event);
On 2015/03/05 12:37:03, sadrul wrote:
> Why have these two separate, instead of overriding the equivalents from
> EventHandler?

EventHandler equivalents were for pre-target handling of Env. Now that I created
a separate pre-target handler class for Env, I'm replaced these two with
EventHandler equivalents.

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.h:102: }
On 2015/03/05 12:37:03, sadrul wrote:
> Can you move these to TestApi pattern?

Done.

https://codereview.chromium.org/698253004/diff/720001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.h:113: bool
immediate_quick_menu_for_testing_;
On 2015/03/05 12:37:03, sadrul wrote:
> Doesn't look like this is ever initialized?
> 
> Also, move to test-api?

Initialization fixed.
Also, moved this to test api. Though, I'm not sure if the result looks good.

https://codereview.chromium.org/698253004/diff/720001/ui/views/touchui/touch_...
File ui/views/touchui/touch_selection_menu_runner_views.cc (right):

https://codereview.chromium.org/698253004/diff/720001/ui/views/touchui/touch_...
ui/views/touchui/touch_selection_menu_runner_views.cc:63: DCHECK(menu == menu_);
// or nullptr?
On 2015/03/05 12:37:03, sadrul wrote:
> ?

Removed comment.

https://codereview.chromium.org/698253004/diff/720001/ui/views/touchui/touch_...
File ui/views/touchui/touch_selection_menu_runner_views.h (right):

https://codereview.chromium.org/698253004/diff/720001/ui/views/touchui/touch_...
ui/views/touchui/touch_selection_menu_runner_views.h:20:
~TouchSelectionMenuRunnerViews() override;
On 2015/03/05 12:37:03, sadrul wrote:
> indenting looks off

Fixed.

[mohsen, 2015-03-12 15:32:50]

To make that patch more manageable, I'm going to split it further up into a few
CLs, mostly based on directory (ui/touch_selection, ui/views, content, ...).

First one that contains changes in ui/touch_selection is uploaded at
http://crrev.com/996373002.

[sadrul, 2015-03-13 17:01:11]

Can you explain why RWHVAndroid owns a ui::TouchSelectionController object,
whereas RWHVAura owns a ui::TouchSelectionControllerClient object? Shouldn't the
two be able to use the same object (with arguably different behaviour through
the client interface) for managing the same functionality?

[mohsen, 2015-03-13 17:57:16]

On 2015/03/13 17:01:11, sadrul wrote:
> Can you explain why RWHVAndroid owns a ui::TouchSelectionController object,
> whereas RWHVAura owns a ui::TouchSelectionControllerClient object? Shouldn't
the
> two be able to use the same object (with arguably different behaviour through
> the client interface) for managing the same functionality?

RWHVAura owns a TouchSelectionControllerAura object, which is-a
TouchSelectionController (analogous to TouchSelectionController owned by
RWHVAndroid). It needs to provide an implementation for
TouchSelectionControllerAuraClient, but since this implementation needs access
to WebContentsViewAura, it is implemented as a separate class in web_contents/,
of which an instance is owned by RWHVAura.

[sadrul, 2015-03-13 18:06:05]

On 2015/03/13 17:57:16, mohsen wrote:
> On 2015/03/13 17:01:11, sadrul wrote:
> > Can you explain why RWHVAndroid owns a ui::TouchSelectionController object,
> > whereas RWHVAura owns a ui::TouchSelectionControllerClient object? Shouldn't
> the
> > two be able to use the same object (with arguably different behaviour
through
> > the client interface) for managing the same functionality?
> 
> RWHVAura owns a TouchSelectionControllerAura object, which is-a
> TouchSelectionController 

It's not though, right? TouchSelectionControllerAura is a
TouchSelectionControllerClient (and RWHVAndriod is also a
TouchSelectionControllerClient, and owns a TouchSelectionController). My claim
is that RWHVAura should also own a TouchSelectionController object, and possibly
be a TouchSelectionControllerClient.

> (analogous to TouchSelectionController owned by
> RWHVAndroid). It needs to provide an implementation for
> TouchSelectionControllerAuraClient, but since this implementation needs access
> to WebContentsViewAura, it is implemented as a separate class in
web_contents/,
> of which an instance is owned by RWHVAura.

[chromium-reviews, 2015-03-13 18:34:03]

Yeah, I should've said "almost is-a" or "meant to be-a" :-)
ToushSelectionControllerAura (TSCA) in Aura is analogous to
TouchSelectionController (TSC) in Android. Ideally, TSCA should've
inherited from TSC, but because of how TSC was written, directly inheriting
didn't make much sense (e.g. I needed to change some of the interface for
Aura). One way would be to refactor TSC entirely and have it as a base
class for TSCAndroid and TSCAura (as mfomitchev@ suggested once) but that
would be a lot of work and I believe jdduke@ was also opposed to that.
Also, my hope is that additional pieces in TSCA would eventually merge into
TSC and we would only have TSC.

I don't see why you think RWHVAura should own TSC, rather than TSCA. In
that case all implementation in TSCA should be moved to RWHVAura, I guess.
We need to use TSCA also for Views textfield.

On Fri, Mar 13, 2015 at 2:06 PM <sadrul@chromium.org> wrote:

> On 2015/03/13 17:57:16, mohsen wrote:
> > On 2015/03/13 17:01:11, sadrul wrote:
> > > Can you explain why RWHVAndroid owns a ui::TouchSelectionController
> > object,
> > > whereas RWHVAura owns a ui::TouchSelectionControllerClient object?
> > Shouldn't
> > the
> > > two be able to use the same object (with arguably different behaviour
> through
> > > the client interface) for managing the same functionality?
>
> > RWHVAura owns a TouchSelectionControllerAura object, which is-a
> > TouchSelectionController
>
> It's not though, right? TouchSelectionControllerAura is a
> TouchSelectionControllerClient (and RWHVAndriod is also a
> TouchSelectionControllerClient, and owns a TouchSelectionController). My
> claim
> is that RWHVAura should also own a TouchSelectionController object, and
> possibly
> be a TouchSelectionControllerClient.
>
> > (analogous to TouchSelectionController owned by
> > RWHVAndroid). It needs to provide an implementation for
> > TouchSelectionControllerAuraClient, but since this implementation needs
> > access
> > to WebContentsViewAura, it is implemented as a separate class in
> web_contents/,
> > of which an instance is owned by RWHVAura.
>
>
> https://codereview.chromium.org/698253004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mohsen, 2015-03-13 18:35:14]

On 2015/03/13 18:34:03, chromium-reviews wrote:
> Yeah, I should've said "almost is-a" or "meant to be-a" :-)
> ToushSelectionControllerAura (TSCA) in Aura is analogous to
> TouchSelectionController (TSC) in Android. Ideally, TSCA should've
> inherited from TSC, but because of how TSC was written, directly inheriting
> didn't make much sense (e.g. I needed to change some of the interface for
> Aura). One way would be to refactor TSC entirely and have it as a base
> class for TSCAndroid and TSCAura (as mfomitchev@ suggested once) but that
> would be a lot of work and I believe jdduke@ was also opposed to that.
> Also, my hope is that additional pieces in TSCA would eventually merge into
> TSC and we would only have TSC.
> 
> I don't see why you think RWHVAura should own TSC, rather than TSCA. In
> that case all implementation in TSCA should be moved to RWHVAura, I guess.
> We need to use TSCA also for Views textfield.
> 
> On Fri, Mar 13, 2015 at 2:06 PM <mailto:sadrul@chromium.org> wrote:
> 
> > On 2015/03/13 17:57:16, mohsen wrote:
> > > On 2015/03/13 17:01:11, sadrul wrote:
> > > > Can you explain why RWHVAndroid owns a ui::TouchSelectionController
> > > object,
> > > > whereas RWHVAura owns a ui::TouchSelectionControllerClient object?
> > > Shouldn't
> > > the
> > > > two be able to use the same object (with arguably different behaviour
> > through
> > > > the client interface) for managing the same functionality?
> >
> > > RWHVAura owns a TouchSelectionControllerAura object, which is-a
> > > TouchSelectionController
> >
> > It's not though, right? TouchSelectionControllerAura is a
> > TouchSelectionControllerClient (and RWHVAndriod is also a
> > TouchSelectionControllerClient, and owns a TouchSelectionController). My
> > claim
> > is that RWHVAura should also own a TouchSelectionController object, and
> > possibly
> > be a TouchSelectionControllerClient.
> >
> > > (analogous to TouchSelectionController owned by
> > > RWHVAndroid). It needs to provide an implementation for
> > > TouchSelectionControllerAuraClient, but since this implementation needs
> > > access
> > > to WebContentsViewAura, it is implemented as a separate class in
> > web_contents/,
> > > of which an instance is owned by RWHVAura.
> >
> >
> > https://codereview.chromium.org/698253004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Sorry, I sent the above comment directly from email and it's not showing my name
on it!

[sadrul, 2015-03-13 19:01:43]

On 2015/03/13 18:34:03, chromium-reviews wrote:
> Yeah, I should've said "almost is-a" or "meant to be-a" :-)
> ToushSelectionControllerAura (TSCA) in Aura is analogous to
> TouchSelectionController (TSC) in Android. Ideally, TSCA should've
> inherited from TSC, but because of how TSC was written, directly inheriting
> didn't make much sense (e.g. I needed to change some of the interface for
> Aura). One way would be to refactor TSC entirely and have it as a base
> class for TSCAndroid and TSCAura (as mfomitchev@ suggested once) but that
> would be a lot of work and I believe jdduke@ was also opposed to that.
> Also, my hope is that additional pieces in TSCA would eventually merge into
> TSC and we would only have TSC.

We should have a common API for TSC on android and aura. That could mean changes
in the current TSC interface/implementation, and/or some no-op implementations
in some platforms, and/or additional client/delegate interface, and/or a common
base interface/implementation with platform-specific implementations/overrides.
But having to deal with separate types of objects with
almost-the-same-but-slightly-different-api for the same functionality does not
make a lot of sense.

> 
> I don't see why you think RWHVAura should own TSC, rather than TSCA. In
> that case all implementation in TSCA should be moved to RWHVAura, I guess.
> We need to use TSCA also for Views textfield.

RWHVAura and Views textfield both would own a TSC, and provide the client that
does things differently for content vs. views.

> 
> On Fri, Mar 13, 2015 at 2:06 PM <mailto:sadrul@chromium.org> wrote:
> 
> > On 2015/03/13 17:57:16, mohsen wrote:
> > > On 2015/03/13 17:01:11, sadrul wrote:
> > > > Can you explain why RWHVAndroid owns a ui::TouchSelectionController
> > > object,
> > > > whereas RWHVAura owns a ui::TouchSelectionControllerClient object?
> > > Shouldn't
> > > the
> > > > two be able to use the same object (with arguably different behaviour
> > through
> > > > the client interface) for managing the same functionality?
> >
> > > RWHVAura owns a TouchSelectionControllerAura object, which is-a
> > > TouchSelectionController
> >
> > It's not though, right? TouchSelectionControllerAura is a
> > TouchSelectionControllerClient (and RWHVAndriod is also a
> > TouchSelectionControllerClient, and owns a TouchSelectionController). My
> > claim
> > is that RWHVAura should also own a TouchSelectionController object, and
> > possibly
> > be a TouchSelectionControllerClient.
> >
> > > (analogous to TouchSelectionController owned by
> > > RWHVAndroid). It needs to provide an implementation for
> > > TouchSelectionControllerAuraClient, but since this implementation needs
> > > access
> > > to WebContentsViewAura, it is implemented as a separate class in
> > web_contents/,
> > > of which an instance is owned by RWHVAura.
> >
> >
> > https://codereview.chromium.org/698253004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[mohsen, 2015-03-13 21:29:46]

On 2015/03/13 19:01:43, sadrul wrote:
> On 2015/03/13 18:34:03, chromium-reviews wrote:
> > Yeah, I should've said "almost is-a" or "meant to be-a" :-)
> > ToushSelectionControllerAura (TSCA) in Aura is analogous to
> > TouchSelectionController (TSC) in Android. Ideally, TSCA should've
> > inherited from TSC, but because of how TSC was written, directly inheriting
> > didn't make much sense (e.g. I needed to change some of the interface for
> > Aura). One way would be to refactor TSC entirely and have it as a base
> > class for TSCAndroid and TSCAura (as mfomitchev@ suggested once) but that
> > would be a lot of work and I believe jdduke@ was also opposed to that.
> > Also, my hope is that additional pieces in TSCA would eventually merge into
> > TSC and we would only have TSC.
> 
> We should have a common API for TSC on android and aura. That could mean
changes
> in the current TSC interface/implementation, and/or some no-op implementations
> in some platforms, and/or additional client/delegate interface, and/or a
common
> base interface/implementation with platform-specific
implementations/overrides.
> But having to deal with separate types of objects with
> almost-the-same-but-slightly-different-api for the same functionality does not
> make a lot of sense.
Those would be perfect solutions, if I had full control over TSC to refactor it.
However, my understanding was that the owner of TSC prefers we minimize TSC
modification at the moment and use it by wrapping it in our implementation
(TSCA) and later incorporate those changes into TSC if needed (Jared, please
correct me if I'm wrong). I found this the most practical way to go forward and
I believe I've shared this design with you and heard back no complaints about
it.

> > I don't see why you think RWHVAura should own TSC, rather than TSCA. In
> > that case all implementation in TSCA should be moved to RWHVAura, I guess.
> > We need to use TSCA also for Views textfield.
> 
> RWHVAura and Views textfield both would own a TSC, and provide the client that
> does things differently for content vs. views.
They are (going to be) owners of TSC, but indirectly through TSCA. That's just
like we've extracted touch selection parts of RWHVA (which are in common with
Views textfield) into TSCA. 

> > On Fri, Mar 13, 2015 at 2:06 PM <mailto:sadrul@chromium.org> wrote:
> > 
> > > On 2015/03/13 17:57:16, mohsen wrote:
> > > > On 2015/03/13 17:01:11, sadrul wrote:
> > > > > Can you explain why RWHVAndroid owns a ui::TouchSelectionController
> > > > object,
> > > > > whereas RWHVAura owns a ui::TouchSelectionControllerClient object?
> > > > Shouldn't
> > > > the
> > > > > two be able to use the same object (with arguably different behaviour
> > > through
> > > > > the client interface) for managing the same functionality?
> > >
> > > > RWHVAura owns a TouchSelectionControllerAura object, which is-a
> > > > TouchSelectionController
> > >
> > > It's not though, right? TouchSelectionControllerAura is a
> > > TouchSelectionControllerClient (and RWHVAndriod is also a
> > > TouchSelectionControllerClient, and owns a TouchSelectionController). My
> > > claim
> > > is that RWHVAura should also own a TouchSelectionController object, and
> > > possibly
> > > be a TouchSelectionControllerClient.
> > >
> > > > (analogous to TouchSelectionController owned by
> > > > RWHVAndroid). It needs to provide an implementation for
> > > > TouchSelectionControllerAuraClient, but since this implementation needs
> > > > access
> > > > to WebContentsViewAura, it is implemented as a separate class in
> > > web_contents/,
> > > > of which an instance is owned by RWHVAura.
> > >
> > >
> > > https://codereview.chromium.org/698253004/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[jdduke (slow), 2015-03-13 21:36:31]

> Those would be perfect solutions, if I had full control over TSC to refactor
it.
> However, my understanding was that the owner of TSC prefers we minimize TSC
> modification at the moment and use it by wrapping it in our implementation
> (TSCA) and later incorporate those changes into TSC if needed (Jared, please
> correct me if I'm wrong). I found this the most practical way to go forward
and
> I believe I've shared this design with you and heard back no complaints about
> it.

I'm not sure where I gave that impression. I don't have a strong opinion about
the current design of the TouchSelectionController as long as we keep it fairly
agnostic to platform-specific differences, and we should evolve it as
appropriate to support a unified model.

> 
> > > I don't see why you think RWHVAura should own TSC, rather than TSCA. In
> > > that case all implementation in TSCA should be moved to RWHVAura, I guess.
> > > We need to use TSCA also for Views textfield.
> > 
> > RWHVAura and Views textfield both would own a TSC, and provide the client
that
> > does things differently for content vs. views.
> They are (going to be) owners of TSC, but indirectly through TSCA. That's just
> like we've extracted touch selection parts of RWHVA (which are in common with
> Views textfield) into TSCA. 

Sadrul, I don't think we should get too hung up on the fact that RWHVAndroid is
a client of the TSC. If we want to make a class that implements the TSC client
for Android, but is separate from RWHVAndroid (yet owned by RWHVAndroid), that
sounds just fine.

[sadrul, 2015-03-13 22:14:42]

On 2015/03/13 21:36:31, jdduke wrote:
> > Those would be perfect solutions, if I had full control over TSC to refactor
> it.
> > However, my understanding was that the owner of TSC prefers we minimize TSC
> > modification at the moment and use it by wrapping it in our implementation
> > (TSCA) and later incorporate those changes into TSC if needed (Jared, please
> > correct me if I'm wrong). I found this the most practical way to go forward
> and
> > I believe I've shared this design with you and heard back no complaints
about
> > it.
> 
> I'm not sure where I gave that impression. I don't have a strong opinion about
> the current design of the TouchSelectionController as long as we keep it
fairly
> agnostic to platform-specific differences, and we should evolve it as
> appropriate to support a unified model.
> 
> > 
> > > > I don't see why you think RWHVAura should own TSC, rather than TSCA. In
> > > > that case all implementation in TSCA should be moved to RWHVAura, I
guess.
> > > > We need to use TSCA also for Views textfield.
> > > 
> > > RWHVAura and Views textfield both would own a TSC, and provide the client
> that
> > > does things differently for content vs. views.
> > They are (going to be) owners of TSC, but indirectly through TSCA. That's
just
> > like we've extracted touch selection parts of RWHVA (which are in common
with
> > Views textfield) into TSCA. 
> 
> Sadrul, I don't think we should get too hung up on the fact that RWHVAndroid
is
> a client of the TSC. If we want to make a class that implements the TSC client
> for Android, but is separate from RWHVAndroid (yet owned by RWHVAndroid), that
> sounds just fine.

Absolutely. I am not too worried whether RWHVA* itself is a TSCClient or not, as
long as both RWHVA*s (and Views textfield) are using the same TSC, and provide
its client as necessary.

[sadrul, 2015-03-13 22:16:35]

On 2015/03/13 22:14:42, sadrul wrote:
> On 2015/03/13 21:36:31, jdduke wrote:
> > > Those would be perfect solutions, if I had full control over TSC to
refactor
> > it.
> > > However, my understanding was that the owner of TSC prefers we minimize
TSC
> > > modification at the moment and use it by wrapping it in our implementation
> > > (TSCA) and later incorporate those changes into TSC if needed (Jared,
please
> > > correct me if I'm wrong). I found this the most practical way to go
forward
> > and
> > > I believe I've shared this design with you and heard back no complaints
> about
> > > it.
> > 
> > I'm not sure where I gave that impression. I don't have a strong opinion
about
> > the current design of the TouchSelectionController as long as we keep it
> fairly
> > agnostic to platform-specific differences, and we should evolve it as
> > appropriate to support a unified model.
> > 
> > > 
> > > > > I don't see why you think RWHVAura should own TSC, rather than TSCA.
In
> > > > > that case all implementation in TSCA should be moved to RWHVAura, I
> guess.
> > > > > We need to use TSCA also for Views textfield.
> > > > 
> > > > RWHVAura and Views textfield both would own a TSC, and provide the
client
> > that
> > > > does things differently for content vs. views.
> > > They are (going to be) owners of TSC, but indirectly through TSCA. That's
> just
> > > like we've extracted touch selection parts of RWHVA (which are in common
> with
> > > Views textfield) into TSCA. 
> > 
> > Sadrul, I don't think we should get too hung up on the fact that RWHVAndroid
> is
> > a client of the TSC. If we want to make a class that implements the TSC
client
> > for Android, but is separate from RWHVAndroid (yet owned by RWHVAndroid),
that
> > sounds just fine.
> 
> Absolutely. I am not too worried whether RWHVA* itself is a TSCClient or not,
as
> long as both RWHVA*s (and Views textfield) are using the same TSC, and provide
> its client as necessary.

And when I say 'RWHVA provide its client', I do not necessarily mean RWHVA
itself should create it (since for the case of aura, the client seems to need to
communicate with WebContents, and so it needs to be created in a different
layer).

[jdduke (slow), 2015-03-13 22:20:41]

On 2015/03/13 22:16:35, sadrul wrote:
> On 2015/03/13 22:14:42, sadrul wrote:
> > Absolutely. I am not too worried whether RWHVA* itself is a TSCClient or
not,
> as
> > long as both RWHVA*s (and Views textfield) are using the same TSC, and
provide
> > its client as necessary.
> 
> And when I say 'RWHVA provide its client', I do not necessarily mean RWHVA
> itself should create it (since for the case of aura, the client seems to need
to
> communicate with WebContents, and so it needs to be created in a different
> layer).

As an aside, both sievers@, tedchoc@ and myself have long wanted to clean up the
Android-specific RWHVAndroid<->ContentViewCore layering that is unique to
Android. We'd like to be more like Aura in this respect, that is, have the Views
and the WebContents be responsible for similar kinds of activities (as compared
to Aura), but ContentViewCore has become quite a beast on Android that it's been
difficult teasing it apart.

[mfomitchev, 2015-03-14 02:38:46]

What if
 -  TouchSelectionController was an interface,
 -  The implementation was TouchSelectionControllerImpl, and it was used by
Android just like now,
 -  TouchSelectionControllerImplAura would implement TouchSelectionController
and contain TouchSelectionControllerImpl,
 -  It looks like RWHVAura would still need to use
TouchSelectionControllerImplAura directly because of OnFlingCompleted(), but at
least TSCIAura would have an is-a relationship with TSC.

This way we'd have a common interface and there would be minimal changes to the
current implementation.

BTW, my memory is hazy now, but I remember that when I initially looked at the
patch back in November I had concerns similar to Sadrul's. Mohsen and I were
considering some options, and Jared pointed us at GestureProviderAura, which
also uses composition rather than inheritance to solve what looks like a similar
design task. This ultimately convinced me that the original design was okay.
Sadrul, what do you think about GestureProviderAura? Here's the thread from
November that I dug up:
https://drive.google.com/a/google.com/file/d/1yNFCWM_4UNm2hmtXpIfu3sXHCdt7z7Z....
To give some context, Mohsen and I were considering splitting up the controller
as well as the client interface into base/aura/android classes.

[mohsen, 2015-03-16 17:44:24]

On 2015/03/14 02:38:46, mfomitchev wrote:
> What if
>  -  TouchSelectionController was an interface,
>  -  The implementation was TouchSelectionControllerImpl, and it was used by
> Android just like now,
>  -  TouchSelectionControllerImplAura would implement TouchSelectionController
> and contain TouchSelectionControllerImpl,
>  -  It looks like RWHVAura would still need to use
> TouchSelectionControllerImplAura directly because of OnFlingCompleted(), but
at
> least TSCIAura would have an is-a relationship with TSC.
> 
> This way we'd have a common interface and there would be minimal changes to
the
> current implementation.
> 
> BTW, my memory is hazy now, but I remember that when I initially looked at the
> patch back in November I had concerns similar to Sadrul's. Mohsen and I were
> considering some options, and Jared pointed us at GestureProviderAura, which
> also uses composition rather than inheritance to solve what looks like a
similar
> design task. This ultimately convinced me that the original design was okay.
> Sadrul, what do you think about GestureProviderAura? Here's the thread from
> November that I dug up:
>
https://drive.google.com/a/google.com/file/d/1yNFCWM_4UNm2hmtXpIfu3sXHCdt7z7Z....
> To give some context, Mohsen and I were considering splitting up the
controller
> as well as the client interface into base/aura/android classes.

sadrul@: So, I've updated the code to have TouchSelectionController as a base
class for TouchSelectionControllerAndroid and TouchSelectionControllerAura.
Please check the new code at http://crrev.com/996373002 and let me know whether
it addresses your concerns or not (the code might still need some cleanup,
though).

[mohsen, 2015-04-27 05:41:55]

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

[commit-bot: I haz the power, 2015-04-27 05:42:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/698253004/840001

[commit-bot: I haz the power, 2015-04-27 05:48:43]

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

[commit-bot: I haz the power, 2015-04-27 05:48:44]

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

[mohsen, 2015-04-27 16:11:41]

sadrul@: I've updated the patch based on our discussions. Can you please take a
look...

[mohsen, 2015-04-27 19:28:50]

Patchset #37 (id:900001) has been deleted

[sadrul, 2015-04-29 19:55:03]

Some nits, some comments. As discussed offline, we should have the old code-path
live and active, and land the new code in smaller pieces before switching to the
new code, and removing the old code.

https://codereview.chromium.org/698253004/diff/920001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/698253004/diff/920001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.h:224: void
OnScrollCompleted() override;
optional: Would it make sense to call these OnSelectionScroll* instead of
OnScroll*? (with the current name, these look a bit awkward (and maybe
misleading) in RenderWidgetHostViewAndroid). I am not good with names though, so
maybe renaming isn't useful).

https://codereview.chromium.org/698253004/diff/920001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/920001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:874:
selection_controller_->HideAndDisallowShowingAutomatically();
Why is this needed? Should |selection_controller_| dtor do this instead?

https://codereview.chromium.org/698253004/diff/920001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1022: NOTREACHED()
<< "Selection bounds should be routed through the compositor.";
As discussed offline, it would be a good idea to keep the old code until we can
completely switch to the new goodness.

https://codereview.chromium.org/698253004/diff/920001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2132:
pointer_state_.CleanupRemovedTouchPoints(*event);
Should there be some Scoped thingy to make sure this always gets called
correctly when early returning from this function?

https://codereview.chromium.org/698253004/diff/920001/content/browser/web_con...
File content/browser/web_contents/aura/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/698253004/diff/920001/content/browser/web_con...
content/browser/web_contents/aura/touch_selection_controller_client_aura.cc:49:
: public ui::EventHandler {
Consider using a UserActivityObserver instead.

https://codereview.chromium.org/698253004/diff/920001/content/browser/web_con...
content/browser/web_contents/aura/touch_selection_controller_client_aura.cc:62:
aura::Window* window_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/698253004/diff/920001/content/browser/web_con...
File content/browser/web_contents/aura/touch_selection_controller_client_aura.h
(right):

https://codereview.chromium.org/698253004/diff/920001/content/browser/web_con...
content/browser/web_contents/aura/touch_selection_controller_client_aura.h:47:
void UpdateQuickMenu();
non-overrides before overrides

https://codereview.chromium.org/698253004/diff/920001/ui/views/touchui/touch_...
File ui/views/touchui/touch_selection_menu_runner_views.cc (right):

https://codereview.chromium.org/698253004/diff/920001/ui/views/touchui/touch_...
ui/views/touchui/touch_selection_menu_runner_views.cc:34: client_ = nullptr;
When can we have a null menu_, but a non-null client_?

https://codereview.chromium.org/698253004/diff/920001/ui/views/touchui/touch_...
ui/views/touchui/touch_selection_menu_runner_views.cc:63: DCHECK(menu == menu_);
DCHECK_EQ

[mohsen, 2015-04-30 05:19:00]

Patchset #38 (id:940001) has been deleted

[mohsen, 2015-04-30 16:07:58]

Patchset #38 (id:960001) has been deleted

[mohsen, 2015-04-30 18:29:34]

Other than addressing review comments, I've also changed "bool
TSC::OnSelectionBoundsUpdated()" back to "void TSC::OnSelectionBoundsChanged()",
since returning bool is not needed anymore in the redesign.

Note that I've also rebased the patch which has messed up the diff with previous
patch set a bit! Sorry!

Please take another look...

https://codereview.chromium.org/698253004/diff/920001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/698253004/diff/920001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.h:224: void
OnScrollCompleted() override;
On 2015/04/29 19:55:03, sadrul wrote:
> optional: Would it make sense to call these OnSelectionScroll* instead of
> OnScroll*? (with the current name, these look a bit awkward (and maybe
> misleading) in RenderWidgetHostViewAndroid). I am not good with names though,
so
> maybe renaming isn't useful).

I agree, that makes more sense. Done.

https://codereview.chromium.org/698253004/diff/920001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/920001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:874:
selection_controller_->HideAndDisallowShowingAutomatically();
On 2015/04/29 19:55:03, sadrul wrote:
> Why is this needed? Should |selection_controller_| dtor do this instead?

My understanding was that this is called on some sort of focus loss. Apparently,
this is not called at all in Aura. So, removed!

https://codereview.chromium.org/698253004/diff/920001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1022: NOTREACHED()
<< "Selection bounds should be routed through the compositor.";
On 2015/04/29 19:55:03, sadrul wrote:
> As discussed offline, it would be a good idea to keep the old code until we
can
> completely switch to the new goodness.

Sure.

https://codereview.chromium.org/698253004/diff/920001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2132:
pointer_state_.CleanupRemovedTouchPoints(*event);
On 2015/04/29 19:55:03, sadrul wrote:
> Should there be some Scoped thingy to make sure this always gets called
> correctly when early returning from this function?

I thought of a similar thing, but thought it would be overkill. How does it look
like, now?

Note that it also moves the cleanup from right after WebTouchEvent creation
(line 2138) to the end of the function, which I guess shouldn't be an issue.

https://codereview.chromium.org/698253004/diff/920001/content/browser/web_con...
File content/browser/web_contents/aura/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/698253004/diff/920001/content/browser/web_con...
content/browser/web_contents/aura/touch_selection_controller_client_aura.cc:49:
: public ui::EventHandler {
On 2015/04/29 19:55:03, sadrul wrote:
> Consider using a UserActivityObserver instead.

Apparently, UserActivityObserver mechanism is not available on all platforms we
want to support (e.g. Windows Aura); so, we can't use it here.

https://codereview.chromium.org/698253004/diff/920001/content/browser/web_con...
content/browser/web_contents/aura/touch_selection_controller_client_aura.cc:62:
aura::Window* window_;
On 2015/04/29 19:55:03, sadrul wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/698253004/diff/920001/content/browser/web_con...
File content/browser/web_contents/aura/touch_selection_controller_client_aura.h
(right):

https://codereview.chromium.org/698253004/diff/920001/content/browser/web_con...
content/browser/web_contents/aura/touch_selection_controller_client_aura.h:47:
void UpdateQuickMenu();
On 2015/04/29 19:55:03, sadrul wrote:
> non-overrides before overrides

Done.

https://codereview.chromium.org/698253004/diff/920001/ui/views/touchui/touch_...
File ui/views/touchui/touch_selection_menu_runner_views.cc (right):

https://codereview.chromium.org/698253004/diff/920001/ui/views/touchui/touch_...
ui/views/touchui/touch_selection_menu_runner_views.cc:34: client_ = nullptr;
On 2015/04/29 19:55:03, sadrul wrote:
> When can we have a null menu_, but a non-null client_?

The above |Create| function returns nullptr if there is no action available to
be shown on the quick menu. Since we've set |client_| before that call, we need
to reset it if no menu is created.

https://codereview.chromium.org/698253004/diff/920001/ui/views/touchui/touch_...
ui/views/touchui/touch_selection_menu_runner_views.cc:63: DCHECK(menu == menu_);
On 2015/04/29 19:55:03, sadrul wrote:
> DCHECK_EQ

Done.

[mohsen, 2015-05-28 19:41:38]

Patchset #43 (id:1080001) has been deleted

[mohsen, 2015-06-15 18:21:38]

Patchset #46 (id:1160001) has been deleted

[mohsen, 2015-06-23 16:16:04]

sadrul@: while I'm preparing tests for http://crrev.com/1162373002, can you
start reviewing this one? It's based on top of http://crrev.com/1162373002.

[sadrul, 2015-06-30 18:33:12]

Looks pretty reasonable.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:43:
bottom_right.y() - origin.y());
Sadness ... this is still pretty wrong.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:49:
: public ui::EventHandler {
Document.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:94:
if (!cursor_client || cursor_client->IsMouseEventsEnabled())
Can you document why this check with cursor-client is necessary here?

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:158:
gfx::RectF rect = rwhva_->selection_controller()->GetRectBetweenBounds();
Can you remind me what coord-space this rect is in? (the doc for
TouchSelectionController::GetRectBetweenBounds() doesn't mention it).

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:169:
gfx::Vector2dF diagonal = bottom_right - origin;
Do these do the right thing for RTL?

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:204:
}
Can this be more like:
  bool menu_should_show = active_status() == ACTIVE && IsQuickMenuAllowed();
  bool menu_is_showing = IsRunning();
  if (!menu_should_show) {
    if (menu_is_showing)
      hide();
    stop timer;
  } else {
    if (!menu_is_showing)
      start timer;
  }

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:236:
gfx::ToRoundedPoint(extent));
{}

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:323:
default:
Shouldn't you also handle DELETE and SELECT_ALL? If not, do you need these in
IsCommandIdEnabled() above?

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.h
(right):

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.h:59:
scoped_ptr<EnvPreTargetHandler> env_pre_target_handler_;
Document the handler here.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2211: 
Remove blank lines

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2242: 
-

https://codereview.chromium.org/698253004/diff/1180001/content/browser/web_co...
File content/browser/web_contents/touch_editable_impl_aura_browsertest.cc
(left):

https://codereview.chromium.org/698253004/diff/1180001/content/browser/web_co...
content/browser/web_contents/touch_editable_impl_aura_browsertest.cc:636: }
Do we have coverage for all these test-cases for the new code?

https://codereview.chromium.org/698253004/diff/1180001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/1180001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.cc:888: 
-

https://codereview.chromium.org/698253004/diff/1180001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/698253004/diff/1180001/content/renderer/rende...
content/renderer/render_widget.cc:2020: // updates are fixed for webviews.
Can you file a crbug and reference that from here?

[mohsen, 2015-07-03 18:07:23]

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:43:
bottom_right.y() - origin.y());
On 2015/06/30 18:33:12, sadrul wrote:
> Sadness ... this is still pretty wrong.

Why?

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:49:
: public ui::EventHandler {
On 2015/06/30 18:33:12, sadrul wrote:
> Document.

Done.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:94:
if (!cursor_client || cursor_client->IsMouseEventsEnabled())
On 2015/06/30 18:33:12, sadrul wrote:
> Can you document why this check with cursor-client is necessary here?

Done.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:158:
gfx::RectF rect = rwhva_->selection_controller()->GetRectBetweenBounds();
On 2015/06/30 18:33:12, sadrul wrote:
> Can you remind me what coord-space this rect is in? (the doc for
> TouchSelectionController::GetRectBetweenBounds() doesn't mention it).

This function will return bounding box of selection end points which are in web
contents coordinates (as sent from blink). Equivalently, it's in RWHVA's native
view's coordinates.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:169:
gfx::Vector2dF diagonal = bottom_right - origin;
On 2015/06/30 18:33:12, sadrul wrote:
> Do these do the right thing for RTL?

Yeah, 'right' is always greater than or equal to 'x'. So, in an RTL environment
'right' is actually 'left'; and these calculations should be fine.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:204:
}
On 2015/06/30 18:33:12, sadrul wrote:
> Can this be more like:
>   bool menu_should_show = active_status() == ACTIVE && IsQuickMenuAllowed();
>   bool menu_is_showing = IsRunning();
>   if (!menu_should_show) {
>     if (menu_is_showing)
>       hide();
>     stop timer;
>   } else {
>     if (!menu_is_showing)
>       start timer;
>   }

We'd want to hide the menu even if menu_should_show, in order to update its
location or contents. However, I simplified the code using your suggested bool
variables.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:236:
gfx::ToRoundedPoint(extent));
On 2015/06/30 18:33:12, sadrul wrote:
> {}

Done.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:323:
default:
On 2015/06/30 18:33:12, sadrul wrote:
> Shouldn't you also handle DELETE and SELECT_ALL? If not, do you need these in
> IsCommandIdEnabled() above?

The quick menu as implemented for Views only can have cut, copy, and paste. So,
I removed delete and select-all from IsCommandIdEnabled(), too.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.h
(right):

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.h:59:
scoped_ptr<EnvPreTargetHandler> env_pre_target_handler_;
On 2015/06/30 18:33:12, sadrul wrote:
> Document the handler here.

Done.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2211: 
On 2015/06/30 18:33:12, sadrul wrote:
> Remove blank lines

Done.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2242: 
On 2015/06/30 18:33:12, sadrul wrote:
> -

Everywhere else in the file, there is a blank line after TRACE_EVENT.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/web_co...
File content/browser/web_contents/touch_editable_impl_aura_browsertest.cc
(left):

https://codereview.chromium.org/698253004/diff/1180001/content/browser/web_co...
content/browser/web_contents/touch_editable_impl_aura_browsertest.cc:636: }
On 2015/06/30 18:33:12, sadrul wrote:
> Do we have coverage for all these test-cases for the new code?

I believe these browser tests are mostly written in lieu of proper unit tests
for touch selection. For the new ui::TouchSelectionController, these cases are
mostly covered by unit tests in TouchSelectionControllerTest:

 - TouchSelectionOriginatingFromWebpageTest: Covered by
HandlesShowOnTapInsideRect and SelectionDragged*.

 - TestTouchSelectionHiddenWhenScrolling: This is not covered and I should add a
unit test for it.

 - TestTouchSelectionReshownAfterFling: This is also not covered and I should
add a unit test for it.

 - TestTouchSelectionWhenOverscrolling: Not applicable anymore as we don't hide
specifically for overscroll and hiding for scrolling covers this case, too.

 - TouchSelectionOnLongPressTest: Covered by SelectionBasic.

 - NoTouchSelectionOnDoubleTapTest: I don't think we need this one. We had it
here because we had specific code to handle this case in old code, but in the
new code this cannot happen and we need specific code to enable it. Also, this
might change in future (http://crbug.com/453476) in which case we'd need to add
a test to cover that behavior.

 - TouchCursorInTextfieldTest: Covered by InsertionBasic and InsertionDragged.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/1180001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.cc:888: 
On 2015/06/30 18:33:12, sadrul wrote:
> -

Done.

https://codereview.chromium.org/698253004/diff/1180001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/698253004/diff/1180001/content/renderer/rende...
content/renderer/render_widget.cc:2020: // updates are fixed for webviews.
On 2015/06/30 18:33:12, sadrul wrote:
> Can you file a crbug and reference that from here?

That would be a bug against a code that is not yet landed!

[mohsen, 2015-07-07 23:12:41]

jdduke@, mfomitchev@: Can you please take another look at this CL?

[jdduke (slow), 2015-07-08 15:22:42]

https://codereview.chromium.org/698253004/diff/1200001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/1200001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2216:
pointer_state_.CleanupRemovedTouchPoints(*event);
I wonder if it's worth adding a scoped helper for MotionEventAura that will
automatically cleanup the touch points on destruction. I think this might even
fix a bug we have in GestureProviderAura
(https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/gestures...).
That could (should?) definitely happen separately though.

https://codereview.chromium.org/698253004/diff/1200001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/698253004/diff/1200001/content/renderer/rende...
content/renderer/render_widget.cc:2021: bool send_ipc = true;
This is pretty scary. Do we know why this isn't working? How does the IME use
this data? Should we only send it when for webview instances (do we have that
info available here?)?

[mohsen, 2015-07-08 21:10:16]

Patchset #48 (id:1220001) has been deleted

[mohsen, 2015-07-08 21:18:24]

Patchset #48 (id:1240001) has been deleted

[mohsen, 2015-07-08 21:26:18]

https://codereview.chromium.org/698253004/diff/1200001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/1200001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2216:
pointer_state_.CleanupRemovedTouchPoints(*event);
On 2015/07/08 15:22:42, jdduke wrote:
> I wonder if it's worth adding a scoped helper for MotionEventAura that will
> automatically cleanup the touch points on destruction. I think this might even
> fix a bug we have in GestureProviderAura
>
(https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/gestures...).
> That could (should?) definitely happen separately though.

Added a scoped helper. How does it look like now?

https://codereview.chromium.org/698253004/diff/1200001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/698253004/diff/1200001/content/renderer/rende...
content/renderer/render_widget.cc:2021: bool send_ipc = true;
On 2015/07/08 15:22:42, jdduke wrote:
> This is pretty scary. Do we know why this isn't working? How does the IME use
> this data? Should we only send it when for webview instances (do we have that
> info available here?)?

IME uses cursor location to show a popup when user changes the keyboard layout
language. For webview, it is not working with composited selection updates
because selection bounds are not propagated to the embedder RenderWidgetHostView
properly. We are working on a fix for that and also touch selection in general,
for webview (see http://crrev.com/1203223002 for some discussions).

I think sending this IPC only for webview would complicate the code as there
would be two different paths to update selection bounds for IME. Currently, my
patch sends both updates, but touch selection only uses one and IME only uses
the other.

[jdduke (slow), 2015-07-08 22:19:43]

Working out the IME kinks separately sounds fine.

Did you ever get a chance to investigate the "laggy" handle behavior when
scrolling with an active selection?

https://codereview.chromium.org/698253004/diff/1280001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/1280001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:308:
DISALLOW_COPY_AND_ASSIGN(ScopedMotionEventAuraCleanup);
This looks good, but let's add it in a separate patch, ideally it would live
in/around MotionEventAura and we'd use it wherever necessary.

https://codereview.chromium.org/698253004/diff/1280001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:312:
ui::MotionEventAura* mouse_event,
motion_event

https://codereview.chromium.org/698253004/diff/1280001/content/browser/web_co...
File content/browser/web_contents/touch_editable_impl_aura_browsertest.cc
(left):

https://codereview.chromium.org/698253004/diff/1280001/content/browser/web_co...
content/browser/web_contents/touch_editable_impl_aura_browsertest.cc:584:
MAYBE_TouchCursorInTextfieldTest) {
Is it worth porting over any of these browser tests? Do we have other existing
integration (smoke?) tests for selection on Aura? We have a few basic ones on
Android, might be nice to at least exercise the system end-to-end in some kind
of test (assuming such tests don't exist elsewhere).

[mohsen, 2015-07-08 22:45:15]

For the laggy handles, I've worked a bit on it, but couldn't find anything. I
was planning to talk to someone from browser compositor team this week to see if
they have any ideas about the issue. However, for this patch, handles are hidden
during scroll.

https://codereview.chromium.org/698253004/diff/1280001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/1280001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:308:
DISALLOW_COPY_AND_ASSIGN(ScopedMotionEventAuraCleanup);
On 2015/07/08 22:19:43, jdduke wrote:
> This looks good, but let's add it in a separate patch, ideally it would live
> in/around MotionEventAura and we'd use it wherever necessary.

Yeah, if that would useful in other places, then we can add it separately as you
suggested. Removed.

https://codereview.chromium.org/698253004/diff/1280001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:312:
ui::MotionEventAura* mouse_event,
On 2015/07/08 22:19:43, jdduke wrote:
> motion_event

Acknowledged.

https://codereview.chromium.org/698253004/diff/1280001/content/browser/web_co...
File content/browser/web_contents/touch_editable_impl_aura_browsertest.cc
(left):

https://codereview.chromium.org/698253004/diff/1280001/content/browser/web_co...
content/browser/web_contents/touch_editable_impl_aura_browsertest.cc:584:
MAYBE_TouchCursorInTextfieldTest) {
On 2015/07/08 22:19:43, jdduke wrote:
> Is it worth porting over any of these browser tests? Do we have other existing
> integration (smoke?) tests for selection on Aura? We have a few basic ones on
> Android, might be nice to at least exercise the system end-to-end in some kind
> of test (assuming such tests don't exist elsewhere).

As I mentioned in a previous response to sadrul, many of these tests are in
place of non-existent unit tests for the old code. For the new code, many of
these cases are covered by unit tests. The only case that is not covered is
checking if handles/quick menu is hidden during scroll which I plan to add. I
agree that we should also have some sort of end-to-end test and I'll work on
adding that, too. Thank for bringing this up.

[mfomitchev, 2015-07-09 22:31:05]

https://codereview.chromium.org/698253004/diff/1300001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/698253004/diff/1300001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:164:
// Clip rect to client bounds.
Would this block of code be equivalent to

gfx::RectF anchor_rect = rwhva_->selection_controller()->GetRectBetweenBounds();
gfx::Rect client_bounds = rwhva_->GetNativeView()->bounds();
if (!anchor_rect.Intersects(client_bounds))
  return;
anchor_rect.Intersect(client_bounds);

If so, I think this would be more clear, plus we won't have rect in an invalid
state where top > bottom.

https://codereview.chromium.org/698253004/diff/1300001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.h
(right):

https://codereview.chromium.org/698253004/diff/1300001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.h:27:
void OnSelectionScrollStarted();
Is there a good reason this is OnSelectionScrollStarted() and not just
OnScrollStarted()?

https://codereview.chromium.org/698253004/diff/1300001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (left):

https://codereview.chromium.org/698253004/diff/1300001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:1088:
touch_editing_client_->OnSelectionOrCursorChanged(
Why don't we need this anymore? Are we relying on
OnSwapCompositorFrame/SelectionUpdated instead?

https://codereview.chromium.org/698253004/diff/1300001/content/browser/web_co...
File content/browser/web_contents/touch_editable_impl_aura_browsertest.cc
(left):

https://codereview.chromium.org/698253004/diff/1300001/content/browser/web_co...
content/browser/web_contents/touch_editable_impl_aura_browsertest.cc:395:
TestTouchSelectionWhenOverscrolling) {
This would also need to be ported over, right?

[mohsen, 2015-07-09 22:59:01]

https://codereview.chromium.org/698253004/diff/1300001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/698253004/diff/1300001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:164:
// Clip rect to client bounds.
On 2015/07/09 22:31:04, mfomitchev wrote:
> Would this block of code be equivalent to
> 
> gfx::RectF anchor_rect =
rwhva_->selection_controller()->GetRectBetweenBounds();
> gfx::Rect client_bounds = rwhva_->GetNativeView()->bounds();
> if (!anchor_rect.Intersects(client_bounds))
>   return;
> anchor_rect.Intersect(client_bounds);

This won't work if GetRectBetweenBounds() returns a rect of zero width and
that's possible as selection bounds are rects of zero width.


> If so, I think this would be more clear, plus we won't have rect in an invalid
> state where top > bottom.

https://codereview.chromium.org/698253004/diff/1300001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.h
(right):

https://codereview.chromium.org/698253004/diff/1300001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.h:27:
void OnSelectionScrollStarted();
On 2015/07/09 22:31:04, mfomitchev wrote:
> Is there a good reason this is OnSelectionScrollStarted() and not just
> OnScrollStarted()?

Not anymore. Renamed back to OnScrollStarted().

https://codereview.chromium.org/698253004/diff/1300001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (left):

https://codereview.chromium.org/698253004/diff/1300001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:1088:
touch_editing_client_->OnSelectionOrCursorChanged(
On 2015/07/09 22:31:04, mfomitchev wrote:
> Why don't we need this anymore? Are we relying on
> OnSwapCompositorFrame/SelectionUpdated instead?

Yes, we wanna use bounds sent through SwapCompositorFrame IPC message instead of
bounds sent with SelectionBoundsChanged IPC messge.

https://codereview.chromium.org/698253004/diff/1300001/content/browser/web_co...
File content/browser/web_contents/touch_editable_impl_aura_browsertest.cc
(left):

https://codereview.chromium.org/698253004/diff/1300001/content/browser/web_co...
content/browser/web_contents/touch_editable_impl_aura_browsertest.cc:395:
TestTouchSelectionWhenOverscrolling) {
On 2015/07/09 22:31:04, mfomitchev wrote:
> This would also need to be ported over, right?

No. In the new code, we only hide handles during scroll. We don't hide handles
specifically for overscrolling and this test is not applicable anymore.

[mfomitchev, 2015-07-10 15:20:00]

https://codereview.chromium.org/698253004/diff/1300001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/698253004/diff/1300001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:164:
// Clip rect to client bounds.
Ah, ok, thanks. This whole 0-width thing is causing us to rewrite a bunch of
basic rect code, which is unfortunate.

https://codereview.chromium.org/698253004/diff/1300001/content/browser/web_co...
File content/browser/web_contents/touch_editable_impl_aura_browsertest.cc
(left):

https://codereview.chromium.org/698253004/diff/1300001/content/browser/web_co...
content/browser/web_contents/touch_editable_impl_aura_browsertest.cc:395:
TestTouchSelectionWhenOverscrolling) {
On 2015/07/09 22:59:00, mohsen wrote:
> On 2015/07/09 22:31:04, mfomitchev wrote:
> > This would also need to be ported over, right?
> 
> No. In the new code, we only hide handles during scroll. We don't hide handles
> specifically for overscrolling and this test is not applicable anymore.

We still hide handles in CompleteOverscrollNavigation()

[mohsen, 2015-07-10 17:18:32]

https://codereview.chromium.org/698253004/diff/1300001/content/browser/web_co...
File content/browser/web_contents/touch_editable_impl_aura_browsertest.cc
(left):

https://codereview.chromium.org/698253004/diff/1300001/content/browser/web_co...
content/browser/web_contents/touch_editable_impl_aura_browsertest.cc:395:
TestTouchSelectionWhenOverscrolling) {
On 2015/07/10 15:20:00, mfomitchev wrote:
> On 2015/07/09 22:59:00, mohsen wrote:
> > On 2015/07/09 22:31:04, mfomitchev wrote:
> > > This would also need to be ported over, right?
> > 
> > No. In the new code, we only hide handles during scroll. We don't hide
handles
> > specifically for overscrolling and this test is not applicable anymore.
> 
> We still hide handles in CompleteOverscrollNavigation()

Right, that's actually a new case that was not covered by this test. I'll add a
test for that, too. Thanks

[jdduke (slow), 2015-07-13 15:31:52]

https://codereview.chromium.org/698253004/diff/1320001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/698253004/diff/1320001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:107:
selection_controller_->HideAndDisallowShowingAutomatically();
Hmm, so you don't show the menu again after the scroll completes?

https://codereview.chromium.org/698253004/diff/1320001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:340:
// selection controller; otherwise, rect would be empty.
I don't really follow this statement, particularly the part about the rect being
empty?

[mohsen, 2015-07-13 16:22:22]

https://codereview.chromium.org/698253004/diff/1320001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/698253004/diff/1320001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:107:
selection_controller_->HideAndDisallowShowingAutomatically();
On 2015/07/13 15:31:52, jdduke wrote:
> Hmm, so you don't show the menu again after the scroll completes?

W're hiding handles whenever user switches to using mouse or keyboard. This is
an example where user switches to scrolling using mousewheel. So, we're not
hiding because of scroll itself to need to bring them back up.

https://codereview.chromium.org/698253004/diff/1320001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:340:
// selection controller; otherwise, rect would be empty.
On 2015/07/13 15:31:52, jdduke wrote:
> I don't really follow this statement, particularly the part about the rect
being
> empty?

The above calculation is based on GetRectBetweenBounds(). If we hide the handles
before calling GetRectBetweenBounds(), it will return an empty rect
(gfx::Rect()) and context menu will be shown at (0,0).

[jdduke (slow), 2015-07-13 16:36:11]

I'm happy to rubberstamp this, but I don't own most of the code here.

https://codereview.chromium.org/698253004/diff/1320001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/698253004/diff/1320001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:340:
// selection controller; otherwise, rect would be empty.
On 2015/07/13 16:22:22, mohsen wrote:
> On 2015/07/13 15:31:52, jdduke wrote:
> > I don't really follow this statement, particularly the part about the rect
> being
> > empty?
> 
> The above calculation is based on GetRectBetweenBounds(). If we hide the
handles
> before calling GetRectBetweenBounds(), it will return an empty rect
> (gfx::Rect()) and context menu will be shown at (0,0).

I see, I read the statement as declaring why we need to hide the handles (at
all), as opposed to why we need to hide the handles *after* we get the rect.

Why do we hide the handles when showing the context menu?

[mohsen, 2015-07-13 17:46:48]

https://codereview.chromium.org/698253004/diff/1320001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/698253004/diff/1320001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:340:
// selection controller; otherwise, rect would be empty.
On 2015/07/13 16:36:11, jdduke wrote:
> On 2015/07/13 16:22:22, mohsen wrote:
> > On 2015/07/13 15:31:52, jdduke wrote:
> > > I don't really follow this statement, particularly the part about the rect
> > being
> > > empty?
> > 
> > The above calculation is based on GetRectBetweenBounds(). If we hide the
> handles
> > before calling GetRectBetweenBounds(), it will return an empty rect
> > (gfx::Rect()) and context menu will be shown at (0,0).
> 
> I see, I read the statement as declaring why we need to hide the handles (at
> all), as opposed to why we need to hide the handles *after* we get the rect.

Changed the comment a bit to make this more clear.

> Why do we hide the handles when showing the context menu?

I think, because at this point touch selection can be considered done. User has
selected the text they wanted and opened the context menu to perform an action
from context menu on the selection. Also, when context menu is open, user cannot
interact with handles anymore and there is no point for them being there.

[sadrul, 2015-07-15 17:00:15]

Some nits. lgtm with those addressed.

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/698253004/diff/1180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:43:
bottom_right.y() - origin.y());
On 2015/07/03 18:07:23, mohsen wrote:
> On 2015/06/30 18:33:12, sadrul wrote:
> > Sadness ... this is still pretty wrong.
> 
> Why?

Consider rotated windows.

I think this is the approach we currently follow elsewhere, so this is OK for
now. But it is still wrong.

https://codereview.chromium.org/698253004/diff/1180001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/698253004/diff/1180001/content/renderer/rende...
content/renderer/render_widget.cc:2020: // updates are fixed for webviews.
On 2015/07/03 18:07:23, mohsen wrote:
> On 2015/06/30 18:33:12, sadrul wrote:
> > Can you file a crbug and reference that from here?
> 
> That would be a bug against a code that is not yet landed!

That's OK. You can file a bug for TODO and reference that from the code.

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:95:
// touch event in which case we don't want to deactivate touch selection.
'mouse event is synthesized from a touch event' ... ? When does this happen? Is
this only on Windows? Can we look at EF_FROM_TOUCH flag?

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:162:
gfx::RectF rect = rwhva_->selection_controller()->GetRectBetweenBounds();
Document that rect is in rwhva_'s window's coord-space.

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.h
(right):

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.h:24:
void OnWindowMoved();
Document of this is called only for the RWHVA's window, or for its parent
window(s) too.

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.h:26:
void OnTouchUp();
Document if this happens for all touch up/down events, or just the first down
and last up of a sequence.

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.h:28:
void OnScrollCompleted();
Document if this is called just for touch-scroll, or also for wheel-scroll.

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2739:
base::TimeTicks() + event->time_stamp(), event->location())) {
location_f()

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2744: if
(selection_controller_->WillHandleTapEvent(event->location()))
ditto

[mohsen, 2015-07-16 22:06:16]

mohsen@chromium.org changed reviewers:
+ avi@chromium.org

[mohsen, 2015-07-16 22:06:19]

+avi@ for OWNERS in contents/

sadrul@: I noticed that there were two changes left over from the interstitial
CL here in this CL: ime_adapter_android.cc and web_contents_impl_unittest.cc. I
removed those changes. Can you take another quick look...

https://codereview.chromium.org/698253004/diff/1180001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/698253004/diff/1180001/content/renderer/rende...
content/renderer/render_widget.cc:2020: // updates are fixed for webviews.
On 2015/07/15 17:00:15, sadrul wrote:
> On 2015/07/03 18:07:23, mohsen wrote:
> > On 2015/06/30 18:33:12, sadrul wrote:
> > > Can you file a crbug and reference that from here?
> > 
> > That would be a bug against a code that is not yet landed!
> 
> That's OK. You can file a bug for TODO and reference that from the code.

Done.

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:95:
// touch event in which case we don't want to deactivate touch selection.
On 2015/07/15 17:00:15, sadrul wrote:
> 'mouse event is synthesized from a touch event' ... ? When does this happen?
Is
> this only on Windows? Can we look at EF_FROM_TOUCH flag?

Yeah, that's mostly happening on Windows and the EF_FROM_TOUCH flags is not
always set for the event.

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:162:
gfx::RectF rect = rwhva_->selection_controller()->GetRectBetweenBounds();
On 2015/07/15 17:00:15, sadrul wrote:
> Document that rect is in rwhva_'s window's coord-space.

Done.

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.h
(right):

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.h:24:
void OnWindowMoved();
On 2015/07/15 17:00:15, sadrul wrote:
> Document of this is called only for the RWHVA's window, or for its parent
> window(s) too.

Done.

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.h:26:
void OnTouchUp();
On 2015/07/15 17:00:15, sadrul wrote:
> Document if this happens for all touch up/down events, or just the first down
> and last up of a sequence.

Done.

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.h:28:
void OnScrollCompleted();
On 2015/07/15 17:00:15, sadrul wrote:
> Document if this is called just for touch-scroll, or also for wheel-scroll.

Done.

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2739:
base::TimeTicks() + event->time_stamp(), event->location())) {
On 2015/07/15 17:00:15, sadrul wrote:
> location_f()

Done.

https://codereview.chromium.org/698253004/diff/1340001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2744: if
(selection_controller_->WillHandleTapEvent(event->location()))
On 2015/07/15 17:00:15, sadrul wrote:
> ditto

Done.

[Avi (use Gerrit), 2015-07-17 02:21:51]

lgtm

stampity stamp

[mohsen, 2015-07-21 21:55:17]

Patchset #57 (id:1440001) has been deleted

[mohsen, 2015-07-21 23:49:40]

sadrul@: I've added a few browser tests to this CL
(touch_selection_controller_client_aura_browsertest.cc). Can you please take a
look...

[sadrul, 2015-07-29 22:39:23]

A couple of comments, otherwise, looks good

https://codereview.chromium.org/698253004/diff/1500001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc
(right):

https://codereview.chromium.org/698253004/diff/1500001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc:72:
: TouchSelectionControllerClientAura(rwhva) {
Initialize expected_event_

https://codereview.chromium.org/698253004/diff/1500001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc:87:
run_loop_ = nullptr;
USe run_loop_.reset() instead

https://codereview.chromium.org/698253004/diff/1500001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc:88:
}
Can you explain why this needs to happen in two steps? Most similar APIs are
just one call to WaitFor<something-to-change>(....)

[mohsen, 2015-07-30 04:29:05]

https://codereview.chromium.org/698253004/diff/1500001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc
(right):

https://codereview.chromium.org/698253004/diff/1500001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc:72:
: TouchSelectionControllerClientAura(rwhva) {
On 2015/07/29 22:39:23, sadrul wrote:
> Initialize expected_event_

Done.

https://codereview.chromium.org/698253004/diff/1500001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc:87:
run_loop_ = nullptr;
On 2015/07/29 22:39:23, sadrul wrote:
> USe run_loop_.reset() instead

Done. Is there any problem with setting to nullptr?

https://codereview.chromium.org/698253004/diff/1500001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc:88:
}
On 2015/07/29 22:39:23, sadrul wrote:
> Can you explain why this needs to happen in two steps? Most similar APIs are
> just one call to WaitFor<something-to-change>(....)

I think using one call would be racy. If the thing that we are waiting for
happens before Wait is called, then it would wait indefinitely for something
that has already happened.

[sadrul, 2015-07-30 04:41:29]

lgtm

https://codereview.chromium.org/698253004/diff/1500001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc
(right):

https://codereview.chromium.org/698253004/diff/1500001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc:87:
run_loop_ = nullptr;
On 2015/07/30 04:29:05, mohsen wrote:
> On 2015/07/29 22:39:23, sadrul wrote:
> > USe run_loop_.reset() instead
> 
> Done. Is there any problem with setting to nullptr?

When reading the code, .reset() makes it clear that it's a scoped_ptr<>, and not
a raw pointer you are leaking.

https://codereview.chromium.org/698253004/diff/1500001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc:88:
}
On 2015/07/30 04:29:05, mohsen wrote:
> On 2015/07/29 22:39:23, sadrul wrote:
> > Can you explain why this needs to happen in two steps? Most similar APIs are
> > just one call to WaitFor<something-to-change>(....)
> 
> I think using one call would be racy. If the thing that we are waiting for
> happens before Wait is called, then it would wait indefinitely for something
> that has already happened.

OK.

[mohsen, 2015-07-30 16:35:28]

https://codereview.chromium.org/698253004/diff/1500001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc
(right):

https://codereview.chromium.org/698253004/diff/1500001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc:87:
run_loop_ = nullptr;
On 2015/07/30 04:41:29, sadrul wrote:
> On 2015/07/30 04:29:05, mohsen wrote:
> > On 2015/07/29 22:39:23, sadrul wrote:
> > > USe run_loop_.reset() instead
> > 
> > Done. Is there any problem with setting to nullptr?
> 
> When reading the code, .reset() makes it clear that it's a scoped_ptr<>, and
not
> a raw pointer you are leaking.

I see. Makes sense.

[mohsen, 2015-07-30 16:35:37]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2015-07-30 16:35:37]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/698253004/#ps1540001
(title: "Rebased")

[commit-bot: I haz the power, 2015-07-30 16:35:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/698253004/1540001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/698253004/1540001

[commit-bot: I haz the power, 2015-07-30 16:41:49]

Committed patchset #61 (id:1540001)

[commit-bot: I haz the power, 2015-07-30 16:42:52]

Patchset 61 (id:??) landed as
https://crrev.com/2a5e3624e4d0b78bfe5f5a3d40f5a98730291056
Cr-Commit-Position: refs/heads/master@{#341134}

[mfomitchev, 2015-07-30 17:04:10]

mfomitchev@chromium.org changed reviewers:
+ avi; congratbot@google.com
- avi@chromium.org

[mfomitchev, 2015-07-30 17:05:14]

Woohoo!!
Congrats on landing this Mohsen!

[jdduke (slow), 2015-07-30 17:11:05]

On 2015/07/30 17:05:14, mfomitchev wrote:
> Woohoo!!
> Congrats on landing this Mohsen!

Absolutely, nice work sir!

[Avi (use Gerrit), 2015-07-30 21:25:19]

A revert of this CL (patchset #61 id:1540001) has been created in
https://codereview.chromium.org/1263703004/ by avi@chromium.org.

The reason for reverting is:
SitePerProcessBrowserTest.CreateChildFrameAfterKillingProcess
SitePerProcessBrowserTest.NavigateRemoteFrameToKilledProcess
SitePerProcessBrowserTest.NavigateRemoteFrameToKilledProcessWithSubtree
SitePerProcessBrowserTest.NavigatingToKilledProcessRestoresAllProxies

started failing on the WinXP, Vista, and Win8 bots:

http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds...
http://build.chromium.org/p/chromium.win/builders/Vista%20Tests%20%281%29/bui...
http://build.chromium.org/p/chromium.win/builders/Win8%20Aura/builds/32038

Reverting as this is my best guess at being the cause..

[mohsen, 2015-08-08 00:35:15]

mohsen@chromium.org changed reviewers:
+ avi@chromium.org
- avi; congratbot@google.com

[mohsen, 2015-08-09 05:12:56]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2015-08-09 05:12:57]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/698253004/#ps1560001
(title: "Rebased after blink issue fixed: r200194")

[commit-bot: I haz the power, 2015-08-09 05:13:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/698253004/1560001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/698253004/1560001

[commit-bot: I haz the power, 2015-08-09 06:20:18]

Committed patchset #62 (id:1560001)

[commit-bot: I haz the power, 2015-08-09 06:21:10]

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