Chromium Code Reviews
|
|
|
Created:
4 years, 10 months ago by hush (inactive) Modified:
4 years, 10 months ago CC:
blink-reviews, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdd "targetViewport" to ScrollBegin and FlingStart
If the bit is true, the scroll will be targeted to the viewport scroll
layer. If false, the scroll may or may not be targeted to the viewport
scroll layer, depending on the hit test.
BUG=493765
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197411
Patch Set 1 #
Total comments: 2
Patch Set 2 : comments #Messages
Total messages: 24 (8 generated)
hush@chromium.org changed reviewers: + bokan@chromium.org
PTAL
lgtm % comment. https://codereview.chromium.org/1187633003/diff/1/public/web/WebInputEvent.h File public/web/WebInputEvent.h (right): https://codereview.chromium.org/1187633003/diff/1/public/web/WebInputEvent.h#... public/web/WebInputEvent.h:472: // True if the scroll is targeted to viewport. I would make the comment more explicit that we'll skip hit testing. Something along the lines of: "If true, this event will skip hit testing to find a scroll target and, instead, scroll the viewport."
On 2015/06/16 14:38:04, bokan wrote: > lgtm % comment. > > https://codereview.chromium.org/1187633003/diff/1/public/web/WebInputEvent.h > File public/web/WebInputEvent.h (right): > > https://codereview.chromium.org/1187633003/diff/1/public/web/WebInputEvent.h#... > public/web/WebInputEvent.h:472: // True if the scroll is targeted to viewport. > I would make the comment more explicit that we'll skip hit testing. Something > along the lines of: "If true, this event will skip hit testing to find a scroll > target and, instead, scroll the viewport." Do we need to fix the scroll targeting code in Blink as well? Or can we just assume that anything that targets the viewport can be scrolled on the impl thread?
On 2015/06/16 16:10:06, jdduke wrote: > On 2015/06/16 14:38:04, bokan wrote: > > lgtm % comment. > > > > https://codereview.chromium.org/1187633003/diff/1/public/web/WebInputEvent.h > > File public/web/WebInputEvent.h (right): > > > > > https://codereview.chromium.org/1187633003/diff/1/public/web/WebInputEvent.h#... > > public/web/WebInputEvent.h:472: // True if the scroll is targeted to viewport. > > I would make the comment more explicit that we'll skip hit testing. Something > > along the lines of: "If true, this event will skip hit testing to find a > scroll > > target and, instead, scroll the viewport." > > Do we need to fix the scroll targeting code in Blink as well? Or can we just > assume that anything that targets the viewport can be scrolled on the impl > thread? I don't think we can assume that in general so we'll need an implementation in Blink too for completeness. (I believe there's still a few ways we can hit main thread scrolling, e.g. background-attachment:fixed).
On 2015/06/16 16:13:01, bokan wrote: > On 2015/06/16 16:10:06, jdduke wrote: > > On 2015/06/16 14:38:04, bokan wrote: > > > lgtm % comment. > > > > > > https://codereview.chromium.org/1187633003/diff/1/public/web/WebInputEvent.h > > > File public/web/WebInputEvent.h (right): > > > > > > > > > https://codereview.chromium.org/1187633003/diff/1/public/web/WebInputEvent.h#... > > > public/web/WebInputEvent.h:472: // True if the scroll is targeted to > viewport. > > > I would make the comment more explicit that we'll skip hit testing. > Something > > > along the lines of: "If true, this event will skip hit testing to find a > > scroll > > > target and, instead, scroll the viewport." > > > > Do we need to fix the scroll targeting code in Blink as well? Or can we just > > assume that anything that targets the viewport can be scrolled on the impl > > thread? > > I don't think we can assume that in general so we'll need an implementation in > Blink too for completeness. (I believe there's still a few ways we can hit main > thread scrolling, e.g. background-attachment:fixed). I will file a bug about it. We're not regressing anything as far as I know. The previous implementation of WebView#flingScroll() always targets the viewport layer and assumes it is a impl-side scroll.
On 2015/06/18 20:22:01, hush wrote: > On 2015/06/16 16:13:01, bokan wrote: > > On 2015/06/16 16:10:06, jdduke wrote: > > > On 2015/06/16 14:38:04, bokan wrote: > > > > lgtm % comment. > > > > > > > > > https://codereview.chromium.org/1187633003/diff/1/public/web/WebInputEvent.h > > > > File public/web/WebInputEvent.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1187633003/diff/1/public/web/WebInputEvent.h#... > > > > public/web/WebInputEvent.h:472: // True if the scroll is targeted to > > viewport. > > > > I would make the comment more explicit that we'll skip hit testing. > > Something > > > > along the lines of: "If true, this event will skip hit testing to find a > > > scroll > > > > target and, instead, scroll the viewport." > > > > > > Do we need to fix the scroll targeting code in Blink as well? Or can we just > > > assume that anything that targets the viewport can be scrolled on the impl > > > thread? > > > > I don't think we can assume that in general so we'll need an implementation in > > Blink too for completeness. (I believe there's still a few ways we can hit > main > > thread scrolling, e.g. background-attachment:fixed). > > I will file a bug about it. > We're not regressing anything as far as I know. > The previous implementation of WebView#flingScroll() always targets the viewport > layer and assumes it is a impl-side scroll. Yup, it's mostly an edge case that we ignore today. It's probably fine to just wait for scroll-unification.
https://codereview.chromium.org/1187633003/diff/1/public/web/WebInputEvent.h File public/web/WebInputEvent.h (right): https://codereview.chromium.org/1187633003/diff/1/public/web/WebInputEvent.h#... public/web/WebInputEvent.h:472: // True if the scroll is targeted to viewport. On 2015/06/16 14:38:04, bokan wrote: > I would make the comment more explicit that we'll skip hit testing. Something > along the lines of: "If true, this event will skip hit testing to find a scroll > target and, instead, scroll the viewport." Done.
The CQ bit was checked by hush@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org Link to the patchset: https://codereview.chromium.org/1187633003/#ps20001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1187633003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)
hush@chromium.org changed reviewers: + dcheng@chromium.org
Daniel, PTAL
bokan@ knows this part of the code a lot better than me, so rs lgtm
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1187633003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59623)
The CQ bit was checked by hush@chromium.org
On 2015/06/18 22:18:48, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_blink_rel on tryserver.blink (JOB_FAILED, > http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59623) flaky tests...
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1187633003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197411 |
