Enable touch based selection and editing for webpages behind a flag.

Enable touch based selection and editing for webpages behind a flag.

BUG=115237
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195688

[jam, 2013-02-22 01:08:12]

I'm redirecting this to Antoine

one comment about the content/port change that needs to be fixed though

https://codereview.chromium.org/12321005/diff/11001/content/port/browser/rend...
File content/port/browser/render_widget_host_view_port.h (right):

https://codereview.chromium.org/12321005/diff/11001/content/port/browser/rend...
content/port/browser/render_widget_host_view_port.h:159: virtual void
TouchEditingHandlesVisibilityChanged(bool visible) {}
note that there are no other default implementation in this file

[varunjain, 2013-02-22 01:15:18]

https://codereview.chromium.org/12321005/diff/11001/content/port/browser/rend...
File content/port/browser/render_widget_host_view_port.h (right):

https://codereview.chromium.org/12321005/diff/11001/content/port/browser/rend...
content/port/browser/render_widget_host_view_port.h:159: virtual void
TouchEditingHandlesVisibilityChanged(bool visible) {}
On 2013/02/22 01:08:12, jam wrote:
> note that there are no other default implementation in this file

Done.

[piman, 2013-02-22 02:43:03]

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2188: // gives
bounds in screen space. So we convert.
Can't we just return window_->bounds() ?

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2196: return
window_->GetRootWindow();
There is already a GetNativeView() in this class. How does this even compile?
Also why are we returning the root window?

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2228:
host_->ForwardMouseEvent(mouse_event);
This doesn't look reasonable - what are we trying to do?
Sending a right mouse click seems like the worst way to open a context menu.
Between the DOM hooking events, the possibility that a plugin (or other things)
is underneath, and the possibility that the page moved/changed, this looks ripe
for bugs/abuse/clickjacking opportunities.

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2240: string16
result;
nit: move to IDS_APP_PASTE case (add brackets there).

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2242: case
IDS_APP_CUT:
nit: rather than importing UI symbols here, it seems like it would be
advantageous to declare an enum somewhere...

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.h:294: // Overridden
from ui::TouchEditable:
I think all this should be a tear-off class.
RWVHA is growing and growing, but this looks like a part that's fairly easy to
separate.
Almost all of it could be done from the public API, or at least the Port API,
and so could be re-used across all ports implementations.

It would also fix the API naming conflict.

https://codereview.chromium.org/12321005/diff/14001/content/port/browser/rend...
File content/port/browser/render_widget_host_view_port.h (right):

https://codereview.chromium.org/12321005/diff/14001/content/port/browser/rend...
content/port/browser/render_widget_host_view_port.h:157: // Notifies the view
that the renderer has just show or hidden handles for
nit: s/show/showed/

[jam, 2013-02-22 17:26:25]

btw i looked at the command line flag that enables this, and it looks only to be
set for cros. so why isn't all this code ifdef'd only for OS_CHROMEOS? are there
plans that it'll be used on other platforms in the future?

[varunjain, 2013-02-28 20:28:19]

Regarding jam@'s comment: I do plan to enable this on win as well. But thats
very much in the future. For now, for non-chromeos, none of this code will be
executed. I think thats sufficient, but I would be happy to add #ifdefs if you
are not satisfied.

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2188: // gives
bounds in screen space. So we convert.
On 2013/02/22 02:43:04, piman wrote:
> Can't we just return window_->bounds() ?

Done.

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2196: return
window_->GetRootWindow();
On 2013/02/22 02:43:04, piman wrote:
> There is already a GetNativeView() in this class. How does this even compile?
> Also why are we returning the root window?

Done.

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2228:
host_->ForwardMouseEvent(mouse_event);
On 2013/02/22 02:43:04, piman wrote:
> This doesn't look reasonable - what are we trying to do?
> Sending a right mouse click seems like the worst way to open a context menu.
> Between the DOM hooking events, the possibility that a plugin (or other
things)
> is underneath, and the possibility that the page moved/changed, this looks
ripe
> for bugs/abuse/clickjacking opportunities.

Removed. I have added proper API to webkit.

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2240: string16
result;
On 2013/02/22 02:43:04, piman wrote:
> nit: move to IDS_APP_PASTE case (add brackets there).

Done.

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2242: case
IDS_APP_CUT:
On 2013/02/22 02:43:04, piman wrote:
> nit: rather than importing UI symbols here, it seems like it would be
> advantageous to declare an enum somewhere...

What would be the advantage? the enum will have to be declared somewhere in ui/
anyways because this interface is defined in ui/base.

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/12321005/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.h:294: // Overridden
from ui::TouchEditable:
On 2013/02/22 02:43:04, piman wrote:
> I think all this should be a tear-off class.
> RWVHA is growing and growing, but this looks like a part that's fairly easy to
> separate.
> Almost all of it could be done from the public API, or at least the Port API,
> and so could be re-used across all ports implementations.
> 
> It would also fix the API naming conflict.

Done.

https://codereview.chromium.org/12321005/diff/14001/content/port/browser/rend...
File content/port/browser/render_widget_host_view_port.h (right):

https://codereview.chromium.org/12321005/diff/14001/content/port/browser/rend...
content/port/browser/render_widget_host_view_port.h:157: // Notifies the view
that the renderer has just show or hidden handles for
On 2013/02/22 02:43:04, piman wrote:
> nit: s/show/showed/

Done.

[varunjain, 2013-04-12 00:12:26]

Hi Antoine, I would like to revive review on this.
I have uploaded a shiny new patch and while I was working on tests, I'd like
early review feedback. Please have a look whenever you have a few mins.

[piman, 2013-04-12 02:20:51]

On Thu, Apr 11, 2013 at 5:12 PM, <varunjain@chromium.org> wrote:

> Hi Antoine, I would like to revive review on this.
> I have uploaded a shiny new patch and while I was working on tests, I'd
> like
> early review feedback. Please have a look whenever you have a few mins.
>

This grew in scope significantly. Can you have a UI person review this
first?


>
>
https://codereview.chromium.**org/12321005/<https://codereview.chromium.org/1...
>

[varunjain, 2013-04-12 03:19:11]

+sky as Antoine has asked for review from a ui person.

[sky, 2013-04-12 16:09:52]

How does IME come into play with this? Should starting some interaction with IME
implicitly end editing on the touch client?

https://codereview.chromium.org/12321005/diff/41001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/12321005/diff/41001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:925: if
(touch_editing_client_)
Isn't this handled on 2518?

https://codereview.chromium.org/12321005/diff/41001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1114:
touch_editing_client_->OnSelectionOrCursorChanged(selection_anchor_rect_,
TouchEditingClient seems to have a mix of functions that are synchronous (such
as the input event and focus ones) and those that come from the renderer. Is
that going to be problematic?

https://codereview.chromium.org/12321005/diff/41001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2068:
touch_editing_client_->EndTouchEditing();
Does the client need to differentiate between a cancel like this and a normal
end?

https://codereview.chromium.org/12321005/diff/41001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/12321005/diff/41001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.h:100: virtual
~TouchEditingClient() {}
Make protected.

[varunjain, 2013-04-12 17:35:18]

Regarding the IME comment: IME interaction should not be affected by touch
selection. Especially since the handles disappear on any keyboard input. I will
test this touch to make sure the handles dont interfere with IME widgets.

https://codereview.chromium.org/12321005/diff/41001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/12321005/diff/41001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:925: if
(touch_editing_client_)
On 2013/04/12 16:09:52, sky wrote:
> Isn't this handled on 2518?

Done.

https://codereview.chromium.org/12321005/diff/41001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1114:
touch_editing_client_->OnSelectionOrCursorChanged(selection_anchor_rect_,
On 2013/04/12 16:09:52, sky wrote:
> TouchEditingClient seems to have a mix of functions that are synchronous (such
> as the input event and focus ones) and those that come from the renderer. Is
> that going to be problematic?

The input event and ack are used to determine if touch handles need to be shown
or not. The actual position of touch handles is always updated asynchronously
through this OnSelectionOrCursorChanged call. I think the most important thing
to consider here is that when touch handles are visible, are they positioned
correctly and I think this condition is eventually true because of this call.
There is no other place where I update the position of the handles. So the
handles might lag behind but will always sync up.

https://codereview.chromium.org/12321005/diff/41001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2068:
touch_editing_client_->EndTouchEditing();
On 2013/04/12 16:09:52, sky wrote:
> Does the client need to differentiate between a cancel like this and a normal
> end?

The effect of cancel or end is the same, ie, hide the handles. So we do not need
to differentiate.

https://codereview.chromium.org/12321005/diff/41001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/12321005/diff/41001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.h:100: virtual
~TouchEditingClient() {}
On 2013/04/12 16:09:52, sky wrote:
> Make protected.

Done.

[sky, 2013-04-12 20:22:58]

I only looked at the TouchEditingClient and implementation of it.

https://codereview.chromium.org/12321005/diff/53001/content/browser/web_conte...
File content/browser/web_contents/touch_editable_impl_aura.cc (right):

https://codereview.chromium.org/12321005/diff/53001/content/browser/web_conte...
content/browser/web_contents/touch_editable_impl_aura.cc:88:
touch_selection_controller_->SelectionChanged();
How does the touch_selection_controller_ end up getting reset for this case?

https://codereview.chromium.org/12321005/diff/53001/content/browser/web_conte...
content/browser/web_contents/touch_editable_impl_aura.cc:313: } else if
(rwhva_->GetNativeView() == window)
nit: {}

[varunjain, 2013-04-17 02:39:48]

Sorry for the delay reviewers. I got distracted with other stuff. I have
attended to comments and also added tests. PTAL

https://codereview.chromium.org/12321005/diff/53001/content/browser/web_conte...
File content/browser/web_contents/touch_editable_impl_aura.cc (right):

https://codereview.chromium.org/12321005/diff/53001/content/browser/web_conte...
content/browser/web_contents/touch_editable_impl_aura.cc:88:
touch_selection_controller_->SelectionChanged();
On 2013/04/12 20:22:58, sky wrote:
> How does the touch_selection_controller_ end up getting reset for this case?

Great catch! I think for that case the TouchSelectionController will need to
notify us that handle dragging has ended. This functionality does not yet exist
in TSC. I will add it in a subsequent CL.

https://codereview.chromium.org/12321005/diff/53001/content/browser/web_conte...
content/browser/web_contents/touch_editable_impl_aura.cc:313: } else if
(rwhva_->GetNativeView() == window)
On 2013/04/12 20:22:58, sky wrote:
> nit: {}

Done.

[piman, 2013-04-18 22:28:42]

https://codereview.chromium.org/12321005/diff/62001/content/browser/web_conte...
File content/browser/web_contents/touch_editable_impl_aura.cc (right):

https://codereview.chromium.org/12321005/diff/62001/content/browser/web_conte...
content/browser/web_contents/touch_editable_impl_aura.cc:297: aura::Window*
parent) {
I don't think this is enough. This only tells you if the direct parent changed,
not any parent all the way to the root. So you will not be correctly updating
top_level_window_ if, say, your parent's parent gets changed.

https://codereview.chromium.org/12321005/diff/62001/content/browser/web_conte...
content/browser/web_contents/touch_editable_impl_aura.cc:312:
touch_selection_controller_->SelectionChanged();
I don't understand why this is needed, and furthermore, why it's done that way?
Why is the fact that the top-level window changed size impact the selection?
It seems to me this bakes an assumption about UI-level behavior down into
content/, which is a layering violation.

https://codereview.chromium.org/12321005/diff/62001/content/browser/web_conte...
File content/browser/web_contents/touch_editable_impl_aura_browsertest.cc
(right):

https://codereview.chromium.org/12321005/diff/62001/content/browser/web_conte...
content/browser/web_contents/touch_editable_impl_aura_browsertest.cc:66:
loop->Run();
The recommended way nowadays is to use base::RunLoop. It correctly handles more
nested message loops and avoids a lot of the induced flakiness. Mind using that
instead?

[varunjain, 2013-04-19 05:14:12]

https://codereview.chromium.org/12321005/diff/62001/content/browser/web_conte...
File content/browser/web_contents/touch_editable_impl_aura.cc (right):

https://codereview.chromium.org/12321005/diff/62001/content/browser/web_conte...
content/browser/web_contents/touch_editable_impl_aura.cc:297: aura::Window*
parent) {
On 2013/04/18 22:28:42, piman wrote:
> I don't think this is enough. This only tells you if the direct parent
changed,
> not any parent all the way to the root. So you will not be correctly updating
> top_level_window_ if, say, your parent's parent gets changed.

removed top_level_window_

https://codereview.chromium.org/12321005/diff/62001/content/browser/web_conte...
content/browser/web_contents/touch_editable_impl_aura.cc:312:
touch_selection_controller_->SelectionChanged();
On 2013/04/18 22:28:42, piman wrote:
> I don't understand why this is needed, and furthermore, why it's done that
way?
> Why is the fact that the top-level window changed size impact the selection?
> It seems to me this bakes an assumption about UI-level behavior down into
> content/, which is a layering violation.

You are correct. This should not be needed here. Removed top_level_window_ and
all associated logic.

https://codereview.chromium.org/12321005/diff/62001/content/browser/web_conte...
File content/browser/web_contents/touch_editable_impl_aura_browsertest.cc
(right):

https://codereview.chromium.org/12321005/diff/62001/content/browser/web_conte...
content/browser/web_contents/touch_editable_impl_aura_browsertest.cc:66:
loop->Run();
On 2013/04/18 22:28:42, piman wrote:
> The recommended way nowadays is to use base::RunLoop. It correctly handles
more
> nested message loops and avoids a lot of the induced flakiness. Mind using
that
> instead?

Done.

[piman, 2013-04-19 05:55:17]

https://codereview.chromium.org/12321005/diff/71001/content/browser/web_conte...
File content/browser/web_contents/touch_editable_impl_aura.cc (right):

https://codereview.chromium.org/12321005/diff/71001/content/browser/web_conte...
content/browser/web_contents/touch_editable_impl_aura.cc:300: const gfx::Rect&
new_bounds) {
Similarly here - you're setting yourself as a WindowObserver, adding a bunch of
logic, but at the end of the day, if all you need is to know when the renderer
changes size, it's simpler to just have RWHVA tell you so from
RenderWidgetHostViewAura::SetBounds

[varunjain, 2013-04-19 16:59:08]

https://codereview.chromium.org/12321005/diff/71001/content/browser/web_conte...
File content/browser/web_contents/touch_editable_impl_aura.cc (right):

https://codereview.chromium.org/12321005/diff/71001/content/browser/web_conte...
content/browser/web_contents/touch_editable_impl_aura.cc:300: const gfx::Rect&
new_bounds) {
On 2013/04/19 05:55:17, piman wrote:
> Similarly here - you're setting yourself as a WindowObserver, adding a bunch
of
> logic, but at the end of the day, if all you need is to know when the renderer
> changes size, it's simpler to just have RWHVA tell you so from
> RenderWidgetHostViewAura::SetBounds

Done.

[piman, 2013-04-19 17:02:47]

LGTM for the content parts. I would prefer if Scott could give his final ack
before landing.

[sky, 2013-04-22 14:20:51]

LGTM

https://codereview.chromium.org/12321005/diff/83001/content/browser/web_conte...
File content/browser/web_contents/touch_editable_impl_aura.cc (right):

https://codereview.chromium.org/12321005/diff/83001/content/browser/web_conte...
content/browser/web_contents/touch_editable_impl_aura.cc:68: } else
nit: {}

[varunjain, 2013-04-22 14:25:23]

https://codereview.chromium.org/12321005/diff/83001/content/browser/web_conte...
File content/browser/web_contents/touch_editable_impl_aura.cc (right):

https://codereview.chromium.org/12321005/diff/83001/content/browser/web_conte...
content/browser/web_contents/touch_editable_impl_aura.cc:68: } else
On 2013/04/22 14:20:51, sky wrote:
> nit: {}

Done.

[commit-bot: I haz the power, 2013-04-22 14:25:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/12321005/89001

[commit-bot: I haz the power, 2013-04-22 14:25:47]

Failed to apply patch for content/content_tests.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/content_tests.gypi
  Hunk #1 FAILED at 767.
  1 out of 1 hunk FAILED -- saving rejects to file
content/content_tests.gypi.rej

Patch:       content/content_tests.gypi
Index: content/content_tests.gypi
diff --git a/content/content_tests.gypi b/content/content_tests.gypi
index
a7cfa3f35716ad3b9164555566d30e9e5a9537c9..d1f9837abdf855a01a266c6180d8ecb2334fe462
100644
--- a/content/content_tests.gypi
+++ b/content/content_tests.gypi
@@ -767,6 +767,7 @@
             'browser/session_history_browsertest.cc',
             'browser/site_per_process_browsertest.cc',
             'browser/speech/speech_recognition_browsertest.cc',
+            'browser/web_contents/touch_editable_impl_aura_browsertest.cc',
             'browser/web_contents/web_contents_view_aura_browsertest.cc',
             'browser/webkit_browsertest.cc',
             'browser/worker_host/test/worker_browsertest.cc',

[commit-bot: I haz the power, 2013-04-22 14:34:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/12321005/92001

[commit-bot: I haz the power, 2013-04-22 14:34:54]

Presubmit check for 12321005-92001 failed and returned exit status 1.

INFO:root:Found 19 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  content/browser/web_contents/web_contents_view_aura.cc:903
    navigation_overlay_->SetupForTesting();

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    content/common/view_messages.h

Presubmit checks took 2.0s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[varunjain, 2013-04-22 14:39:19]

+cevans for view_messages.h

[Tom Sepez, 2013-04-22 19:58:45]

Rubberstamp LGTM on view_messages.h for new browser -> renderer message.

[commit-bot: I haz the power, 2013-04-22 19:59:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/12321005/92001

[commit-bot: I haz the power, 2013-04-22 21:33:49]

Retried try job too often on linux_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-04-22 21:50:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/12321005/97004

[commit-bot: I haz the power, 2013-04-22 21:52:22]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-04-23 01:32:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/12321005/97004

[commit-bot: I haz the power, 2013-04-23 02:00:26]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, googleurl_unittests,
media_unittests, net_unittests, sql_unittests, ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-04-23 02:01:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/12321005/97004

[commit-bot: I haz the power, 2013-04-23 02:10:40]

Change committed as 195688