Adds link disambiguation popup support to Windows.

Adds link disambiguation popup support to Windows.

The Android link disambiguation popup is rendered by PopupZoomer on the Java side. This CL tries to recreate that UI in Aura using a BubbleDelegate.

This part works just like the Android code:

When Blink detects an ambiguous gesture it calls RenderViewImpl::didTapMultipleTargets(), which uses paintCompositedDeprecated() in the WebWidget to get a zoomed bitmap region containing the ambiguous links the gesture could possibly indicate. RenderViewImpl then sends a ShowDisambiguationPopup message via IPC which ultimately ends up in RenderWidgetHostImpl::OnShowDisambiguationPopup(), which marshals the bitmap and then then calls ShowDisambiguationPopup on the view_.

This part is specific to aura:

After a series of delegation steps the callback ends up in WebContentsViewAura::OnLinkDisambiguationPopupRequested, which converts the target rectangle from pixels to DIPs and then requests the popup be rendered by asking its WebContentsViewDelegate, ChromeWebContentsViewDelegateViews, which owns a LinkDisambiguationPopup and asks it to render.

The LinkDisambiguationPopup uses BubbleDelegate to show the zoomed image. Any GestureEvents it receives converted into the coordinate space of the content view and then are relayed via callback back to WebContentsViewAura, which then converts them in to Blink events and sends them on to the RenderWidgetHostImpl for relaying back to Blink.

jam: content/ parts
sky: everything else?

BUG=385249
Committed: https://crrev.com/b171b53a96f5926e2462985f05450ca9054b7ac3
Cr-Commit-Position: refs/heads/master@{#296355}

[cpu_(ooo_6.6-7.5), 2014-07-20 01:43:15]

My first impression is that it is solid. I am not an expert in most of the code
it goes thru though.

Some comments as they come to me.

First, a meaty CL description is in order, it should be a short narrative of how
this is done, like step 1, step 2, etc then it goes to didTapMultipleTargets,
etc.

Second if the UI is at all different to that of android please include a
screenshot in the bug.

Third, list for each reviewer what files to focus on.

[cpu_(ooo_6.6-7.5), 2014-07-20 01:50:44]

https://codereview.chromium.org/351683002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/351683002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1643: #else
add comment about the rect coordinates, for future historians?

https://codereview.chromium.org/351683002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1647:
zoomed_bitmap.setPixels(0);
interesting it goes to 0, what is the ownership model? if it is explained
somewhere else. The browser makes a copy I am guessing?

https://codereview.chromium.org/351683002/diff/160001/ui/aura/window_delegate.h
File ui/aura/window_delegate.h (right):

https://codereview.chromium.org/351683002/diff/160001/ui/aura/window_delegate...
ui/aura/window_delegate.h:106: 
are we missing the fwd declaration of SkBitmap here?

https://codereview.chromium.org/351683002/diff/160001/ui/events/gesture_event...
File ui/events/gesture_event_details.cc (right):

https://codereview.chromium.org/351683002/diff/160001/ui/events/gesture_event...
ui/events/gesture_event_details.cc:73: memcpy(&data, &details.data,
sizeof(Details));
can the sizeof() be of (*details.data) ?

[cpu_(ooo_6.6-7.5), 2014-07-20 01:54:06]

https://codereview.chromium.org/351683002/diff/160001/ui/keyboard/keyboard_co...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/351683002/diff/160001/ui/keyboard/keyboard_co...
ui/keyboard/keyboard_controller.cc:122: const gfx::Rect& target_rect, const
SkBitmap& zoomed_bitmap) OVERRIDE {}
what happens if the user starts typing, should we dismiss?

If this is missing it should go in a follow up CL, or maybe the is a case in
which we should just eat the keystrokes until the LDP is closed?

[luken, 2014-07-21 18:01:29]

https://codereview.chromium.org/351683002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/351683002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1643: #else
On 2014/07/20 01:50:44, cpu wrote:
> add comment about the rect coordinates, for future historians?

Done.

https://codereview.chromium.org/351683002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1647:
zoomed_bitmap.setPixels(0);
On 2014/07/20 01:50:44, cpu wrote:
> interesting it goes to 0, what is the ownership model? if it is explained
> somewhere else. The browser makes a copy I am guessing?

It's not explained anywhere else. I think they have to make a copy to marshal
the image to Java, so it's assumed that the image is no longer needed. I put a
comment.

https://codereview.chromium.org/351683002/diff/160001/ui/aura/window_delegate.h
File ui/aura/window_delegate.h (right):

https://codereview.chromium.org/351683002/diff/160001/ui/aura/window_delegate...
ui/aura/window_delegate.h:106: 
On 2014/07/20 01:50:44, cpu wrote:
> are we missing the fwd declaration of SkBitmap here?

Done.

https://codereview.chromium.org/351683002/diff/160001/ui/events/gesture_event...
File ui/events/gesture_event_details.cc (right):

https://codereview.chromium.org/351683002/diff/160001/ui/events/gesture_event...
ui/events/gesture_event_details.cc:73: memcpy(&data, &details.data,
sizeof(Details));
On 2014/07/20 01:50:44, cpu wrote:
> can the sizeof() be of (*details.data) ?
Isn't it the same thing? details.data is of type Details? And why the deref?

https://codereview.chromium.org/351683002/diff/160001/ui/keyboard/keyboard_co...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/351683002/diff/160001/ui/keyboard/keyboard_co...
ui/keyboard/keyboard_controller.cc:122: const gfx::Rect& target_rect, const
SkBitmap& zoomed_bitmap) OVERRIDE {}
On 2014/07/20 01:54:06, cpu wrote:
> what happens if the user starts typing, should we dismiss?
> 
> If this is missing it should go in a follow up CL, or maybe the is a case in
> which we should just eat the keystrokes until the LDP is closed?

The LDP eats keystrokes, but will close if the user hits the ESC key. Also any
click outside of the LDP will close it. That's what currently happens. I'm open
to suggestions, what do you think should happen? Dismiss on all keystrokes?

[jam, 2014-07-22 01:04:27]

I don't understand why all the different classes need to change, in particular
aura::WindowDelegate. I see that RenderWidgetHostImpl now calls
RWHV::ShowDisambiguationPopup on Windows, which gets to
RenderWidgetHostViewAura::ShowDisambiguationPopup. That method can get to a
RenderViewHostDelegate (see RenderWidgetHostViewAura::OnGestureEvent as an
example), and then call RVHD->GetDelegateView which is an WebContentsViewAura
object). all very convoluted i admit.

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

https://codereview.chromium.org/351683002/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1219:
OnLinkDisambiguationPopupRequested(target_rect, zoomed_bitmap);
why is this method both called by aura and by RenderWidgetHostImpl? i.e. i'm
confused as to why there's a new method on RenderWidgetHostViewBase and
aura::WindowDelegate

https://codereview.chromium.org/351683002/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1693: 
nit: extra line

https://codereview.chromium.org/351683002/diff/200001/content/public/browser/...
File content/public/browser/web_contents_view_delegate.h (right):

https://codereview.chromium.org/351683002/diff/200001/content/public/browser/...
content/public/browser/web_contents_view_delegate.h:40: typedef
base::Callback<void(ui::GestureEvent* event)> GestureCallback;
nit: convention in content api is to put the callback right above where it's
referenced, i.e. move this to line 51

https://codereview.chromium.org/351683002/diff/200001/content/public/browser/...
content/public/browser/web_contents_view_delegate.h:51: virtual void
ShowLinkDisambiguationPopup(const gfx::Rect& target_rect,
nit: document, especially what target_rect is relative to

https://codereview.chromium.org/351683002/diff/200001/content/public/browser/...
content/public/browser/web_contents_view_delegate.h:54: GestureCallback
callback) = 0;
nit: const ref for callback

https://codereview.chromium.org/351683002/diff/200001/ui/aura/window_delegate.h
File ui/aura/window_delegate.h (right):

https://codereview.chromium.org/351683002/diff/200001/ui/aura/window_delegate...
ui/aura/window_delegate.h:105: virtual void OnLinkDisambiguationPopupRequested(
nit: every method in this class is documented...

[sky, 2014-07-22 04:17:24]

https://codereview.chromium.org/351683002/diff/200001/ui/aura/window_delegate.h
File ui/aura/window_delegate.h (right):

https://codereview.chromium.org/351683002/diff/200001/ui/aura/window_delegate...
ui/aura/window_delegate.h:105: virtual void OnLinkDisambiguationPopupRequested(
You should not need to change WindowDelegate to accomplish what you're after. In
fact you shouldn't need to change aura or views at all. John suggested a way to
get at a delegate that would be better to add this function too.

[Rick Byers, 2014-07-29 19:14:44]

Before landing this CL let's discuss some of the higher-level issues over on the
bug: https://code.google.com/p/chromium/issues/detail?id=385249

We (input-dev team) have a bunch of experience with touch adjustment and the
disambiguation window - we should make sure we've got a consistent story for the
chrome platform here (when is such UI provided, when is it better to rely just
on touch adjustment, etc.).

[luken, 2014-07-30 19:17:31]

PTAL. Yes, the bots are all red but it seems to be not related to my change. I
promise I'll continue working on making them happy but in the mean time if there
are further big changes to this I'd like to know. Thanks!

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

https://codereview.chromium.org/351683002/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1219:
OnLinkDisambiguationPopupRequested(target_rect, zoomed_bitmap);
On 2014/07/22 01:04:26, jam wrote:
> why is this method both called by aura and by RenderWidgetHostImpl? i.e. i'm
> confused as to why there's a new method on RenderWidgetHostViewBase and
> aura::WindowDelegate

I made a mess plumbing things. I've now followed your suggestion, it's much
cleaner.

https://codereview.chromium.org/351683002/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1693: 
On 2014/07/22 01:04:26, jam wrote:
> nit: extra line

Done.

https://codereview.chromium.org/351683002/diff/200001/content/public/browser/...
File content/public/browser/web_contents_view_delegate.h (right):

https://codereview.chromium.org/351683002/diff/200001/content/public/browser/...
content/public/browser/web_contents_view_delegate.h:40: typedef
base::Callback<void(ui::GestureEvent* event)> GestureCallback;
On 2014/07/22 01:04:26, jam wrote:
> nit: convention in content api is to put the callback right above where it's
> referenced, i.e. move this to line 51

Done.

https://codereview.chromium.org/351683002/diff/200001/content/public/browser/...
content/public/browser/web_contents_view_delegate.h:51: virtual void
ShowLinkDisambiguationPopup(const gfx::Rect& target_rect,
On 2014/07/22 01:04:26, jam wrote:
> nit: document, especially what target_rect is relative to

Done.

https://codereview.chromium.org/351683002/diff/200001/content/public/browser/...
content/public/browser/web_contents_view_delegate.h:54: GestureCallback
callback) = 0;
On 2014/07/22 01:04:26, jam wrote:
> nit: const ref for callback

Done.

https://codereview.chromium.org/351683002/diff/200001/ui/aura/window_delegate.h
File ui/aura/window_delegate.h (right):

https://codereview.chromium.org/351683002/diff/200001/ui/aura/window_delegate...
ui/aura/window_delegate.h:105: virtual void OnLinkDisambiguationPopupRequested(
On 2014/07/22 01:04:26, jam wrote:
> nit: every method in this class is documented...
This code is deprecated.

https://codereview.chromium.org/351683002/diff/200001/ui/aura/window_delegate...
ui/aura/window_delegate.h:105: virtual void OnLinkDisambiguationPopupRequested(
On 2014/07/22 04:17:23, sky wrote:
> You should not need to change WindowDelegate to accomplish what you're after.
In
> fact you shouldn't need to change aura or views at all. John suggested a way
to
> get at a delegate that would be better to add this function too.

Done.

[jam, 2014-07-30 20:29:03]

lgtm for content

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

https://codereview.chromium.org/351683002/diff/380001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:993: void
WebContentsViewAura::ProcessLinkDisambiguationGesture(
this should be behind the ifdef as well?

https://codereview.chromium.org/351683002/diff/380001/content/public/browser/...
File content/public/browser/web_contents_view_delegate.h (right):

https://codereview.chromium.org/351683002/diff/380001/content/public/browser/...
content/public/browser/web_contents_view_delegate.h:61: const GestureCallback
callback) = 0;
const reference

[sky, 2014-07-30 21:50:37]

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
File chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc
(right):

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:43:
gfx::Rect target_rect_;
const

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:44: };
DISALLOW...

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:58:
scale_ = static_cast<float>(zoomed_skia_image->width()) /
set in member initializer list.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:122:
LinkDisambiguationPopup::LinkDisambiguationPopup() : view_(NULL) {
initialize content_.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:134:
zoomed_image_ = gfx::Image::CreateFrom1xBitmap(zoomed_bitmap);
You shouldn't need to cache this. Instead make ZoomedBubbleView take a const
ImageSkia&. This is ok as ImageSkia supports copy semantics.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:144:
base::Unretained(this)),
view_ may live longer than this. Also, I think using a callback here is a bit
overkill since ZoomBubbleView is an inner class already. Seems fine to have it
get a reference to the Popup.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:155:
int bounds_x = std::max(0, target_screen.x() - (zoomed_image_.Width() / 2));
Constrain to the work area, which is not necessarily 0 based.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:160:
view_->GetWidget()->SetCapture(view_);
Why do you need the capture here?

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
File chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h
(right):

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h:23:
explicit LinkDisambiguationPopup();
nit: no explicit

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h:33:
const gfx::Rect& target_rect,
Might target_rect change while the popup is up? What if the page scrolls?

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h:35:
content::WebContentsViewDelegate::GestureCallback callback);
const &?

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h:49: };
DISALLOW_...

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
File
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.h
(right):

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.h:53:
#if defined(OS_WIN)
Why is this windows specific?

https://codereview.chromium.org/351683002/diff/380001/ui/events/gesture_event...
File ui/events/gesture_event_details.h (right):

https://codereview.chromium.org/351683002/diff/380001/ui/events/gesture_event...
ui/events/gesture_event_details.h:20: GestureEventDetails(const
GestureEventDetails& details);
Why do you need this? Don't you get it for free?

[luken, 2014-07-31 01:41:48]

PTAL, thanks. jam@, I had to add a method to hide the window, you might want to
take another quick pass through content/

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
File chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc
(right):

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:43:
gfx::Rect target_rect_;
On 2014/07/30 21:50:36, sky wrote:
> const

Done.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:44: };
On 2014/07/30 21:50:36, sky wrote:
> DISALLOW...

Done.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:58:
scale_ = static_cast<float>(zoomed_skia_image->width()) /
On 2014/07/30 21:50:36, sky wrote:
> set in member initializer list.

Done.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:122:
LinkDisambiguationPopup::LinkDisambiguationPopup() : view_(NULL) {
On 2014/07/30 21:50:36, sky wrote:
> initialize content_.

Done.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:134:
zoomed_image_ = gfx::Image::CreateFrom1xBitmap(zoomed_bitmap);
On 2014/07/30 21:50:36, sky wrote:
> You shouldn't need to cache this. Instead make ZoomedBubbleView take a const
> ImageSkia&. This is ok as ImageSkia supports copy semantics.
I stopped caching it, but ToImageSkia() returns a weak ImageSkia*, so I left
ZoomedBubbleView taking that weak pointer instead of const ref.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:144:
base::Unretained(this)),
On 2014/07/30 21:50:36, sky wrote:
> view_ may live longer than this. Also, I think using a callback here is a bit
> overkill since ZoomBubbleView is an inner class already. Seems fine to have it
> get a reference to the Popup.

Done.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:155:
int bounds_x = std::max(0, target_screen.x() - (zoomed_image_.Width() / 2));
On 2014/07/30 21:50:36, sky wrote:
> Constrain to the work area, which is not necessarily 0 based.

Done.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:160:
view_->GetWidget()->SetCapture(view_);
On 2014/07/30 21:50:36, sky wrote:
> Why do you need the capture here?

I don't. Removed.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
File chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h
(right):

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h:23:
explicit LinkDisambiguationPopup();
On 2014/07/30 21:50:36, sky wrote:
> nit: no explicit

Done.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h:33:
const gfx::Rect& target_rect,
On 2014/07/30 21:50:37, sky wrote:
> Might target_rect change while the popup is up? What if the page scrolls?
You are correct, good catch, |target_rect| does change when the page scrolls.
However if the page scrolls I think the popup should just close, similar to how
Context Menus work. This is also how the gesture scrolling works, in that if the
user touches the underlying page in order to scroll it the popup is dismissed.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h:35:
content::WebContentsViewDelegate::GestureCallback callback);
On 2014/07/30 21:50:37, sky wrote:
> const &?

Done.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h:49: };
On 2014/07/30 21:50:36, sky wrote:
> DISALLOW_...

Done.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
File
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.h
(right):

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.h:53:
#if defined(OS_WIN)
On 2014/07/30 21:50:37, sky wrote:
> Why is this windows specific?

I had to make the function windows-specific in WebContentsViewDelegate, as there
are several implementations of that interface (for example in Android:
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/na...)
that I didn't want to have to clutter up the CL with empty definitions of.

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

https://codereview.chromium.org/351683002/diff/380001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:993: void
WebContentsViewAura::ProcessLinkDisambiguationGesture(
On 2014/07/30 20:29:03, jam wrote:
> this should be behind the ifdef as well?

Done.

https://codereview.chromium.org/351683002/diff/380001/content/public/browser/...
File content/public/browser/web_contents_view_delegate.h (right):

https://codereview.chromium.org/351683002/diff/380001/content/public/browser/...
content/public/browser/web_contents_view_delegate.h:61: const GestureCallback
callback) = 0;
On 2014/07/30 20:29:03, jam wrote:
> const reference

Done.

https://codereview.chromium.org/351683002/diff/380001/ui/events/gesture_event...
File ui/events/gesture_event_details.h (right):

https://codereview.chromium.org/351683002/diff/380001/ui/events/gesture_event...
ui/events/gesture_event_details.h:20: GestureEventDetails(const
GestureEventDetails& details);
On 2014/07/30 21:50:37, sky wrote:
> Why do you need this? Don't you get it for free?
I'm naturally not very trusting of compilers :). I removed it.

[sky, 2014-07-31 15:37:40]

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
File chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc
(right):

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:134:
zoomed_image_ = gfx::Image::CreateFrom1xBitmap(zoomed_bitmap);
On 2014/07/31 01:41:46, luken wrote:
> On 2014/07/30 21:50:36, sky wrote:
> > You shouldn't need to cache this. Instead make ZoomedBubbleView take a const
> > ImageSkia&. This is ok as ImageSkia supports copy semantics.
> I stopped caching it, but ToImageSkia() returns a weak ImageSkia*, so I left
> ZoomedBubbleView taking that weak pointer instead of const ref.

You can still deref (assuming non-null) and pass that in. But the pointer is
fine.

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
File chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h
(right):

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h:33:
const gfx::Rect& target_rect,
On 2014/07/31 01:41:47, luken wrote:
> On 2014/07/30 21:50:37, sky wrote:
> > Might target_rect change while the popup is up? What if the page scrolls?
> You are correct, good catch, |target_rect| does change when the page scrolls.
> However if the page scrolls I think the popup should just close, similar to
how
> Context Menus work. This is also how the gesture scrolling works, in that if
the
> user touches the underlying page in order to scroll it the popup is dismissed.

Certainly the popup will close if the users touches on the page, but what if the
page scrolls itself?

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
File
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.h
(right):

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.h:53:
#if defined(OS_WIN)
On 2014/07/31 01:41:47, luken wrote:
> On 2014/07/30 21:50:37, sky wrote:
> > Why is this windows specific?
> 
> I had to make the function windows-specific in WebContentsViewDelegate, as
there
> are several implementations of that interface (for example in Android:
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/na...)
> that I didn't want to have to clutter up the CL with empty definitions of.

That's not a good reason for all the ifdefs. If you want to minimize the scope
land the cl that the function and all the overrides first.

[sky, 2014-07-31 15:38:32]

Also, I think this should be behind a flag and something folks can turn on both
on any views-aura platform. This way we can experiment on both windows and
chromeos/linux.

[luken, 2014-07-31 23:57:42]

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
File
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.h
(right):

https://codereview.chromium.org/351683002/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.h:53:
#if defined(OS_WIN)
On 2014/07/31 15:37:40, sky wrote:
> On 2014/07/31 01:41:47, luken wrote:
> > On 2014/07/30 21:50:37, sky wrote:
> > > Why is this windows specific?
> > 
> > I had to make the function windows-specific in WebContentsViewDelegate, as
> there
> > are several implementations of that interface (for example in Android:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/na...)
> > that I didn't want to have to clutter up the CL with empty definitions of.
> 
> That's not a good reason for all the ifdefs. If you want to minimize the scope
> land the cl that the function and all the overrides first.
OK, I'll start a CL that just lands the function and the overrides first. Just
to be clear, is this the only function addition you object to being behind #if
defined(OS_WIN)? Or are there others?
Thanks

[sky, 2014-08-04 20:05:20]

I went through all the ifdefs. Here's what I think should happen to each.

Additionally this should be down through a flag (default value not enabled) that
can be enabled for chromeos too. If we like this, we can make the default true
where appropriate.

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
File chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc
(right):

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:36:
virtual void OnWidgetDestroying(views::Widget* widget) OVERRIDE;
This is async. I think OnWidgetClosing should work and isn't async. You'll need
to verify that though.

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:83: if
(event->location().x() > bounds().width() ||
Under what circumstances does this happen?

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:99:
event->details().bounding_box().width() / scale_,
Why do you scale the size of the bounding box? Shouldn't that map directly?

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:156:
int bounds_x = std::max(work_area.left(),
AdjustRectToFit?

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
File chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h
(right):

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h:45:
const aura::Window* content_;
Add description.

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
File
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc
(right):

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc:24:
#if defined(OS_WIN)
USE_AURA

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc:179:
link_disambiguation_popup_.reset(new LinkDisambiguationPopup);
Move ifdef inside body and make it USE_AURA.

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
File
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.h
(right):

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.h:53:
#if defined(OS_WIN)
I don't think you should have any ifdefs in this header.

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:185: #if
defined(OS_ANDROID) || defined(OS_WIN)
OS_WIN->TOOLKIT_VIEWS

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:140: #if
defined(OS_ANDROID) || defined(OS_WIN)
OS_WIN->TOOLKIT_VIEWS

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:445: #if
defined(OS_ANDROID) || defined(OS_WIN)
OS_WIN->TOOLKIT_VIEWS

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.h:130: #if
defined(OS_ANDROID) || defined(OS_WIN)
OS_WIN->TOOLKIT_VIEWS

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate_view.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate_view.h:76: #if
defined(OS_WIN)
OS_WIN->TOOLKIT_VIEWS

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1681: #if
defined(OS_ANDROID) || defined(OS_WIN)
OS_WIN->TOOLKIT_VIEWS

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1692:
zoomed_bitmap.setPixels(0);
Is this code necessary?

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

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1228: #if
defined(OS_WIN)
No ifdef here.

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1243:
CopyFromCompositingSurface(
If this is async, how do you need it completes before anything interesting
happens (including this being deleted)?

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

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:226: #if
defined(OS_WIN)
No ifdef here.

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:327: #if
defined(OS_ANDROID) || defined(OS_WIN)
OS_WIN->TOOLKIT_VIEWS

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

https://codereview.chromium.org/351683002/diff/420001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:993: #if defined(OS_WIN)
No ifdefs here.

https://codereview.chromium.org/351683002/diff/420001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1292: #if defined(OS_WIN)
No ifdefs here.

https://codereview.chromium.org/351683002/diff/420001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.h:105: #if defined(OS_WIN)
No ifdefs here.

https://codereview.chromium.org/351683002/diff/420001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.h:143: #if defined(OS_WIN)
No ifdefs here.

https://codereview.chromium.org/351683002/diff/420001/content/public/browser/...
File content/public/browser/web_contents_view_delegate.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/public/browser/...
content/public/browser/web_contents_view_delegate.h:49: #if defined(OS_WIN)
OS_WIN->TOOLKIT_VIEWS

https://codereview.chromium.org/351683002/diff/420001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/351683002/diff/420001/content/renderer/render...
content/renderer/render_view_impl.cc:222: #if defined(OS_ANDROID) ||
defined(OS_WIN)
OS_WIN->TOOLKIT_VIEWS

https://codereview.chromium.org/351683002/diff/420001/content/renderer/render...
content/renderer/render_view_impl.cc:4133: #if defined(OS_ANDROID) ||
defined(OS_WIN)
OS_WIN->TOOLKIT_VIEWS

https://codereview.chromium.org/351683002/diff/420001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/renderer/render...
content/renderer/render_view_impl.h:412: #if defined(OS_ANDROID) ||
defined(OS_WIN)
OS_WIN->TOOLKIT_VIEWS

https://codereview.chromium.org/351683002/diff/420001/content/shell/browser/s...
File content/shell/browser/shell_web_contents_view_delegate.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/shell/browser/s...
content/shell/browser/shell_web_contents_view_delegate.h:36: virtual void
ShowLinkDisambiguationPopup(const gfx::Rect& target_rect,
OS_WIN->TOOLKIT_VIEWS

https://codereview.chromium.org/351683002/diff/420001/content/test/test_rende...
File content/test/test_render_view_host.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/test/test_rende...
content/test/test_render_view_host.h:142: #if defined(OS_ANDROID) ||
defined(OS_WIN)
OS_WIN->TOOLKIT_VIEWS

[jam, 2014-08-06 21:20:50]

On 2014/07/31 01:41:48, luken wrote:
> PTAL, thanks. jam@, I had to add a method to hide the window, you might want
to
> take another quick pass through content/

slgtm

[luken, 2014-09-11 01:07:28]

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
File chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc
(right):

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:36:
virtual void OnWidgetDestroying(views::Widget* widget) OVERRIDE;
On 2014/08/04 20:05:19, sky wrote:
> This is async. I think OnWidgetClosing should work and isn't async. You'll
need
> to verify that though.

Done.

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:83: if
(event->location().x() > bounds().width() ||
On 2014/08/04 20:05:19, sky wrote:
> Under what circumstances does this happen?
I found with a debugger that the ZoomBubbleView would get GestureEvents for
stuff outside of the bounds of the view. Was I mistaken?

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:99:
event->details().bounding_box().width() / scale_,
On 2014/08/04 20:05:19, sky wrote:
> Why do you scale the size of the bounding box? Shouldn't that map directly?
I found that this worked better than not. The user is trying to pick a link that
with their regular-scale finger resulted in ambiguity. They are now touching a
zoomed version, so the gesture sent back to the original page should be at finer
scale.

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:156:
int bounds_x = std::max(work_area.left(),
On 2014/08/04 20:05:19, sky wrote:
> AdjustRectToFit?

Done.

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
File chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h
(right):

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.h:45:
const aura::Window* content_;
On 2014/08/04 20:05:19, sky wrote:
> Add description.

Done.

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
File
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc
(right):

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc:24:
#if defined(OS_WIN)
On 2014/08/04 20:05:19, sky wrote:
> USE_AURA

Done.

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc:179:
link_disambiguation_popup_.reset(new LinkDisambiguationPopup);
On 2014/08/04 20:05:19, sky wrote:
> Move ifdef inside body and make it USE_AURA.

Done.

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
File
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.h
(right):

https://codereview.chromium.org/351683002/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.h:53:
#if defined(OS_WIN)
On 2014/08/04 20:05:19, sky wrote:
> I don't think you should have any ifdefs in this header.

Done.

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:185: #if
defined(OS_ANDROID) || defined(OS_WIN)
On 2014/08/04 20:05:19, sky wrote:
> OS_WIN->TOOLKIT_VIEWS

Done.

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:140: #if
defined(OS_ANDROID) || defined(OS_WIN)
On 2014/08/04 20:05:19, sky wrote:
> OS_WIN->TOOLKIT_VIEWS

Done.

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:445: #if
defined(OS_ANDROID) || defined(OS_WIN)
On 2014/08/04 20:05:19, sky wrote:
> OS_WIN->TOOLKIT_VIEWS

Done.

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.h:130: #if
defined(OS_ANDROID) || defined(OS_WIN)
On 2014/08/04 20:05:19, sky wrote:
> OS_WIN->TOOLKIT_VIEWS

Done.

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate_view.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate_view.h:76: #if
defined(OS_WIN)
On 2014/08/04 20:05:19, sky wrote:
> OS_WIN->TOOLKIT_VIEWS

Done.

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1681: #if
defined(OS_ANDROID) || defined(OS_WIN)
On 2014/08/04 20:05:19, sky wrote:
> OS_WIN->TOOLKIT_VIEWS

Done.

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1692:
zoomed_bitmap.setPixels(0);
On 2014/08/04 20:05:20, sky wrote:
> Is this code necessary?

This code comes from the Android support, which has to make a copy of the image
to marshal it to Javaland.

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

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1228: #if
defined(OS_WIN)
On 2014/08/04 20:05:20, sky wrote:
> No ifdef here.

Done.

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1243:
CopyFromCompositingSurface(
On 2014/08/04 20:05:20, sky wrote:
> If this is async, how do you need it completes before anything interesting
> happens (including this being deleted)?
I switched over to a weak this pointer. If this view has been deleted there's no
need to show the disambiguation popup.

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

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:226: #if
defined(OS_WIN)
On 2014/08/04 20:05:20, sky wrote:
> No ifdef here.

Done.

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:327: #if
defined(OS_ANDROID) || defined(OS_WIN)
On 2014/08/04 20:05:20, sky wrote:
> OS_WIN->TOOLKIT_VIEWS

Done.

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

https://codereview.chromium.org/351683002/diff/420001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:993: #if defined(OS_WIN)
On 2014/08/04 20:05:20, sky wrote:
> No ifdefs here.

Done.

https://codereview.chromium.org/351683002/diff/420001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1292: #if defined(OS_WIN)
On 2014/08/04 20:05:20, sky wrote:
> No ifdefs here.

Done.

https://codereview.chromium.org/351683002/diff/420001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.h:105: #if defined(OS_WIN)
On 2014/08/04 20:05:20, sky wrote:
> No ifdefs here.

Done.

https://codereview.chromium.org/351683002/diff/420001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.h:143: #if defined(OS_WIN)
On 2014/08/04 20:05:20, sky wrote:
> No ifdefs here.

Done.

https://codereview.chromium.org/351683002/diff/420001/content/public/browser/...
File content/public/browser/web_contents_view_delegate.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/public/browser/...
content/public/browser/web_contents_view_delegate.h:49: #if defined(OS_WIN)
On 2014/08/04 20:05:20, sky wrote:
> OS_WIN->TOOLKIT_VIEWS

Done.

https://codereview.chromium.org/351683002/diff/420001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/351683002/diff/420001/content/renderer/render...
content/renderer/render_view_impl.cc:222: #if defined(OS_ANDROID) ||
defined(OS_WIN)
On 2014/08/04 20:05:20, sky wrote:
> OS_WIN->TOOLKIT_VIEWS

Done.

https://codereview.chromium.org/351683002/diff/420001/content/renderer/render...
content/renderer/render_view_impl.cc:4133: #if defined(OS_ANDROID) ||
defined(OS_WIN)
On 2014/08/04 20:05:20, sky wrote:
> OS_WIN->TOOLKIT_VIEWS

Done.

https://codereview.chromium.org/351683002/diff/420001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/renderer/render...
content/renderer/render_view_impl.h:412: #if defined(OS_ANDROID) ||
defined(OS_WIN)
On 2014/08/04 20:05:20, sky wrote:
> OS_WIN->TOOLKIT_VIEWS

Done.

https://codereview.chromium.org/351683002/diff/420001/content/shell/browser/s...
File content/shell/browser/shell_web_contents_view_delegate.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/shell/browser/s...
content/shell/browser/shell_web_contents_view_delegate.h:36: virtual void
ShowLinkDisambiguationPopup(const gfx::Rect& target_rect,
On 2014/08/04 20:05:20, sky wrote:
> OS_WIN->TOOLKIT_VIEWS

Done.

https://codereview.chromium.org/351683002/diff/420001/content/test/test_rende...
File content/test/test_render_view_host.h (right):

https://codereview.chromium.org/351683002/diff/420001/content/test/test_rende...
content/test/test_render_view_host.h:142: #if defined(OS_ANDROID) ||
defined(OS_WIN)
On 2014/08/04 20:05:20, sky wrote:
> OS_WIN->TOOLKIT_VIEWS

Done.

[sky, 2014-09-11 14:53:44]

https://codereview.chromium.org/351683002/diff/480001/chrome/browser/ui/views...
File chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc
(right):

https://codereview.chromium.org/351683002/diff/480001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:38:
float scale_;
nit: const

https://codereview.chromium.org/351683002/diff/480001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:79:
void LinkDisambiguationPopup::ZoomBubbleView::OnGestureEvent(
What about if the user clicks the mouse instead of a gesture?

https://codereview.chromium.org/351683002/diff/480001/chrome/browser/ui/views...
File
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc
(right):

https://codereview.chromium.org/351683002/diff/480001/chrome/browser/ui/views...
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc:178:
#if defined(USE_AURA)
Last I remember chromeos didn't want this. Did that decision change?

To test whether to show (inside the USE_AURA ifdef) I would add a value to
chrome/browser/defaults that is false for chromeos and true elsewhere. That is,
assuming we don't want this for chromeos.

https://codereview.chromium.org/351683002/diff/480001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/351683002/diff/480001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:1307:
'browser/ui/views/link_disambiguation/link_disambiguation_popup.cc',
Remove if not aura?

https://codereview.chromium.org/351683002/diff/480001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/351683002/diff/480001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1693:
view_->ShowDisambiguationPopup(rect, zoomed_bitmap);
rect->rect_pixels and similarly rename in
RenderWidgetHostView::ShowDisambiguationPopup so that it's clear what the
coordinates are. In fact, is there a reason we can't convert to dips here?

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

https://codereview.chromium.org/351683002/diff/480001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1241:
disambiguation_target_rect_ = gfx::ToEnclosingRect(screen_target_rect_f);
Won't this conversion be losing? Will it matter?

https://codereview.chromium.org/351683002/diff/480001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1251:
base::Bind(&RenderWidgetHostViewAura::DisambiguationPopupRendered,
How do you know by the time this is returned that the page has shifted around?

https://codereview.chromium.org/351683002/diff/480001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1903:
HideDisambiguationPopup();
Why is this in the ifdef? Also, it seems like the popup should take care of this
itself.

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

https://codereview.chromium.org/351683002/diff/480001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1298:
base::Unretained(this)));
Why the unretained here? How do you know the popup outlives the
WebContentsViewAura?

[luken, 2014-09-17 00:00:48]

https://codereview.chromium.org/351683002/diff/480001/chrome/browser/ui/views...
File chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc
(right):

https://codereview.chromium.org/351683002/diff/480001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:38:
float scale_;
On 2014/09/11 14:53:43, sky wrote:
> nit: const

Done.

https://codereview.chromium.org/351683002/diff/480001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:79:
void LinkDisambiguationPopup::ZoomBubbleView::OnGestureEvent(
On 2014/09/11 14:53:43, sky wrote:
> What about if the user clicks the mouse instead of a gesture?

A click outside the popup dismisses the popup. Clicks inside the popup are
ignored. I think a click inside the popup should either be ignored or should
dismiss the popup. What do you think?

https://codereview.chromium.org/351683002/diff/480001/chrome/browser/ui/views...
File
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc
(right):

https://codereview.chromium.org/351683002/diff/480001/chrome/browser/ui/views...
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc:178:
#if defined(USE_AURA)
On 2014/09/11 14:53:43, sky wrote:
> Last I remember chromeos didn't want this. Did that decision change?
> 
> To test whether to show (inside the USE_AURA ifdef) I would add a value to
> chrome/browser/defaults that is false for chromeos and true elsewhere. That
is,
> assuming we don't want this for chromeos.

Done.

https://codereview.chromium.org/351683002/diff/480001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/351683002/diff/480001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:1307:
'browser/ui/views/link_disambiguation/link_disambiguation_popup.cc',
On 2014/09/11 14:53:43, sky wrote:
> Remove if not aura?

It's in the Windows-only section, and the comment says to assume ash/aura/views.
Is there some convention here that I'm not seeing?

https://codereview.chromium.org/351683002/diff/480001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/351683002/diff/480001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1693:
view_->ShowDisambiguationPopup(rect, zoomed_bitmap);
On 2014/09/11 14:53:43, sky wrote:
> rect->rect_pixels and similarly rename in
> RenderWidgetHostView::ShowDisambiguationPopup so that it's clear what the
> coordinates are. In fact, is there a reason we can't convert to dips here?

Renamed. The Android code needs pixels. I just added the comment to help clear
it up.

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

https://codereview.chromium.org/351683002/diff/480001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1241:
disambiguation_target_rect_ = gfx::ToEnclosingRect(screen_target_rect_f);
On 2014/09/11 14:53:43, sky wrote:
> Won't this conversion be losing? Will it matter?

Yes, it loses accuracy. No, it doesn't seem to matter to the quality of the
results.

https://codereview.chromium.org/351683002/diff/480001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1251:
base::Bind(&RenderWidgetHostViewAura::DisambiguationPopupRendered,
On 2014/09/11 14:53:43, sky wrote:
> How do you know by the time this is returned that the page has shifted around?

I've added code to save the last_scroll_offset_ for comparison once the callback
fires. If they are different we don't render the popup.

https://codereview.chromium.org/351683002/diff/480001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1903:
HideDisambiguationPopup();
On 2014/09/11 14:53:43, sky wrote:
> Why is this in the ifdef? Also, it seems like the popup should take care of
this
> itself.

I moved it outside of the ifdef. I wasn't able to figure out a way to determine
from the popup that the user had used the mouse wheel to scroll this window, and
I saw the code above that is dismissing other popups so it seemed like the right
place. Do you have any other ideas?

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

https://codereview.chromium.org/351683002/diff/480001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1298:
base::Unretained(this)));
On 2014/09/11 14:53:43, sky wrote:
> Why the unretained here? How do you know the popup outlives the
> WebContentsViewAura?

Good point, I don't know one will outlive the other. I moved the callback back
to RWHVA where we have a weak this ptr.

[sky, 2014-09-17 14:34:30]

https://codereview.chromium.org/351683002/diff/480001/chrome/browser/ui/views...
File chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc
(right):

https://codereview.chromium.org/351683002/diff/480001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:79:
void LinkDisambiguationPopup::ZoomBubbleView::OnGestureEvent(
On 2014/09/17 00:00:48, luken wrote:
> On 2014/09/11 14:53:43, sky wrote:
> > What about if the user clicks the mouse instead of a gesture?
> 
> A click outside the popup dismisses the popup. Clicks inside the popup are
> ignored. I think a click inside the popup should either be ignored or should
> dismiss the popup. What do you think?

My feeling is that if the user uses the mouse, we should honor it.

https://codereview.chromium.org/351683002/diff/480001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/351683002/diff/480001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:1307:
'browser/ui/views/link_disambiguation/link_disambiguation_popup.cc',
On 2014/09/17 00:00:48, luken wrote:
> On 2014/09/11 14:53:43, sky wrote:
> > Remove if not aura?
> 
> It's in the Windows-only section, and the comment says to assume
ash/aura/views.
> Is there some convention here that I'm not seeing?

My mistake.

[luken, 2014-09-18 22:14:55]

https://codereview.chromium.org/351683002/diff/480001/chrome/browser/ui/views...
File chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc
(right):

https://codereview.chromium.org/351683002/diff/480001/chrome/browser/ui/views...
chrome/browser/ui/views/link_disambiguation/link_disambiguation_popup.cc:79:
void LinkDisambiguationPopup::ZoomBubbleView::OnGestureEvent(
On 2014/09/17 14:34:30, sky wrote:
> On 2014/09/17 00:00:48, luken wrote:
> > On 2014/09/11 14:53:43, sky wrote:
> > > What about if the user clicks the mouse instead of a gesture?
> > 
> > A click outside the popup dismisses the popup. Clicks inside the popup are
> > ignored. I think a click inside the popup should either be ignored or should
> > dismiss the popup. What do you think?
> 
> My feeling is that if the user uses the mouse, we should honor it.

Done.

[sky, 2014-09-19 15:19:23]

Did you make sure there aren't any problems with this in non-metro mode where
touch generates both gestures and mouse events?

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

https://codereview.chromium.org/351683002/diff/480001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1251:
base::Bind(&RenderWidgetHostViewAura::DisambiguationPopupRendered,
On 2014/09/17 00:00:48, luken wrote:
> On 2014/09/11 14:53:43, sky wrote:
> > How do you know by the time this is returned that the page has shifted
around?
> 
> I've added code to save the last_scroll_offset_ for comparison once the
callback
> fires. If they are different we don't render the popup.

Is that really enough? Might the dom or other random things have changed
resulting in things shifting around?

https://codereview.chromium.org/351683002/diff/540001/chrome/browser/defaults.h
File chrome/browser/defaults.h (right):

https://codereview.chromium.org/351683002/diff/540001/chrome/browser/defaults...
chrome/browser/defaults.h:36: #if defined(TOOLKIT_VIEWS)
TOOLKIT_VIEWS->USE_AURA as that is that matches the ifdef you use to look up
this value.

[luken, 2014-09-19 19:27:41]

I've tested this feature on both metro and non-metro mode.

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

https://codereview.chromium.org/351683002/diff/480001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1251:
base::Bind(&RenderWidgetHostViewAura::DisambiguationPopupRendered,
On 2014/09/19 15:19:23, sky wrote:
> On 2014/09/17 00:00:48, luken wrote:
> > On 2014/09/11 14:53:43, sky wrote:
> > > How do you know by the time this is returned that the page has shifted
> around?
> > 
> > I've added code to save the last_scroll_offset_ for comparison once the
> callback
> > fires. If they are different we don't render the popup.
> 
> Is that really enough? Might the dom or other random things have changed
> resulting in things shifting around?

AFAICT we are already doing more than what this feature on Android is doing in
trying to keep these two views in sync. Blink doesn't provide any signals to the
client that we should stop showing the link disambiguation popup. Any activity
by the user outside of the bubble will dismiss it.

It is a good point that there's no step taken to update the image within the
bubble or possibly dismiss it if the DOM or other elements move around on the
content page. I'm open to suggestion here - do you have any ideas about how we
could implement this?

https://codereview.chromium.org/351683002/diff/540001/chrome/browser/defaults.h
File chrome/browser/defaults.h (right):

https://codereview.chromium.org/351683002/diff/540001/chrome/browser/defaults...
chrome/browser/defaults.h:36: #if defined(TOOLKIT_VIEWS)
On 2014/09/19 15:19:23, sky wrote:
> TOOLKIT_VIEWS->USE_AURA as that is that matches the ifdef you use to look up
> this value.

Done.

[sky, 2014-09-22 15:01:30]

LGTM - you can resolve the issue of problems with document mutation separately.
Also, I suspect you'll want to put this behind a flag.

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

https://codereview.chromium.org/351683002/diff/480001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1251:
base::Bind(&RenderWidgetHostViewAura::DisambiguationPopupRendered,
On 2014/09/19 19:27:41, luken wrote:
> On 2014/09/19 15:19:23, sky wrote:
> > On 2014/09/17 00:00:48, luken wrote:
> > > On 2014/09/11 14:53:43, sky wrote:
> > > > How do you know by the time this is returned that the page has shifted
> > around?
> > > 
> > > I've added code to save the last_scroll_offset_ for comparison once the
> > callback
> > > fires. If they are different we don't render the popup.
> > 
> > Is that really enough? Might the dom or other random things have changed
> > resulting in things shifting around?
> 
> AFAICT we are already doing more than what this feature on Android is doing in
> trying to keep these two views in sync. Blink doesn't provide any signals to
the
> client that we should stop showing the link disambiguation popup. Any activity
> by the user outside of the bubble will dismiss it.
> 
> It is a good point that there's no step taken to update the image within the
> bubble or possibly dismiss it if the DOM or other elements move around on the
> content page. I'm open to suggestion here - do you have any ideas about how we
> could implement this?

Have you talked with any of the guys on the Android side to see if this issue
comes up much in practice?

[luken, 2014-09-23 18:32:36]

The CQ bit was checked by luken@chromium.org

[commit-bot: I haz the power, 2014-09-23 20:31:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/351683002/620001

[commit-bot: I haz the power, 2014-09-23 21:11:05]

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

[commit-bot: I haz the power, 2014-09-23 21:11:07]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[luken, 2014-09-24 05:49:31]

The CQ bit was checked by luken@chromium.org

[commit-bot: I haz the power, 2014-09-24 05:50:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/351683002/620001

[commit-bot: I haz the power, 2014-09-24 05:50:46]

Committed patchset #32 (id:620001) as e4edf37ab174f714f097fb6157611c4585dd2ba1

[commit-bot: I haz the power, 2014-09-24 05:51:57]

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

[csaavedra, 2014-09-30 12:18:35]

On 2014/09/24 05:51:57, I haz the power (commit-bot) wrote:
> Patchset 32 (id:??) landed as
> https://crrev.com/b171b53a96f5926e2462985f05450ca9054b7ac3
> Cr-Commit-Position: refs/heads/master@{#296355}

This seems to have broken the content_shell build with embedded=1. In specific,
RenderWidgetHostViewBase::ShowDisambiguationPopup() is #ifdef'ed out.

[luken, 2014-09-30 17:48:40]

On 2014/09/30 12:18:35, csaavedra wrote:
> On 2014/09/24 05:51:57, I haz the power (commit-bot) wrote:
> > Patchset 32 (id:??) landed as
> > https://crrev.com/b171b53a96f5926e2462985f05450ca9054b7ac3
> > Cr-Commit-Position: refs/heads/master@{#296355}
> 
> This seems to have broken the content_shell build with embedded=1. In
specific,
> RenderWidgetHostViewBase::ShowDisambiguationPopup() is #ifdef'ed out.

There's a patch that just landed from
https://codereview.chromium.org/610893003/, does
this fix that?

[csaavedra, 2014-10-01 11:45:51]

On 2014/09/30 17:48:40, luken wrote:
> On 2014/09/30 12:18:35, csaavedra wrote:
> > On 2014/09/24 05:51:57, I haz the power (commit-bot) wrote:
> > > Patchset 32 (id:??) landed as
> > > https://crrev.com/b171b53a96f5926e2462985f05450ca9054b7ac3
> > > Cr-Commit-Position: refs/heads/master@{#296355}
> > 
> > This seems to have broken the content_shell build with embedded=1. In
> specific,
> > RenderWidgetHostViewBase::ShowDisambiguationPopup() is #ifdef'ed out.
> 
> There's a patch that just landed from
> https://codereview.chromium.org/610893003/, does
> this fix that?

Seems to fix it, yes. Thanks!