Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
868123002:
Reset gesture detection upon page navigation for Aura (Closed)
|
Created:
5 years, 11 months ago by lanwei Modified:
5 years, 10 months ago CC:
chromium-reviews, creis+watch_chromium.org, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, nasko+codewatch_chromium.org, jam, penghuang+watch_chromium.org, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su, Rick Byers Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReset gesture detection upon page navigation for Aura
In order to stop the gesture events to be generated and sent while navigating
to a different page on the main frame or the window loses focus, we decide
to cancel all the active touch events on the current Aura window before
the touch release event.
BUG=418402
Committed: https://crrev.com/9d343ad2ea6ec395c377a4efa266057155bfa9c1
Cr-Commit-Position: refs/heads/master@{#315705}
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Rename the method #
Total comments: 11
Patch Set 3 : #
Total comments: 4
Patch Set 4 : Change comment #Messages
Total messages: 30 (11 generated)
lanwei@chromium.org changed reviewers: + tdresser@chromium.org
lanwei@chromium.org changed reviewers: + jdduke@chromium.org, rbyers@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
lanwei@chromium.org changed reviewers: + sadrul@chromium.org
I thought we decided on giving RWHVB a "DidNavigate" method? We can't call the method ResetGestures(), as it doesn't always reset gestures. https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_aura.cc:2142: Get rid of whitespace changes in this file. https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_base.cc:699: // We only provide implementation in the RenderWidgetHostViewAura. This comment isn't correct, as RWHVAndroid also has an implementation. Also, won't this method be called on non-Android non-Aura platforms? https://codereview.chromium.org/868123002/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/868123002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2609: // Cancel all the active touch events on the aura window to prevent they This comment breaks some layering assumptions. You shouldn't reference aura here. https://codereview.chromium.org/868123002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2613: rwhv->ResetGestureDetection(); Won't this call Android's ResetGestureDetection() method? I thought we decided to name the method "DidNavigate()" and on Aura, we'd cancel active touches on DidNavigate(). https://codereview.chromium.org/868123002/diff/60001/content/public/browser/r... File content/public/browser/render_widget_host_view.h (right): https://codereview.chromium.org/868123002/diff/60001/content/public/browser/r... content/public/browser/render_widget_host_view.h:141: // Cancel all the active touch events for the current window, only for Aura. We shouldn't reference Aura here, and this shouldn't be called ResetGestureDetection, as it doesn't always reset gesture detection.
https://codereview.chromium.org/868123002/diff/60001/content/public/browser/r... File content/public/browser/render_widget_host_view.h (right): https://codereview.chromium.org/868123002/diff/60001/content/public/browser/r... content/public/browser/render_widget_host_view.h:141: // Cancel all the active touch events for the current window, only for Aura. On 2015/01/24 14:29:01, tdresser wrote: > We shouldn't reference Aura here, and this shouldn't be called > ResetGestureDetection, as it doesn't always reset gesture detection. Would it be crazy to make this callback more generic, e.g., "OnMainFrameNavigation" or something like that?
On 2015/01/26 16:09:00, jdduke wrote: > https://codereview.chromium.org/868123002/diff/60001/content/public/browser/r... > File content/public/browser/render_widget_host_view.h (right): > > https://codereview.chromium.org/868123002/diff/60001/content/public/browser/r... > content/public/browser/render_widget_host_view.h:141: // Cancel all the active > touch events for the current window, only for Aura. > On 2015/01/24 14:29:01, tdresser wrote: > > We shouldn't reference Aura here, and this shouldn't be called > > ResetGestureDetection, as it doesn't always reset gesture detection. > > Would it be crazy to make this callback more generic, e.g., > "OnMainFrameNavigation" or something like that? That name sounds good to me.
rbyers@chromium.org changed reviewers: - rbyers@chromium.org
https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_aura.cc:2142: On 2015/01/24 14:29:00, tdresser wrote: > Get rid of whitespace changes in this file. Done. https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_base.cc:699: // We only provide implementation in the RenderWidgetHostViewAura. On 2015/01/24 14:29:00, tdresser wrote: > This comment isn't correct, as RWHVAndroid also has an implementation. > > Also, won't this method be called on non-Android non-Aura platforms? > Done. https://codereview.chromium.org/868123002/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/868123002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2609: // Cancel all the active touch events on the aura window to prevent they On 2015/01/24 14:29:01, tdresser wrote: > This comment breaks some layering assumptions. You shouldn't reference aura > here. Done. https://codereview.chromium.org/868123002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2613: rwhv->ResetGestureDetection(); On 2015/01/24 14:29:01, tdresser wrote: > Won't this call Android's ResetGestureDetection() method? > > I thought we decided to name the method "DidNavigate()" and on Aura, we'd cancel > active touches on DidNavigate(). Done. https://codereview.chromium.org/868123002/diff/60001/content/public/browser/r... File content/public/browser/render_widget_host_view.h (right): https://codereview.chromium.org/868123002/diff/60001/content/public/browser/r... content/public/browser/render_widget_host_view.h:141: // Cancel all the active touch events for the current window, only for Aura. On 2015/01/24 14:29:01, tdresser wrote: > We shouldn't reference Aura here, and this shouldn't be called > ResetGestureDetection, as it doesn't always reset gesture detection. Done. https://codereview.chromium.org/868123002/diff/60001/content/public/browser/r... content/public/browser/render_widget_host_view.h:141: // Cancel all the active touch events for the current window, only for Aura. On 2015/01/26 16:09:00, jdduke wrote: > On 2015/01/24 14:29:01, tdresser wrote: > > We shouldn't reference Aura here, and this shouldn't be called > > ResetGestureDetection, as it doesn't always reset gesture detection. > > Would it be crazy to make this callback more generic, e.g., > "OnMainFrameNavigation" or something like that? Done.
On 2015/01/30 14:21:50, lanwei wrote: > https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... > File content/browser/renderer_host/render_widget_host_view_aura.cc (right): > > https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_aura.cc:2142: > On 2015/01/24 14:29:00, tdresser wrote: > > Get rid of whitespace changes in this file. > > Done. > > https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... > File content/browser/renderer_host/render_widget_host_view_base.cc (right): > > https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_base.cc:699: // We only > provide implementation in the RenderWidgetHostViewAura. > On 2015/01/24 14:29:00, tdresser wrote: > > This comment isn't correct, as RWHVAndroid also has an implementation. > > > > Also, won't this method be called on non-Android non-Aura platforms? > > > > Done. > > https://codereview.chromium.org/868123002/diff/60001/content/browser/web_cont... > File content/browser/web_contents/web_contents_impl.cc (right): > > https://codereview.chromium.org/868123002/diff/60001/content/browser/web_cont... > content/browser/web_contents/web_contents_impl.cc:2609: // Cancel all the active > touch events on the aura window to prevent they > On 2015/01/24 14:29:01, tdresser wrote: > > This comment breaks some layering assumptions. You shouldn't reference aura > > here. > > Done. > > https://codereview.chromium.org/868123002/diff/60001/content/browser/web_cont... > content/browser/web_contents/web_contents_impl.cc:2613: > rwhv->ResetGestureDetection(); > On 2015/01/24 14:29:01, tdresser wrote: > > Won't this call Android's ResetGestureDetection() method? > > > > I thought we decided to name the method "DidNavigate()" and on Aura, we'd > cancel > > active touches on DidNavigate(). > > Done. > > https://codereview.chromium.org/868123002/diff/60001/content/public/browser/r... > File content/public/browser/render_widget_host_view.h (right): > > https://codereview.chromium.org/868123002/diff/60001/content/public/browser/r... > content/public/browser/render_widget_host_view.h:141: // Cancel all the active > touch events for the current window, only for Aura. > On 2015/01/24 14:29:01, tdresser wrote: > > We shouldn't reference Aura here, and this shouldn't be called > > ResetGestureDetection, as it doesn't always reset gesture detection. > > Done. > > https://codereview.chromium.org/868123002/diff/60001/content/public/browser/r... > content/public/browser/render_widget_host_view.h:141: // Cancel all the active > touch events for the current window, only for Aura. > On 2015/01/26 16:09:00, jdduke wrote: > > On 2015/01/24 14:29:01, tdresser wrote: > > > We shouldn't reference Aura here, and this shouldn't be called > > > ResetGestureDetection, as it doesn't always reset gesture detection. > > > > Would it be crazy to make this callback more generic, e.g., > > "OnMainFrameNavigation" or something like that? > > Done. Did you determine that WebContentsObserver::DidNavigateMainFrame won't work? https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
https://codereview.chromium.org/868123002/diff/80001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/868123002/diff/80001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1133: rwhv->OnMainFrameNavigation(); Oh, let's keep |ResetGestureDetection| in RWHVAndroid, and have |onMainFrameNavigation| delegate to it. Also, we can remove the |resetGestureDetection| call in ContentViewCore.java for main frame navigation. https://codereview.chromium.org/868123002/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/868123002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_base.cc:699: NOTIMPLEMENTED(); This will spam on Mac, which we probably don't want. An empty implementation seems fine. https://codereview.chromium.org/868123002/diff/80001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/868123002/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2608: // Cancel all the active touch events on the main frame to prevent they Let's leave this comment out. https://codereview.chromium.org/868123002/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2612: rwhv->OnMainFrameNavigation(); I don't have a strong opinion on the wording here, maybe |OnDidNavigateMainFrameToNewPage| to differentiate it from the observer methods? I'll defer to thoughts from owners.
https://codereview.chromium.org/868123002/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/868123002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1261: OnMainFrameNavigation(); This feels wrong? Perhaps have ResetGestureDetector(), and call that from here and in OnMainFrameNavigation() override? https://codereview.chromium.org/868123002/diff/80001/content/public/browser/r... File content/public/browser/render_widget_host_view.h (right): https://codereview.chromium.org/868123002/diff/80001/content/public/browser/r... content/public/browser/render_widget_host_view.h:143: Maybe you should add this to RenderWidgetHostViewBase, instead of this public interface? Also, it might be a good idea to get a content-owner opinion about this change (e.g. nasko@). It looks like RWHV/RWH do not currently explicitly deal with navigation. That might be deliberate, I am unsure.
On 2015/01/30 14:37:24, tdresser wrote: > On 2015/01/30 14:21:50, lanwei wrote: > > > https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... > > File content/browser/renderer_host/render_widget_host_view_aura.cc (right): > > > > > https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... > > content/browser/renderer_host/render_widget_host_view_aura.cc:2142: > > On 2015/01/24 14:29:00, tdresser wrote: > > > Get rid of whitespace changes in this file. > > > > Done. > > > > > https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... > > File content/browser/renderer_host/render_widget_host_view_base.cc (right): > > > > > https://codereview.chromium.org/868123002/diff/60001/content/browser/renderer... > > content/browser/renderer_host/render_widget_host_view_base.cc:699: // We only > > provide implementation in the RenderWidgetHostViewAura. > > On 2015/01/24 14:29:00, tdresser wrote: > > > This comment isn't correct, as RWHVAndroid also has an implementation. > > > > > > Also, won't this method be called on non-Android non-Aura platforms? > > > > > > > Done. > > > > > https://codereview.chromium.org/868123002/diff/60001/content/browser/web_cont... > > File content/browser/web_contents/web_contents_impl.cc (right): > > > > > https://codereview.chromium.org/868123002/diff/60001/content/browser/web_cont... > > content/browser/web_contents/web_contents_impl.cc:2609: // Cancel all the > active > > touch events on the aura window to prevent they > > On 2015/01/24 14:29:01, tdresser wrote: > > > This comment breaks some layering assumptions. You shouldn't reference aura > > > here. > > > > Done. > > > > > https://codereview.chromium.org/868123002/diff/60001/content/browser/web_cont... > > content/browser/web_contents/web_contents_impl.cc:2613: > > rwhv->ResetGestureDetection(); > > On 2015/01/24 14:29:01, tdresser wrote: > > > Won't this call Android's ResetGestureDetection() method? > > > > > > I thought we decided to name the method "DidNavigate()" and on Aura, we'd > > cancel > > > active touches on DidNavigate(). > > > > Done. > > > > > https://codereview.chromium.org/868123002/diff/60001/content/public/browser/r... > > File content/public/browser/render_widget_host_view.h (right): > > > > > https://codereview.chromium.org/868123002/diff/60001/content/public/browser/r... > > content/public/browser/render_widget_host_view.h:141: // Cancel all the active > > touch events for the current window, only for Aura. > > On 2015/01/24 14:29:01, tdresser wrote: > > > We shouldn't reference Aura here, and this shouldn't be called > > > ResetGestureDetection, as it doesn't always reset gesture detection. > > > > Done. > > > > > https://codereview.chromium.org/868123002/diff/60001/content/public/browser/r... > > content/public/browser/render_widget_host_view.h:141: // Cancel all the active > > touch events for the current window, only for Aura. > > On 2015/01/26 16:09:00, jdduke wrote: > > > On 2015/01/24 14:29:01, tdresser wrote: > > > > We shouldn't reference Aura here, and this shouldn't be called > > > > ResetGestureDetection, as it doesn't always reset gesture detection. > > > > > > Would it be crazy to make this callback more generic, e.g., > > > "OnMainFrameNavigation" or something like that? > > > > Done. > > Did you determine that WebContentsObserver::DidNavigateMainFrame won't work? > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... It is complicate, in order to make this work, we need to make RWHVAura inherit this Observer and overriding the didnavigation function. But we cannot guarantee the lifetime of RWHVAura in WebContents, so that RWHVAura's didnavigation function will be called for sure.
Patchset #3 (id:100001) has been deleted
https://codereview.chromium.org/868123002/diff/80001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/868123002/diff/80001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1133: rwhv->OnMainFrameNavigation(); On 2015/01/30 18:49:48, jdduke wrote: > Oh, let's keep |ResetGestureDetection| in RWHVAndroid, and have > |onMainFrameNavigation| delegate to it. Also, we can remove the > |resetGestureDetection| call in ContentViewCore.java for main frame navigation. Done. https://codereview.chromium.org/868123002/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/868123002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1261: OnMainFrameNavigation(); On 2015/02/03 01:26:16, sadrul wrote: > This feels wrong? Perhaps have ResetGestureDetector(), and call that from here > and in OnMainFrameNavigation() override? Done. https://codereview.chromium.org/868123002/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/868123002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_base.cc:699: NOTIMPLEMENTED(); On 2015/01/30 18:49:48, jdduke wrote: > This will spam on Mac, which we probably don't want. An empty implementation > seems fine. Done. https://codereview.chromium.org/868123002/diff/80001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/868123002/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2608: // Cancel all the active touch events on the main frame to prevent they On 2015/01/30 18:49:48, jdduke wrote: > Let's leave this comment out. Done. https://codereview.chromium.org/868123002/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2612: rwhv->OnMainFrameNavigation(); On 2015/01/30 18:49:48, jdduke wrote: > I don't have a strong opinion on the wording here, maybe > |OnDidNavigateMainFrameToNewPage| to differentiate it from the observer methods? > I'll defer to thoughts from owners. Done.
lanwei@chromium.org changed reviewers: + nasko@chromium.org
nasko@chromium.org: Please review changes in Can you please take look at this issue, do you think it is fine to add the navigation function into the render_widget_host_view class? Thank you.
nasko@chromium.org changed reviewers: + kenrb@chromium.org
Adding kenrb@, since he is much more knowledgable about the interaction between WebContents and RenderWidgetHostView*. A couple of nits while reading the code. https://codereview.chromium.org/868123002/diff/120001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/868123002/diff/120001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2669: nit: no need for extra empty lines https://codereview.chromium.org/868123002/diff/120001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/868123002/diff/120001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.h:322: // Cancel all the active touch events for the current main frame. If this is all this method does, then it has the wrong name.
I don't consider myself to be authoritative on this question, but I don't see a problem with it. RenderWidgetHostView doesn't deal with navigation because it isn't part of the content state, but if we need to change something as a result of a navigation then a method like this should be okay.
https://codereview.chromium.org/868123002/diff/120001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/868123002/diff/120001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2669: On 2015/02/04 22:02:42, nasko wrote: > nit: no need for extra empty lines Done. https://codereview.chromium.org/868123002/diff/120001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/868123002/diff/120001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.h:322: // Cancel all the active touch events for the current main frame. On 2015/02/04 22:02:42, nasko wrote: > If this is all this method does, then it has the wrong name. Done.
LGTM
On 2015/02/05 22:58:22, nasko wrote: > LGTM Android bits lgtm too!
The CQ bit was checked by lanwei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/868123002/140001
Message was sent while issue was closed.
Committed patchset #4 (id:140001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9d343ad2ea6ec395c377a4efa266057155bfa9c1 Cr-Commit-Position: refs/heads/master@{#315705} |
