Implement disambiguation popup

Implement disambiguation popup

This patch implements the renderer-process side of disambiguation popup.
Individual platforms shall provide its own browser-side handling.

Correspoding WebKit CL: https://bugs.webkit.org/show_bug.cgi?id=94182

BUG=100752
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162612

[trchen, 2012-08-29 00:25:32]

This patch is for the disambiguation popup upstreaming from clank. Please take a
look. Thank you!

[darin (slow to review), 2012-08-30 17:39:23]

http://codereview.chromium.org/10885004/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/10885004/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host_impl.cc:1717: // TODO:
implement the platform-specific disambiguation popup
nit: indent by 2 spaces

http://codereview.chromium.org/10885004/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10885004/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:5913: // Compute the scaling factor to
ensure the smallest touch candidate reaches
render_view_impl.cc is already packed with code for too many different things. 
it is a cross-roads of hell type of file.

can you please break this implementation code out into a helper file?  maybe it
is just all of these calculations that would move out.  ideally, you'd reduce
the RenderViewImpl::handleDisambiguationPopup down to the bits of code that deal
with webwidget_ and send the IPC.

http://codereview.chromium.org/10885004/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:5967: bool
RenderViewImpl::handleDisambiguationPopup(
why is this function named "handleDisambiguationPopup"?  maybe it should be
named "showDisambiguationPopup"?  maybe the IPC message should be similarly
named?

what i'm not clear about is whether you are trying to say that a popup must be
shown or if you are just trying to tell the browser-side that multiple targets
were touched.  if the latter, then the WebKit API method should be named
consistently.

[trchen, 2012-08-30 21:15:21]

http://codereview.chromium.org/10885004/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/10885004/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host_impl.cc:1717: // TODO:
implement the platform-specific disambiguation popup
On 2012/08/30 17:39:23, darin wrote:
> nit: indent by 2 spaces

Done.

http://codereview.chromium.org/10885004/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10885004/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:5913: // Compute the scaling factor to
ensure the smallest touch candidate reaches
On 2012/08/30 17:39:23, darin wrote:
> render_view_impl.cc is already packed with code for too many different things.

> it is a cross-roads of hell type of file.
> 
> can you please break this implementation code out into a helper file?  maybe
it
> is just all of these calculations that would move out.  ideally, you'd reduce
> the RenderViewImpl::handleDisambiguationPopup down to the bits of code that
deal
> with webwidget_ and send the IPC.

That sounds like a great idea. I think splitting into a helper class also helps
individual platform to override popup size calculation logic. (the current one
is too Android-specific)

http://codereview.chromium.org/10885004/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:5967: bool
RenderViewImpl::handleDisambiguationPopup(
On 2012/08/30 17:39:23, darin wrote:
> why is this function named "handleDisambiguationPopup"?  maybe it should be
> named "showDisambiguationPopup"?  maybe the IPC message should be similarly
> named?
> 
> what i'm not clear about is whether you are trying to say that a popup must be
> shown or if you are just trying to tell the browser-side that multiple targets
> were touched.  if the latter, then the WebKit API method should be named
> consistently.

Yea... I agree the naming is confusing here.

Up to this point, the disambiguation popup can still be cancelled. If the
embedder doesn't want to implement the popup, it can simply return false in the
handler. (The default behavior provided by WebViewClient)

After the IPC has been sent to the browser process, there is no way to fallback
to default behavior, since the gesture event has already been swallowed.

I think you're right that the WebKit API should be named multipleTargetTapped
and the IPC should be renamed to ViewHostMsg_ShowDisambiguationPopup. The
confusing name is inherited from our old code.

[trchen, 2012-08-31 06:29:03]

I uploaded a new patch that renames the WebViewClient events and IPCs.
Corresponding WebKit change: https://bugs.webkit.org/show_bug.cgi?id=95529

The popup size calculation has been moved to a separate helper class as well.

[darin (slow to review), 2012-08-31 23:00:16]

lots of google c++ style guide errors in here.  please review the style guide.

http://codereview.chromium.org/10885004/diff/6001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/10885004/diff/6001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:1717: // TODO:
implement the platform-specific disambiguation popup
nit: indent comment by 2 spaces

http://codereview.chromium.org/10885004/diff/6001/content/renderer/disambigua...
File content/renderer/disambiguation_popup_helper.cc (right):

http://codereview.chromium.org/10885004/diff/6001/content/renderer/disambigua...
content/renderer/disambiguation_popup_helper.cc:9: using namespace WebKit;
nit: google style guide does not allow 'using namespace Foo'

http://codereview.chromium.org/10885004/diff/6001/content/renderer/disambigua...
content/renderer/disambiguation_popup_helper.cc:13: static const int
kDisambiguationPopupPadding = 8;
nit: instead of 'static' variables and functions, it is more conventional in
chromium code to use an anonymous namespace.

http://codereview.chromium.org/10885004/diff/6001/content/renderer/disambigua...
content/renderer/disambiguation_popup_helper.cc:52: } else
nit: add {}'s for the last else clause to be consistent with the previous lines.

http://codereview.chromium.org/10885004/diff/6001/content/renderer/disambigua...
content/renderer/disambiguation_popup_helper.cc:59: const gfx::Size&
viewport_size,
nit: indentation

http://codereview.chromium.org/10885004/diff/6001/content/renderer/disambigua...
content/renderer/disambiguation_popup_helper.cc:75: touch_point.y() - top,
nit: indentation

http://codereview.chromium.org/10885004/diff/6001/content/renderer/disambigua...
File content/renderer/disambiguation_popup_helper.h (right):

http://codereview.chromium.org/10885004/diff/6001/content/renderer/disambigua...
content/renderer/disambiguation_popup_helper.h:11: class Rect;
nit: do not indent

http://codereview.chromium.org/10885004/diff/6001/content/renderer/disambigua...
content/renderer/disambiguation_popup_helper.h:13: };
nit: no ';'

http://codereview.chromium.org/10885004/diff/6001/content/renderer/disambigua...
content/renderer/disambiguation_popup_helper.h:16: class WebRect;
nit: do not indent

http://codereview.chromium.org/10885004/diff/6001/content/renderer/disambigua...
content/renderer/disambiguation_popup_helper.h:17: };
nit: no ';'

http://codereview.chromium.org/10885004/diff/6001/content/renderer/disambigua...
content/renderer/disambiguation_popup_helper.h:22: static float
ComputeZoomAreaAndScaleFactor(gfx::Rect& zoom_rect,
nit: move zoom_rect down to the next line:

  static float ComputeZoomAreaAndScaleFactor(
      gfx::Rect& zoom_rect,
      ...);

Note: zoom_rect should be passed as a pointer instead, and it should be the last
argument.

[trchen, 2012-09-05 02:51:35]

Style nits fixed. Please take a look again. Thank you!

[trchen, 2012-09-24 22:30:56]

Rebased. This patch has been stalled for a few weeks. Can we start to review it?
Thanks! :)

[trchen, 2012-10-02 21:08:34]

Ping? Darin and James, we need some help to get this patch in. Thanks in
advance. :)

[darin (slow to review), 2012-10-08 21:03:46]

http://codereview.chromium.org/10885004/diff/13001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/10885004/diff/13001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1727: void
RenderWidgetHostImpl::OnMsgShowDisambiguationPopup(
nit: please list this after OnMsgUnlockMouse so that methods in the .cc file are
listed in the same order as the .h file.

http://codereview.chromium.org/10885004/diff/13001/content/renderer/disambigu...
File content/renderer/disambiguation_popup_helper.cc (right):

http://codereview.chromium.org/10885004/diff/13001/content/renderer/disambigu...
content/renderer/disambiguation_popup_helper.cc:85: float
DisambiguationPopupHelper::ComputeZoomAreaAndScaleFactor(
There's enough code in here that I think you could write some good unit tests.

http://codereview.chromium.org/10885004/diff/13001/content/renderer/disambigu...
File content/renderer/disambiguation_popup_helper.h (right):

http://codereview.chromium.org/10885004/diff/13001/content/renderer/disambigu...
content/renderer/disambiguation_popup_helper.h:20: class
DisambiguationPopupHelper {
nit: this should be in the content namespace

http://codereview.chromium.org/10885004/diff/13001/content/renderer/disambigu...
content/renderer/disambiguation_popup_helper.h:21: public:
nit: indent by 1 space

http://codereview.chromium.org/10885004/diff/13001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10885004/diff/13001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:6295: gfx::Rect finger_rect = gfx::Rect(
no need to call the constructor and the copy constructor!

gfx::Rect finger_rect(
    event.x - event.data.tap.width / 2,
    event.y - event.data.tap.height / 2,
    event.data.tap.width,
    event.data.tap.height);

http://codereview.chromium.org/10885004/diff/13001/content/renderer/render_vi...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/10885004/diff/13001/content/renderer/render_vi...
content/renderer/render_view_impl.h:497: virtual bool
didTapMultipleTargets(const WebKit::WebGestureEvent&,
nit: first argument should be on the next line.

  virtual bool didTapMultipleTargets(
      const WebKit::WebGestureEvent& event,
      ...

WebGestureEvent parameter needs a name.  I suggest "event".

"targetRects" should be "target_rects"

[trchen, 2012-10-11 23:54:05]

New patch uploaded. Let's get this in! Thanks! :)

http://codereview.chromium.org/10885004/diff/13001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/10885004/diff/13001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1727: void
RenderWidgetHostImpl::OnMsgShowDisambiguationPopup(
On 2012/10/08 21:03:46, darin wrote:
> nit: please list this after OnMsgUnlockMouse so that methods in the .cc file
are
> listed in the same order as the .h file.

Done.

http://codereview.chromium.org/10885004/diff/13001/content/renderer/disambigu...
File content/renderer/disambiguation_popup_helper.cc (right):

http://codereview.chromium.org/10885004/diff/13001/content/renderer/disambigu...
content/renderer/disambiguation_popup_helper.cc:85: float
DisambiguationPopupHelper::ComputeZoomAreaAndScaleFactor(
On 2012/10/08 21:03:46, darin wrote:
> There's enough code in here that I think you could write some good unit tests.

Added 3 simple test cases. However all it does is to make sure the output values
have basic sanity, as the behavior is pretty much implementation defined.

http://codereview.chromium.org/10885004/diff/13001/content/renderer/disambigu...
File content/renderer/disambiguation_popup_helper.h (right):

http://codereview.chromium.org/10885004/diff/13001/content/renderer/disambigu...
content/renderer/disambiguation_popup_helper.h:20: class
DisambiguationPopupHelper {
On 2012/10/08 21:03:46, darin wrote:
> nit: this should be in the content namespace

Done.

http://codereview.chromium.org/10885004/diff/13001/content/renderer/disambigu...
content/renderer/disambiguation_popup_helper.h:21: public:
On 2012/10/08 21:03:46, darin wrote:
> nit: indent by 1 space

Done.

http://codereview.chromium.org/10885004/diff/13001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10885004/diff/13001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:6295: gfx::Rect finger_rect = gfx::Rect(
On 2012/10/08 21:03:46, darin wrote:
> no need to call the constructor and the copy constructor!
> 
> gfx::Rect finger_rect(
>     event.x - event.data.tap.width / 2,
>     event.y - event.data.tap.height / 2,
>     event.data.tap.width,
>     event.data.tap.height);

Done.

http://codereview.chromium.org/10885004/diff/13001/content/renderer/render_vi...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/10885004/diff/13001/content/renderer/render_vi...
content/renderer/render_view_impl.h:497: virtual bool
didTapMultipleTargets(const WebKit::WebGestureEvent&,
On 2012/10/08 21:03:46, darin wrote:
> nit: first argument should be on the next line.
> 
>   virtual bool didTapMultipleTargets(
>       const WebKit::WebGestureEvent& event,
>       ...
> 
> WebGestureEvent parameter needs a name.  I suggest "event".
> 
> "targetRects" should be "target_rects"

Done.

[darin (slow to review), 2012-10-17 19:14:01]

LGTM w/ nits fixed

http://codereview.chromium.org/10885004/diff/23001/content/renderer/disambigu...
File content/renderer/disambiguation_popup_helper.cc (right):

http://codereview.chromium.org/10885004/diff/23001/content/renderer/disambigu...
content/renderer/disambiguation_popup_helper.cc:66: gfx::Size max_size =
viewport_size;
nit: indent by 2 spaces

http://codereview.chromium.org/10885004/diff/23001/content/renderer/disambigu...
File content/renderer/disambiguation_popup_helper_unittest.cc (right):

http://codereview.chromium.org/10885004/diff/23001/content/renderer/disambigu...
content/renderer/disambiguation_popup_helper_unittest.cc:14: using namespace
content;
nit: google style forbids 'using namespace Foo'

You should just put these tests into the content namespace.

namespace content {

TEST_F(...) {
}

}

http://codereview.chromium.org/10885004/diff/23001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10885004/diff/23001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:6352: using
content::DisambiguationPopupHelper;
nit: RenderViewImpl is already in the content namespace, so you shouldn't need
this using directive.  maybe that change only happened recently?

[trchen, 2012-10-17 22:25:08]

Style nits fixed. Checking CQ. Thank you very much!

http://codereview.chromium.org/10885004/diff/23001/content/renderer/disambigu...
File content/renderer/disambiguation_popup_helper.cc (right):

http://codereview.chromium.org/10885004/diff/23001/content/renderer/disambigu...
content/renderer/disambiguation_popup_helper.cc:66: gfx::Size max_size =
viewport_size;
On 2012/10/17 19:14:01, darin wrote:
> nit: indent by 2 spaces

Done.

http://codereview.chromium.org/10885004/diff/23001/content/renderer/disambigu...
File content/renderer/disambiguation_popup_helper_unittest.cc (right):

http://codereview.chromium.org/10885004/diff/23001/content/renderer/disambigu...
content/renderer/disambiguation_popup_helper_unittest.cc:14: using namespace
content;
On 2012/10/17 19:14:01, darin wrote:
> nit: google style forbids 'using namespace Foo'
> 
> You should just put these tests into the content namespace.
> 
> namespace content {
> 
> TEST_F(...) {
> }
> 
> }

Done.

http://codereview.chromium.org/10885004/diff/23001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10885004/diff/23001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:6352: using
content::DisambiguationPopupHelper;
On 2012/10/17 19:14:01, darin wrote:
> nit: RenderViewImpl is already in the content namespace, so you shouldn't need
> this using directive.  maybe that change only happened recently?

Nope, only RenderView is in content namespace while RenderViewImpl is global.

[commit-bot: I haz the power, 2012-10-17 22:26:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/10885004/31001

[commit-bot: I haz the power, 2012-10-17 22:36:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/10885004/27003

[commit-bot: I haz the power, 2012-10-17 23:05:40]

Retried try job too often for step(s) base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, content_browsertests,
content_unittests, crypto_unittests, gpu_unittests, interactive_ui_tests,
ipc_tests, jingle_unittests, media_unittests, net_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests

[commit-bot: I haz the power, 2012-10-17 23:24:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/10885004/28015

[commit-bot: I haz the power, 2012-10-18 01:55:04]

Change committed as 162612