[Android] Add a flag to disable the pull-to-refresh effect

[Android] Add a flag to disable the pull-to-refresh effect

Some people may not like pull-to-refresh. Some people may not like our
implementation of pull-to-reresh. Some people may not like refreshing
at all. Let's accomodate such users with a disabling override,
particularly now as we're experimenting with the initial version of the
effect.

The flag also allows for experiments gauging accidental activation and
other potentially negative side-effects.

Also remove the kEnableOverscrollNotifications flag; overscroll
notifications are now bundled with event acks, creating
minimal extra overhead for non-consuming platforms.

BUG=428429
Committed: https://crrev.com/0fa51299fbcd835fd5e8860f28f65b57a149598e
Cr-Commit-Position: refs/heads/master@{#305154}

[jdduke (slow), 2014-11-18 22:57:48]

jdduke@chromium.org changed reviewers:
+ aelias@chromium.org

[jdduke (slow), 2014-11-18 22:57:49]

aelias@: Could you take a look? Sorry for the churn here, I should have included
a flag in the initial patch...

https://codereview.chromium.org/733293002/diff/1/content/browser/android/over...
File content/browser/android/overscroll_controller_android.h (right):

https://codereview.chromium.org/733293002/diff/1/content/browser/android/over...
content/browser/android/overscroll_controller_android.h:83:
scoped_ptr<OverscrollGlow> glow_effect_;
This is kind of unfortunate, but oh well.

https://codereview.chromium.org/733293002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/733293002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:1551: if
(!overscroll_controller_ &&
I considered moving |overscroll_controller_| to ContentViewCoreImpl, or making
it a value member, but both cases have subtle complications...

[aelias_OOO_until_Jul13, 2014-11-19 06:21:51]

https://codereview.chromium.org/733293002/diff/1/content/browser/android/over...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/733293002/diff/1/content/browser/android/over...
content/browser/android/overscroll_controller_android.cc:42: static bool
sEnableGlowEffect =
Please remove these statics, static global state often comes back to bite in
unexpected ways.  I'd rather just pay the string lookup cost every time.

https://codereview.chromium.org/733293002/diff/1/content/browser/android/over...
content/browser/android/overscroll_controller_android.cc:81: enabled_ =
glow_effect_ || refresh_effect_;
This complicates the meaning of "enabled_" by baking into it information we
already have from the other members anyway.  I'd rather it just be allowed to be
"true" even if glow_effect_ and refresh_effect_ are both null.

https://codereview.chromium.org/733293002/diff/1/content/browser/android/over...
content/browser/android/overscroll_controller_android.cc:233: enabled_ =
glow_effect_ || refresh_effect_;
As mentioned above, I'd prefer this to just be "enabled_ = true"

[jdduke (slow), 2014-11-19 16:56:22]

Adding more owners.

sievers@: content/public/common/
mpearson@: tools/metrics/histograms (speaking of which, is there a reason I get
the histograms test failure despite the fact that this flag is Android
specific?)
mkosiba@: android_webview

https://codereview.chromium.org/733293002/diff/1/content/browser/android/over...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/733293002/diff/1/content/browser/android/over...
content/browser/android/overscroll_controller_android.cc:42: static bool
sEnableGlowEffect =
On 2014/11/19 06:21:51, aelias wrote:
> Please remove these statics, static global state often comes back to bite in
> unexpected ways.  I'd rather just pay the string lookup cost every time.

Done.

https://codereview.chromium.org/733293002/diff/1/content/browser/android/over...
content/browser/android/overscroll_controller_android.cc:81: enabled_ =
glow_effect_ || refresh_effect_;
On 2014/11/19 06:21:50, aelias wrote:
> This complicates the meaning of "enabled_" by baking into it information we
> already have from the other members anyway.  I'd rather it just be allowed to
be
> "true" even if glow_effect_ and refresh_effect_ are both null.

Done.

https://codereview.chromium.org/733293002/diff/1/content/browser/android/over...
content/browser/android/overscroll_controller_android.cc:233: enabled_ =
glow_effect_ || refresh_effect_;
On 2014/11/19 06:21:51, aelias wrote:
> As mentioned above, I'd prefer this to just be "enabled_ = true"

Done.

[aelias_OOO_until_Jul13, 2014-11-19 19:28:16]

lgtm

[jdduke (slow), 2014-11-20 00:37:50]

jdduke@chromium.org changed reviewers:
+ mkosiba@chromium.org, mpearson@chromium.org, sievers@chromium.org

[jdduke (slow), 2014-11-20 00:37:50]

Adding additional owners (for real this time).

sievers@: content/public/common/
mpearson@: tools/metrics/histograms (speaking of which, is there a reason I get
the histograms test failure despite the fact that this flag is Android
specific?)
mkosiba@: android_webview

[no sievers, 2014-11-20 01:00:20]

On 2014/11/20 00:37:50, jdduke wrote:
> Adding additional owners (for real this time).
> 
> sievers@: content/public/common/
lgtm

[Mark P, 2014-11-20 01:07:52]

histograms.xml lgtm

>>>
mpearson@: tools/metrics/histograms (speaking of which, is there a reason I get
the histograms test failure despite the fact that this flag is Android
specific?)
>>>
It doesn't matter that the flag is android specific.  The histograms code wants
to know about all flags that users can toggle.

--mark

[boliu, 2014-11-20 23:12:48]

boliu@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2014-11-20 23:12:48]

android_webview rs lgtm

https://codereview.chromium.org/733293002/diff/20001/content/browser/android/...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/733293002/diff/20001/content/browser/android/...
content/browser/android/overscroll_controller_android.cc:44: return
scoped_ptr<OverscrollGlow>();
return nullptr should work, the new c++11 hotness

https://codereview.chromium.org/733293002/diff/20001/content/browser/android/...
content/browser/android/overscroll_controller_android.cc:57: return
scoped_ptr<OverscrollRefresh>();
ditto

[jdduke (slow), 2014-11-20 23:14:49]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2014-11-20 23:14:54]

https://codereview.chromium.org/733293002/diff/20001/content/browser/android/...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/733293002/diff/20001/content/browser/android/...
content/browser/android/overscroll_controller_android.cc:44: return
scoped_ptr<OverscrollGlow>();
On 2014/11/20 23:12:48, boliu wrote:
> return nullptr should work, the new c++11 hotness

yay!

https://codereview.chromium.org/733293002/diff/20001/content/browser/android/...
content/browser/android/overscroll_controller_android.cc:57: return
scoped_ptr<OverscrollRefresh>();
On 2014/11/20 23:12:48, boliu wrote:
> ditto

Done.

[commit-bot: I haz the power, 2014-11-20 23:15:49]

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

[commit-bot: I haz the power, 2014-11-20 23:22:11]

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

[commit-bot: I haz the power, 2014-11-20 23:22:13]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/84517)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/89252)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jdduke (slow), 2014-11-21 00:38:55]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-11-21 00:40:18]

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

[commit-bot: I haz the power, 2014-11-21 03:18:55]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-21 03:20:12]

Patchset 4 (id:??) landed as
https://crrev.com/0fa51299fbcd835fd5e8860f28f65b57a149598e
Cr-Commit-Position: refs/heads/master@{#305154}