Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2591783003:
Implement pointer lock across outer/inner WebContents. (Closed)
|
Created:
4 years ago by lfg Modified:
3 years, 11 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement pointer lock across outer/inner WebContents.
This test also enables WebViewPointerLockInteractiveTests on OOPIF-based
<webview>.
BUG=582562
Committed: https://crrev.com/f0cd46e1c8981d455a4897c3730aba457c9d6212
Cr-Commit-Position: refs/heads/master@{#441258}
Patch Set 1 #
Total comments: 6
Patch Set 2 : addressing comments #Patch Set 3 : rebase #
Total comments: 2
Messages
Total messages: 29 (19 generated)
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lfg@chromium.org changed reviewers: + nick@chromium.org
Nick, please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2591783003/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2591783003/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:1921: if (current->mouse_lock_widget_) { I'm wondering if we should contemplate having some refptr'ed context object (like a focus manager or something) that is shared among the inner/outer WebContentses, or something else that deduplicates this state. It is fine to proceed with things as you've implemented it, since it looks correct to me. But if this pattern is something that will happen again with other data members, then I'd hope we could shift it to a shared context object. https://codereview.chromium.org/2591783003/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:3086: if (mouse_lock_widget_->delegate()->GetAsWebContents() != this) { When does this case happen? From what I can tell, GotResponseToLockMouseRequest() is a response method, meant to be called by implementations of WebContentsDelegate::RequestToLockMouse(). And it looks like GotResponseToLockMouseRequest is always called on the WebContents passed as the first argument to RequestToLockMouse(). But the only call I see to delegate_->RequestToLockMouse() seems to originate from the inner WebContents that's actually wanting the mouse lock (i.e. the lowest one for which mouse_lock_widget_ is non-null), so it's not clear why we need to descend further down the tree here. If it turns out that we don't need this case, then we probably don't need the GetAsWebContents() method either -- the other usage of GetAsWebContents() can just be replaced by casting the WebContentsImpl to a RenderWidgetHostDelegate and doing the equality comparison between the two Delegate* pointers. https://codereview.chromium.org/2591783003/diff/1/extensions/browser/guest_vi... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/2591783003/diff/1/extensions/browser/guest_vi... extensions/browser/guest_view/web_view/web_view_guest.cc:1384: [](WebContents* web_contents, bool allowed) { Why not: base::Bind(base::IgnoreResult(&WebContents::GotResponseToLockMouseRequest), base::Unretained(web_contents)); ? I think dcheng had mentioned that lambdas w/ Bind are to be avoided if possible, because we prefer the forms that make the caller use base::Unretained or some other indication of pointer ownership semantics.
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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-...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2591783003/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2591783003/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:1921: if (current->mouse_lock_widget_) { On 2016/12/22 00:01:07, ncarter (ooo til Jan 3) wrote: > I'm wondering if we should contemplate having some refptr'ed context object > (like a focus manager or something) that is shared among the inner/outer > WebContentses, or something else that deduplicates this state. > > It is fine to proceed with things as you've implemented it, since it looks > correct to me. But if this pattern is something that will happen again with > other data members, then I'd hope we could shift it to a shared context object. I thought about that a bit, but for a single pointer I didn't think this was worth the refactoring. Something like that would also introduce complicated lifetime management. https://codereview.chromium.org/2591783003/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:3086: if (mouse_lock_widget_->delegate()->GetAsWebContents() != this) { On 2016/12/22 00:01:07, ncarter (ooo til Jan 3) wrote: > When does this case happen? > > From what I can tell, GotResponseToLockMouseRequest() is a response method, > meant to be called by implementations of > WebContentsDelegate::RequestToLockMouse(). And it looks like > GotResponseToLockMouseRequest is always called on the WebContents passed as the > first argument to RequestToLockMouse(). > > But the only call I see to delegate_->RequestToLockMouse() seems to originate > from the inner WebContents that's actually wanting the mouse lock (i.e. the > lowest one for which mouse_lock_widget_ is non-null), so it's not clear why we > need to descend further down the tree here. > > If it turns out that we don't need this case, then we probably don't need the > GetAsWebContents() method either -- the other usage of GetAsWebContents() can > just be replaced by casting the WebContentsImpl to a RenderWidgetHostDelegate > and doing the equality comparison between the two Delegate* pointers. GotResponseToLockMouseRequest is a poor name for the function, as it can also used to cancel the mouse lock (see the implementation in RenderWidgetHostImpl, that calls RejectMouseLockOrUnlockIfNecessary). And it's the only function exposed in the content/public interface, as LostMouseLock is not. https://codereview.chromium.org/2591783003/diff/1/extensions/browser/guest_vi... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/2591783003/diff/1/extensions/browser/guest_vi... extensions/browser/guest_view/web_view/web_view_guest.cc:1384: [](WebContents* web_contents, bool allowed) { On 2016/12/22 00:01:07, ncarter (ooo til Jan 3) wrote: > Why not: > > base::Bind(base::IgnoreResult(&WebContents::GotResponseToLockMouseRequest), > base::Unretained(web_contents)); ? > > I think dcheng had mentioned that lambdas w/ Bind are to be avoided if possible, > because we prefer the forms that make the caller use base::Unretained or some > other indication of pointer ownership semantics. Oh, that's nice, I wasn't aware of base::IgnoreResult. Done.
lgtm
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lfg@chromium.org changed reviewers: + wjmaclean@chromium.org
wjmaclean@chromium.org: Please review changes in chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
lgtm, see comments ... https://codereview.chromium.org/2591783003/diff/40001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2591783003/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:552: DISABLED_PointerLock) { I assume this will be re-enabled in a subsequent CL? https://codereview.chromium.org/2591783003/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:626: DISABLED_PointerLockFocus) { I assume this will be re-enabled in a subsequent CL?
On 2017/01/03 23:11:52, wjmaclean wrote: > https://codereview.chromium.org/2591783003/diff/40001/chrome/browser/apps/gue... > File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): > > https://codereview.chromium.org/2591783003/diff/40001/chrome/browser/apps/gue... > chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:552: > DISABLED_PointerLock) { > I assume this will be re-enabled in a subsequent CL? > > https://codereview.chromium.org/2591783003/diff/40001/chrome/browser/apps/gue... > chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:626: > DISABLED_PointerLockFocus) { > I assume this will be re-enabled in a subsequent CL? Those tests are disabled for other reasons, and there are already bugs filed already to enable them. I'm ok with this for now as there are other tests that aren't disabled, so we at least have some coverage for the feature.
The CQ bit was checked by lfg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org Link to the patchset: https://codereview.chromium.org/2591783003/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483487958476700,
"parent_rev": "fd24aedbe0fa82a43cc619e41d229fc22a3c4ae6", "commit_rev":
"9220fd29b3ef1c80eaf6a99a7b69457208413821"}
Message was sent while issue was closed.
Description was changed from ========== Implement pointer lock across outer/inner WebContents. This test also enables WebViewPointerLockInteractiveTests on OOPIF-based <webview>. BUG=582562 ========== to ========== Implement pointer lock across outer/inner WebContents. This test also enables WebViewPointerLockInteractiveTests on OOPIF-based <webview>. BUG=582562 Review-Url: https://codereview.chromium.org/2591783003 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Implement pointer lock across outer/inner WebContents. This test also enables WebViewPointerLockInteractiveTests on OOPIF-based <webview>. BUG=582562 Review-Url: https://codereview.chromium.org/2591783003 ========== to ========== Implement pointer lock across outer/inner WebContents. This test also enables WebViewPointerLockInteractiveTests on OOPIF-based <webview>. BUG=582562 Committed: https://crrev.com/f0cd46e1c8981d455a4897c3730aba457c9d6212 Cr-Commit-Position: refs/heads/master@{#441258} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f0cd46e1c8981d455a4897c3730aba457c9d6212 Cr-Commit-Position: refs/heads/master@{#441258} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
