Disabled pinch zooming in non-browser Aura windows.

Disabled pinch zooming in non-browser Aura windows.

Pinch zoom should not affect native-looking UI. This patch disabled zoom
in all cases except for the browser view.

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

[bokan, 2014-01-10 20:24:30]

Sadrul, could you have a look? I really haven't done anything outside Blink so
this could be the totally wrong way of doing this. Thanks.

[sadrul, 2014-01-11 20:30:20]

On 2014/01/10 20:24:30, bokan wrote:
> Sadrul, could you have a look? I really haven't done anything outside Blink so
> this could be the totally wrong way of doing this. Thanks.

This is not quite right. A better approach would be to introduce
WebContentsDelegate::PreHandleGestureEvent/PreHandlePinchEvent() (like the
existing ::PreHandleKeyboardEvent() etc. in that interface). The function by
default should always consume the pinch event. To turn on pinch on the
browser-pages, you would override it in Browser::PreHandle...Event(), and decide
whether or not to allow the pinch event to be dispatched or not.

[bokan, 2014-01-13 20:34:18]

Thanks Sadrul, that's what I was looking for. PTAL? Is it ok to use the Blink
WebGestureEvent or should I be wrapping it in a Chromium type?

[sadrul, 2014-01-13 21:55:19]

On 2014/01/13 20:34:18, bokan wrote:
> Thanks Sadrul, that's what I was looking for. PTAL? Is it ok to use the Blink
> WebGestureEvent or should I be wrapping it in a Chromium type?

We use Web*Events in content/ and chrome/ code. So this is OK.

LGTM. For owner review, I would suggest jam@

https://codereview.chromium.org/134683004/diff/100001/content/public/browser/...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/134683004/diff/100001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:117: // By default, swallow
pinch events
This is only on desktop, right? So check !defined(OS_ANDROID)?

[bokan, 2014-01-13 22:45:08]

https://codereview.chromium.org/134683004/diff/100001/content/public/browser/...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/134683004/diff/100001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:117: // By default, swallow
pinch events
On 2014/01/13 21:55:19, sadrul wrote:
> This is only on desktop, right? So check !defined(OS_ANDROID)?

Yup. Added. I'm assuming Android doesn't use browser/ui/browser.cc and has it's
own implementation of WebContentsDelegate?

[bokan, 2014-01-13 22:48:38]

+jam@ for OWNER

[jam, 2014-01-14 17:02:44]

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:118: // On Desktop, swallow
pinch events by default
why is this logic in this interface? will every content embedder want this as
the default?

i'm wondering why this method doesn't just return false always, and places in
chrome that want to disable gesture events can override it and return true.

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
content/public/browser/web_contents_delegate.h:275: virtual bool
PreHandleGestureEvent(WebContents* source,
nit: add comment

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
content/public/browser/web_contents_delegate.h:276: const
blink::WebGestureEvent& event);
nit: indentation

[bokan, 2014-01-14 18:49:04]

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:118: // On Desktop, swallow
pinch events by default
On 2014/01/14 17:02:45, jam wrote:
> why is this logic in this interface? will every content embedder want this as
> the default?
> 
> i'm wondering why this method doesn't just return false always, and places in
> chrome that want to disable gesture events can override it and return true.

The idea is to disable pinch in all cases except when the content is in the
browser. Native-looking UI shouldn't respond to pinch gestures. From that point
of view I think having the embedder explicitly opt-in to pinches is cleaner, but
I'm happy to change it if you disagree.

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
content/public/browser/web_contents_delegate.h:275: virtual bool
PreHandleGestureEvent(WebContents* source,
On 2014/01/14 17:02:45, jam wrote:
> nit: add comment

Done.

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
content/public/browser/web_contents_delegate.h:276: const
blink::WebGestureEvent& event);
On 2014/01/14 17:02:45, jam wrote:
> nit: indentation

Done.

[jam, 2014-01-16 16:42:44]

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:118: // On Desktop, swallow
pinch events by default
On 2014/01/14 18:49:05, bokan wrote:
> On 2014/01/14 17:02:45, jam wrote:
> > why is this logic in this interface? will every content embedder want this
as
> > the default?
> > 
> > i'm wondering why this method doesn't just return false always, and places
in
> > chrome that want to disable gesture events can override it and return true.
> 
> The idea is to disable pinch in all cases except when the content is in the
> browser. Native-looking UI shouldn't respond to pinch gestures.

Native looking UI shouldn't be using WebContents, but instead should be using
views etc. Which UI are you talking about?

> From that point
> of view I think having the embedder explicitly opt-in to pinches is cleaner,
but
> I'm happy to change it if you disagree.

[bokan, 2014-01-16 16:49:19]

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:118: // On Desktop, swallow
pinch events by default
On 2014/01/16 16:42:44, jam wrote:
> On 2014/01/14 18:49:05, bokan wrote:
> > On 2014/01/14 17:02:45, jam wrote:
> 
> Native looking UI shouldn't be using WebContents, but instead should be using
> views etc. Which UI are you talking about?
> 

OOBE pages like the login screen, create new user, etc. We also want it disabled
in packaged apps. Sorry, I haven't done much work in Chromium so I'm still
getting the terminology down.

[jam, 2014-01-17 05:49:51]

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:118: // On Desktop, swallow
pinch events by default
On 2014/01/16 16:49:20, bokan wrote:
> On 2014/01/16 16:42:44, jam wrote:
> > On 2014/01/14 18:49:05, bokan wrote:
> > > On 2014/01/14 17:02:45, jam wrote:
> > 
> > Native looking UI shouldn't be using WebContents, but instead should be
using
> > views etc. Which UI are you talking about?
> > 
> 
> OOBE pages like the login screen, create new user, etc. We also want it
disabled
> in packaged apps. Sorry, I haven't done much work in Chromium so I'm still
> getting the terminology down.

I see. So why not just have those places override the method and return whatever
value they need? basically we shouldn't put that logic spread across the Browser
callback and here.

[bokan, 2014-01-20 19:05:11]

On 2014/01/17 05:49:51, jam wrote:
>
https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
> File content/public/browser/web_contents_delegate.cc (right):
> 
>
https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
> content/public/browser/web_contents_delegate.cc:118: // On Desktop, swallow
> pinch events by default
> On 2014/01/16 16:49:20, bokan wrote:
> > On 2014/01/16 16:42:44, jam wrote:
> > > On 2014/01/14 18:49:05, bokan wrote:
> > > > On 2014/01/14 17:02:45, jam wrote:
> > > 
> > > Native looking UI shouldn't be using WebContents, but instead should be
> using
> > > views etc. Which UI are you talking about?
> > > 
> > 
> > OOBE pages like the login screen, create new user, etc. We also want it
> disabled
> > in packaged apps. Sorry, I haven't done much work in Chromium so I'm still
> > getting the terminology down.
> 
> I see. So why not just have those places override the method and return
whatever
> value they need? basically we shouldn't put that logic spread across the
Browser
> callback and here.

PTAL, I've made the pre-handler return true by default (i.e. pass all events)
and overridden the subclasses to block pinch events where necessary.

[bokan, 2014-01-22 20:07:09]

On 2014/01/17 05:49:51, jam wrote:
>
https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
> File content/public/browser/web_contents_delegate.cc (right):
> 
>
https://codereview.chromium.org/134683004/diff/230001/content/public/browser/...
> content/public/browser/web_contents_delegate.cc:118: // On Desktop, swallow
> pinch events by default
> On 2014/01/16 16:49:20, bokan wrote:
> > On 2014/01/16 16:42:44, jam wrote:
> > > On 2014/01/14 18:49:05, bokan wrote:
> > > > On 2014/01/14 17:02:45, jam wrote:
> > > 
> > > Native looking UI shouldn't be using WebContents, but instead should be
> using
> > > views etc. Which UI are you talking about?
> > > 
> > 
> > OOBE pages like the login screen, create new user, etc. We also want it
> disabled
> > in packaged apps. Sorry, I haven't done much work in Chromium so I'm still
> > getting the terminology down.
> 
> I see. So why not just have those places override the method and return
whatever
> value they need? basically we shouldn't put that logic spread across the
Browser
> callback and here.

jam@, PTAL?

[jam, 2014-01-23 01:34:47]

lgtm

https://codereview.chromium.org/134683004/diff/400001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/134683004/diff/400001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:187: #include
"third_party/WebKit/public/web/WebInputEvent.h"
this isn't needed?

https://codereview.chromium.org/134683004/diff/400001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/134683004/diff/400001/content/public/browser/...
content/public/browser/web_contents_delegate.h:60: class WebGestureEvent;
nit: class then struct

https://codereview.chromium.org/134683004/diff/400001/ui/keyboard/DEPS
File ui/keyboard/DEPS (right):

https://codereview.chromium.org/134683004/diff/400001/ui/keyboard/DEPS#newcode5
ui/keyboard/DEPS:5: "+third_party/WebKit",
ditto

https://codereview.chromium.org/134683004/diff/400001/ui/views/DEPS
File ui/views/DEPS (right):

https://codereview.chromium.org/134683004/diff/400001/ui/views/DEPS#newcode7
ui/views/DEPS:7: "+third_party/WebKit",
ditto

https://codereview.chromium.org/134683004/diff/400001/ui/web_dialogs/DEPS
File ui/web_dialogs/DEPS (right):

https://codereview.chromium.org/134683004/diff/400001/ui/web_dialogs/DEPS#new...
ui/web_dialogs/DEPS:3: "+third_party/WebKit",
that's too granular. just make it third_party/WebKit/public/web/WebInputEvent.h

https://codereview.chromium.org/134683004/diff/400001/ui/web_dialogs/web_dial...
File ui/web_dialogs/web_dialog_web_contents_delegate.h (right):

https://codereview.chromium.org/134683004/diff/400001/ui/web_dialogs/web_dial...
ui/web_dialogs/web_dialog_web_contents_delegate.h:68: 
nit: no blank line

[bokan, 2014-01-24 16:16:23]

+scheib@ for src/apps
+sky@ for src/ui/web_dialogs

[bokan, 2014-01-24 16:17:02]

+scheib@ for src/apps
+sky@ for src/ui/web_dialogs

[scheib, 2014-01-24 17:48:58]

I'm not aware of if we pass gestures through to web content, or if they are only
ever used by the browser to zoom/scroll. Do we? If so, will this patch block
those gestures from being usable from packaged apps?

This should be more directly addressed in the comments at
content/public/browser/web_contents_delegate.h

[sky, 2014-01-24 18:29:40]

ui/web_dialogs LGTM

[bokan, 2014-01-24 19:20:51]

On 2014/01/24 17:48:58, scheib wrote:
> I'm not aware of if we pass gestures through to web content, or if they are
only
> ever used by the browser to zoom/scroll. Do we? If so, will this patch block
> those gestures from being usable from packaged apps?
> 
> This should be more directly addressed in the comments at
> content/public/browser/web_contents_delegate.h

Gestures are passed to any web contents objects, not just the browser, so this
patch will block (only) pinch-zoom in packaged apps (by design). Pinch gestures
are dropped in the various subclasses of WebContentsDelegate. The
WebContentsDelegate::PreHandleGestureEvent method just provides a hook to
pre-handle a gesture, it doesn't actually block anything by default so I'm not
sure what you'd like me to add in those comments. Perhaps I misunderstood your
statement?

[scheib, 2014-01-24 20:44:16]

lgtm, with some comments added from bokan to address confused people like me.

Posterity: we discussed offline my confusion around open web content access to
touch gestures, and that this patch will only filter out browser zoom control
from these views. 

On 2014/01/24 19:20:51, bokan wrote:
> On 2014/01/24 17:48:58, scheib wrote:
> > I'm not aware of if we pass gestures through to web content, or if they are
> only
> > ever used by the browser to zoom/scroll. Do we? If so, will this patch block
> > those gestures from being usable from packaged apps?
> > 
> > This should be more directly addressed in the comments at
> > content/public/browser/web_contents_delegate.h
> 
> Gestures are passed to any web contents objects, not just the browser, so this
> patch will block (only) pinch-zoom in packaged apps (by design). Pinch
gestures
> are dropped in the various subclasses of WebContentsDelegate. The
> WebContentsDelegate::PreHandleGestureEvent method just provides a hook to
> pre-handle a gesture, it doesn't actually block anything by default so I'm not
> sure what you'd like me to add in those comments. Perhaps I misunderstood your
> statement?

[commit-bot: I haz the power, 2014-01-27 15:41:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/134683004/610001

[commit-bot: I haz the power, 2014-01-27 16:06:20]

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

[bokan, 2014-01-27 16:12:55]

+abarth OWNERE needed for new added dependency on Blink's WebInputEvent in DEPS
files

[abarth-chromium, 2014-01-27 18:12:55]

jamesr is a better person to review Chromium dependencies on Blink.

[jamesr, 2014-01-27 19:18:12]

https://codereview.chromium.org/134683004/diff/610001/ui/keyboard/DEPS
File ui/keyboard/DEPS (right):

https://codereview.chromium.org/134683004/diff/610001/ui/keyboard/DEPS#newcode5
ui/keyboard/DEPS:5: "+third_party/WebKit/public/web/WebInputEvent.h",
no, ui/ should not be depending on blink API types.

[jamesr, 2014-01-27 19:24:41]

Hmm, although content/public/browser/native_web_keyboard_event.h #includes the
WebInputEvent.h header, and these files are already depending on that.

For ui/keyboard and ui/web_dialogs/ this seems OK, but for ui/views/ this is
adding another effectively content/ dependency.  Given that we don't appear to
have one currently this seems unfortunate.

[sadrul, 2014-01-27 19:25:59]

https://codereview.chromium.org/134683004/diff/610001/ui/views/DEPS
File ui/views/DEPS (right):

https://codereview.chromium.org/134683004/diff/610001/ui/views/DEPS#newcode7
ui/views/DEPS:7: "+third_party/WebKit/public/web/WebInputEvent.h",
This should be in ui/views/controls/webview/DEPS instead

[jamesr, 2014-01-27 19:27:18]

Why can't the ui code handle the gesture event before it's passed in to content/
to be translated to a WebInputEvent?

[bokan, 2014-01-28 22:55:14]

On 2014/01/27 19:24:41, jamesr wrote:
> Hmm, although content/public/browser/native_web_keyboard_event.h #includes the
> WebInputEvent.h header, and these files are already depending on that.
> 
> For ui/keyboard and ui/web_dialogs/ this seems OK, but for ui/views/ this is
> adding another effectively content/ dependency.  Given that we don't appear to
> have one currently this seems unfortunate.

On closer examination, I've removed the webview addition (I had the wrong
WebView) and keyboard blocks pinches implicitly by calling preventDefault on
touches in the content so this was unneeded.

For the web_dialogs case, I tried to handle the gesture in the ui layer but I
couldn't find a simple way to do it. There doesn't seem to be an EventTarget
specific to WebDialogs so I'm not sure what the right way to do this would be as
I'm not well versed with the Aura/ui code. 

If this dependency is ok, do you want me to remove the new line in DEPS and use
native_web_keyboard_event.h?

[jamesr, 2014-01-28 22:56:21]

The DEPS entry in ui/web_dialogs/ lgtm - if you depend on the public content API
you also depend on the blink public API - but I'm not sure that this is the
right way to route things.

[commit-bot: I haz the power, 2014-01-28 23:57:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/134683004/760001

[commit-bot: I haz the power, 2014-01-29 04:20:45]

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

[commit-bot: I haz the power, 2014-01-29 15:54:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/134683004/780001

[commit-bot: I haz the power, 2014-01-29 16:43:54]

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

[commit-bot: I haz the power, 2014-01-29 22:42:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/134683004/780001

[commit-bot: I haz the power, 2014-01-30 05:34:57]

Change committed as 247843