Autoplay: move navigation activation propagation logic to browser process. [chromium/src : master]
Mounir Lamouri (Gerrit)
Can you PTAL at:
- clamy: content/
- jochen: chrome/
- dcheng: content/common/ and Blink
Thanks :)
To view, visit change 873450. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
3 comments:
File content/browser/frame_host/navigation_request.cc:
Patch Set #3, Line 1288:
url::Origin::Create(frame_tree_node_->current_url()),Does FrameTreeNode::GetCurrentOrigin() work? I'm guessing this will better respect sandboxed iframes and such.
File content/common/frame.mojom:
Patch Set #3, Line 88:
bool was_activated);Can we create an enum for this arg? A bool param to a method that already takes so many other params can be a bit hard to understand.
File content/renderer/render_frame_impl.cc:
Patch Set #3, Line 3086:
#if defined(OS_ANDROID)Why Android-specific?
To view, visit change 873450. To unsubscribe, or for help writing mail filters, visit settings.
Camille Lamy (Gerrit)
Thanks! A few comments below, generally the CL looks ok, but it seems you have a few tests failing.
5 comments:
File content/browser/frame_host/navigation_controller_impl.cc:
Patch Set #3, Line 790:
entry->set_has_user_gesture(params.has_user_gesture);nit: put this above the if defined.
File content/browser/frame_host/navigation_request.cc:
Patch Set #3, Line 1287:
if (ShouldPropagateUserActivation(I would move all of this to OnResponseStarted. See comments in frame.mojom about making the activation bit a part of RequestNavigationParams. There are a few calls in OnResponseStarted that update the request_navigation_params_ there based on the final response URL. This would fit better there.
Patch Set #3, Line 1289:
url::Origin::Create(navigation_handle_->GetURL())) &&What happens in the case of redirects? For example, the navigation is initially for a domain that is not same eTLD+1, but gets redirect to one that is. Should we propagate activation in this case (ie what this code is doing)?
File content/browser/site_per_process_browsertest.cc:
Patch Set #3, Line 806:
void SimulateAndWaitForRendererInitiatedNavigation(Shell* shell,Renderer-initiated navigations generally don't take the OpenURL path so the name of this function is a bit misleading. I would prefer something like OpenURLBlockUntilNavigationComplete.
File content/common/frame.mojom:
Patch Set #3, Line 88:
bool was_activated);
Can we create an enum for this arg? A bool param to a method that already takes so many other params […]
Could we move this param to RequestNavigationParams? This is the struct that takes params initialized in the browser process that the renderer needs to commit the navigation, so it would be a good fit for this member.
To view, visit change 873450. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
Comments applied.
I looked into the issues and it is related to the user gesture bit from the browser process now sent to the renderer. That's why I added the #ifdef OS_ANDROID but it's unclear if that's the right fix.
clamy@, do you have a suggestion that could allow me to use the user gesture bit without changing the current behaviour too much?
7 comments:
nit: put this above the if defined.
Done
File content/browser/frame_host/navigation_request.cc:
Patch Set #3, Line 1287:
RenderFrameHostImpl* render_frame_host,I would move all of this to OnResponseStarted. See comments in frame. […]
Done
Patch Set #3, Line 1288:
const base::Optional<std::string>& error_page_content) {Does FrameTreeNode::GetCurrentOrigin() work? I'm guessing this will better respect sandboxed iframes […]
I think so. Let see if tests agree :)
Patch Set #3, Line 1289:
TransferNavigationHandleOwnership(render_frame_host);What happens in the case of redirects? For example, the navigation is initially for a domain that is […]
The current redirect behaviour sounds good to me. I think it also stays consistent with the behaviour that was implemented with DocumentLoader, right?
File content/browser/site_per_process_browsertest.cc:
Patch Set #3, Line 806:
return true;Renderer-initiated navigations generally don't take the OpenURL path so the name of this function is […]
Done
File content/common/frame.mojom:
Could we move this param to RequestNavigationParams? This is the struct that takes params initialize […]
I followed Camille's advice.
File content/renderer/render_frame_impl.cc:
Patch Set #3, Line 3086:
// navigation right away.Why Android-specific?
`has_user_gesture` only works on Android today. I had a few tests failures just because of this. I realise that I might no longer need to expose this to the renderer so I will see if I can simply prevent the bit to be set in the renderer instead of #ifdef this.
To view, visit change 873450. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
6 comments:
File content/browser/frame_host/navigation_request.cc:
Patch Set #4, Line 815:
url::Origin::Create(navigation_handle_->GetURL()))) {How come this one checks the referrer, while line 807 checks the frame tree node's current origin? Would it be reasonable to always use the referrer? If there's a reason to have these differences, it would be useful to document the subtleties of why.
Also, what's the referrer for something opened via the context menu? What about middle-click?
File content/browser/site_per_process_browsertest.cc:
Patch Set #4, Line 810:
void OpenURLBlockUntilNavigationComplete(Shell* shell, const GURL& url) {Just curious: why did we have to write a new test helper for this?
File content/common/navigation_params.h:
// True if a navigation is following the rules of user activation propagation.
// This is different from |has_user_gesture| (in CommonNavigationParams) as
// the activation may have happened before the navigation was triggered, for
// example.
bool was_activated;Will these continue to remain distinct concepts for the forseeable future? It's not always clear to me what should be guarded by user activation vs user gesture.
File content/common/navigation_params.cc:
Patch Set #4, Line 106:
was_activated(false) {}Consider using an in-class member initializer to simplify this.
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp:
Patch Set #4, Line 1069:
// TODO(crbug.com/736415): Clear this bit unconditionally for all frames.Btw, how likely will we be able to implement this? It would be nice to move this over to Document if we don't need to support it being sticky on subframes.
Patch Set #4, Line 1069:
// TODO(crbug.com/736415): Clear this bit unconditionally for all frames.Btw, how likely will we be able to implement this? It would be nice to move this over to Document if we don't need to support it being sticky on subframes.
To view, visit change 873450. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
PTAL :)
7 comments:
File content/browser/frame_host/navigation_controller_impl.cc:
I've slightly changed this to make the tests pass. I've added a comment to explain.
File content/browser/frame_host/navigation_request.cc:
Patch Set #4, Line 815:
// context menu. This should apply to pages that open in a new tab and weHow come this one checks the referrer, while line 807 checks the frame tree node's current origin? W […]
I've added a comment explaining. Folding everything into checking the referrer would probably work fine most of the case but I'm worried about relying on the referrer as it can be stripped by the page for example. Probably not a big deal and happy to do it if you or clamy@ would prefer this (I assume rare are the pages setting no referrer policy on internal links).
Middle-click, ctrl+click and context menu will all set the URL of the WebContents from which the action was generated as referrer. Assuming there is no referrer policy I assume.
File content/browser/site_per_process_browsertest.cc:
Patch Set #4, Line 810:
void OpenURLBlockUntilNavigationComplete(Shell* shell, const GURL& url) {Just curious: why did we have to write a new test helper for this?
I only carry the bit for renderer initiated navigations and `OpenURL()` is browser initiated by default. I basically add `true` in the call to override this.
File content/common/navigation_params.h:
// True if a navigation is following the rules of user activation propagation.
// This is different from |has_user_gesture| (in CommonNavigationParams) as
// the activation may have happened before the navigation was triggered, for
// example.
bool was_activated;
Will these continue to remain distinct concepts for the forseeable future? It's not always clear to […]
I think the distinction here isn't activation vs gesture but whether there was an activation prior to the navigation or to start the navigation. `was_activated` will answer the former question. `user_gesture` will answer the latter. At the moment, `was_activated` is used by autoplay and vibration API I believe.
File content/common/navigation_params.cc:
Patch Set #4, Line 106:
was_activated(false) {}Consider using an in-class member initializer to simplify this.
Wouldn't it be better to have all the members in-class or none instead of a mix? I also usually avoid doing code style changes on code I do not own. Happy to do it in a follow-up if you want though :)
File content/renderer/render_frame_impl.cc:
I've reverted as the other change above seems to work better :)
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp:
Patch Set #4, Line 1069:
// TODO(crbug.com/736415): Clear this bit unconditionally for all frames.
Btw, how likely will we be able to implement this? It would be nice to move this over to Document if […]
I don't think we don't want to implement stickyness on subframes. Either way, I'm not sure we should implement this as part of this CL as I would like to attempt a merge :(
To view, visit change 873450. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
6 comments:
File content/browser/frame_host/navigation_controller_impl.cc:
Nit: delete
Patch Set #5, Line 786:
// in many tests.What sort of confusion does it create? Does it cause tests that assume no user gesture due to a test to break? Maybe this should be a TODO to clean this up in the long-term (i.e. fix the navigation utils that most browser tests use?).
File content/browser/frame_host/navigation_request.cc:
Patch Set #4, Line 815:
// context menu. This should apply to pages that open in a new tab and we
I've added a comment explaining. […]
IIRC, we have an initiator origin on BeginNavigationParams, which is set if it's a renderer-initiated navigation (which ought to cover middle-click and ctrl+click). I think it's not on navigation handle atm, but I don't think it would be unreasonable to add it if clamy@ is OK with it. Then we'd only need to fall back to referrer for the context menu, or find some other way of plumbing it.
I feel a bit uncomfortable with the current approach, because it seems like referrer is something that's easy for the page to inadvertently affect.
File content/common/navigation_params.h:
// True if a navigation is following the rules of user activation propagation.
// This is different from |has_user_gesture| (in CommonNavigationParams) as
// the activation may have happened before the navigation was triggered, for
// example.
bool was_activated;
I think the distinction here isn't activation vs gesture but whether there was an activation prior t […]
Can you update this comment to include the explanation you just gave me? =)
I'm still a bit confused about when one might want to use one vs the other, but it still helps.
File content/common/navigation_params.cc:
Patch Set #4, Line 106:
was_activated(false) {}
Wouldn't it be better to have all the members in-class or none instead of a mix? I also usually avoi […]
I don't feel strongly about this (and I'm not an OWNER of this particular code), but I think it's OK to have internal inconsistency as we incrementally migrate over =)
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp:
Patch Set #4, Line 1069:
// TODO(crbug.com/736415): Clear this bit unconditionally for all frames.
I don't think we don't want to implement stickyness on subframes. […]
Fair enough—to be clear, I wasn't asking for changes in this CL, just trying to get an overall feeling of where these patches will go.
To view, visit change 873450. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
9 comments:
File content/browser/frame_host/navigation_controller_impl.cc:
I've slightly changed this to make the tests pass. I've added a comment to explain.
Done
File content/browser/frame_host/navigation_controller_impl.cc:
Nit: delete
Hmm, this is `has_user_gesture` (markdown format, so `foo` instead of |foo|). Do you want me to remove the marker?
Patch Set #5, Line 786:
// in many tests.
What sort of confusion does it create? Does it cause tests that assume no user gesture due to a test […]
clamy@, you told me this is something your team was looking into. Can I assign a TODO on your name or is there a bug I can link to?
File content/browser/frame_host/navigation_request.cc:
Patch Set #4, Line 815:
// context menu. This should apply to pages that open in a new tab and we
IIRC, we have an initiator origin on BeginNavigationParams, which is set if it's a renderer-initiate […]
I agree, and that's why I wanted to use the `frame_tree_node_` URL when possible. For the sake of landing quickly and because it will most likely be fine in 99% of cases, can we stick with this and I will do the changes in a follow-up? I feel that the longer we wait, the harder it will be to merge and today middle click will never work as expected instead of not working in rare edge cases.
File content/browser/site_per_process_browsertest.cc:
Patch Set #4, Line 810:
void OpenURLBlockUntilNavigationComplete(Shell* shell, const GURL& url) {
I only carry the bit for renderer initiated navigations and `OpenURL()` is browser initiated by defa […]
Done
File content/common/navigation_params.h:
// True if a navigation is following the rules of user activation propagation.
// This is different from |has_user_gesture| (in CommonNavigationParams) as
// the activation may have happened before the navigation was triggered, for
// example.
bool was_activated;
Can you update this comment to include the explanation you just gave me? =) […]
Done
File content/common/navigation_params.cc:
Patch Set #4, Line 106:
was_activated(false) {}
I don't feel strongly about this (and I'm not an OWNER of this particular code), but I think it's OK […]
Will do a follow-up to move all of this as in-class initialized. Hopefully, that's the best compromise :)
File content/renderer/render_frame_impl.cc:
I've reverted as the other change above seems to work better :)
Patch Set #4, Line 1069:
// TODO(crbug.com/736415): Clear this bit unconditionally for all frames.
Fair enough—to be clear, I wasn't asking for changes in this CL, just trying to get an overall feeli […]
Ack
To view, visit change 873450. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
4 comments:
File content/browser/frame_host/navigation_controller_impl.cc:
Hmm, this is `has_user_gesture` (markdown format, so `foo` instead of |foo|). […]
Oh, I missed the matching backtick. I thought it was just a typo.
I think | is far more common than `, but I guess it doesn't really matter.
File content/browser/frame_host/navigation_request.cc:
Patch Set #4, Line 810:
// apply to same page navigations and is preferred over using the referrerI would suggest ordering these checks so the origin check (which is more expensive, since it requires creating temporaries and such) happens last.
Patch Set #4, Line 815:
// context menu. This should apply to pages that open in a new tab and we
I agree, and that's why I wanted to use the `frame_tree_node_` URL when possible. […]
Well, it's because this is going to M65 that I'm nervous though. =) I guess this is OK, but we should try to clean it up. It's a bit messy and hard to understand down the road.
// have to follow the referrer. It means that the activation might not be
// transmitted if it should have.
request_params_.was_activated = false;
if (ShouldPropagateUserActivation(
frame_tree_node_->current_origin(),
url::Origin::Create(navigation_haSimilarly, I'd suggest checking (navigation_handle_->HasUserGesture() && navigation_handle_->IsRendererInitiated()) || handle_navigation_->WasStartedFromContextMenu() in the outer if, and then check the origin in the inner if.
To view, visit change 873450. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
PTAL
4 comments:
Oh, I missed the matching backtick. I thought it was just a typo. […]
Ack
File content/browser/frame_host/navigation_request.cc:
Patch Set #4, Line 810:
// apply to same page navigations and is preferred over using the referrer
I would suggest ordering these checks so the origin check (which is more expensive, since it require […]
Done
Patch Set #4, Line 815:
// context menu. This should apply to pages that open in a new tab and we
Well, it's because this is going to M65 that I'm nervous though. […]
Ack
// have to follow the referrer. It means that the activation might not be
// transmitted if it should have.
request_params_.was_activated = false;
if (ShouldPropagateUserActivation(
frame_tree_node_->current_origin(),
url::Origin::Create(navigation_ha
Similarly, I'd suggest checking (navigation_handle_->HasUserGesture() && navigation_handle_->IsRende […]
Done
To view, visit change 873450. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
sending again to try as it seems to be a network problem with goma
Patch set 9:Commit-Queue +1
Mounir Lamouri (Gerrit)
clamy@, dcheng@, jochen@, PTAL :)
Jochen Eisinger (Gerrit)
waiting for dcheng/clamy, as they know this code better
Daniel Cheng (Gerrit)
3 comments:
File content/browser/frame_host/navigation_request.cc:
Patch Set #9, Line 798:
// A navigation is user activated if it contains a user gesture or the frameclamy: just to double check, at the point this function is called, we're certain of what the URL we're going to commit to is right?
Patch Set #9, Line 820:
url::Origin::Create(navigation_handle_->GetURL())clamy: Something to consider for a followup: maybe we could consider caching this on NavigationHandle itself. There's already a few places that construct a temporary origin out of this—and if it's going to happen on many navigations, then we may as well just eat the cost once in NavigationHandle?
Patch Set #9, Line 822:
} else if (((navigation_handle_->HasUserGesture() &&Please file a bug and add a TODO here to follow up with a better way of doing this. We could plumb the window disposition on the navigation handle or include it with the initiator info, et cetera.
To view, visit change 873450. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
Unfortunately, clamy@ is OOO until the 29th it seems :(
1 comment:
File content/browser/frame_host/navigation_request.cc:
Patch Set #9, Line 822:
} else if (((navigation_handle_->HasUserGesture() &&Please file a bug and add a TODO here to follow up with a better way of doing this. […]
Done
To view, visit change 873450. To unsubscribe, or for help writing mail filters, visit settings.
Camille Lamy (Gerrit)
Thanks! content/ lgtm with a few nits.
Patch set 10:Code-Review +1
3 comments:
File content/browser/frame_host/navigation_request.cc:
Patch Set #9, Line 798:
// A navigation is user activated if it contains a user gesture or the frameclamy: just to double check, at the point this function is called, we're certain of what the URL we' […]
Yes, this is the moment where we update our parameters following the reception of the final response.
Patch Set #9, Line 820:
url::Origin::Create(navigation_handle_->GetURL())
clamy: Something to consider for a followup: maybe we could consider caching this on NavigationHandl […]
Yes that's a good idea. Let's add a TODO.
File content/browser/frame_host/navigation_request.cc:
Patch Set #10, Line 824:
// source for the originating URL when the navigation is rendere initiated.nit: s/rendere/renderer
To view, visit change 873450. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Mounir Lamouri (slow) (Gerrit)
jochen@, PTAL at chrome/browser/ :)
3 comments:
Yes, this is the moment where we update our parameters following the reception of the final response […]
Ack
Patch Set #9, Line 820:
frame_tree_node_->current_origin(),
Yes that's a good idea. Let's add a TODO.
That was added in the following patchset.
File content/browser/frame_host/navigation_request.cc:
Patch Set #10, Line 824:
// navigation_handle_->GetReferrer() but should ideally use a more reliablenit: s/rendere/renderer
Done
To view, visit change 873450. To unsubscribe, or for help writing mail filters, visit settings.
Jochen Eisinger (Gerrit)
Patch set 11:Code-Review +1
Mounir Lamouri (slow) (Gerrit)
Patch set 11:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Autoplay: move navigation activation propagation logic to browser process.
This is also allowing navigations with gesture to carry the flag such as
middle click on links and CTRL+click. A side effect is to be able to
prevent non-renderer initiated navigations to carry the flag such as
reloads.
Bug: 799136, 759448
Change-Id: I25836f8a4e9aca5a60dc095c6d0a81d480027124
Reviewed-on: https://chromium-review.googlesource.com/873450
Reviewed-by: Jochen Eisinger <joc...@chromium.org>
Reviewed-by: Camille Lamy <cl...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Commit-Queue: Mounir Lamouri (slow) <mlam...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532949}
---
M chrome/browser/media/unified_autoplay_browsertest.cc
M chrome/browser/ui/browser_navigator.cc
M content/browser/frame_host/frame_tree_node.h
M content/browser/frame_host/navigation_controller_impl.cc
M content/browser/frame_host/navigation_entry_impl.cc
M content/browser/frame_host/navigation_entry_impl.h
M content/browser/frame_host/navigation_request.cc
M content/browser/site_per_process_browsertest.cc
M content/common/frame_messages.h
M content/common/navigation_params.cc
M content/common/navigation_params.h
M content/public/browser/navigation_controller.cc
M content/public/browser/navigation_controller.h
M content/renderer/render_frame_impl.cc
M third_party/WebKit/Source/core/exported/WebDocumentLoaderImpl.cpp
M third_party/WebKit/Source/core/frame/FrameTest.cpp
M third_party/WebKit/Source/core/loader/DocumentLoader.cpp
M third_party/WebKit/public/web/WebDocumentLoader.h
18 files changed, 188 insertions(+), 112 deletions(-)