Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1180503002:
Force a GuestView's embedder to be focused on TouchStart. (Closed)
|
Created:
5 years, 6 months ago by wjmaclean Modified:
5 years, 6 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, creis+watch_chromium.org, darin-cc_chromium.org, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionForce a GuestView's embedder to be focused on TouchStart.
It is currently possible that touching (TouchStart) an element inside a
GuestView may not focus the element because the embedder itself has not
received focus. An example of this is when a <webview> tag is contained
inside a views::WebView. The views::WebView is unable to receive touch
events in order to focus itself, and the BrowserPlugin marks the touch
event as handled so that Aura never generates the GestureTap that
RenderWidgetHostViewAura requires to focus the views::WebView.
This CL adds logic in RenderWidgetHostViewGuest to detect an unfocused
embedder, and to focus it on TouchStart.
BUG=450147
Committed: https://crrev.com/6aefc3a6c8aa7463906af62205b0f0c314c3ef76
Cr-Commit-Position: refs/heads/master@{#335569}
Patch Set 1 #Patch Set 2 : Add test. #
Total comments: 2
Patch Set 3 : New, improved way to be sure compositor layers have settled (thanks dtapuska@!). #Patch Set 4 : Add USE_AURA compile guards. #Patch Set 5 : Try waiting an extra frame before sending touch event. #Patch Set 6 : Fix test. #Patch Set 7 : Refactor utility code to browser_test_utils. #
Total comments: 5
Patch Set 8 : Improve variable name. #
Total comments: 7
Patch Set 9 : Address comments. #
Total comments: 5
Patch Set 10 : Address more comments #Patch Set 11 : Release observer on destruction. Make webview focused (for Windows). #
Messages
Total messages: 71 (35 generated)
wjmaclean@chromium.org changed reviewers: + creis@chromium.org
wjmaclean@chromium.org changed reviewers: + sadrul@chromium.org
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180503002/20001
wjmaclean@chromium.org changed reviewers: + jdduke@chromium.org
wjmaclean@chromium.org changed reviewers: - creis@chromium.org, sadrul@chromium.org
wjmaclean@chromium.org changed reviewers: - jdduke@chromium.org
wjmaclean@chromium.org changed reviewers: + avi@chromium.org, lazyboy@chromium.org
avi@chromium.org: Please review changes in RenderWidgetHostViewGuest. This small change is something Sadrul agreed to, but he wanted a test (and now he's away). PTAL? lazyboy@chromium.org: Please review changes in Can you please look at the tests? I still need to add the bug number to track to usage of GiveItTime(), which has precendence in other touch-event tests, and was suggested by tdresser@.
https://codereview.chromium.org/1180503002/diff/20001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/1180503002/diff/20001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_browsertest.cc:2407: typedef WebViewTest WebViewFocusTest; I just realized, these needs to be in a #if defined(USE_AURA) block. https://codereview.chromium.org/1180503002/diff/20001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_browsertest.cc:2456: GiveItSomeTime(100 /* milliseconds */); I'll add a bug number to explain this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180503002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180503002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180503002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
FYI, it seems the test is not passing on the bots yet, please ping the review thread once they're fixed and I can review. Ty.
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
Patchset #6 (id:140001) has been deleted
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180503002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
wjmaclean@chromium.org changed reviewers: - avi@chromium.org
kenrb@chromium.org changed reviewers: + kenrb@chromium.org
lgtm the RenderWidgetHostView change
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org Link to the patchset: https://codereview.chromium.org/1180503002/#ps180001 (title: "Refactor utility code to browser_test_utils.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180503002/180001
wjmaclean@chromium.org changed reviewers: + nasko@chromium.org
nasko@ - don't worry about looking at this until Monday. If you want me to find another reviewer for the code I moved into browser_test_utils, just let me know.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1180503002/diff/180001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/1180503002/diff/180001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_browsertest.cc:225: views::View* FindWebView(views::View* v) { nit: Using just one letter for variable names (other than counters) is discouraged. Why not "view"? https://codereview.chromium.org/1180503002/diff/180001/content/public/test/br... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/1180503002/diff/180001/content/public/test/br... content/public/test/browser_test_utils.cc:439: RenderWidgetHostImpl::From(web_contents->GetRenderViewHost()); Why do a lookup when it can be retrieved directly. Wouldn't web_contents->GetRenderWidgetHostView()->GetRenderWidgetHost() work? The same pattern is present in a few places in this file.
Comments addressed, PTAL? https://codereview.chromium.org/1180503002/diff/180001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/1180503002/diff/180001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_browsertest.cc:225: views::View* FindWebView(views::View* v) { On 2015/06/22 08:40:52, nasko (paris) wrote: > nit: Using just one letter for variable names (other than counters) is > discouraged. Why not "view"? Done. https://codereview.chromium.org/1180503002/diff/180001/content/public/test/br... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/1180503002/diff/180001/content/public/test/br... content/public/test/browser_test_utils.cc:439: RenderWidgetHostImpl::From(web_contents->GetRenderViewHost()); On 2015/06/22 08:40:53, nasko (paris) wrote: > Why do a lookup when it can be retrieved directly. Wouldn't > web_contents->GetRenderWidgetHostView()->GetRenderWidgetHost() work? The same > pattern is present in a few places in this file. Everywhere else in this file when a RenderWidgetHostViewImpl is retrieved, it is done using the same method I'm employing. In fact, I ran git gs on the entire codebase and only found one instance of what you propose (render_widget_host_view_browsertest.cc:99); it looks like RenderWidgetHostImpl* const rwh = RenderWidgetHostImpl::From( shell()->web_contents()->GetRenderWidgetHostView()-> GetRenderWidgetHost()); Note: GetRenderWidgetHost() returns RenderWidgetHostView*, which still requires conversion. I can still change it if you like.
lgtm https://codereview.chromium.org/1180503002/diff/180001/content/public/test/br... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/1180503002/diff/180001/content/public/test/br... content/public/test/browser_test_utils.cc:439: RenderWidgetHostImpl::From(web_contents->GetRenderViewHost()); On 2015/06/22 13:39:44, wjmaclean wrote: > On 2015/06/22 08:40:53, nasko (paris) wrote: > > Why do a lookup when it can be retrieved directly. Wouldn't > > web_contents->GetRenderWidgetHostView()->GetRenderWidgetHost() work? The same > > pattern is present in a few places in this file. > > Everywhere else in this file when a RenderWidgetHostViewImpl is retrieved, it is > done using the same method I'm employing. In fact, I ran git gs on the entire > codebase and only found one instance of what you propose > (render_widget_host_view_browsertest.cc:99); it looks like > > RenderWidgetHostImpl* const rwh = RenderWidgetHostImpl::From( > shell()->web_contents()->GetRenderWidgetHostView()-> > GetRenderWidgetHost()); > > Note: GetRenderWidgetHost() returns RenderWidgetHostView*, which still requires > conversion. You mean RenderWidgetHost, not RenderWidgetHostView, correct? Just the public interface, not the impl, which will require a cast, as you pointed in the code snippet above. > I can still change it if you like. No, it is fine, especially since the rest of the file uses the same pattern.
On 2015/06/22 14:05:49, nasko (paris) wrote: > lgtm > > You mean RenderWidgetHost, not RenderWidgetHostView, correct? Just the public > interface, not the impl, which will require a cast, as you pointed in the code > snippet above. Ooops, yes ... mistyped!
https://chromiumcodereview.appspot.com/1180503002/diff/200001/chrome/browser/... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://chromiumcodereview.appspot.com/1180503002/diff/200001/chrome/browser/... chrome/browser/apps/guest_view/web_view_browsertest.cc:57: #include "ui/aura/window.h" This probably won't compile for mac? I think you need if AURA guard here. https://chromiumcodereview.appspot.com/1180503002/diff/200001/chrome/browser/... chrome/browser/apps/guest_view/web_view_browsertest.cc:2475: other_focusable_view->SetBounds(bounds.x() + bounds.width(), 0, 100, 100); nit: bounds.y() instead of 0? https://chromiumcodereview.appspot.com/1180503002/diff/200001/chrome/browser/... chrome/browser/apps/guest_view/web_view_browsertest.cc:2495: while (!aura_webview->HasFocus()) Instead of busy waiting on !HasFocus(), can you use FocusManager's observer views::FocusChangeListener to check this? You can see similar stuff here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... https://chromiumcodereview.appspot.com/1180503002/diff/200001/chrome/browser/... chrome/browser/apps/guest_view/web_view_browsertest.cc:2497: GiveItSomeTime(10 /* milliseconds */); Do you need this wait? Can you add a comment explaining why? If you don't need this one, then you should inline GiveItSomeTime in ForceCompositorFrame. This is so that we don't encourage people to re-use this and think twice before re-using it.
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1180503002/#ps220001 (title: "Address comments.")
Suggestions addressed, PTAL? https://codereview.chromium.org/1180503002/diff/200001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/1180503002/diff/200001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_browsertest.cc:57: #include "ui/aura/window.h" On 2015/06/22 16:15:15, lazyboy wrote: > This probably won't compile for mac? I think you need if AURA guard here. It seems to have passed the CQ dry run ok ... https://codereview.chromium.org/1180503002/diff/200001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_browsertest.cc:2475: other_focusable_view->SetBounds(bounds.x() + bounds.width(), 0, 100, 100); On 2015/06/22 16:15:15, lazyboy wrote: > nit: bounds.y() instead of 0? Done. https://codereview.chromium.org/1180503002/diff/200001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_browsertest.cc:2497: GiveItSomeTime(10 /* milliseconds */); On 2015/06/22 16:15:15, lazyboy wrote: > Do you need this wait? Can you add a comment explaining why? > > If you don't need this one, then you should inline > GiveItSomeTime in ForceCompositorFrame. This is so that we don't > encourage people to re-use this and think twice before re-using it. I'll remove it ... it's likely a left-over ...
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180503002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
https://codereview.chromium.org/1180503002/diff/220001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/1180503002/diff/220001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_browsertest.cc:2425: base::RunLoop run_loop; nit: add a comment saying we're waiting 10ms because ... https://codereview.chromium.org/1180503002/diff/220001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_browsertest.cc:2524: FocusWaiter waiter(aura_webview); I think you want content::SimulateTouchPressAt() after this line and before .Wait()
PTAL https://codereview.chromium.org/1180503002/diff/220001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/1180503002/diff/220001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_browsertest.cc:2425: base::RunLoop run_loop; On 2015/06/22 17:27:24, lazyboy wrote: > nit: add a comment saying we're waiting 10ms because ... Done. https://codereview.chromium.org/1180503002/diff/220001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_browsertest.cc:2524: FocusWaiter waiter(aura_webview); On 2015/06/22 17:27:24, lazyboy wrote: > I think you want content::SimulateTouchPressAt() after this line and before > .Wait() I've changed this, but it really doesn't matter. Since the tap event has to travel to the renderer process, and then back to RWHVG, there's no chance that it will get back faster than we can create the waiter. But, even if it did, all we care about is whether aura_webview has focus or not, and we don't need to capture the focus-changed event to do that ... note that Wait() early outs if the target already has focus.
lgtm https://codereview.chromium.org/1180503002/diff/220001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/1180503002/diff/220001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_browsertest.cc:2524: FocusWaiter waiter(aura_webview); On 2015/06/22 17:34:05, wjmaclean wrote: > On 2015/06/22 17:27:24, lazyboy wrote: > > I think you want content::SimulateTouchPressAt() after this line and before > > .Wait() > > I've changed this, but it really doesn't matter. > > Since the tap event has to travel to the renderer process, and then back to > RWHVG, there's no chance that it will get back faster than we can create the > waiter. > > But, even if it did, all we care about is whether aura_webview has focus or not, > and we don't need to capture the focus-changed event to do that ... note that > Wait() early outs if the target already has focus. Acknowledged.
The CQ bit was checked by wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1180503002/#ps240001 (title: "Address more comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180503002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, lazyboy@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1180503002/#ps260001 (title: "Release observer on destruction. Make webview focused (for Windows).")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180503002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wjmaclean@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180503002/260001
Message was sent while issue was closed.
Committed patchset #11 (id:260001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/6aefc3a6c8aa7463906af62205b0f0c314c3ef76 Cr-Commit-Position: refs/heads/master@{#335569}
Message was sent while issue was closed.
On 2015/06/22 21:59:38, commit-bot: I haz the power wrote: > Patchset 11 (id:??) landed as > https://crrev.com/6aefc3a6c8aa7463906af62205b0f0c314c3ef76 > Cr-Commit-Position: refs/heads/master@{#335569} This test broke the Site Isolation FYI bots. Can you please disable it or fix it such that it passes? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
