Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1335303004:
ServiceWorker: Fix the bug in navigate() if windowclient is nested frame. (Closed)
|
Created:
5 years, 3 months ago by zino Modified:
5 years, 2 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org, kinuko+serviceworker, kinuko+watch, blink-worker-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Fix the bug in navigate() if windowclient is nested frame.
In the current implementation, calling navigate() method, the top level page
instead of the subframe page will always navigate even though the windowclient
is nested frame type such as <iframe>. This change is fixing the bug by passing
the frame_tree_node_id of the subframe to WebContents::OpenURL().
Releated CLs:
- http://crrev.com/1202453002 (chromium)
- http://crrev.com/1196193003 (blink)
- http://crrev.com/1211253007 (layout test)
BUG=500911
Committed: https://crrev.com/3cd134a8b280b6561fac0b70305254b7f442b0f9
Cr-Commit-Position: refs/heads/master@{#350990}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 2
Patch Set 4 : nit #Messages
Total messages: 25 (7 generated)
jinho.bang@samsung.com changed reviewers: + nhiroki@chromium.org
PTAL, While I was writing the test codes for navigate() method, I found the problem. Thank you.
On 2015/09/14 06:26:38, zino wrote: > PTAL, > > While I was writing the test codes for navigate() method, I found the problem. > > Thank you. Ping nhiroki@ Could you please review this? :)
Sorry for my late reply. Looks good overall from ServiceWorker's point of view. I'm not familiar with navigation code, so could you ask someone who is an expert on the navigation to review this? (according to the git history of WebContents, avi@ could be familiar with it??) https://codereview.chromium.org/1335303004/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1335303004/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version.cc:270: common_params.transition = ui::PAGE_TRANSITION_AUTO_TOPLEVEL; For iframes, the trantition type could be PAGE_TRANSITION_AUTO_SUBFRAME?
jinho.bang@samsung.com changed reviewers: + avi@chromium.org
jinho.bang@samsung.com changed reviewers: + nick@semenkovich.com
+ Avi@, nick@ as reviewers for navigation. Could you please review this? (nhiroki@, Thanks!)
creis@chromium.org changed reviewers: + creis@chromium.org
Drive by for navigation API question. https://codereview.chromium.org/1335303004/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1335303004/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version.cc:271: render_frame_host->Navigate(common_params, StartNavigationParams(), It's generally not ok to directly call RenderFrameHost::Navigate, since that bypasses NavigationController. The previous WebContents::OpenURL approach can continue to work if you set the frame_tree_node_id on the params object.
Reviewers, Thank you for review and I addressed all of comments. Could you review again? https://codereview.chromium.org/1335303004/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1335303004/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version.cc:270: common_params.transition = ui::PAGE_TRANSITION_AUTO_TOPLEVEL; On 2015/09/16 08:00:18, nhiroki wrote: > For iframes, the trantition type could be PAGE_TRANSITION_AUTO_SUBFRAME? Done. https://codereview.chromium.org/1335303004/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version.cc:271: render_frame_host->Navigate(common_params, StartNavigationParams(), On 2015/09/16 18:58:46, Charlie Reis wrote: > It's generally not ok to directly call RenderFrameHost::Navigate, since that > bypasses NavigationController. > > The previous WebContents::OpenURL approach can continue to work if you set the > frame_tree_node_id on the params object. Done.
On 2015/09/17 01:27:45, zino wrote: > Reviewers, > > Thank you for review and I addressed all of comments. > > Could you review again? > > https://codereview.chromium.org/1335303004/diff/1/content/browser/service_wor... > File content/browser/service_worker/service_worker_version.cc (right): > > https://codereview.chromium.org/1335303004/diff/1/content/browser/service_wor... > content/browser/service_worker/service_worker_version.cc:270: > common_params.transition = ui::PAGE_TRANSITION_AUTO_TOPLEVEL; > On 2015/09/16 08:00:18, nhiroki wrote: > > For iframes, the trantition type could be PAGE_TRANSITION_AUTO_SUBFRAME? > > Done. > > https://codereview.chromium.org/1335303004/diff/1/content/browser/service_wor... > content/browser/service_worker/service_worker_version.cc:271: > render_frame_host->Navigate(common_params, StartNavigationParams(), > On 2015/09/16 18:58:46, Charlie Reis wrote: > > It's generally not ok to directly call RenderFrameHost::Navigate, since that > > bypasses NavigationController. > > > > The previous WebContents::OpenURL approach can continue to work if you set the > > frame_tree_node_id on the params object. > > Done. This change will work, but it exposes a problem with our public API, where we ask you to fill in a field, yet give you no way of doing so. Hang on.
Please write a test for this if possible. Also, one question below about a possible bug. https://codereview.chromium.org/1335303004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1335303004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version.cc:204: if (render_frame_host != render_frame_host_) What happens if this was a cross-process navigation? The destination render_frame_host won't match the render_frame_host_ from the start of the navigation. (The same bug appears to exist in the old code.) Seems like you should compare the FrameTreeNodes (or FrameTreeNode IDs) of the two RenderFrameHosts instead. You can cast them to RenderFrameHostImpls to get that here in content, though it gets back to Avi's question about exposing a RenderFrameHost::GetFrameID() method in the public API in general.
On 2015/09/17 17:14:34, Charlie Reis wrote: > Please write a test for this if possible. Also, one question below about a > possible bug. > > https://codereview.chromium.org/1335303004/diff/20001/content/browser/service... > File content/browser/service_worker/service_worker_version.cc (right): > > https://codereview.chromium.org/1335303004/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_version.cc:204: if > (render_frame_host != render_frame_host_) > What happens if this was a cross-process navigation? The destination > render_frame_host won't match the render_frame_host_ from the start of the > navigation. (The same bug appears to exist in the old code.) > > Seems like you should compare the FrameTreeNodes (or FrameTreeNode IDs) of the > two RenderFrameHosts instead. You can cast them to RenderFrameHostImpls to get > that here in content, though it gets back to Avi's question about exposing a > RenderFrameHost::GetFrameID() method in the public API in general. Re my question, I don't want to hold up this change for an API disagreement that we're having internally. When Charlie is happy I will stamp.
Reviewers, PTAL. There is already a layout test for this. If it's not enough, do we need unit test? If so, could you let me know some guide(test case and expected value and so on) to write the test codes? https://codereview.chromium.org/1335303004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1335303004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version.cc:204: if (render_frame_host != render_frame_host_) On 2015/09/17 17:14:34, Charlie Reis wrote: > What happens if this was a cross-process navigation? The destination > render_frame_host won't match the render_frame_host_ from the start of the > navigation. (The same bug appears to exist in the old code.) > > Seems like you should compare the FrameTreeNodes (or FrameTreeNode IDs) of the > two RenderFrameHosts instead. You can cast them to RenderFrameHostImpls to get > that here in content, though it gets back to Avi's question about exposing a > RenderFrameHost::GetFrameID() method in the public API in general. Done.
On 2015/09/22 08:01:14, zino wrote: > Reviewers, PTAL. > > There is already a layout test for this. > > If it's not enough, do we need unit test? > > If so, could you let me know some guide(test case and expected value and so on) > to write the test codes? > > https://codereview.chromium.org/1335303004/diff/20001/content/browser/service... > File content/browser/service_worker/service_worker_version.cc (right): > > https://codereview.chromium.org/1335303004/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_version.cc:204: if > (render_frame_host != render_frame_host_) > On 2015/09/17 17:14:34, Charlie Reis wrote: > > What happens if this was a cross-process navigation? The destination > > render_frame_host won't match the render_frame_host_ from the start of the > > navigation. (The same bug appears to exist in the old code.) > > > > Seems like you should compare the FrameTreeNodes (or FrameTreeNode IDs) of the > > two RenderFrameHosts instead. You can cast them to RenderFrameHostImpls to > get > > that here in content, though it gets back to Avi's question about exposing a > > RenderFrameHost::GetFrameID() method in the public API in general. > > Done. Please take a look :)
Sorry for the delay. LGTM with nit, since you have a layout test for it. https://codereview.chromium.org/1335303004/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1335303004/diff/40001/content/browser/service... content/browser/service_worker/service_worker_version.cc:242: void DidOpenURL(const OpenURLCallback& callback, WebContents* web_contents) { Let's add a comment saying this should only be called for main frame navigations.
Patchset #4 (id:60001) has been deleted
No problem. Thank you for review! :) https://codereview.chromium.org/1335303004/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1335303004/diff/40001/content/browser/service... content/browser/service_worker/service_worker_version.cc:242: void DidOpenURL(const OpenURLCallback& callback, WebContents* web_contents) { On 2015/09/24 18:36:58, Charlie Reis wrote: > Let's add a comment saying this should only be called for main frame > navigations. Done.
Avi@, On 2015/09/17 18:07:29, Avi wrote: > Re my question, I don't want to hold up this change for an API disagreement that > we're having internally. When Charlie is happy I will stamp. Would you mind if I commit this?
On 2015/09/25 01:11:07, zino wrote: > Avi@, > > On 2015/09/17 18:07:29, Avi wrote: > > Re my question, I don't want to hold up this change for an API disagreement > that > > we're having internally. When Charlie is happy I will stamp. > > Would you mind if I commit this? Do it! lgtm
The CQ bit was checked by jinho.bang@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1335303004/#ps80001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1335303004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1335303004/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3cd134a8b280b6561fac0b70305254b7f442b0f9 Cr-Commit-Position: refs/heads/master@{#350990} |
