Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1102743002:
OverscrollGlow for mainThread-{CHROMIUM CHANGES} (Closed)
|
Created:
5 years, 8 months ago by MuVen Modified:
5 years, 6 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nasko+codewatch_chromium.org, jam, penghuang+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, mkwst+moarreviews-shell_chromium.org, James Su, jochen+watch_chromium.org, sivag Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOverscrollGlow for mainThread.
Current patch passes on the mainthread overscroll params to
RWHVA::DidOverscroll which shall trigger glow effect animation.
Blink changes @ https://codereview.chromium.org/1056983004/
BUG=420207
Committed: https://crrev.com/582c9cedbf4fa60162fca35251b1244f5ba3b2de
Cr-Commit-Position: refs/heads/master@{#333460}
Patch Set 1 #
Total comments: 8
Patch Set 2 : addressed review comments #Patch Set 3 : addressed jared comments #Patch Set 4 : addressed majidvp comments #Patch Set 5 : rebased to latest #
Total comments: 2
Patch Set 6 : addressed review comments #
Total comments: 6
Patch Set 7 : addressed review comments #
Total comments: 1
Patch Set 8 : addressed nits #Patch Set 9 : rebased to latest #Messages
Total messages: 26 (10 generated)
sataya.m@samsung.com changed reviewers: + jdduke@chromium.org
sataya.m@samsung.com changed reviewers: + majidvp@chromium.org
Hi Jdduke, PTAL of the changes, This is just a working prototype, Based on your comments i shall modify further. Thanks,
https://codereview.chromium.org/1102743002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1102743002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:768: void RenderWidgetHostViewAndroid::OnDidOverScrollOnMainThread( Looks like we don't make any distinctino between main/impl overscroll, so we should get this for free if we reuse the existing overscroll message. https://codereview.chromium.org/1102743002/diff/1/content/renderer/render_vie... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/1102743002/diff/1/content/renderer/render_vie... content/renderer/render_view_impl.h:333: virtual void didOverScrollOnMainThread(const float& unusedDeltaX, Do we need to override this? Can we just have the implemention in RenderWidget? https://codereview.chromium.org/1102743002/diff/1/content/renderer/render_wid... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1102743002/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:2248: Send(new ViewHostMsg_DidOverScrollOnMainThread(routing_id(), unusedDelta, Can't we just resuse the existing InputHostMsg_DidOverscroll message? https://code.google.com/p/chromium/codesearch#chromium/src/content/common/inp... https://codereview.chromium.org/1102743002/diff/1/content/shell/android/java/... File content/shell/android/java/src/org/chromium/content_shell/ShellManager.java (right): https://codereview.chromium.org/1102743002/diff/1/content/shell/android/java/... content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:30: public static final String DEFAULT_SHELL_URL = "http://jsbin.com/cikunu/2/quiet"; Hmm, not sure about this change?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1102743002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1102743002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:768: void RenderWidgetHostViewAndroid::OnDidOverScrollOnMainThread( On 2015/04/24 16:57:23, jdduke wrote: > Looks like we don't make any distinctino between main/impl overscroll, so we > should get this for free if we reuse the existing overscroll message. Done. https://codereview.chromium.org/1102743002/diff/1/content/renderer/render_wid... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1102743002/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:2248: Send(new ViewHostMsg_DidOverScrollOnMainThread(routing_id(), unusedDelta, On 2015/04/24 16:57:23, jdduke wrote: > Can't we just resuse the existing InputHostMsg_DidOverscroll message? > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/inp... we can call InputHostMsg_DidOverscroll from Render_view_impl itself ? https://codereview.chromium.org/1102743002/diff/1/content/shell/android/java/... File content/shell/android/java/src/org/chromium/content_shell/ShellManager.java (right): https://codereview.chromium.org/1102743002/diff/1/content/shell/android/java/... content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:30: public static final String DEFAULT_SHELL_URL = "http://jsbin.com/cikunu/2/quiet"; On 2015/04/24 16:57:23, jdduke wrote: > Hmm, not sure about this change? This is just for testing. Once feature is stabilized i shall remove this.
https://codereview.chromium.org/1102743002/diff/1/content/renderer/render_wid... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1102743002/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:2248: Send(new ViewHostMsg_DidOverScrollOnMainThread(routing_id(), unusedDelta, On 2015/04/27 09:49:15, MuVen wrote: > On 2015/04/24 16:57:23, jdduke wrote: > > Can't we just resuse the existing InputHostMsg_DidOverscroll message? > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/inp... > > we can call InputHostMsg_DidOverscroll from Render_view_impl itself ? That's probably fine. To be honest the distribution of methods between RW/RVI doesn't seem to be very consistent. I guess the argument to put it in RenderWidget would be that the message gets handled by RenderWidgetHostImp (by way of the InputRouter), so I have a slight preference for putting the implementation there.
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/1102743002/diff/140001/content/renderer/rende... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/1102743002/diff/140001/content/renderer/rende... content/renderer/render_view_impl.h:334: void didOverscroll(const blink::WebFloatSize& unusedDelta, Do we need this here? Why can't we just override |didOverscroll| in RenderWidget? It's a WebWidgetClient.
PTAL. https://codereview.chromium.org/1102743002/diff/140001/content/renderer/rende... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/1102743002/diff/140001/content/renderer/rende... content/renderer/render_view_impl.h:334: void didOverscroll(const blink::WebFloatSize& unusedDelta, On 2015/06/04 18:40:04, jdduke(OOO_until_June_8) wrote: > Do we need this here? Why can't we just override |didOverscroll| in > RenderWidget? It's a WebWidgetClient. true, not required.
https://codereview.chromium.org/1102743002/diff/160001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1102743002/diff/160001/content/renderer/rende... content/renderer/render_widget.cc:2230: DidOverscrollParams params; Can you add a "TODO(jdduke): Consider bundling the overscroll with the input event ack to save an IPC." That's what we do with the compositor (see https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/i...). https://codereview.chromium.org/1102743002/diff/160001/content/renderer/rende... File content/renderer/render_widget.h (right): https://codereview.chromium.org/1102743002/diff/160001/content/renderer/rende... content/renderer/render_widget.h:194: Nit: No need for line break here. https://codereview.chromium.org/1102743002/diff/160001/content/renderer/rende... content/renderer/render_widget.h:195: void didOverscroll(const blink::WebFloatSize& unusedDelta, virtual?
https://codereview.chromium.org/1102743002/diff/160001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1102743002/diff/160001/content/renderer/rende... content/renderer/render_widget.cc:2230: DidOverscrollParams params; On 2015/06/05 14:32:51, jdduke(OOO_until_June_8) wrote: > Can you add a > > "TODO(jdduke): Consider bundling the overscroll with the input event ack to save > an IPC." > > That's what we do with the compositor (see > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/i...). Done. https://codereview.chromium.org/1102743002/diff/160001/content/renderer/rende... File content/renderer/render_widget.h (right): https://codereview.chromium.org/1102743002/diff/160001/content/renderer/rende... content/renderer/render_widget.h:194: On 2015/06/05 14:32:51, jdduke(OOO_until_June_8) wrote: > Nit: No need for line break here. Done. https://codereview.chromium.org/1102743002/diff/160001/content/renderer/rende... content/renderer/render_widget.h:195: void didOverscroll(const blink::WebFloatSize& unusedDelta, On 2015/06/05 14:32:51, jdduke(OOO_until_June_8) wrote: > virtual? Done.
lgtm, but you'l need a content/renderer owner. https://codereview.chromium.org/1102743002/diff/180001/content/renderer/rende... File content/renderer/render_widget.h (right): https://codereview.chromium.org/1102743002/diff/180001/content/renderer/rende... content/renderer/render_widget.h:194: virtual void didOverscroll( Nit: This should be in the same order as the declaration (after didHandleGestureEvent).
sataya.m@samsung.com changed reviewers: + sievers@chromium.org
sievers, PTAL. Thanks, Muven.
lgtm. Can you reference the Blink patch in the cl description since it depends on it?
The CQ bit was checked by sataya.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1102743002/#ps220001 (title: "rebased to latest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1102743002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by sataya.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1102743002/220001
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/582c9cedbf4fa60162fca35251b1244f5ba3b2de Cr-Commit-Position: refs/heads/master@{#333460} |
