Disable overscroll on Windows machines w/o touchscreen

Disable overscroll on Windows machines w/o touchscreen.

Currently GestureNav/horizontal overscroll is enabled on all Aura builds. However to engage GestureNav you either need a touch screen or a 2-finger gesture support for the touchpad. This means we are paying the cost of screenshotting for every navigation on some machines where you won't ever be able to experience GestureNav.

BUG=398516
Committed: https://crrev.com/f09a66bb347776e5e2b5181e5f8550b7d31c0fe6
Cr-Commit-Position: refs/heads/master@{#297771}

[mfomitchev, 2014-09-29 20:33:07]

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

[mfomitchev, 2014-09-29 20:33:07]

Sadrul, WDYT about this approach?
From what I understand, it is possible to enable horizontal overscroll via
touchpad on Windows and Unix by using custom drivers and/or doing some special
configurations. So I thought for the sake of those power-users who go through
this effort (and who can be pretty vocal about things they don't like), it would
be nice if we didn't disable gesture-nav completely, but only disabled
screenshot taking.

[mfomitchev, 2014-09-29 20:33:50]

*by "horizontal overscroll" I meant "horizontal scroll"

[sadrul, 2014-09-29 20:39:24]

https://codereview.chromium.org/617543002/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/617543002/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:462: if
(controller_->ShouldTakeScreenshotOnNavigation()) {
Can we change the Browser::CanOverscrollContent() override instead?

[mfomitchev, 2014-09-29 20:41:39]

https://codereview.chromium.org/617543002/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/617543002/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:462: if
(controller_->ShouldTakeScreenshotOnNavigation()) {
On 2014/09/29 20:39:24, sadrul wrote:
> Can we change the Browser::CanOverscrollContent() override instead?

This was my first intention, but then I thought it might be better to leave the
overscroll gesture enabled and just disable the screenshot taking - please see
my first comment on the review.

[sadrul, 2014-09-29 20:45:36]

On 2014/09/29 20:41:39, mfomitchev wrote:
>
https://codereview.chromium.org/617543002/diff/1/content/browser/frame_host/n...
> File content/browser/frame_host/navigator_impl.cc (right):
> 
>
https://codereview.chromium.org/617543002/diff/1/content/browser/frame_host/n...
> content/browser/frame_host/navigator_impl.cc:462: if
> (controller_->ShouldTakeScreenshotOnNavigation()) {
> On 2014/09/29 20:39:24, sadrul wrote:
> > Can we change the Browser::CanOverscrollContent() override instead?
> 
> This was my first intention, but then I thought it might be better to leave
the
> overscroll gesture enabled and just disable the screenshot taking - please see
> my first comment on the review.

I'd rather we disable/enable both gesture-nav+screenshot at the same time, and
wait for feedback from dev/beta channels before changing this. (and if we do
have gesture-nav without screenshot, we should really switch to the simple UI
instead).

[mfomitchev, 2014-09-29 20:58:25]

On 2014/09/29 20:45:36, sadrul wrote:
> On 2014/09/29 20:41:39, mfomitchev wrote:
> >
>
https://codereview.chromium.org/617543002/diff/1/content/browser/frame_host/n...
> > File content/browser/frame_host/navigator_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/617543002/diff/1/content/browser/frame_host/n...
> > content/browser/frame_host/navigator_impl.cc:462: if
> > (controller_->ShouldTakeScreenshotOnNavigation()) {
> > On 2014/09/29 20:39:24, sadrul wrote:
> > > Can we change the Browser::CanOverscrollContent() override instead?
> > 
> > This was my first intention, but then I thought it might be better to leave
> the
> > overscroll gesture enabled and just disable the screenshot taking - please
see
> > my first comment on the review.
> 
> I'd rather we disable/enable both gesture-nav+screenshot at the same time, and
> wait for feedback from dev/beta channels before changing this. (and if we do
> have gesture-nav without screenshot, we should really switch to the simple UI
> instead).

Hmm... so what if we set --overscroll-history-navigation to 2 by default on
Windows and Unix instead of modifying CanOverscrollContent() or adding
ShouldTakeScreenshotOnNavigation()?

[mfomitchev, 2014-09-30 15:57:35]

Ok, setting --overscroll-history-navigation to 2 is probably not a good idea
since it would affect machines with touch screen as well. Plus we'd be adding
more complexity. I guess just disabling it altogether is okay. I decided to do
it on Windows only, since it seems like it may be easier to enable touchpad
horizontal scrolling on Unix machines.

Sadrul, can you PTAL?

[sadrul, 2014-09-30 16:32:15]

https://codereview.chromium.org/617543002/diff/40001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/617543002/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:194: #include "ui/base/touch/touch_device.h"
Add this include below for OS_WIN in line ~200?

https://codereview.chromium.org/617543002/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:1137: bool allowOverscroll =
ui::IsTouchDevicePresent();
allow_overscroll

Also, AURA is always turned on for Windows, so just

#if defined(OS_WIN)
..
#elif defined(USE_AURA)
..
#else
..
#endif

or

#if defined(USE_AURA)
#if defined(OS_WIN)
...
#else
...
#endif
...
#endif

[mfomitchev, 2014-09-30 18:30:33]

https://codereview.chromium.org/617543002/diff/40001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/617543002/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:194: #include "ui/base/touch/touch_device.h"
On 2014/09/30 16:32:15, sadrul wrote:
> Add this include below for OS_WIN in line ~200?

Done.

https://codereview.chromium.org/617543002/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:1137: bool allowOverscroll =
ui::IsTouchDevicePresent();
On 2014/09/30 16:32:15, sadrul wrote:
> allow_overscroll
> 
> Also, AURA is always turned on for Windows, so just
> 
> #if defined(OS_WIN)
> ..
> #elif defined(USE_AURA)
> ..
> #else
> ..
> #endif
> 
> or
> 
> #if defined(USE_AURA)
> #if defined(OS_WIN)
> ...
> #else
> ...
> #endif
> ...
> #endif

Done.

[sadrul, 2014-09-30 20:59:38]

LGTM

[mfomitchev, 2014-09-30 21:00:24]

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

[mfomitchev, 2014-09-30 21:00:25]

+sky@chromium.org for owner review

[sky, 2014-09-30 21:46:05]

https://codereview.chromium.org/617543002/diff/60001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/617543002/diff/60001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:1131: #if defined(USE_AURA)
Oy, this is hard to read with all the ifdefs. How about a
IsOverscrollEnabledOnPlatform? You could even put the win implementation in
browser_win.

[mfomitchev, 2014-09-30 22:32:30]

https://codereview.chromium.org/617543002/diff/60001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/617543002/diff/60001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:1131: #if defined(USE_AURA)
On 2014/09/30 21:46:05, sky wrote:
> Oy, this is hard to read with all the ifdefs. How about a
> IsOverscrollEnabledOnPlatform? You could even put the win implementation in
> browser_win.

Hmm.. how can I put the win implementation into browser_win if I also have a
more generic implementation in browser.cc? There's no separate BrowserWin
class... I guess I could have two separate implementations for aura and non-aura
in browser.cc (rather than one implementation for both) - is this what you want?
I'd still need an ifdef for OS_WIN in the aura implementation, since Windows
uses Aura.

[sky, 2014-09-30 23:54:22]

On 2014/09/30 22:32:30, mfomitchev wrote:
>
https://codereview.chromium.org/617543002/diff/60001/chrome/browser/ui/browse...
> File chrome/browser/ui/browser.cc (right):
> 
>
https://codereview.chromium.org/617543002/diff/60001/chrome/browser/ui/browse...
> chrome/browser/ui/browser.cc:1131: #if defined(USE_AURA)
> On 2014/09/30 21:46:05, sky wrote:
> > Oy, this is hard to read with all the ifdefs. How about a
> > IsOverscrollEnabledOnPlatform? You could even put the win implementation in
> > browser_win.
> 
> Hmm.. how can I put the win implementation into browser_win if I also have a
> more generic implementation in browser.cc? There's no separate BrowserWin
> class... I guess I could have two separate implementations for aura and
non-aura
> in browser.cc (rather than one implementation for both) - is this what you
want?
> I'd still need an ifdef for OS_WIN in the aura implementation, since Windows
> uses Aura.

browser_win is the windows specific things of Browser. I'm suggesting you
declare a function in browser.h, which an implementation in browser_win for the
windows specific and browser.cc (wrapped in an ifdef) for the non-win parts.

[mfomitchev, 2014-10-01 15:18:07]

Yikes, browser.h is already almost 1000 lines. I'd rather not make it even
bigger... also this code wouldn't depend on any of the Browser state. Perhaps
there is some other place we could put this in? If not, maybe create
browser_util? FormatTitleForDisplay(base::string16* title) could be moved there
as well...

Alternatively, I could rewrite the ifdefs like this:
#if ()
...
#elif
...
#else
...
#endif
I.e. w/o using nested #if.

[sky, 2014-10-01 15:45:43]

I'm fine with browser_util.

On Wed, Oct 1, 2014 at 8:18 AM,  <mfomitchev@chromium.org> wrote:
> Yikes, browser.h is already almost 1000 lines. I'd rather not make it even
> bigger... also this code wouldn't depend on any of the Browser state.
> Perhaps
> there is some other place we could put this in? If not, maybe create
> browser_util? FormatTitleForDisplay(base::string16* title) could be moved
> there
> as well...
>
> Alternatively, I could rewrite the ifdefs like this:
> #if ()
> ...
> #elif
> ...
> #else
> ...
> #endif
> I.e. w/o using nested #if.
>
> https://codereview.chromium.org/617543002/

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

[mfomitchev, 2014-10-01 18:24:09]

On 2014/10/01 15:45:43, sky wrote:
> I'm fine with browser_util.
> 
> On Wed, Oct 1, 2014 at 8:18 AM,  <mailto:mfomitchev@chromium.org> wrote:
> > Yikes, browser.h is already almost 1000 lines. I'd rather not make it even
> > bigger... also this code wouldn't depend on any of the Browser state.
> > Perhaps
> > there is some other place we could put this in? If not, maybe create
> > browser_util? FormatTitleForDisplay(base::string16* title) could be moved
> > there
> > as well...
> >
> > Alternatively, I could rewrite the ifdefs like this:
> > #if ()
> > ...
> > #elif
> > ...
> > #else
> > ...
> > #endif
> > I.e. w/o using nested #if.
> >
> > https://codereview.chromium.org/617543002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Done, PTAL

[sky, 2014-10-01 20:05:04]

https://codereview.chromium.org/617543002/diff/90001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_util.h (right):

https://codereview.chromium.org/617543002/diff/90001/chrome/browser/ui/browse...
chrome/browser/ui/browser_util.h:12: class BrowserUtil {
Sorry. I had thought there was already a browser_util. I'm not a fan of a class
named BrowserUtil. It's begging to become a dumping ground for all sorts of
random unrelated functions. I would rather have more focused classes (or
standalone functions, a class for these functions isn't particularly helpful).

I actually prefer what you had before over this.

[mfomitchev, 2014-10-01 20:10:57]

Ok, how about this - simplified ifdefs?

[sky, 2014-10-01 23:26:08]

BINGO. LGTM

[mfomitchev, 2014-10-02 00:34:37]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-10-02 00:35:41]

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

[commit-bot: I haz the power, 2014-10-02 02:21:01]

Committed patchset #7 (id:110001) as 338ed842b7659699d7bac28c6ea167c0ba2884d9

[commit-bot: I haz the power, 2014-10-02 02:22:07]

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