Use scroll-boundary-behavior to control overscroll-refresh/glow on android.

Use scroll-boundary-behavior to control overscroll-refresh/glow on android.

Overscroll navigation is a feature of Chrome. Users can pull verticallly to
refresh the page on android, or swipe horizontally to browse history on desktop
platform. Overscroll-glow is a visual effect indicating user that his scroll
has passed the extent of the scroller. However, for some mobile web
applications, navigation or glow does not make sense. This patch allows users
to use a css attribute: scroll-boundary-behavior on the viewport defining
element in order to control overscroll-navigation and overscroll-glow.

On main thread, we set this property in OverscrollController, and on the
compositor thread, we plumb this property to LayerTreeImpl. When overscroll
happens, scroll-boundary-behavior is bundled in did_overscroll_params and is
passed to OverscrollControllerAndroid::Overscrolled(). Thus, we make the
decision of whether to pull-to-refresh and whether to glow on Overscrolled()
as opposed to immediately after ScrollUpdateAck.

Spec: https://wicg.github.io/scroll-boundary-behavior/
Design Doc: https://docs.google.com/a/google.com/document/d/1cqyeFamVmSu5EN5sBpQFB3Madm-41BtJEMDEbW1xdh4/edit?usp=sharing

BUG=656801
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2884423003
Cr-Commit-Position: refs/heads/master@{#498816}
Committed: https://chromium.googlesource.com/chromium/src/+/bbea8a99d2643b97d9dfe9d368cba6d129850a28

[sunyunjia, 2017-05-17 00:59:14]

Description was changed from

==========
Use scroll-boundary-behavior to control pull-to-refresh on android.

BUG=656801
==========

to

==========
Use scroll-boundary-behavior to control pull-to-refresh on android.

BUG=656801
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sunyunjia, 2017-05-17 01:02:17]

sunyunjia@chromium.org changed reviewers:
+ majidvp@chromium.org

[sunyunjia, 2017-05-17 01:02:18]

Hi Majid,

Here is the new patch that uses scroll-boundary-behavior to control
pull-to-refresh on android. Please take a look at the design then we can figure
out the best way to write tests. Thanks!

[majidvp, 2017-05-18 15:26:10]

I am not a CSS parser expert so I didn't review that part
too closely. It looked reasonable to me but you want a CSS
expert to review it more closely at some point.

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

https://codereview.chromium.org/2884423003/diff/20001/content/browser/android...
content/browser/android/overscroll_controller_android.cc:139: LOG(ERROR) <<
handled;
remove.

https://codereview.chromium.org/2884423003/diff/20001/content/browser/android...
content/browser/android/overscroll_controller_android.cc:212:
refresh_effect_->OnScrollUpdateAck(true);
This method is no longer invoked on ScrollUpdateAck also its
input is no longer just the consumed state but also involves scroll boundary
behavior. So I think it makes sense
to make changes to the signature e.g.,

OverscrollRefresh::OnOverscrolled(bool can_navigate)

where:

bool can_navigate = !consumed_ && scroll_boundary_behavior.y ==             
kScrollBoundaryBehaviorTypeAuto

https://codereview.chromium.org/2884423003/diff/20001/content/browser/android...
content/browser/android/overscroll_controller_android.cc:216:
refresh_effect_->IsAwaitingScrollUpdateAck())) {
nit: the first condition is the same of the above so you can 
merge the two.

https://codereview.chromium.org/2884423003/diff/20001/content/browser/android...
content/browser/android/overscroll_controller_android.cc:230: params.ClearX();
I believe this is for glow. So please update the CL description to
mention glow as well.

Also it will be good to explain in CL that we are now making the
decision to p2r also on DidOverscrollParams as opposed to immediately
after ScrollUpdateAck.

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

https://codereview.chromium.org/2884423003/diff/20001/content/browser/android...
content/browser/android/overscroll_controller_android.h:78: bool consumed_;
Can we use a more descriptive name e.g., scroll_update_consumed_

https://codereview.chromium.org/2884423003/diff/20001/content/renderer/input/...
File content/renderer/input/input_handler_wrapper.h (right):

https://codereview.chromium.org/2884423003/diff/20001/content/renderer/input/...
content/renderer/input/input_handler_wrapper.h:57: const
cc::ScrollBoundaryBehavior& scroll_boundary_behavior) override;
Not sure if you can use this cc type in content/renderer. 
Perehaps switch to the blink type.

https://codereview.chromium.org/2884423003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2884423003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:1949:
WebScrollBoundaryBehavior::ScrollBoundaryBehaviorType;
Why not  ScrollBoundaryBehaviorType? this seems inconsistent.

https://codereview.chromium.org/2884423003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h (right):

https://codereview.chromium.org/2884423003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h:13: using
ScrollBoundaryBehavior::ScrollBoundaryBehavior;
I am confused by this using statement. 
First should this be cc::ScrollBoundaryBehavior,
Second is it trying to bring in the enum into the namespace? so
ScrollBoundaryBehaviorType?

https://codereview.chromium.org/2884423003/diff/20001/ui/events/blink/did_ove...
File ui/events/blink/did_overscroll_params.cc (right):

https://codereview.chromium.org/2884423003/diff/20001/ui/events/blink/did_ove...
ui/events/blink/did_overscroll_params.cc:23: }
These methods are used only once and it the logic is pretty simple. I suggest
not having them in favor of the same logic 
at the call site.

https://codereview.chromium.org/2884423003/diff/20001/ui/events/blink/did_ove...
File ui/events/blink/did_overscroll_params.h (right):

https://codereview.chromium.org/2884423003/diff/20001/ui/events/blink/did_ove...
ui/events/blink/did_overscroll_params.h:24: cc::ScrollBoundaryBehavior
scroll_boundary_behavior;
I wonder if there is a preference to use the WebScrollBoundaryBehavior instead?
Perhaps best to consult
one of the OWNERs e.g., @dtapuska to see if there is a guideline.

[sunyunjia, 2017-05-19 17:05:54]

Description was changed from

==========
Use scroll-boundary-behavior to control pull-to-refresh on android.

BUG=656801
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Use scroll-boundary-behavior to control overscroll-refresh/glow on android.

BUG=656801
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sunyunjia, 2017-05-19 19:03:59]

Description was changed from

==========
Use scroll-boundary-behavior to control overscroll-refresh/glow on android.

BUG=656801
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Use scroll-boundary-behavior to control overscroll-refresh/glow on android.

Overscroll-navigation is a feature of Chrome. Users can pull verticallly to
refresh the page, or swipe horizontally to browse history. Overscroll-glow is
an effect indicating user that his scroll has reached the extend of the
scroller. However, in some mobile web applications, navigation or glow does not
make sense. This patch allows users to use a css attribute
(scroll-boundary-behavior) to the root element in order to control
overscroll-navigation and overscroll-glow. The plumbing and machinery have been
implemented in both main thread and compositor thread.

The scroll-boundary-behavior is bundled in did_overscroll_params and is passed
through OverscrollControllerAndroid::Overscrolled(). Thus, we make the decision
of whether to pull-to-refresh and whether to glow on Overscrolled() as opposed
to immediately after ScrollUpdateAck.

BUG=656801
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sunyunjia, 2017-05-19 20:11:25]

sunyunjia@chromium.org changed reviewers:
+ dtapuska@chromium.org

[sunyunjia, 2017-05-19 20:14:24]

Hi Dave,

Can you help look at this patch? We're uncertain about the css declarations, and
whether the cc::ScrollBoundaryBehavior types are used in the right place.
Thanks!

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

https://codereview.chromium.org/2884423003/diff/20001/content/browser/android...
content/browser/android/overscroll_controller_android.cc:139: LOG(ERROR) <<
handled;
On 2017/05/18 15:26:10, majidvp wrote:
> remove.

Done.

https://codereview.chromium.org/2884423003/diff/20001/content/browser/android...
content/browser/android/overscroll_controller_android.cc:212:
refresh_effect_->OnScrollUpdateAck(true);
On 2017/05/18 15:26:10, majidvp wrote:
> This method is no longer invoked on ScrollUpdateAck also its
> input is no longer just the consumed state but also involves scroll boundary
> behavior. So I think it makes sense
> to make changes to the signature e.g.,
> 
> OverscrollRefresh::OnOverscrolled(bool can_navigate)
> 
> where:
> 
> bool can_navigate = !consumed_ && scroll_boundary_behavior.y ==             
> kScrollBoundaryBehaviorTypeAuto 
> 

Done.

https://codereview.chromium.org/2884423003/diff/20001/content/browser/android...
content/browser/android/overscroll_controller_android.cc:216:
refresh_effect_->IsAwaitingScrollUpdateAck())) {
On 2017/05/18 15:26:10, majidvp wrote:
> nit: the first condition is the same of the above so you can 
> merge the two.

Done.

https://codereview.chromium.org/2884423003/diff/20001/content/browser/android...
content/browser/android/overscroll_controller_android.cc:230: params.ClearX();
On 2017/05/18 15:26:10, majidvp wrote:
> I believe this is for glow. So please update the CL description to
> mention glow as well.
> 
> Also it will be good to explain in CL that we are now making the
> decision to p2r also on DidOverscrollParams as opposed to immediately
> after ScrollUpdateAck.

Done.

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

https://codereview.chromium.org/2884423003/diff/20001/content/browser/android...
content/browser/android/overscroll_controller_android.h:78: bool consumed_;
On 2017/05/18 15:26:10, majidvp wrote:
> Can we use a more descriptive name e.g., scroll_update_consumed_

Done.

https://codereview.chromium.org/2884423003/diff/20001/content/renderer/input/...
File content/renderer/input/input_handler_wrapper.h (right):

https://codereview.chromium.org/2884423003/diff/20001/content/renderer/input/...
content/renderer/input/input_handler_wrapper.h:57: const
cc::ScrollBoundaryBehavior& scroll_boundary_behavior) override;
On 2017/05/18 15:26:10, majidvp wrote:
> Not sure if you can use this cc type in content/renderer. 
> Perehaps switch to the blink type.

This value is passed from input_handler_proxy from LayerTreeHostImpl. I'm not
sure if the conversion is worth it.

https://codereview.chromium.org/2884423003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2884423003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:1949:
WebScrollBoundaryBehavior::ScrollBoundaryBehaviorType;
On 2017/05/18 15:26:10, majidvp wrote:
> Why not  ScrollBoundaryBehaviorType? this seems inconsistent.

Done.

https://codereview.chromium.org/2884423003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h (right):

https://codereview.chromium.org/2884423003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h:13: using
ScrollBoundaryBehavior::ScrollBoundaryBehavior;
On 2017/05/18 15:26:10, majidvp wrote:
> I am confused by this using statement. 
> First should this be cc::ScrollBoundaryBehavior,
> Second is it trying to bring in the enum into the namespace? so
> ScrollBoundaryBehaviorType?

That was a way I found on stackoverflow to inherit the constructor, which is
really confusing. I've changed it to a normal approach.

https://codereview.chromium.org/2884423003/diff/20001/ui/events/blink/did_ove...
File ui/events/blink/did_overscroll_params.cc (right):

https://codereview.chromium.org/2884423003/diff/20001/ui/events/blink/did_ove...
ui/events/blink/did_overscroll_params.cc:23: }
On 2017/05/18 15:26:10, majidvp wrote:
> These methods are used only once and it the logic is pretty simple. I suggest
> not having them in favor of the same logic 
> at the call site.

Done.

https://codereview.chromium.org/2884423003/diff/20001/ui/events/blink/did_ove...
File ui/events/blink/did_overscroll_params.h (right):

https://codereview.chromium.org/2884423003/diff/20001/ui/events/blink/did_ove...
ui/events/blink/did_overscroll_params.h:24: cc::ScrollBoundaryBehavior
scroll_boundary_behavior;
On 2017/05/18 15:26:10, majidvp wrote:
> I wonder if there is a preference to use the WebScrollBoundaryBehavior
instead?
> Perhaps best to consult
> one of the OWNERs e.g., @dtapuska to see if there is a guideline.
> 

Dave said that the cc value is fine in this struct

[dtapuska, 2017-05-23 17:13:40]

https://codereview.chromium.org/2884423003/diff/40001/cc/ipc/cc_param_traits.h
File cc/ipc/cc_param_traits.h (right):

https://codereview.chromium.org/2884423003/diff/40001/cc/ipc/cc_param_traits....
cc/ipc/cc_param_traits.h:11: #include "cc/input/scroll_boundary_behavior.h"
This is an odd include. Where is this used?

https://codereview.chromium.org/2884423003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/2884423003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebLayerTreeView.h:117: virtual void
SetScrollBoundaryBehavior(WebScrollBoundaryBehavior) const {}
This is odd that this is a const method especially since it is "Set"

https://codereview.chromium.org/2884423003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h (right):

https://codereview.chromium.org/2884423003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h:14: struct
WebScrollBoundaryBehavior : public ScrollBoundaryBehavior {
Is this all really needed?

TouchAction was used this way...

https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebTo...

[sunyunjia, 2017-06-11 01:11:46]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-11 01:11:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-11 01:14:42]

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

[commit-bot: I haz the power, 2017-06-11 01:14:43]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2017-06-11 01:59:39]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-11 01:59:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-11 02:22:14]

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

[commit-bot: I haz the power, 2017-06-11 02:22:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-06-12 14:20:29]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 14:20:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 14:41:28]

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

[commit-bot: I haz the power, 2017-06-12 14:41:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-06-12 15:45:05]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 15:45:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 16:53:10]

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

[commit-bot: I haz the power, 2017-06-12 16:53:11]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sunyunjia, 2017-06-13 00:15:57]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[sunyunjia, 2017-06-13 00:16:16]

Patchset #4 (id:60001) has been deleted

[sunyunjia, 2017-06-13 00:16:38]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-06-13 00:16:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-06-13 00:17:10]

Patchset #5 (id:120001) has been deleted

[sunyunjia, 2017-06-13 00:17:24]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-06-13 01:43:12]

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

[commit-bot: I haz the power, 2017-06-13 01:43:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-06-13 13:26:51]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 13:27:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-06-13 15:53:48]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 15:54:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-06-13 15:56:21]

Hi Majid and Dave,

I added some quite basic tests for overscroll_controller_android. Please take a
look to see if they are appropriate. Thanks!

https://codereview.chromium.org/2884423003/diff/40001/cc/ipc/cc_param_traits.h
File cc/ipc/cc_param_traits.h (right):

https://codereview.chromium.org/2884423003/diff/40001/cc/ipc/cc_param_traits....
cc/ipc/cc_param_traits.h:11: #include "cc/input/scroll_boundary_behavior.h"
On 2017/05/23 17:13:40, dtapuska wrote:
> This is an odd include. Where is this used?

Sorry, I forgot to remove it.

https://codereview.chromium.org/2884423003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/2884423003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebLayerTreeView.h:117: virtual void
SetScrollBoundaryBehavior(WebScrollBoundaryBehavior) const {}
On 2017/05/23 17:13:40, dtapuska wrote:
> This is odd that this is a const method especially since it is "Set"

Done.

https://codereview.chromium.org/2884423003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h (right):

https://codereview.chromium.org/2884423003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h:14: struct
WebScrollBoundaryBehavior : public ScrollBoundaryBehavior {
On 2017/05/23 17:13:40, dtapuska wrote:
> Is this all really needed?
> 
> TouchAction was used this way...
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebTo...

IMHO, feels like it is more readable to separate x and y into two fields than to
combine them in one enum?

[commit-bot: I haz the power, 2017-06-13 16:59:33]

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

[commit-bot: I haz the power, 2017-06-13 16:59:35]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sunyunjia, 2017-06-13 17:15:51]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 17:16:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 18:53:05]

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

[commit-bot: I haz the power, 2017-06-13 18:53:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[majidvp, 2017-07-12 21:04:22]

https://codereview.chromium.org/2884423003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h (right):

https://codereview.chromium.org/2884423003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h:14: struct
WebScrollBoundaryBehavior : public ScrollBoundaryBehavior {
On 2017/06/13 15:56:21, sunyunjia wrote:
> On 2017/05/23 17:13:40, dtapuska wrote:
> > Is this all really needed?
> > 
> > TouchAction was used this way...
> > 
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebTo...
> 
> IMHO, feels like it is more readable to separate x and y into two fields than
to
> combine them in one enum?

I believe dtapuska@ referring to the fact that we have two structs and one
inherits from the other. He is suggesting to just make the Web one alias for
the cc one. It seems reasonable.

You can still have two separate fields I believe.

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:121:
refresh_effect_(CreateRefreshEffect(overscroll_refresh_handler)) {
This constructor should delegate to the one above only
passing in newly created glow and refresh effects.

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:219:
DidOverscrollParams params = overscroll_params;
Seems a bit inefficient and unnecessary to create a copy of
whole params, reset some value conditionally and then copy
those values again to pass into the |glow_effect_->OnOverscrolled|.

Instead probably best to do

Vector2d accumulated_overscroll =
gfx::ScaleVector2d(overscroll_params.accumulated_overscroll, dpi_scale_);

if  (boundar_behavior = none)
  accumulated_overscroll.set_y(0);


glow_effect_->OnOverscrolled(..., accumulated_overscroll, ...)

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
File content/browser/android/overscroll_controller_android_unittest.cc (right):

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:26: 
all test logic should be inside an unnamed namespace e.g.,
namespace  {
 /// Test
}

See other tests in this directory for example.

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:75:
TEST_F(OverscrollControllerAndroidUnitTest, BasicNavigation) {
The test name needs to be a bit more descriptive in particular
it should mention you are verifying ScrollBoundaryBehaviorTypeAuto

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:95:
params.accumulated_overscroll = gfx::Vector2dF(0, 1);
nit: first test uses |20| but this one uses |1|. I believe any non-zero
value is fine so there is no difference here. So you might as well
use the same value so future reader know it is the same.

The only difference is scroll_boundary_behavior.

Event better to have a helper function that creates a 
simple ui::DidOverscrollParams with default vertical delta and ...
and then you only override the scroll_boundary_behavior. This helps
readability as well.

https://codereview.chromium.org/2884423003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2884423003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2449: }
Most of the CSS logic is present in the other patch as well [1].
I am assuming you do not need me to review them here?

[1] https://codereview.chromium.org/2769793002/

https://codereview.chromium.org/2884423003/diff/220001/ui/android/overscroll_...
File ui/android/overscroll_glow.h (right):

https://codereview.chromium.org/2884423003/diff/220001/ui/android/overscroll_...
ui/android/overscroll_glow.h:43: // This constructor is for test only.
Why is this needed? Can't we just use the above constructor
and pass in null for client?

Presumably in your test client_ is never used otherwise they
would crash. So it should be fair to just move the DCHECK to
methods that actually use client_ instead of constructor.

https://codereview.chromium.org/2884423003/diff/220001/ui/android/overscroll_...
File ui/android/overscroll_refresh.h (right):

https://codereview.chromium.org/2884423003/diff/220001/ui/android/overscroll_...
ui/android/overscroll_refresh.h:34: OverscrollRefresh();
ditto!

[sunyunjia, 2017-07-19 20:15:03]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-19 20:15:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-07-19 20:25:21]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-19 20:25:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-07-19 20:42:10]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-19 20:42:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-07-19 20:44:12]

PTAL, thanks!

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:121:
refresh_effect_(CreateRefreshEffect(overscroll_refresh_handler)) {
On 2017/07/12 21:04:20, majidvp wrote:
> This constructor should delegate to the one above only
> passing in newly created glow and refresh effects.

Done. I'm not sure if the delegate is worth it. I have to initialize compositor_
and dpi_scale_ any way, as they are constants.

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:219:
DidOverscrollParams params = overscroll_params;
On 2017/07/12 21:04:20, majidvp wrote:
> Seems a bit inefficient and unnecessary to create a copy of
> whole params, reset some value conditionally and then copy
> those values again to pass into the |glow_effect_->OnOverscrolled|.
> 
> Instead probably best to do
> 
> Vector2d accumulated_overscroll =
> gfx::ScaleVector2d(overscroll_params.accumulated_overscroll, dpi_scale_);
> 
> if  (boundar_behavior = none)
>   accumulated_overscroll.set_y(0);
> 
> 
> glow_effect_->OnOverscrolled(..., accumulated_overscroll, ...)

Done.

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
File content/browser/android/overscroll_controller_android_unittest.cc (right):

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:26: 
On 2017/07/12 21:04:21, majidvp wrote:
> all test logic should be inside an unnamed namespace e.g.,
> namespace  {
>  /// Test
> }
> 
> See other tests in this directory for example.

Done.

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:75:
TEST_F(OverscrollControllerAndroidUnitTest, BasicNavigation) {
On 2017/07/12 21:04:20, majidvp wrote:
> The test name needs to be a bit more descriptive in particular
> it should mention you are verifying ScrollBoundaryBehaviorTypeAuto

Done.

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:95:
params.accumulated_overscroll = gfx::Vector2dF(0, 1);
On 2017/07/12 21:04:21, majidvp wrote:
> nit: first test uses |20| but this one uses |1|. I believe any non-zero
> value is fine so there is no difference here. So you might as well
> use the same value so future reader know it is the same.
> 
> The only difference is scroll_boundary_behavior.
> 
> Event better to have a helper function that creates a 
> simple ui::DidOverscrollParams with default vertical delta and ...
> and then you only override the scroll_boundary_behavior. This helps
> readability as well.

Done.

https://codereview.chromium.org/2884423003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2884423003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2449: }
On 2017/07/12 21:04:21, majidvp wrote:
> Most of the CSS logic is present in the other patch as well [1].
> I am assuming you do not need me to review them here?
> 
> [1] https://codereview.chromium.org/2769793002/

Yeah, they are no longer needed in this patch.

https://codereview.chromium.org/2884423003/diff/220001/ui/android/overscroll_...
File ui/android/overscroll_glow.h (right):

https://codereview.chromium.org/2884423003/diff/220001/ui/android/overscroll_...
ui/android/overscroll_glow.h:43: // This constructor is for test only.
On 2017/07/12 21:04:21, majidvp wrote:
> Why is this needed? Can't we just use the above constructor
> and pass in null for client?
> 
> Presumably in your test client_ is never used otherwise they
> would crash. So it should be fair to just move the DCHECK to
> methods that actually use client_ instead of constructor.

Done.

https://codereview.chromium.org/2884423003/diff/220001/ui/android/overscroll_...
File ui/android/overscroll_refresh.h (right):

https://codereview.chromium.org/2884423003/diff/220001/ui/android/overscroll_...
ui/android/overscroll_refresh.h:34: OverscrollRefresh();
On 2017/07/12 21:04:21, majidvp wrote:
> ditto!

Done.

[commit-bot: I haz the power, 2017-07-19 22:55:44]

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

[commit-bot: I haz the power, 2017-07-19 22:55:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[majidvp, 2017-07-20 13:26:55]

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
File content/browser/android/overscroll_controller_android_unittest.cc (right):

https://codereview.chromium.org/2884423003/diff/220001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:75:
TEST_F(OverscrollControllerAndroidUnitTest, BasicNavigation) {
On 2017/07/19 20:44:11, sunyunjia wrote:
> On 2017/07/12 21:04:20, majidvp wrote:
> > The test name needs to be a bit more descriptive in particular
> > it should mention you are verifying ScrollBoundaryBehaviorTypeAuto
> 
> Done.

not: the new name still does not convey that it is related to
ScrollBoundaryBehavior.

https://codereview.chromium.org/2884423003/diff/280001/content/browser/androi...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/2884423003/diff/280001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:117:
OverscrollControllerAndroid(
This is not the correct way to do delegating constructor (see [1]).
If the constructor is actually delegated then there is no need to initialize
compositor_ and dpi_scale_ twice.

[1]
http://en.cppreference.com/w/cpp/language/initializer_list?#Delegating_constr...

https://codereview.chromium.org/2884423003/diff/280001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:261: dpi_scale_))) {
Although the last argument is not modified like others but it is better
to move it up next to the rest and give it a name. 
Mainly because then it becomes obvious to the reader that all these properties
are being scaled consistently.

https://codereview.chromium.org/2884423003/diff/280001/content/browser/androi...
File content/browser/android/overscroll_controller_android_unittest.cc (right):

https://codereview.chromium.org/2884423003/diff/280001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:83:
std::unique_ptr<OverscrollControllerAndroid> controller;
nit: It does not seem to me that you actually need to dynamically
allocate these.  These can be just  data member which means
getting rig of all unique ptrs and de-refs.

https://codereview.chromium.org/2884423003/diff/280001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebViewClient.h (right):

https://codereview.chromium.org/2884423003/diff/280001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebViewClient.h:1: 
nit: remove blank line.

https://codereview.chromium.org/2884423003/diff/280001/ui/android/overscroll_...
File ui/android/overscroll_glow.h (left):

https://codereview.chromium.org/2884423003/diff/280001/ui/android/overscroll_...
ui/android/overscroll_glow.h:39: // |client| must be valid for the duration of
the effect's lifetime.
Why remove this? This is still true for normal operation of this class no?

https://codereview.chromium.org/2884423003/diff/280001/ui/android/overscroll_...
File ui/android/overscroll_refresh.h (right):

https://codereview.chromium.org/2884423003/diff/280001/ui/android/overscroll_...
ui/android/overscroll_refresh.h:42: // scroll-boundary-behavior.
Similar to the previous comment let's try to specify *exactly* the condition
under which the effect will be activated.

[sunyunjia, 2017-07-20 18:39:56]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-20 18:40:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-07-20 19:07:01]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-20 19:07:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-20 20:19:49]

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

[commit-bot: I haz the power, 2017-07-20 20:19:50]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sunyunjia, 2017-07-20 21:36:51]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-20 21:36:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-20 23:50:58]

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

[commit-bot: I haz the power, 2017-07-20 23:51:00]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sunyunjia, 2017-07-21 13:51:43]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-21 13:51:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-07-21 13:52:23]

PTAL, thanks!

https://codereview.chromium.org/2884423003/diff/280001/content/browser/androi...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/2884423003/diff/280001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:117:
OverscrollControllerAndroid(
On 2017/07/20 13:26:55, majidvp wrote:
> This is not the correct way to do delegating constructor (see [1]).
> If the constructor is actually delegated then there is no need to initialize
> compositor_ and dpi_scale_ twice.
> 
> [1]
>
http://en.cppreference.com/w/cpp/language/initializer_list#Delegating_constru...

Done.

https://codereview.chromium.org/2884423003/diff/280001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:261: dpi_scale_))) {
On 2017/07/20 13:26:55, majidvp wrote:
> Although the last argument is not modified like others but it is better
> to move it up next to the rest and give it a name. 
> Mainly because then it becomes obvious to the reader that all these properties
> are being scaled consistently.

Done.

https://codereview.chromium.org/2884423003/diff/280001/content/browser/androi...
File content/browser/android/overscroll_controller_android_unittest.cc (right):

https://codereview.chromium.org/2884423003/diff/280001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:83:
std::unique_ptr<OverscrollControllerAndroid> controller;
On 2017/07/20 13:26:55, majidvp wrote:
> nit: It does not seem to me that you actually need to dynamically
> allocate these.  These can be just  data member which means
> getting rig of all unique ptrs and de-refs.

Done.

https://codereview.chromium.org/2884423003/diff/280001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebViewClient.h (right):

https://codereview.chromium.org/2884423003/diff/280001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebViewClient.h:1: 
On 2017/07/20 13:26:55, majidvp wrote:
> nit: remove blank line.

Done.

https://codereview.chromium.org/2884423003/diff/280001/ui/android/overscroll_...
File ui/android/overscroll_glow.h (left):

https://codereview.chromium.org/2884423003/diff/280001/ui/android/overscroll_...
ui/android/overscroll_glow.h:39: // |client| must be valid for the duration of
the effect's lifetime.
On 2017/07/20 13:26:55, majidvp wrote:
> Why remove this? This is still true for normal operation of this class no? 

Added it back. I was thinking of the test case.

https://codereview.chromium.org/2884423003/diff/280001/ui/android/overscroll_...
File ui/android/overscroll_refresh.h (right):

https://codereview.chromium.org/2884423003/diff/280001/ui/android/overscroll_...
ui/android/overscroll_refresh.h:42: // scroll-boundary-behavior.
On 2017/07/20 13:26:55, majidvp wrote:
> Similar to the previous comment let's try to specify *exactly* the condition
> under which the effect will be activated.

Done.

[commit-bot: I haz the power, 2017-07-21 13:58:02]

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

[commit-bot: I haz the power, 2017-07-21 13:58:03]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sunyunjia, 2017-07-21 14:41:04]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-21 14:41:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[majidvp, 2017-07-21 15:24:12]

I think the CL description can be improved. Here are some comments:

> Overscroll-navigation is a feature of Chrome. Users can pull verticallly to
refresh the page, or swipe horizontally to browse history.
no need to hyphenate overscroll navigation. Also please mention that p2r is for
android and swipe is for desktop platform.


>  Overscroll-glow is
an effect indicating user that his scroll has reached the extend of the
scroller. 

s/effect/visual effect/  
s/extend/extent/
Also it is not that the scroll has reached the extent but that in fact it has
passed it.


> However, in some mobile web applications, navigation or glow does not
make sense. 

s/in/for
s


This patch allows users to use a css attribute
(scroll-boundary-behavior) to the root element in order to control
overscroll-navigation and overscroll-glow. 

s/to the root element/on the viewport defining element/

> The plumbing and machinery have been
implemented in both main thread and compositor thread.

Instead of this sentence I suggest describing exactly what has changed in main
and cc.


The scroll-boundary-behavior is bundled in did_overscroll_params and is passed
through OverscrollControllerAndroid::Overscrolled(). Thus, we make the decision
of whether to pull-to-refresh and whether to glow on Overscrolled() as opposed
to immediately after ScrollUpdateAck.


Also please link to specification:
https://wicg.github.io/scroll-boundary-behavior/

and any relevant design docs.

https://codereview.chromium.org/2884423003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2884423003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1982:
GetPage()->GetOverscrollController().SetScrollBoundaryBehavior(
If I understand this correctly, the current logic allows any document in
the page to control the overscroll navigation behavior of the page. Meaning that
if I have this css property on an iframe then it will prevent the navigation.
This is incorrect.

We should only allow the top level document to control this behavior. Which I
think means limiting this to when |Document::IsInMainFrame()| is true.

Also, this suggests to me that we are missing a few tests. For example there is
no test covering this. I suspect you can add tests to WebFrameTest to ensure we
receive the correct values according to the css property and whether this is the
main frame or not.

[commit-bot: I haz the power, 2017-07-21 17:05:00]

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

[commit-bot: I haz the power, 2017-07-21 17:05:01]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sunyunjia, 2017-07-21 17:06:29]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-21 17:06:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-21 19:26:02]

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

[commit-bot: I haz the power, 2017-07-21 19:26:03]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sunyunjia, 2017-07-24 00:14:39]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-24 00:14:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-07-24 00:55:36]

Description was changed from

==========
Use scroll-boundary-behavior to control overscroll-refresh/glow on android.

Overscroll-navigation is a feature of Chrome. Users can pull verticallly to
refresh the page, or swipe horizontally to browse history. Overscroll-glow is
an effect indicating user that his scroll has reached the extend of the
scroller. However, in some mobile web applications, navigation or glow does not
make sense. This patch allows users to use a css attribute
(scroll-boundary-behavior) to the root element in order to control
overscroll-navigation and overscroll-glow. The plumbing and machinery have been
implemented in both main thread and compositor thread.

The scroll-boundary-behavior is bundled in did_overscroll_params and is passed
through OverscrollControllerAndroid::Overscrolled(). Thus, we make the decision
of whether to pull-to-refresh and whether to glow on Overscrolled() as opposed
to immediately after ScrollUpdateAck.

BUG=656801
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Use scroll-boundary-behavior to control overscroll-refresh/glow on android.

Overscroll navigation is a feature of Chrome. Users can pull verticallly to
refresh the page on android, or swipe horizontally to browse history on desktop
platform. Overscroll-glow is a visual effect indicating user that his scroll
has passed the extent of the scroller. However, for some mobile web
applications, navigation or glow does not make sense. This patch allows users
to use a css attribute: scroll-boundary-behavior on the viewport defining
element in order to control overscroll-navigation and overscroll-glow.

On main thread, we set this property in OverscrollController, and on the
compositor thread, we plumb this property to LayerTreeImpl. When overscroll
happens, scroll-boundary-behavior is bundled in did_overscroll_params and is
passed to OverscrollControllerAndroid::Overscrolled(). Thus, we make the
decision of whether to pull-to-refresh and whether to glow on Overscrolled() as
opposed to immediately after ScrollUpdateAck.

Spec: https://wicg.github.io/scroll-boundary-behavior/
Design Doc:
https://docs.google.com/a/google.com/document/d/1cqyeFamVmSu5EN5sBpQFB3Madm-4...

BUG=656801
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sunyunjia, 2017-07-24 00:56:13]

Description was changed from

==========
Use scroll-boundary-behavior to control overscroll-refresh/glow on android.

Overscroll navigation is a feature of Chrome. Users can pull verticallly to
refresh the page on android, or swipe horizontally to browse history on desktop
platform. Overscroll-glow is a visual effect indicating user that his scroll
has passed the extent of the scroller. However, for some mobile web
applications, navigation or glow does not make sense. This patch allows users
to use a css attribute: scroll-boundary-behavior on the viewport defining
element in order to control overscroll-navigation and overscroll-glow.

On main thread, we set this property in OverscrollController, and on the
compositor thread, we plumb this property to LayerTreeImpl. When overscroll
happens, scroll-boundary-behavior is bundled in did_overscroll_params and is
passed to OverscrollControllerAndroid::Overscrolled(). Thus, we make the
decision of whether to pull-to-refresh and whether to glow on Overscrolled() as
opposed to immediately after ScrollUpdateAck.

Spec: https://wicg.github.io/scroll-boundary-behavior/
Design Doc:
https://docs.google.com/a/google.com/document/d/1cqyeFamVmSu5EN5sBpQFB3Madm-4...

BUG=656801
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Use scroll-boundary-behavior to control overscroll-refresh/glow on android.

Overscroll navigation is a feature of Chrome. Users can pull verticallly to
refresh the page on android, or swipe horizontally to browse history on desktop
platform. Overscroll-glow is a visual effect indicating user that his scroll
has passed the extent of the scroller. However, for some mobile web
applications, navigation or glow does not make sense. This patch allows users
to use a css attribute: scroll-boundary-behavior on the viewport defining
element in order to control overscroll-navigation and overscroll-glow.

On main thread, we set this property in OverscrollController, and on the
compositor thread, we plumb this property to LayerTreeImpl. When overscroll
happens, scroll-boundary-behavior is bundled in did_overscroll_params and is
passed to OverscrollControllerAndroid::Overscrolled(). Thus, we make the
decision of whether to pull-to-refresh and whether to glow on Overscrolled()
as opposed to immediately after ScrollUpdateAck.

Spec: https://wicg.github.io/scroll-boundary-behavior/
Design Doc:
https://docs.google.com/a/google.com/document/d/1cqyeFamVmSu5EN5sBpQFB3Madm-4...

BUG=656801
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2017-07-24 02:27:02]

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

[commit-bot: I haz the power, 2017-07-24 02:27:03]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sunyunjia, 2017-07-24 02:27:49]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-24 02:28:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-24 04:35:12]

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

[commit-bot: I haz the power, 2017-07-24 04:35:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sunyunjia, 2017-07-25 15:09:05]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-25 15:09:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-25 15:13:40]

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

[commit-bot: I haz the power, 2017-07-25 15:13:41]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[sunyunjia, 2017-07-25 17:37:05]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-25 17:37:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-07-25 20:20:56]

PTAL, thanks!

https://codereview.chromium.org/2884423003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2884423003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1982:
GetPage()->GetOverscrollController().SetScrollBoundaryBehavior(
On 2017/07/21 15:24:12, majidvp wrote:
> If I understand this correctly, the current logic allows any document in
> the page to control the overscroll navigation behavior of the page. Meaning
that
> if I have this css property on an iframe then it will prevent the navigation.
> This is incorrect.
> 
> We should only allow the top level document to control this behavior. Which I
> think means limiting this to when |Document::IsInMainFrame()| is true.
> 
> Also, this suggests to me that we are missing a few tests. For example there
is
> no test covering this. I suspect you can add tests to WebFrameTest to ensure
we
> receive the correct values according to the css property and whether this is
the
> main frame or not.
> 

Done.

[majidvp, 2017-07-25 20:38:29]

https://codereview.chromium.org/2884423003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2884423003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10222:
TEST_P(WebFrameOverscrollTest, OnlyMainFrameScrollBoundaryBehaviorHasEffect) {
Please also add a test to ensure that updating main frame 
"scroll-boundary-behavior" actually results on the 
DodOverscroll getting called with a new value.

https://codereview.chromium.org/2884423003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10237:
"scroll-boundary-behavior: auto;'")));
If I understand it correctly, the height and width are set to make
the frame actually scrollable. Are they actually needed?


If you need a scrollable frame then perhaps it is more appropriate to use:
overscroll/iframe-overscroll.html
overscroll/scrollable-iframe.html

[commit-bot: I haz the power, 2017-07-25 21:01:35]

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

[commit-bot: I haz the power, 2017-07-25 21:01:37]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2017-08-03 14:54:21]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-03 14:54:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-08-03 14:54:34]

PTAL, thanks!

https://codereview.chromium.org/2884423003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2884423003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10222:
TEST_P(WebFrameOverscrollTest, OnlyMainFrameScrollBoundaryBehaviorHasEffect) {
On 2017/07/25 20:38:29, majidvp wrote:
> Please also add a test to ensure that updating main frame 
> "scroll-boundary-behavior" actually results on the 
> DodOverscroll getting called with a new value. 

Done.

https://codereview.chromium.org/2884423003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10237:
"scroll-boundary-behavior: auto;'")));
On 2017/07/25 20:38:29, majidvp wrote:
> If I understand it correctly, the height and width are set to make
> the frame actually scrollable. Are they actually needed?
> 
> 
> If you need a scrollable frame then perhaps it is more appropriate to use:
> overscroll/iframe-overscroll.html
> overscroll/scrollable-iframe.html

Done.

[commit-bot: I haz the power, 2017-08-03 14:58:37]

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

[commit-bot: I haz the power, 2017-08-03 14:58:39]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-08-03 15:07:56]

Patchset #19 (id:440001) has been deleted

[sunyunjia, 2017-08-03 15:11:09]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-03 15:11:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-03 15:24:09]

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

[commit-bot: I haz the power, 2017-08-03 15:24:10]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[sunyunjia, 2017-08-03 20:40:24]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-03 20:40:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-03 23:58:30]

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

[commit-bot: I haz the power, 2017-08-03 23:58:31]

Dry run: This issue passed the CQ dry run.

[majidvp, 2017-08-08 15:16:15]

https://codereview.chromium.org/2884423003/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2884423003/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:141:
frame_->GetDocument()->ViewportDefiningElement(nullptr))
This feels odd to me. It is not clear to me why exactly it is needed
and seems to be in conflict with the conditional statement above.
e.

Also, is this something that should be part of |CanPropagate| logic?

https://codereview.chromium.org/2884423003/diff/480001/ui/android/overscroll_...
File ui/android/overscroll_refresh.h (right):

https://codereview.chromium.org/2884423003/diff/480001/ui/android/overscroll_...
ui/android/overscroll_refresh.h:42: // scroll_boundary_behavior on y axis is
'auto'. See how it's called in
nit: This sentence does not read well. It has two back to back 'if'.

Suggestion:
The effect will only be activated if |can_navigate| is true which happens when
the scroll update is not consumed and the scroll_boundary_behavior on y axis is
'auto'.

https://codereview.chromium.org/2884423003/diff/480001/ui/android/overscroll_...
ui/android/overscroll_refresh.h:43: // OverscrollControllerAndroid.
nit: This last sentence seems unnecessary.

[sunyunjia, 2017-08-09 21:38:20]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-09 21:38:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-10 00:36:36]

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

[commit-bot: I haz the power, 2017-08-10 00:36:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-08-10 14:01:49]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-10 14:01:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-10 14:11:44]

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

[commit-bot: I haz the power, 2017-08-10 14:11:46]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-08-10 16:17:10]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-10 16:17:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-08-10 16:17:27]

Hi Majid,

PTAL, thanks!

https://codereview.chromium.org/2884423003/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2884423003/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:141:
frame_->GetDocument()->ViewportDefiningElement(nullptr))
On 2017/08/08 15:16:14, majidvp wrote:
> This feels odd to me. It is not clear to me why exactly it is needed
> and seems to be in conflict with the conditional statement above.
> e.
> 
> Also, is this something that should be part of |CanPropagate| logic?

Overscroll effect happens deep down at ApplyScroll. If we want the overscroll
information sent to browser side, we need to make sure there is something to
scroll in the scroll chain so that we could walk down this path to
OverscrollController.

I've already moved the logic inside CanPropagate.

https://codereview.chromium.org/2884423003/diff/480001/ui/android/overscroll_...
File ui/android/overscroll_refresh.h (right):

https://codereview.chromium.org/2884423003/diff/480001/ui/android/overscroll_...
ui/android/overscroll_refresh.h:42: // scroll_boundary_behavior on y axis is
'auto'. See how it's called in
On 2017/08/08 15:16:14, majidvp wrote:
> nit: This sentence does not read well. It has two back to back 'if'.
> 
> Suggestion:
> The effect will only be activated if |can_navigate| is true which happens when
> the scroll update is not consumed and the scroll_boundary_behavior on y axis
is
> 'auto'.

Done.

https://codereview.chromium.org/2884423003/diff/480001/ui/android/overscroll_...
ui/android/overscroll_refresh.h:43: // OverscrollControllerAndroid.
On 2017/08/08 15:16:14, majidvp wrote:
> nit: This last sentence seems unnecessary.

Done.

[commit-bot: I haz the power, 2017-08-10 21:19:02]

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

[commit-bot: I haz the power, 2017-08-10 21:19:04]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sunyunjia, 2017-08-10 21:27:32]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-10 21:27:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-10 23:27:54]

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

[commit-bot: I haz the power, 2017-08-10 23:27:56]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2017-08-11 17:48:59]

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

[sunyunjia, 2017-08-11 17:49:01]

Hi Alexandre,

Could you please take a look at this patch? Here are some contexts--
design doc:
https://docs.google.com/a/google.com/document/d/1cqyeFamVmSu5EN5sBpQFB3Madm-4...

spec: https://wicg.github.io/scroll-boundary-behavior/

Thanks!

[aelias_OOO_until_Jul13, 2017-08-11 21:50:32]

Looks good overall, thanks!

https://codereview.chromium.org/2884423003/diff/520001/content/browser/androi...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/2884423003/diff/520001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:216:
(!scroll_update_consumed_) &&
nit: please remove parens since operator precedence is easy to see here
(likewise around "==" statement).

https://codereview.chromium.org/2884423003/diff/520001/content/browser/androi...
File content/browser/android/overscroll_controller_android.h (right):

https://codereview.chromium.org/2884423003/diff/520001/content/browser/androi...
content/browser/android/overscroll_controller_android.h:85: bool
scroll_update_consumed_;
This is refresh-effect specific so it should be stored in the OverscrollRefresh
class.  I also think an additional boolean can and should be avoided, I suggest
making this part of the existing OverScrollRefresh::scroll_consumption_state_
state machine.

[sunyunjia, 2017-08-14 21:29:10]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-14 21:29:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-14 22:17:26]

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

[commit-bot: I haz the power, 2017-08-14 22:17:28]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[sunyunjia, 2017-08-14 23:28:55]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-14 23:29:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-15 00:01:09]

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

[commit-bot: I haz the power, 2017-08-15 00:01:11]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[majidvp, 2017-08-15 14:30:12]

https://codereview.chromium.org/2884423003/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2884423003/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:141:
frame_->GetDocument()->ViewportDefiningElement(nullptr))
On 2017/08/10 16:17:27, sunyunjia wrote:
> On 2017/08/08 15:16:14, majidvp wrote:
> > This feels odd to me. It is not clear to me why exactly it is needed
> > and seems to be in conflict with the conditional statement above.
> > e.
> > 
> > Also, is this something that should be part of |CanPropagate| logic?
> 
> Overscroll effect happens deep down at ApplyScroll. If we want the overscroll
> information sent to browser side, we need to make sure there is something to
> scroll in the scroll chain so that we could walk down this path to
> OverscrollController.
> 
> I've already moved the logic inside CanPropagate.

We had some offline discussion about this and here is the summary that:
 - First this case should only happen when <body> is the viewport defining
element. In this case
   body itself should actually not be scrollable!
 - It seems the fundamental fix here is to actually only consider
scroll-boundary-behavior for
   element that are scrollable and can potentially be in scroll chain. But the
current logic
   considers it for any element that is in containing block chain.

So the solution we came up with was to do the following: 

for (element in containing block chain) {
  if (IsScrollable(element)) { 
    if (CanScroll(element, scroll_state)) { 
      scroll_chain.push(element)  // add to scroll chain 
    }

    if (!CanPropagate(element)) { 
      break; // stop the chain here! 
    } 
  }
}


This should correctly filter out the viewport defining element that does not
scroll.

[sunyunjia, 2017-08-16 16:56:07]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-16 16:56:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-16 17:57:41]

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

[commit-bot: I haz the power, 2017-08-16 17:57:43]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2017-08-17 19:16:55]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-17 19:17:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-17 20:59:04]

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

[commit-bot: I haz the power, 2017-08-17 20:59:06]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2017-08-18 18:44:06]

I figured out CanScroll() has more complicated logic than the 2-step we figured
out the other day (getScrollableArea then decide base on delta value). So I
would leave CanScroll as it is and add the GetScrollableArea part to
CanPropagate.

PTAL, thanks!

https://codereview.chromium.org/2884423003/diff/520001/content/browser/androi...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/2884423003/diff/520001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:216:
(!scroll_update_consumed_) &&
On 2017/08/11 21:50:32, aelias_OOO_until_Jul13 wrote:
> nit: please remove parens since operator precedence is easy to see here
> (likewise around "==" statement).

Done.

https://codereview.chromium.org/2884423003/diff/520001/content/browser/androi...
File content/browser/android/overscroll_controller_android.h (right):

https://codereview.chromium.org/2884423003/diff/520001/content/browser/androi...
content/browser/android/overscroll_controller_android.h:85: bool
scroll_update_consumed_;
On 2017/08/11 21:50:32, aelias_OOO_until_Jul13 wrote:
> This is refresh-effect specific so it should be stored in the
OverscrollRefresh
> class.  I also think an additional boolean can and should be avoided, I
suggest
> making this part of the existing OverScrollRefresh::scroll_consumption_state_
> state machine.

Done.

[sunyunjia, 2017-08-22 13:46:58]

sunyunjia@chromium.org changed reviewers:
+ tedchoc@chromium.org

[sunyunjia, 2017-08-22 13:49:05]

Hi Ted,

Could you please review this patch for the
content/browser/android/  and  ui/android/  ?

Thanks!

[dtapuska, 2017-08-22 14:50:34]

On 2017/08/22 13:49:05, sunyunjia wrote:
> Hi Ted,
> 
> Could you please review this patch for the
> content/browser/android/  and  ui/android/  ?
> 
> Thanks!

lgtm % nits

[dtapuska, 2017-08-22 14:51:04]

https://codereview.chromium.org/2884423003/diff/600001/content/common/input_m...
File content/common/input_messages.h (right):

https://codereview.chromium.org/2884423003/diff/600001/content/common/input_m...
content/common/input_messages.h:88: kScrollBoundaryBehaviorTypeContain)
Can you add a max value in the enum value set? It catches the problem when
someone re-orders the values and then this code borks because of it.

https://codereview.chromium.org/2884423003/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2884423003/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1964:
static_cast<ScrollBoundaryBehaviorType>(scroll_boundary_behavior_x),
Is there someone where asserting that the CSS cast to the
ScrollBoundaryBehaviorType is the same?

https://codereview.chromium.org/2884423003/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/ScrollManager.h (right):

https://codereview.chromium.org/2884423003/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.h:15: #include
"platform/scroll/ScrollableArea.h"
Unclear why this is needed in the header file?

[majidvp, 2017-08-22 15:33:50]

On 2017/08/18 18:44:06, sunyunjia wrote:
> I figured out CanScroll() has more complicated logic than the 2-step we
figured
> out the other day (getScrollableArea then decide base on delta value). So I
> would leave CanScroll as it is and add the GetScrollableArea part to
> CanPropagate.
> 
> PTAL, thanks!
> 

lgtm

[sunyunjia, 2017-08-22 21:37:36]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-22 21:37:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-22 23:39:03]

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

[commit-bot: I haz the power, 2017-08-22 23:39:06]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2017-08-22 23:55:37]

https://codereview.chromium.org/2884423003/diff/600001/content/common/input_m...
File content/common/input_messages.h (right):

https://codereview.chromium.org/2884423003/diff/600001/content/common/input_m...
content/common/input_messages.h:88: kScrollBoundaryBehaviorTypeContain)
On 2017/08/22 14:51:04, dtapuska wrote:
> Can you add a max value in the enum value set? It catches the problem when
> someone re-orders the values and then this code borks because of it.

Done.

https://codereview.chromium.org/2884423003/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2884423003/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1964:
static_cast<ScrollBoundaryBehaviorType>(scroll_boundary_behavior_x),
On 2017/08/22 14:51:04, dtapuska wrote:
> Is there someone where asserting that the CSS cast to the
> ScrollBoundaryBehaviorType is the same?

Yes see
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Com...

https://codereview.chromium.org/2884423003/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/ScrollManager.h (right):

https://codereview.chromium.org/2884423003/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.h:15: #include
"platform/scroll/ScrollableArea.h"
On 2017/08/22 14:51:04, dtapuska wrote:
> Unclear why this is needed in the header file?

Done.

[sunyunjia, 2017-08-22 23:56:59]

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

[sunyunjia, 2017-08-22 23:58:46]

Hi boliu,

Could you please review this patch under

content/browser/android/ and ui/android ?

Thanks!

[boliu, 2017-08-23 19:17:03]

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
File content/browser/android/overscroll_controller_android.cc (left):

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:107:
DCHECK(compositor_);
ditto don't remove production checks

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:98:
OverscrollControllerAndroid::OverscrollControllerAndroid(
this is only for tests right? Make that clear by making a static factory method
CreateForTests instead

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:202:
event.data.scroll_update.previous_update_in_sequence_prevented)
style nit: need braces

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:317: if (compositor_)
for testing again, right?

should either set a non-null compositor in tests, or alternatively, make this
method virtual and override it in the test to no-op. preferably the first option

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
File content/browser/android/overscroll_controller_android_unittest.cc (right):

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:1: //
Copyright (c) 2017 The Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
File ui/android/overscroll_glow.cc (left):

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
ui/android/overscroll_glow.cc:42: DCHECK(client);
ditto fix test rather than relax production checks

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
File ui/android/overscroll_glow.h (right):

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
ui/android/overscroll_glow.h:50: virtual bool OnOverscrolled(base::TimeTicks
current_time,
ditto comment about virtual for mocking

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
File ui/android/overscroll_refresh.cc (right):

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
ui/android/overscroll_refresh.cc:30: if (handler_)
hmm, handler is constant, so if this one needs a null check, then they all need
null checks?

Looks like this is all for testing? In general, tests should not negatively
affect quality of production code. Can you make sure tests passes in a fake
handler here

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
File ui/android/overscroll_refresh.h (right):

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
ui/android/overscroll_refresh.h:33: ~OverscrollRefresh();
does the destructor need to be virtual too?

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
ui/android/overscroll_refresh.h:42: // scroll_boundary_behavior on y axis is
'auto'.
comment on virtual methods that they are virtual for mocking

[sunyunjia, 2017-08-23 23:42:50]

Patchset #28 (id:640001) has been deleted

[sunyunjia, 2017-08-23 23:46:49]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-23 23:46:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-08-23 23:47:06]

Hi Bo,

PTAL, thanks!

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
File content/browser/android/overscroll_controller_android.cc (left):

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:107:
DCHECK(compositor_);
On 2017/08/23 19:17:02, boliu wrote:
> ditto don't remove production checks

Done.

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:98:
OverscrollControllerAndroid::OverscrollControllerAndroid(
On 2017/08/23 19:17:02, boliu wrote:
> this is only for tests right? Make that clear by making a static factory
method
> CreateForTests instead

Done.

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:202:
event.data.scroll_update.previous_update_in_sequence_prevented)
On 2017/08/23 19:17:02, boliu wrote:
> style nit: need braces

Done.

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
content/browser/android/overscroll_controller_android.cc:317: if (compositor_)
On 2017/08/23 19:17:02, boliu wrote:
> for testing again, right?
> 
> should either set a non-null compositor in tests, or alternatively, make this
> method virtual and override it in the test to no-op. preferably the first
option

Done.

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
File content/browser/android/overscroll_controller_android_unittest.cc (right):

https://codereview.chromium.org/2884423003/diff/620001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:1: //
Copyright (c) 2017 The Chromium Authors. All rights reserved.
On 2017/08/23 19:17:02, boliu wrote:
> no (c)

Done.

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
File ui/android/overscroll_glow.cc (left):

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
ui/android/overscroll_glow.cc:42: DCHECK(client);
On 2017/08/23 19:17:02, boliu wrote:
> ditto fix test rather than relax production checks

Done.

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
File ui/android/overscroll_glow.h (right):

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
ui/android/overscroll_glow.h:50: virtual bool OnOverscrolled(base::TimeTicks
current_time,
On 2017/08/23 19:17:02, boliu wrote:
> ditto comment about virtual for mocking

Done.

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
File ui/android/overscroll_refresh.cc (right):

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
ui/android/overscroll_refresh.cc:30: if (handler_)
On 2017/08/23 19:17:02, boliu wrote:
> hmm, handler is constant, so if this one needs a null check, then they all
need
> null checks?
> 
> Looks like this is all for testing? In general, tests should not negatively
> affect quality of production code. Can you make sure tests passes in a fake
> handler here

Feels like it's a bit complicated to mock an OverscrollRefreshHandler as its
constructor needs some java object?
For now I created another constructor and restored all the DCHECKs. Please let
me know if you have any suggestions!

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
File ui/android/overscroll_refresh.h (right):

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
ui/android/overscroll_refresh.h:33: ~OverscrollRefresh();
On 2017/08/23 19:17:02, boliu wrote:
> does the destructor need to be virtual too?

Why does it need to be virtual?

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
ui/android/overscroll_refresh.h:42: // scroll_boundary_behavior on y axis is
'auto'.
On 2017/08/23 19:17:03, boliu wrote:
> comment on virtual methods that they are virtual for mocking

Done.

[commit-bot: I haz the power, 2017-08-23 23:50:04]

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

[commit-bot: I haz the power, 2017-08-23 23:50:06]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2017-08-24 00:06:53]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-24 00:07:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-24 02:09:36]

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

[commit-bot: I haz the power, 2017-08-24 02:09:38]

Dry run: This issue passed the CQ dry run.

[boliu, 2017-08-24 02:33:55]

lgtm % protect constructor for refresh

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
File ui/android/overscroll_refresh.h (right):

https://codereview.chromium.org/2884423003/diff/620001/ui/android/overscroll_...
ui/android/overscroll_refresh.h:33: ~OverscrollRefresh();
On 2017/08/23 23:47:06, sunyunjia wrote:
> On 2017/08/23 19:17:02, boliu wrote:
> > does the destructor need to be virtual too?
> 
> Why does it need to be virtual?

I guess simple and safe rule of thumb is if there are virtual methods (meaning
there might be subclasses), then should have a virtual destructor. In this it's
only a mock, so probably doesn't matter

https://codereview.chromium.org/2884423003/diff/680001/ui/android/overscroll_...
File ui/android/overscroll_refresh.h (right):

https://codereview.chromium.org/2884423003/diff/680001/ui/android/overscroll_...
ui/android/overscroll_refresh.h:34: OverscrollRefresh();
protected?

[sunyunjia, 2017-08-24 16:13:08]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-24 16:13:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-08-24 16:16:00]

sunyunjia@chromium.org changed reviewers:
- aelias@chromium.org, tedchoc@chromium.org

[sunyunjia, 2017-08-24 16:30:47]

sunyunjia@chromium.org changed reviewers:
+ avi@chromium.org, chrishtr@chromium.org

[sunyunjia, 2017-08-24 16:42:33]

sunyunjia@chromium.org changed reviewers:
- avi@chromium.org, chrishtr@chromium.org

[sunyunjia, 2017-08-24 16:42:36]

https://codereview.chromium.org/2884423003/diff/680001/ui/android/overscroll_...
File ui/android/overscroll_refresh.h (right):

https://codereview.chromium.org/2884423003/diff/680001/ui/android/overscroll_...
ui/android/overscroll_refresh.h:34: OverscrollRefresh();
On 2017/08/24 02:33:54, boliu wrote:
> protected?

Done.

[sunyunjia, 2017-08-24 16:47:36]

sunyunjia@chromium.org changed reviewers:
+ aelias@google.com, avi@chromium.org, chrishtr@chromium.org,
dcheng@chromium.org, tedchoc@chromium.org

[sunyunjia, 2017-08-24 16:47:38]

Hi chrishtr@, could you please review the code under cc/ and
third_party/WebKit/public?

avi@, could you please review the code under content/browser/android and
content/renderer?

dcheng@, could you please review the code in content/common/input_messages.h ?

Thank you very much!

[commit-bot: I haz the power, 2017-08-24 18:12:47]

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

[commit-bot: I haz the power, 2017-08-24 18:12:49]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2017-08-24 19:06:49]

sunyunjia@chromium.org changed reviewers:
+ aelias@chromium.org
- aelias@google.com

[Avi (use Gerrit), 2017-08-24 21:12:43]

lgtm

https://codereview.chromium.org/2884423003/diff/700001/content/browser/androi...
File content/browser/android/overscroll_controller_android.h (right):

https://codereview.chromium.org/2884423003/diff/700001/content/browser/androi...
content/browser/android/overscroll_controller_android.h:84: //
OverscrollGlowClient implementation.
extra blank line before this one.

[chrishtr, 2017-08-25 20:55:03]

chrishtr@chromium.org changed reviewers:
+ meade@chromium.org

[chrishtr, 2017-08-25 20:55:06]

+meade for style review of change to Document.cpp.

Other parts of Blink look fine, modulo the one inline
comment I made.

https://codereview.chromium.org/2884423003/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2884423003/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:90: if
(!element.GetLayoutBox()->GetScrollableArea())
Why did you need to change this?

[sunyunjia, 2017-08-25 21:19:44]

https://codereview.chromium.org/2884423003/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2884423003/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:90: if
(!element.GetLayoutBox()->GetScrollableArea())
On 2017/08/25 20:55:06, chrishtr wrote:
> Why did you need to change this?

This is a bug in the previous patch--the scroll-chaining part of
scroll-boundary-behavior. We would like the scroll-boundary-behavior affects
only the scrollable containers, not all the elements.

Specifically, if the body has scroll-boundary-behavior: contain, and it hasn't
been added to the scroll-chain because it's not scrollable, the scroll-chain
would be cut from that point. Then the overscroll message won't be generated at
all, because it is only generated at the rootscroller level, which is the
document. And we won't have the glow-effect we would still like to see with the
value "contain"

We don't need this check at cc side, because for now, cc handles overscroll
outside the scroll-chain. Cutting scroll-chain early won't affect overscroll.
See crbug.com/755164 .

You can find the discussion at
message143(https://codereview.chromium.org/2884423003/#msg143) and
message152(https://codereview.chromium.org/2884423003/#msg152)

[chrishtr, 2017-08-25 21:24:36]

On 2017/08/25 at 21:19:44, sunyunjia wrote:
>
https://codereview.chromium.org/2884423003/diff/700001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):
> 
>
https://codereview.chromium.org/2884423003/diff/700001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/input/ScrollManager.cpp:90: if
(!element.GetLayoutBox()->GetScrollableArea())
> On 2017/08/25 20:55:06, chrishtr wrote:
> > Why did you need to change this?
> 
> This is a bug in the previous patch--the scroll-chaining part of
scroll-boundary-behavior. We would like the scroll-boundary-behavior affects
only the scrollable containers, not all the elements.
> 
> Specifically, if the body has scroll-boundary-behavior: contain, and it hasn't
been added to the scroll-chain because it's not scrollable, the scroll-chain
would be cut from that point. Then the overscroll message won't be generated at
all, because it is only generated at the rootscroller level, which is the
document. And we won't have the glow-effect we would still like to see with the
value "contain"
> 
> We don't need this check at cc side, because for now, cc handles overscroll
outside the scroll-chain. Cutting scroll-chain early won't affect overscroll.
See crbug.com/755164 .
> 
> You can find the discussion at
message143(https://codereview.chromium.org/2884423003/#msg143) and
message152(https://codereview.chromium.org/2884423003/#msg152)

Ok. Could you split that off into another patch?

[chrishtr, 2017-08-25 21:24:55]

On 2017/08/25 at 21:24:36, chrishtr wrote:
> On 2017/08/25 at 21:19:44, sunyunjia wrote:
> >
https://codereview.chromium.org/2884423003/diff/700001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):
> > 
> >
https://codereview.chromium.org/2884423003/diff/700001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/input/ScrollManager.cpp:90: if
(!element.GetLayoutBox()->GetScrollableArea())
> > On 2017/08/25 20:55:06, chrishtr wrote:
> > > Why did you need to change this?
> > 
> > This is a bug in the previous patch--the scroll-chaining part of
scroll-boundary-behavior. We would like the scroll-boundary-behavior affects
only the scrollable containers, not all the elements.
> > 
> > Specifically, if the body has scroll-boundary-behavior: contain, and it
hasn't been added to the scroll-chain because it's not scrollable, the
scroll-chain would be cut from that point. Then the overscroll message won't be
generated at all, because it is only generated at the rootscroller level, which
is the document. And we won't have the glow-effect we would still like to see
with the value "contain"
> > 
> > We don't need this check at cc side, because for now, cc handles overscroll
outside the scroll-chain. Cutting scroll-chain early won't affect overscroll.
See crbug.com/755164 .
> > 
> > You can find the discussion at
message143(https://codereview.chromium.org/2884423003/#msg143) and
message152(https://codereview.chromium.org/2884423003/#msg152)
> 
> Ok. Could you split that off into another patch?

(with unittest)

[dcheng, 2017-08-25 23:12:18]

I guess this works out for OOPIF, since it's main-frame only, but at some point,
we should probably figure out if there's any other problematic bits here for
OOPIF (if people are attending at BlinkOn...)

https://codereview.chromium.org/2884423003/diff/700001/cc/input/scroll_bounda...
File cc/input/scroll_boundary_behavior.h (right):

https://codereview.chromium.org/2884423003/diff/700001/cc/input/scroll_bounda...
cc/input/scroll_boundary_behavior.h:22: kScrollBoundaryBehaviorTypeMax = 3
Please write this as kScrollBoundaryBehaviorTypeMax =
kScrollBoundaryBehaviorTypeContain

https://codereview.chromium.org/2884423003/diff/700001/cc/input/scroll_state.h
File cc/input/scroll_state.h (right):

https://codereview.chromium.org/2884423003/diff/700001/cc/input/scroll_state....
cc/input/scroll_state.h:100: void set_scroll_chain_cut(bool cut) {
data_.is_scroll_chain_cut = cut; }
Nit: for symmetry I would name this set_is_scroll_chain_cut (for example, see
line 63-64 in this file)

https://codereview.chromium.org/2884423003/diff/700001/content/browser/androi...
File content/browser/android/overscroll_controller_android_unittest.cc (right):

https://codereview.chromium.org/2884423003/diff/700001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:75: controller
= OverscrollControllerAndroid::CreateForTests(
Doesn't this leak? Who deletes it?

I'd recommend changing CreateForTests to return a unique_ptr.

(Similar question about the MockCompositor object that's new'd)

https://codereview.chromium.org/2884423003/diff/700001/ui/android/overscroll_...
File ui/android/overscroll_glow.h (right):

https://codereview.chromium.org/2884423003/diff/700001/ui/android/overscroll_...
ui/android/overscroll_glow.h:53: gfx::Vector2dF overscroll_delta,
Not introduced by this CL, but shouldn't these all be uniformly const
gfx::Vector2dF& or consistently pass by value?

[Ted C, 2017-08-28 18:58:48]

lgtm w/ existing comments addressed

https://codereview.chromium.org/2884423003/diff/700001/content/browser/androi...
File content/browser/android/overscroll_controller_android.h (right):

https://codereview.chromium.org/2884423003/diff/700001/content/browser/androi...
content/browser/android/overscroll_controller_android.h:78: // This method
should only be called from CreateForTest.
CreateForTest[s]

[aelias_OOO_until_Jul13, 2017-08-28 21:23:02]

lgtm w/ existing comments addressed

[sunyunjia, 2017-08-29 17:18:24]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-29 17:18:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-08-29 17:32:31]

Hi chrishtr and dcheng,

For the OOPIF case, I've filed a bug as crbug.com/760209 and will fix it ASAP.
I'm attending this coming BlinkOn as well. 

PTAL, thanks!

https://codereview.chromium.org/2884423003/diff/700001/cc/input/scroll_bounda...
File cc/input/scroll_boundary_behavior.h (right):

https://codereview.chromium.org/2884423003/diff/700001/cc/input/scroll_bounda...
cc/input/scroll_boundary_behavior.h:22: kScrollBoundaryBehaviorTypeMax = 3
On 2017/08/25 23:12:17, dcheng wrote:
> Please write this as kScrollBoundaryBehaviorTypeMax =
> kScrollBoundaryBehaviorTypeContain

Done.

https://codereview.chromium.org/2884423003/diff/700001/cc/input/scroll_state.h
File cc/input/scroll_state.h (right):

https://codereview.chromium.org/2884423003/diff/700001/cc/input/scroll_state....
cc/input/scroll_state.h:100: void set_scroll_chain_cut(bool cut) {
data_.is_scroll_chain_cut = cut; }
On 2017/08/25 23:12:17, dcheng wrote:
> Nit: for symmetry I would name this set_is_scroll_chain_cut (for example, see
> line 63-64 in this file)

Done.

https://codereview.chromium.org/2884423003/diff/700001/content/browser/androi...
File content/browser/android/overscroll_controller_android.h (right):

https://codereview.chromium.org/2884423003/diff/700001/content/browser/androi...
content/browser/android/overscroll_controller_android.h:78: // This method
should only be called from CreateForTest.
On 2017/08/28 18:58:48, Ted C wrote:
> CreateForTest[s]

Done.

https://codereview.chromium.org/2884423003/diff/700001/content/browser/androi...
content/browser/android/overscroll_controller_android.h:84: //
OverscrollGlowClient implementation.
On 2017/08/24 21:12:43, Avi (ping after 24h) wrote:
> extra blank line before this one.

Done.

https://codereview.chromium.org/2884423003/diff/700001/content/browser/androi...
File content/browser/android/overscroll_controller_android_unittest.cc (right):

https://codereview.chromium.org/2884423003/diff/700001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:75: controller
= OverscrollControllerAndroid::CreateForTests(
On 2017/08/25 23:12:17, dcheng wrote:
> Doesn't this leak? Who deletes it?
> 
> I'd recommend changing CreateForTests to return a unique_ptr.
> 
> (Similar question about the MockCompositor object that's new'd)

Done.

https://codereview.chromium.org/2884423003/diff/700001/ui/android/overscroll_...
File ui/android/overscroll_glow.h (right):

https://codereview.chromium.org/2884423003/diff/700001/ui/android/overscroll_...
ui/android/overscroll_glow.h:53: gfx::Vector2dF overscroll_delta,
On 2017/08/25 23:12:17, dcheng wrote:
> Not introduced by this CL, but shouldn't these all be uniformly const
> gfx::Vector2dF& or consistently pass by value?

Done.

[commit-bot: I haz the power, 2017-08-29 19:26:29]

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

[commit-bot: I haz the power, 2017-08-29 19:26:32]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sunyunjia, 2017-08-29 19:46:25]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-29 19:46:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-29 21:22:33]

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

[commit-bot: I haz the power, 2017-08-29 21:22:36]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2017-08-30 19:45:35]

sunyunjia@chromium.org changed reviewers:
+ kenrb@chromium.org

[sunyunjia, 2017-08-30 19:47:01]

Hi kenrb@,

Could you please do owner's review for content/common/input_messages.h ? Thanks!

[kenrb, 2017-08-30 21:02:18]

ipc lgtm

[sunyunjia, 2017-08-30 21:03:40]

The CQ bit was checked by sunyunjia@chromium.org

[sunyunjia, 2017-08-30 21:03:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
majidvp@chromium.org, dtapuska@chromium.org, boliu@chromium.org,
aelias@chromium.org, tedchoc@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2884423003/#ps740001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-08-30 21:03:55]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-08-30 23:27:27]

https://codereview.chromium.org/2884423003/diff/740001/content/browser/androi...
File content/browser/android/overscroll_controller_android_unittest.cc (right):

https://codereview.chromium.org/2884423003/diff/740001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:74:
base::MakeUnique<MockCompositor>();
Doesn't compositor_ptr have to outlive the scope of this function? I'm not
seeing what takes ownership of it, so I think it's going to be destroyed before
any of the test code actually runs?

Basically, I think |compositor_ptr| should be a class member too? Am I missing
something?

https://codereview.chromium.org/2884423003/diff/740001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:92:
std::unique_ptr<OverscrollControllerAndroid> controller;
Minor nit: these should have _ suffixed and be protected.

[commit-bot: I haz the power, 2017-08-30 23:40:13]

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

[commit-bot: I haz the power, 2017-08-30 23:40:17]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sunyunjia, 2017-08-31 00:07:54]

PTAL, thanks!

https://codereview.chromium.org/2884423003/diff/740001/content/browser/androi...
File content/browser/android/overscroll_controller_android_unittest.cc (right):

https://codereview.chromium.org/2884423003/diff/740001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:74:
base::MakeUnique<MockCompositor>();
On 2017/08/30 23:27:27, dcheng wrote:
> Doesn't compositor_ptr have to outlive the scope of this function? I'm not
> seeing what takes ownership of it, so I think it's going to be destroyed
before
> any of the test code actually runs?
> 
> Basically, I think |compositor_ptr| should be a class member too? Am I missing
> something?

Done.

https://codereview.chromium.org/2884423003/diff/740001/content/browser/androi...
content/browser/android/overscroll_controller_android_unittest.cc:92:
std::unique_ptr<OverscrollControllerAndroid> controller;
On 2017/08/30 23:27:27, dcheng wrote:
> Minor nit: these should have _ suffixed and be protected.

Done.

[sunyunjia, 2017-08-31 00:09:33]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-31 00:09:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-08-31 00:58:55]

lgtm

[commit-bot: I haz the power, 2017-08-31 02:49:10]

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

[commit-bot: I haz the power, 2017-08-31 02:49:12]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2017-08-31 02:51:33]

The CQ bit was checked by sunyunjia@chromium.org

[sunyunjia, 2017-08-31 02:51:34]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
kenrb@chromium.org, boliu@chromium.org, tedchoc@chromium.org,
majidvp@chromium.org, avi@chromium.org, dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/2884423003/#ps760001
(title: "Update the members in the test.")

[commit-bot: I haz the power, 2017-08-31 02:51:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-31 03:02:22]

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

[commit-bot: I haz the power, 2017-08-31 03:02:25]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sunyunjia, 2017-08-31 03:20:29]

The CQ bit was checked by sunyunjia@chromium.org

[sunyunjia, 2017-08-31 03:20:30]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
kenrb@chromium.org, dcheng@chromium.org, boliu@chromium.org,
tedchoc@chromium.org, majidvp@chromium.org, avi@chromium.org,
dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/2884423003/#ps780001
(title: "Rebase")

[commit-bot: I haz the power, 2017-08-31 03:20:44]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-31 05:35:25]

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

[commit-bot: I haz the power, 2017-08-31 05:35:29]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sunyunjia, 2017-08-31 10:22:42]

The CQ bit was checked by sunyunjia@chromium.org

[commit-bot: I haz the power, 2017-08-31 10:22:59]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-31 11:18:57]

CQ is committing da patch.

Bot data: {"patchset_id": 780001, "attempt_start_ts": 1504174962583570,
"parent_rev": "b65f4bf201bd2220bab6cf83887e071a714afcee", "commit_rev":
"bbea8a99d2643b97d9dfe9d368cba6d129850a28"}

[commit-bot: I haz the power, 2017-08-31 11:19:20]

Description was changed from

==========
Use scroll-boundary-behavior to control overscroll-refresh/glow on android.

Overscroll navigation is a feature of Chrome. Users can pull verticallly to
refresh the page on android, or swipe horizontally to browse history on desktop
platform. Overscroll-glow is a visual effect indicating user that his scroll
has passed the extent of the scroller. However, for some mobile web
applications, navigation or glow does not make sense. This patch allows users
to use a css attribute: scroll-boundary-behavior on the viewport defining
element in order to control overscroll-navigation and overscroll-glow.

On main thread, we set this property in OverscrollController, and on the
compositor thread, we plumb this property to LayerTreeImpl. When overscroll
happens, scroll-boundary-behavior is bundled in did_overscroll_params and is
passed to OverscrollControllerAndroid::Overscrolled(). Thus, we make the
decision of whether to pull-to-refresh and whether to glow on Overscrolled()
as opposed to immediately after ScrollUpdateAck.

Spec: https://wicg.github.io/scroll-boundary-behavior/
Design Doc:
https://docs.google.com/a/google.com/document/d/1cqyeFamVmSu5EN5sBpQFB3Madm-4...

BUG=656801
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Use scroll-boundary-behavior to control overscroll-refresh/glow on android.

Overscroll navigation is a feature of Chrome. Users can pull verticallly to
refresh the page on android, or swipe horizontally to browse history on desktop
platform. Overscroll-glow is a visual effect indicating user that his scroll
has passed the extent of the scroller. However, for some mobile web
applications, navigation or glow does not make sense. This patch allows users
to use a css attribute: scroll-boundary-behavior on the viewport defining
element in order to control overscroll-navigation and overscroll-glow.

On main thread, we set this property in OverscrollController, and on the
compositor thread, we plumb this property to LayerTreeImpl. When overscroll
happens, scroll-boundary-behavior is bundled in did_overscroll_params and is
passed to OverscrollControllerAndroid::Overscrolled(). Thus, we make the
decision of whether to pull-to-refresh and whether to glow on Overscrolled()
as opposed to immediately after ScrollUpdateAck.

Spec: https://wicg.github.io/scroll-boundary-behavior/
Design Doc:
https://docs.google.com/a/google.com/document/d/1cqyeFamVmSu5EN5sBpQFB3Madm-4...

BUG=656801
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2884423003
Cr-Commit-Position: refs/heads/master@{#498816}
Committed:
https://chromium.googlesource.com/chromium/src/+/bbea8a99d2643b97d9dfe9d368cb...
==========

[commit-bot: I haz the power, 2017-08-31 11:19:24]

Committed patchset #34 (id:780001) as
https://chromium.googlesource.com/chromium/src/+/bbea8a99d2643b97d9dfe9d368cb...