Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2331343005:
PlzNavigate: Get StreamPrivate API to work. (Closed)
|
Created:
4 years, 3 months ago by ananta Modified:
4 years, 2 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, Randy Smith (Not in Mondays), darin-cc_chromium.org, loading-reviews_chromium.org, chromium-apps-reviews_chromium.org, mmenke Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: Get StreamPrivate API to work.
This is based off clamy's original patch https://codereview.chromium.org/1485693002/
This patch gets the StreamPrivate API which is exposed for extensions to work
with PlzNavigate. This should fix top level PDF file loads and in turn some
browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc.
Changes in this patch are as below:
1. Allow stream based requests to be handled in navigation_resource_handler.cc.
2. Fixing the way WebContents is obtained in the other places with the
ResourceRequestInfoImpl::GetWebContentsGetterForRequest function.
3. The MimeHandlerStreamManager::AddStream function takes in the WebContents pointer.
4 A getter has been added to the ResourceRequestInfo class to get the frame_tree_node_id field.
5. The EmbedderObserver class now uses the WCO::RenderFrameHostChanged notification to determine
if a RFH is swapped with another. This is used to determine when we should clean up the stream.
The RFH being swapped out is tracked in a map.
6. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on
the main frame and use that to cleanup the stream.
BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/4b7467a5a8e36f6d59cbc2df29be42e9c12ec45b
Cr-Commit-Position: refs/heads/master@{#420537}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Use DidStartNavigation on the main frame as a signal in the EmbedderObserver that we need to clean … #Patch Set 3 : git cl format #Patch Set 4 : Remove include file #
Total comments: 7
Patch Set 5 : Address review comments #Patch Set 6 : Pass the frame_tree_node_id, render_frame_id and render_process_id parameters to the MimeHandlerStr… #
Total comments: 5
Patch Set 7 : Address review comments #Patch Set 8 : Fixed comment #
Total comments: 4
Patch Set 9 : Don't return the FrameTreeNode id from the RenderFrameHostImpl instance if it got swapped out. #Patch Set 10 : Fix browser_test redness #Patch Set 11 : Add a new method GetHostForFrameTreeNode to the RenderFrameHost class #
Total comments: 2
Patch Set 12 : Use WCO::RenderFrameHostChanged to detect the case when a RFH is swapped out for another in the Emb… #
Total comments: 2
Patch Set 13 : Add a static getter to WC to get the WC associated with a FTNID #Patch Set 14 : Remove newlines #Patch Set 15 : Rebase to tip #Patch Set 16 : Attempt to fix bot update step #Patch Set 17 : Update chrome_resource_dispatcher_host_delegate.cc #Patch Set 18 : Update chrome_resource_dispatcher_host_delegate.cc #
Total comments: 14
Patch Set 19 : Address review comments #Patch Set 20 : Fix compile failures #
Total comments: 4
Patch Set 21 : Address review comments #Patch Set 22 : Update the RFNID, RFID, RPID to those of the new frame when we receive a frame change notification #
Total comments: 2
Patch Set 23 : Add DCHECK for FTNID #Patch Set 24 : Add DCHECK for FTNID #Patch Set 25 : Removed include for map #Patch Set 26 : Fix DCHECK leading to browser test redness #Messages
Total messages: 128 (83 generated)
ananta@chromium.org changed reviewers: + clamy@chromium.org
https://codereview.chromium.org/2331343005/diff/1/content/browser/loader/navi... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/2331343005/diff/1/content/browser/loader/navi... content/browser/loader/navigation_resource_handler.cc:137: if (!info->is_stream()) Deferring this causes a DCHECK to fire in InterceptionResourceHandler
The CQ bit was checked by ananta@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...
Description was changed from ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handed in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. There is still an issue with the MimeHandlerStreamManager::EmbedderObserver class which relies on matching the render frame id and process id in notifications like RenderFrameDeleted in WebContentsObserver. That will not work with PlzNavigate. I will address this in a future patch. BUG=504347 ========== to ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. There is still an issue with the MimeHandlerStreamManager::EmbedderObserver class which relies on matching the render frame id and process id in notifications like RenderFrameDeleted in WebContentsObserver. That will not work with PlzNavigate. I will address this in a future patch. BUG=504347 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ananta@chromium.org changed reviewers: + nasko@chromium.org
+nasko
jam@chromium.org changed reviewers: + jam@chromium.org
https://codereview.chromium.org/2331343005/diff/1/chrome/browser/renderer_hos... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2331343005/diff/1/chrome/browser/renderer_hos... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:735: info->GetWebContentsGetterForRequest())); ditto: let's just pass only the WC getter https://codereview.chromium.org/2331343005/diff/1/extensions/browser/guest_vi... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.h (right): https://codereview.chromium.org/2331343005/diff/1/extensions/browser/guest_vi... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.h:40: content::WebContents* web_contents, why are we passing both WC & RF IDs? it seems we should only have one, since otherwise there could be bugs where they don't match.
Description was changed from ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. There is still an issue with the MimeHandlerStreamManager::EmbedderObserver class which relies on matching the render frame id and process id in notifications like RenderFrameDeleted in WebContentsObserver. That will not work with PlzNavigate. I will address this in a future patch. BUG=504347 ========== to ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 ==========
Description was changed from ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 ========== to ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 ==========
https://codereview.chromium.org/2331343005/diff/1/chrome/browser/renderer_hos... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2331343005/diff/1/chrome/browser/renderer_hos... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:735: info->GetWebContentsGetterForRequest())); On 2016/09/14 22:49:52, jam wrote: > ditto: let's just pass only the WC getter Done. https://codereview.chromium.org/2331343005/diff/1/extensions/browser/guest_vi... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.h (right): https://codereview.chromium.org/2331343005/diff/1/extensions/browser/guest_vi... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.h:40: content::WebContents* web_contents, On 2016/09/14 22:49:52, jam wrote: > why are we passing both WC & RF IDs? it seems we should only have one, since > otherwise there could be bugs where they don't match. Done. As per our discussion, I changed the EmbedderObserver to cleanup the stream on the top level frame navigation.
The CQ bit was checked by ananta@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.
Thanks! It'd also be nice to get an extension API owner to review the changes in extensions. https://codereview.chromium.org/2331343005/diff/60001/content/browser/loader/... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/2331343005/diff/60001/content/browser/loader/... content/browser/loader/navigation_resource_handler.cc:136: // mime type sniffing, etc. Could you add a TODO to check if they should still go through the throttle checks? I understand that this doesn't work currently because the InterceptingResourceHandler placed before it doesn't expect the old handler to defer the request. https://codereview.chromium.org/2331343005/diff/60001/extensions/browser/gues... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc (left): https://codereview.chromium.org/2331343005/diff/60001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:168: void MimeHandlerStreamManager::EmbedderObserver::RenderFrameDeleted( RenderFrameDeleted & RenderProcessGone would also cover additional cases beyond the main frame navigation. Is it ok to remove them?
https://codereview.chromium.org/2331343005/diff/60001/content/browser/loader/... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/2331343005/diff/60001/content/browser/loader/... content/browser/loader/navigation_resource_handler.cc:136: // mime type sniffing, etc. On 2016/09/15 13:15:24, clamy wrote: > Could you add a TODO to check if they should still go through the throttle > checks? I understand that this doesn't work currently because the > InterceptingResourceHandler placed before it doesn't expect the old handler to > defer the request. Done. https://codereview.chromium.org/2331343005/diff/60001/extensions/browser/gues... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc (left): https://codereview.chromium.org/2331343005/diff/60001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:168: void MimeHandlerStreamManager::EmbedderObserver::RenderFrameDeleted( On 2016/09/15 13:15:24, clamy wrote: > RenderFrameDeleted & RenderProcessGone would also cover additional cases beyond > the main frame navigation. Is it ok to remove them? I think so. The only issue is the stream would remain around in some cases which should not be a problem?.
ananta@chromium.org changed reviewers: + asargent@chromium.org
+asargent for extensions owners. Please look at the extension API changes.
The CQ bit was checked by ananta@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.
asargent@chromium.org changed reviewers: + lazyboy@chromium.org
I'm not super familiar with this code - lazyboy@ may be a better reviewer
https://codereview.chromium.org/2331343005/diff/60001/extensions/browser/gues... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc (left): https://codereview.chromium.org/2331343005/diff/60001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:168: void MimeHandlerStreamManager::EmbedderObserver::RenderFrameDeleted( On 2016/09/15 21:25:03, ananta wrote: > On 2016/09/15 13:15:24, clamy wrote: > > RenderFrameDeleted & RenderProcessGone would also cover additional cases > beyond > > the main frame navigation. Is it ok to remove them? > > I think so. The only issue is the stream would remain around in some cases which > should not be a problem?. Could that potentially lead to wasted memory?
https://codereview.chromium.org/2331343005/diff/60001/extensions/browser/gues... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc (left): https://codereview.chromium.org/2331343005/diff/60001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:168: void MimeHandlerStreamManager::EmbedderObserver::RenderFrameDeleted( On 2016/09/16 11:41:21, clamy wrote: > On 2016/09/15 21:25:03, ananta wrote: > > On 2016/09/15 13:15:24, clamy wrote: > > > RenderFrameDeleted & RenderProcessGone would also cover additional cases > > beyond > > > the main frame navigation. Is it ok to remove them? > > > > I think so. The only issue is the stream would remain around in some cases > which > > should not be a problem?. > > Could that potentially lead to wasted memory? +1, If the embedder page loads an iframe, and we put a pdf inside that iframe... then navigating the iframe would lead to keeping the frame's associated streams around, right?
https://codereview.chromium.org/2331343005/diff/60001/extensions/browser/gues... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc (left): https://codereview.chromium.org/2331343005/diff/60001/extensions/browser/gues... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:168: void MimeHandlerStreamManager::EmbedderObserver::RenderFrameDeleted( On 2016/09/16 17:59:45, lazyboy wrote: > On 2016/09/16 11:41:21, clamy wrote: > > On 2016/09/15 21:25:03, ananta wrote: > > > On 2016/09/15 13:15:24, clamy wrote: > > > > RenderFrameDeleted & RenderProcessGone would also cover additional cases > > > beyond > > > > the main frame navigation. Is it ok to remove them? > > > > > > I think so. The only issue is the stream would remain around in some cases > > which > > > should not be a problem?. > > > > Could that potentially lead to wasted memory? > > +1, If the embedder page loads an iframe, and we put a pdf inside that iframe... > then navigating the iframe would lead to keeping the frame's associated streams > around, right? I reverted portions of this file to bring back the render_frame_id and render_process_id parameters and also pass in the frame_tree_node_id parameters. This parameter is used for validating if the corresponding frame navigated away in the PlzNavigate case. In the non PlzNavigate case we check using the render_frame_id and process_id as before. For PlzNavigate we cleanup the stream only when we receive a navigate for the main frame or the child frame, or when the renderer process has gone.
Description was changed from ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 ========== to ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. I added a getter to the ResourceRequestInfo class to get the frame_tree_node_id field. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 ==========
Description was changed from ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. I added a getter to the ResourceRequestInfo class to get the frame_tree_node_id field. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 ========== to ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. I added a getter to the ResourceRequestInfo class to get the frame_tree_node_id field. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main or child frame and use that to cleanup the stream. The old methods of cleaning up when the RenderFrameHost goes or when the renderer process goes away are still there. Although the former only cleans up for the non PlzNavigate case. BUG=504347 ==========
Description was changed from ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. I added a getter to the ResourceRequestInfo class to get the frame_tree_node_id field. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main or child frame and use that to cleanup the stream. The old methods of cleaning up when the RenderFrameHost goes or when the renderer process goes away are still there. Although the former only cleans up for the non PlzNavigate case. BUG=504347 ========== to ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. I added a getter to the ResourceRequestInfo class to get the frame_tree_node_id field. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main or child frame and use that to cleanup the stream. The old methods of cleaning up when the RenderFrameHost goes or when the renderer process goes away are still there. Although the former only cleans up for the non PlzNavigate case. BUG=504347 ==========
Description was changed from ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. I added a getter to the ResourceRequestInfo class to get the frame_tree_node_id field. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main or child frame and use that to cleanup the stream. The old methods of cleaning up when the RenderFrameHost goes or when the renderer process goes away are still there. Although the former only cleans up for the non PlzNavigate case. BUG=504347 ========== to ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. I added a getter to the ResourceRequestInfo class to get the frame_tree_node_id field. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main or child frame and use that to cleanup the stream. The old methods of cleaning up when the RenderFrameHost goes or when the renderer process goes away are still there. Although the former only cleans up for the non PlzNavigate case. BUG=504347 ==========
The CQ bit was checked by ananta@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...
https://codereview.chromium.org/2331343005/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/streams_private/streams_private_api.h (right): https://codereview.chromium.org/2331343005/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/streams_private/streams_private_api.h:44: // PlzNavigate ships. is this true? i.e. we'll still need them for the non-top level plugin case? nit: please document the three different IDs (i.e. mentioning that for plznavigate top level plugins, FTNID has the value, otherwise it's the two other IDs) https://codereview.chromium.org/2331343005/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2331343005/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:729: // when PlzNavigate is enabled by default. ditto: even with plznavigate, i believe the RFIDs will be valid for non top level case
https://codereview.chromium.org/2331343005/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/streams_private/streams_private_api.h (right): https://codereview.chromium.org/2331343005/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/streams_private/streams_private_api.h:44: // PlzNavigate ships. On 2016/09/17 00:10:07, jam wrote: > is this true? i.e. we'll still need them for the non-top level plugin case? > > nit: please document the three different IDs (i.e. mentioning that for > plznavigate top level plugins, FTNID has the value, otherwise it's the two other > IDs) Thanks. Changed the comment https://codereview.chromium.org/2331343005/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2331343005/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:729: // when PlzNavigate is enabled by default. On 2016/09/17 00:10:07, jam wrote: > ditto: even with plznavigate, i believe the RFIDs will be valid for non top > level case Thanks. Removed the comment.
The CQ bit was checked by ananta@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 checked by ananta@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...
https://codereview.chromium.org/2331343005/diff/110010/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc (right): https://codereview.chromium.org/2331343005/diff/110010/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:177: if (content::IsBrowserSideNavigationEnabled() || Q: Isn't it better to keep this as is? That way, node.removeChild(iframe_containing_pdf) will continue to work gracefully. https://codereview.chromium.org/2331343005/diff/110010/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:211: if (frame_tree_node_id_ != -1) { s/-1/FrameTreeNode::FrameTreeNodeInvalidId
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2331343005/diff/110010/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc (right): https://codereview.chromium.org/2331343005/diff/110010/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:177: if (content::IsBrowserSideNavigationEnabled() || On 2016/09/17 00:59:01, lazyboy wrote: > Q: Isn't it better to keep this as is? That way, > node.removeChild(iframe_containing_pdf) will continue to work gracefully. Thanks. Done. I made a change to RenderFrameHostImpl to not return the FrameTreeNode's id if the RenderFrameHost got swapped with another host. This causes us to inadvarently clean up the stream which causes the first load of PDF to fail. Scenario is as below: 1. A RenderFrameHostImpl instance is created for the PDF navigation. 2. We setup the stream with the frame tree node id etc. Assume it is 1 3. Another RenderFrameHostImpl instance is created and the instance created in step 1 gets swapped out. Now the FrameTreeNode is associated with a different instance. We want the stream to remain valid in this case. https://codereview.chromium.org/2331343005/diff/110010/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:211: if (frame_tree_node_id_ != -1) { On 2016/09/17 00:59:01, lazyboy wrote: > s/-1/FrameTreeNode::FrameTreeNodeInvalidId Can't do that as that constant is defined in the FrameTreeNode class which is not in content public
Description was changed from ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. I added a getter to the ResourceRequestInfo class to get the frame_tree_node_id field. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main or child frame and use that to cleanup the stream. The old methods of cleaning up when the RenderFrameHost goes or when the renderer process goes away are still there. Although the former only cleans up for the non PlzNavigate case. BUG=504347 ========== to ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. I added a getter to the ResourceRequestInfo class to get the frame_tree_node_id field. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main or child frame and use that to cleanup the stream. The old methods of cleaning up when the RenderFrameHost goes or when the renderer process goes away are still there. Although the former only cleans up for the non PlzNavigate case. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
fenerliserdar81@gmail.com changed reviewers: + fenerliserdar81@gmail.com
fenerliserdar81@gmail.com changed reviewers: + fenerliserdar81@gmail.com
lgtm
lgtm lgtm
Description was changed from ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. I added a getter to the ResourceRequestInfo class to get the frame_tree_node_id field. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main or child frame and use that to cleanup the stream. The old methods of cleaning up when the RenderFrameHost goes or when the renderer process goes away are still there. Although the former only cleans up for the non PlzNavigate case. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. I added a getter to the ResourceRequestInfo class to get the frame_tree_node_id field. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. The old methods of cleaning up when the RenderFrameHost goes or when the renderer process goes away are still there. Although the former only cleans up for the non PlzNavigate case. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
I updated the patch to not use NavigationHandle::GetRenderFrameHost in cases outside main frame. This causes a CHECK to fire here https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_ha... Brought back the old DidStartProvisionalLoadForFrame which works well for other frames.
The CQ bit was checked by ananta@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...
https://codereview.chromium.org/2331343005/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/streams_private/streams_private_api.h (right): https://codereview.chromium.org/2331343005/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/streams_private/streams_private_api.h:44: // PlzNavigate ships. On 2016/09/17 00:23:27, ananta wrote: > On 2016/09/17 00:10:07, jam wrote: > > is this true? i.e. we'll still need them for the non-top level plugin case? > > > > nit: please document the three different IDs (i.e. mentioning that for > > plznavigate top level plugins, FTNID has the value, otherwise it's the two > other > > IDs) > > Thanks. Changed the comment I debugged the pdf case inside an iframe. The render_frame_id and render_process_id are still -1 in this case. That should not be the top level case which you mentioned?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are to allow stream based requests to be handled in navigation_resource_handler.cc and fixing the way WebContents is obtained in the other places. I changed the MimeHandlerStreamManager::AddStream function to take in the WebContents directly. I added a getter to the ResourceRequestInfo class to get the frame_tree_node_id field. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. The old methods of cleaning up when the RenderFrameHost goes or when the renderer process goes away are still there. Although the former only cleans up for the non PlzNavigate case. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are as below: 1.Allow stream based requests to be handled in navigation_resource_handler.cc. 2.Fixing the way WebContents is obtained in the other places with the ResourceRequestInfoImpl::GetWebContentsGetterForRequest function. 3.The MimeHandlerStreamManager::AddStream function takes in the WebContents pointer. 4 A getter has been added to the ResourceRequestInfo class to get the frame_tree_node_id field. 5.A new function GetHostForFrameTreeNode has been added to the RenderFrameHost class to return the associated RenderFrameHost instance for the frame tree node. This is used in the EmbedderObserver class to check if the RenderFrameHost instance going away requires a clean up of the stream. If the RenderFrameHost has been swapped with another instance then we need to retain the stream. 6.We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are as below: 1.Allow stream based requests to be handled in navigation_resource_handler.cc. 2.Fixing the way WebContents is obtained in the other places with the ResourceRequestInfoImpl::GetWebContentsGetterForRequest function. 3.The MimeHandlerStreamManager::AddStream function takes in the WebContents pointer. 4 A getter has been added to the ResourceRequestInfo class to get the frame_tree_node_id field. 5.A new function GetHostForFrameTreeNode has been added to the RenderFrameHost class to return the associated RenderFrameHost instance for the frame tree node. This is used in the EmbedderObserver class to check if the RenderFrameHost instance going away requires a clean up of the stream. If the RenderFrameHost has been swapped with another instance then we need to retain the stream. 6.We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are as below: 1. Allow stream based requests to be handled in navigation_resource_handler.cc. 2. Fixing the way WebContents is obtained in the other places with the ResourceRequestInfoImpl::GetWebContentsGetterForRequest function. 3. The MimeHandlerStreamManager::AddStream function takes in the WebContents pointer. 4 A getter has been added to the ResourceRequestInfo class to get the frame_tree_node_id field. 5. A new function GetHostForFrameTreeNode has been added to the RenderFrameHost class to return the associated RenderFrameHost instance for the frame tree node. This is used in the EmbedderObserver class to check if the RenderFrameHost instance going away requires a clean up of the stream. If the RenderFrameHost has been swapped with another instance then we need to retain the stream. 6. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are as below: 1. Allow stream based requests to be handled in navigation_resource_handler.cc. 2. Fixing the way WebContents is obtained in the other places with the ResourceRequestInfoImpl::GetWebContentsGetterForRequest function. 3. The MimeHandlerStreamManager::AddStream function takes in the WebContents pointer. 4 A getter has been added to the ResourceRequestInfo class to get the frame_tree_node_id field. 5. A new function GetHostForFrameTreeNode has been added to the RenderFrameHost class to return the associated RenderFrameHost instance for the frame tree node. This is used in the EmbedderObserver class to check if the RenderFrameHost instance going away requires a clean up of the stream. If the RenderFrameHost has been swapped with another instance then we need to retain the stream. 6. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are as below: 1. Allow stream based requests to be handled in navigation_resource_handler.cc. 2. Fixing the way WebContents is obtained in the other places with the ResourceRequestInfoImpl::GetWebContentsGetterForRequest function. 3. The MimeHandlerStreamManager::AddStream function takes in the WebContents pointer. 4 A getter has been added to the ResourceRequestInfo class to get the frame_tree_node_id field. 5. A new function GetHostForFrameTreeNode has been added to the RenderFrameHost class to return the associated RenderFrameHost instance for the frame tree node. This is used in the EmbedderObserver class to check if the RenderFrameHost instance going away requires a clean up of the stream. If the RenderFrameHost has been swapped with another instance then we need to retain the stream. 6. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by ananta@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 checked by ananta@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.
Could you check with nasko@ what the situation is regarding FTN ids and extensions? I know we already expose them in NavigationHandle, so there probably is a way to get a RFH from the value already. Also be aware that I have this patch in review: https://codereview.chromium.org/2335133003/, which introduces some data that the content/ embedder can add to the navigation on the UI thread and that will be available in the ResourceRequest. In particular, we use it in extensions code to pass identifiers necessary to identify in which frame the navigation is taking place. This may be enough to get this patch to work without further modifications to the RFH & RequestInfo API?
I don't have the time to review this CL in-depth today and tomorrow, so please work on something else and give me a chance to read it through. Thanks in advance! https://codereview.chromium.org/2331343005/diff/190001/content/public/browser... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2331343005/diff/190001/content/public/browser... content/public/browser/render_frame_host.h:96: virtual RenderFrameHost* GetHostForFrameTreeNode() = 0; I don't quite understand why we need this method and it sounds dangerous to me.
https://codereview.chromium.org/2331343005/diff/190001/content/public/browser... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2331343005/diff/190001/content/public/browser... content/public/browser/render_frame_host.h:96: virtual RenderFrameHost* GetHostForFrameTreeNode() = 0; On 2016/09/19 22:51:19, nasko (slow) wrote: > I don't quite understand why we need this method and it sounds dangerous to me. A RFH can get swapped out with another RFH and the FrametreeNode remains the same with the same id. If this happens then the RFH which got swapped out is destroyed, which then fires the frame destroyed event through WCO. We don't want to clean up the streams in the extension in this case. The extension code calls this method to get the current RFH for the FrameTreeNode and compares it with the one going away. Another alternative might be to set a state on the RFH when it is swapped out and have a public accessor method on RFH (possibly IsSwappedOut()?) We could check this in places which may be affected by this like the extension code.
On 2016/09/20 00:32:32, ananta wrote: > https://codereview.chromium.org/2331343005/diff/190001/content/public/browser... > File content/public/browser/render_frame_host.h (right): > > https://codereview.chromium.org/2331343005/diff/190001/content/public/browser... > content/public/browser/render_frame_host.h:96: virtual RenderFrameHost* > GetHostForFrameTreeNode() = 0; > On 2016/09/19 22:51:19, nasko (slow) wrote: > > I don't quite understand why we need this method and it sounds dangerous to > me. > > A RFH can get swapped out with another RFH and the FrametreeNode remains the > same with the same id. If this happens then the RFH which got swapped out is > destroyed, which then fires the frame destroyed event through WCO. We don't want > to clean up the streams in the extension in this case. If that's the case, why not listen for WCO::RenderFrameHostChanged? It will tell you which RenderFrameHost is changed with which new RenderFrameHost. No need for extra public API. > The extension code calls this method to get the current RFH for the > FrameTreeNode and compares it with the one going away. > > Another alternative might be to set a state on the RFH when it is swapped out > and have a public accessor method on RFH (possibly IsSwappedOut()?) We could > check this in places which may be affected by this like the extension code. Swapped out as a concept is gone. Once the unload handler has completed running, the RenderFrameHost is deleted.
On 2016/09/20 00:35:36, nasko (slow) wrote: > On 2016/09/20 00:32:32, ananta wrote: > > > https://codereview.chromium.org/2331343005/diff/190001/content/public/browser... > > File content/public/browser/render_frame_host.h (right): > > > > > https://codereview.chromium.org/2331343005/diff/190001/content/public/browser... > > content/public/browser/render_frame_host.h:96: virtual RenderFrameHost* > > GetHostForFrameTreeNode() = 0; > > On 2016/09/19 22:51:19, nasko (slow) wrote: > > > I don't quite understand why we need this method and it sounds dangerous to > > me. > > > > A RFH can get swapped out with another RFH and the FrametreeNode remains the > > same with the same id. If this happens then the RFH which got swapped out is > > destroyed, which then fires the frame destroyed event through WCO. We don't > want > > to clean up the streams in the extension in this case. > > If that's the case, why not listen for WCO::RenderFrameHostChanged? It will tell > you which RenderFrameHost is changed with which new RenderFrameHost. No need for > extra public API. > > > The extension code calls this method to get the current RFH for the > > FrameTreeNode and compares it with the one going away. > > > > Another alternative might be to set a state on the RFH when it is swapped out > > and have a public accessor method on RFH (possibly IsSwappedOut()?) We could > > check this in places which may be affected by this like the extension code. > > Swapped out as a concept is gone. Once the unload handler has completed running, > the RenderFrameHost is deleted. Thanks for pointing that out. Done. I also added a flag to the EmbedderObserver class to track for PlzNavigate whether a load notification is the first one. With PlzNavigate we get the load notification early on and we don't want to clean up the stream when it is being loaded. This only happens for PlzNavigate.
Description was changed from ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are as below: 1. Allow stream based requests to be handled in navigation_resource_handler.cc. 2. Fixing the way WebContents is obtained in the other places with the ResourceRequestInfoImpl::GetWebContentsGetterForRequest function. 3. The MimeHandlerStreamManager::AddStream function takes in the WebContents pointer. 4 A getter has been added to the ResourceRequestInfo class to get the frame_tree_node_id field. 5. A new function GetHostForFrameTreeNode has been added to the RenderFrameHost class to return the associated RenderFrameHost instance for the frame tree node. This is used in the EmbedderObserver class to check if the RenderFrameHost instance going away requires a clean up of the stream. If the RenderFrameHost has been swapped with another instance then we need to retain the stream. 6. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are as below: 1. Allow stream based requests to be handled in navigation_resource_handler.cc. 2. Fixing the way WebContents is obtained in the other places with the ResourceRequestInfoImpl::GetWebContentsGetterForRequest function. 3. The MimeHandlerStreamManager::AddStream function takes in the WebContents pointer. 4 A getter has been added to the ResourceRequestInfo class to get the frame_tree_node_id field. 5. The EmbedderObserver class now uses the WCO::RenderFrameHostChanged notification to determine if a RFH is swapped with another. This is used to determine when we should clean up the stream. The RFH being swapped out is tracked in a map. 6. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
On 2016/09/19 12:04:16, clamy wrote: > Could you check with nasko@ what the situation is regarding FTN ids and > extensions? I know we already expose them in NavigationHandle, so there probably > is a way to get a RFH from the value already. > > Also be aware that I have this patch in review: > https://codereview.chromium.org/2335133003/, which introduces some data that the > content/ embedder can add to the navigation on the UI thread and that will be > available in the ResourceRequest. In particular, we use it in extensions code to > pass identifiers necessary to identify in which frame the navigation is taking > place. This may be enough to get this patch to work without further > modifications to the RFH & RequestInfo API? nasko mentioned that we have WCO::RenderFrameHostChanged. I use this to detemine when a RFH is swapped out in the EmbedderObserver class. The RFH public interface change has been reverted. The only other change is to get the frame id from RequestInfo class. If it is ok with you, we can land this patch once other reviewers are happy and revisit?
The CQ bit was checked by ananta@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.
https://codereview.chromium.org/2331343005/diff/210001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2331343005/diff/210001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:187: const ResourceRequestInfo::WebContentsGetter& web_contents_getter, it seems redundant passing both this and FTNID and RPID/RFID. i.e. what happens if it's not the same? nasko: if we're exposing FTNID outside of content/public, how about having a static WebContents getter from FTNID?
https://codereview.chromium.org/2331343005/diff/210001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2331343005/diff/210001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:187: const ResourceRequestInfo::WebContentsGetter& web_contents_getter, On 2016/09/20 21:29:35, jam wrote: > it seems redundant passing both this and FTNID and RPID/RFID. i.e. what happens > if it's not the same? > > nasko: if we're exposing FTNID outside of content/public, how about having a > static WebContents getter from FTNID? Added a static getter to get the WebContents associated with a FTNID. Currently this leads to duplicated code trying to fetch the WC in three places. Here, in StreamsPrivateAPI::ExecuteMimeTypeHandler and the EmbedderObserver ctor. If we add a helper returning the WC given a FTNID, RFID, RPID then it looks like ResourceRequestInfoImpl::GetWebContentsGetter. I updated the patch anyways. Please let me know what you think. I agree that passing in a WC along with the various IDs seems redundant.
The CQ bit was checked by ananta@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ananta@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Am 21.09.2016 07:26 schrieb <ananta@chromium.org>: > > https://codereview.chromium.org/2331343005/diff/210001/ > chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc > File > chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc > (right): > > https://codereview.chromium.org/2331343005/diff/210001/ > chrome/browser/renderer_host/chrome_resource_dispatcher_ > host_delegate.cc#newcode187 > chrome/browser/renderer_host/chrome_resource_dispatcher_ > host_delegate.cc:187: > const ResourceRequestInfo::WebContentsGetter& web_contents_getter, > On 2016/09/20 21:29:35, jam wrote: > > it seems redundant passing both this and FTNID and RPID/RFID. i.e. > what happens > > if it's not the same? > > > > nasko: if we're exposing FTNID outside of content/public, how about > having a > > static WebContents getter from FTNID? > > Added a static getter to get the WebContents associated with a FTNID. > Currently this leads to duplicated code trying to fetch the WC in three > places. Here, in StreamsPrivateAPI::ExecuteMimeTypeHandler and the > EmbedderObserver ctor. If we add a helper returning the WC given a > FTNID, RFID, RPID then it looks like > ResourceRequestInfoImpl::GetWebContentsGetter. > > I updated the patch anyways. Please let me know what you think. I agree > that passing in a WC along with the various IDs seems redundant. > > https://codereview.chromium.org/2331343005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by ananta@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...
On 2016/09/21 05:26:33, ananta wrote: > https://codereview.chromium.org/2331343005/diff/210001/chrome/browser/rendere... > File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc > (right): > > https://codereview.chromium.org/2331343005/diff/210001/chrome/browser/rendere... > chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:187: > const ResourceRequestInfo::WebContentsGetter& web_contents_getter, > On 2016/09/20 21:29:35, jam wrote: > > it seems redundant passing both this and FTNID and RPID/RFID. i.e. what > happens > > if it's not the same? > > > > nasko: if we're exposing FTNID outside of content/public, how about having a > > static WebContents getter from FTNID? > > Added a static getter to get the WebContents associated with a FTNID. Currently > this leads to duplicated code trying to fetch the WC in three places. Here, in > StreamsPrivateAPI::ExecuteMimeTypeHandler and the EmbedderObserver ctor. If we > add a helper returning the WC given a FTNID, RFID, RPID then it looks like > ResourceRequestInfoImpl::GetWebContentsGetter. > > I updated the patch anyways. Please let me know what you think. I agree that > passing in a WC along with the various IDs seems redundant. lgtm IMO the if statement to get the WC from either set of IDs is pretty minimal. if we really find ourselves doing this in many places, we could move it to a helper function.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Let's clarify a bit how MimeHandlerStreamManager works before committing this. It might allow for lots of code simplification. https://codereview.chromium.org/2331343005/diff/330001/chrome/browser/extensi... File chrome/browser/extensions/api/streams_private/streams_private_api.cc (left): https://codereview.chromium.org/2331343005/diff/330001/chrome/browser/extensi... chrome/browser/extensions/api/streams_private/streams_private_api.cc:68: content::WebContents* web_contents, Why can't we pass this through to the MimeHandlerStreamManager? https://codereview.chromium.org/2331343005/diff/330001/content/public/browser... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2331343005/diff/330001/content/public/browser... content/public/browser/web_contents.h:191: int frame_tree_node_id); I know we might need to do this at some point, but I'm not convinced that this CL requires it quite yet. https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc (right): https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:76: : public content::WebContentsObserver { Does this object need to track subframes or are is it only restricted to main frames? If the latter, I think this code can be much simpler. From what I recall, this is only used for GuestView, which basically has a WebContents embedded in a different WebContents. Why not get the WebContents as a param and get the main frame from it. Is there a case where we will get a subframe routing id here? https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:115: // stream. For e.g if a RFH is swapped with another RFH, and the first one nit: e.g. https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:202: if (!IsTrackedRenderFrameHost(render_frame_host)) { nit: Why add {}? The rest of the file omits {} on one line if statements. https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:247: changed_hosts_map_[old_host] = new_host; Why do we need to keep a map? Can't we always just remember the new host into a member variable?
https://codereview.chromium.org/2331343005/diff/330001/chrome/browser/extensi... File chrome/browser/extensions/api/streams_private/streams_private_api.cc (left): https://codereview.chromium.org/2331343005/diff/330001/chrome/browser/extensi... chrome/browser/extensions/api/streams_private/streams_private_api.cc:68: content::WebContents* web_contents, On 2016/09/21 21:32:18, nasko (slow) wrote: > Why can't we pass this through to the MimeHandlerStreamManager? Please refer to jam's comment a couple patchsets before. His point was it seems redundant to pass WebContents, FTNID, RFID, RPID to a function. And it could lead to bad practices like passing an unrelated WebContents pointer to such functions. His suggestion was to add a static getter to WebContents which returns it based on a FTNID https://codereview.chromium.org/2331343005/diff/330001/content/public/browser... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2331343005/diff/330001/content/public/browser... content/public/browser/web_contents.h:191: int frame_tree_node_id); On 2016/09/21 21:32:18, nasko (slow) wrote: > I know we might need to do this at some point, but I'm not convinced that this > CL requires it quite yet. Please refer to the comments from jam in an earlier patchset. https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc (right): https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:76: : public content::WebContentsObserver { On 2016/09/21 21:32:18, nasko (slow) wrote: > Does this object need to track subframes or are is it only restricted to main > frames? If the latter, I think this code can be much simpler. > > From what I recall, this is only used for GuestView, which basically has a > WebContents embedded in a different WebContents. Why not get the WebContents as > a param and get the main frame from it. Is there a case where we will get a > subframe routing id here? This codepath also kicks in for iframes. https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:115: // stream. For e.g if a RFH is swapped with another RFH, and the first one On 2016/09/21 21:32:18, nasko (slow) wrote: > nit: e.g. Comment has changed https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:202: if (!IsTrackedRenderFrameHost(render_frame_host)) { On 2016/09/21 21:32:18, nasko (slow) wrote: > nit: Why add {}? The rest of the file omits {} on one line if statements. Done. https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:247: changed_hosts_map_[old_host] = new_host; On 2016/09/21 21:32:18, nasko (slow) wrote: > Why do we need to keep a map? Can't we always just remember the new host into a > member variable? Thanks. Done
https://codereview.chromium.org/2331343005/diff/330001/chrome/browser/extensi... File chrome/browser/extensions/api/streams_private/streams_private_api.cc (left): https://codereview.chromium.org/2331343005/diff/330001/chrome/browser/extensi... chrome/browser/extensions/api/streams_private/streams_private_api.cc:68: content::WebContents* web_contents, On 2016/09/21 21:59:12, ananta wrote: > On 2016/09/21 21:32:18, nasko (slow) wrote: > > Why can't we pass this through to the MimeHandlerStreamManager? > > Please refer to jam's comment a couple patchsets before. His point was it seems > redundant to pass WebContents, FTNID, RFID, RPID to a function. And it could > lead to bad practices like passing an unrelated WebContents pointer to such > functions. I completely agree with avoiding the redundancy. What I'm trying to say is that if this code (aka MimeHandlerStreamManager) is only using GuestView, there is no reason to pass in an RFH, since passing the WebContents and using its main frame is equivalent. It will also be very clear that the unit of operation of this code is WebContents rather than a frame granularity. > His suggestion was to add a static getter to WebContents which returns it based > on a FTNID https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc (right): https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:76: : public content::WebContentsObserver { On 2016/09/21 21:59:12, ananta wrote: > On 2016/09/21 21:32:18, nasko (slow) wrote: > > Does this object need to track subframes or are is it only restricted to main > > frames? If the latter, I think this code can be much simpler. > > > > From what I recall, this is only used for GuestView, which basically has a > > WebContents embedded in a different WebContents. Why not get the WebContents > as > > a param and get the main frame from it. Is there a case where we will get a > > subframe routing id here? > > This codepath also kicks in for iframes. Can you help me understand in what cases that happens?
On 2016/09/21 22:08:55, nasko (slow) wrote: > https://codereview.chromium.org/2331343005/diff/330001/chrome/browser/extensi... > File chrome/browser/extensions/api/streams_private/streams_private_api.cc > (left): > > https://codereview.chromium.org/2331343005/diff/330001/chrome/browser/extensi... > chrome/browser/extensions/api/streams_private/streams_private_api.cc:68: > content::WebContents* web_contents, > On 2016/09/21 21:59:12, ananta wrote: > > On 2016/09/21 21:32:18, nasko (slow) wrote: > > > Why can't we pass this through to the MimeHandlerStreamManager? > > > > Please refer to jam's comment a couple patchsets before. His point was it > seems > > redundant to pass WebContents, FTNID, RFID, RPID to a function. And it could > > lead to bad practices like passing an unrelated WebContents pointer to such > > functions. > > I completely agree with avoiding the redundancy. What I'm trying to say is that > if this code (aka MimeHandlerStreamManager) is only using GuestView, there is no > reason to pass in an RFH, since passing the WebContents and using its main frame > is equivalent. It will also be very clear that the unit of operation of this > code is WebContents rather than a frame granularity. > > > His suggestion was to add a static getter to WebContents which returns it > based > > on a FTNID > > https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... > File > extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc > (right): > > https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... > extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:76: > : public content::WebContentsObserver { > On 2016/09/21 21:59:12, ananta wrote: > > On 2016/09/21 21:32:18, nasko (slow) wrote: > > > Does this object need to track subframes or are is it only restricted to > main > > > frames? If the latter, I think this code can be much simpler. > > > > > > From what I recall, this is only used for GuestView, which basically has a > > > WebContents embedded in a different WebContents. Why not get the WebContents > > as > > > a param and get the main frame from it. Is there a case where we will get a > > > subframe routing id here? > > > > This codepath also kicks in for iframes. > > Can you help me understand in what cases that happens? Discussed with lazyboy about this over IM. He mentioned that we are interested in cleaning up prior to the guest view taking ownership of the stream. Hence we need to watch the main WC and not the guest WC. This also requires that we need the RFID, RPID and now the FTNID to track frame deletion and cleanup.
The CQ bit was checked by ananta@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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ananta@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.
On 2016/09/22 01:41:21, ananta wrote: > On 2016/09/21 22:08:55, nasko (slow) wrote: > > > https://codereview.chromium.org/2331343005/diff/330001/chrome/browser/extensi... > > File chrome/browser/extensions/api/streams_private/streams_private_api.cc > > (left): > > > > > https://codereview.chromium.org/2331343005/diff/330001/chrome/browser/extensi... > > chrome/browser/extensions/api/streams_private/streams_private_api.cc:68: > > content::WebContents* web_contents, > > On 2016/09/21 21:59:12, ananta wrote: > > > On 2016/09/21 21:32:18, nasko (slow) wrote: > > > > Why can't we pass this through to the MimeHandlerStreamManager? > > > > > > Please refer to jam's comment a couple patchsets before. His point was it > > seems > > > redundant to pass WebContents, FTNID, RFID, RPID to a function. And it could > > > lead to bad practices like passing an unrelated WebContents pointer to such > > > functions. > > > > I completely agree with avoiding the redundancy. What I'm trying to say is > that > > if this code (aka MimeHandlerStreamManager) is only using GuestView, there is > no > > reason to pass in an RFH, since passing the WebContents and using its main > frame > > is equivalent. It will also be very clear that the unit of operation of this > > code is WebContents rather than a frame granularity. > > > > > His suggestion was to add a static getter to WebContents which returns it > > based > > > on a FTNID > > > > > https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... > > File > > extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc > > (right): > > > > > https://codereview.chromium.org/2331343005/diff/330001/extensions/browser/gue... > > > extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:76: > > : public content::WebContentsObserver { > > On 2016/09/21 21:59:12, ananta wrote: > > > On 2016/09/21 21:32:18, nasko (slow) wrote: > > > > Does this object need to track subframes or are is it only restricted to > > main > > > > frames? If the latter, I think this code can be much simpler. > > > > > > > > From what I recall, this is only used for GuestView, which basically has a > > > > WebContents embedded in a different WebContents. Why not get the > WebContents > > > as > > > > a param and get the main frame from it. Is there a case where we will get > a > > > > subframe routing id here? > > > > > > This codepath also kicks in for iframes. > > > > Can you help me understand in what cases that happens? > > Discussed with lazyboy about this over IM. He mentioned that we are interested > in cleaning up prior > to the guest view taking ownership of the stream. Hence we need to watch the > main WC and not the guest WC. > This also requires that we need the RFID, RPID and now the FTNID to track frame > deletion and cleanup. Thanks for the follow up on this! I'm glad we have a better understanding now.
> > Discussed with lazyboy about this over IM. He mentioned that we are interested > > in cleaning up prior > > to the guest view taking ownership of the stream. Hence we need to watch the > > main WC and not the guest WC. > > This also requires that we need the RFID, RPID and now the FTNID to track > frame > > deletion and cleanup. > > Thanks for the follow up on this! I'm glad we have a better understanding now. Explicit LGTM.
I have one question regarding AbortStream() cleanup. https://codereview.chromium.org/2331343005/diff/370001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc (right): https://codereview.chromium.org/2331343005/diff/370001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:201: return; This comment needs to include a bit more details I think for clarity. We don't want to abort the stream if the frame we were tracking was changed to |new_host_|, right? We would clear when/if |new_host_| is deleted. However, couldn't IsTrackedRenderFrameHost() return false with |new_host_| and bail out on line 200 above? I see that when FTN id is used, it will return true with IsTrackedRenderFrameHost(), but that wouldn't work when we are tracking the frame via process_id/routing_id pair? Here is the recap of what I'm talking about: Embedder/tab opens stream from RFH1 we start tracking RFH1 RenderFrameHostChange(RFH1, RFH2) new_host_ = RFH2 RenderFrameDeleted(RFH1) we bail out in 204 because RFH1 doesn't match new_host_ RenderFrameDeleted(RFH2) happens somehow. Wouldn't IsTrackedRFH(RFH2) return false if we try to match RFH2->GetProcess()->GetID() and RFH2->GetRoutingId() with |render_frame_id_| and |render_process_id_|? https://codereview.chromium.org/2331343005/diff/370001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:256: return (render_frame_host->GetFrameTreeNodeId() == frame_tree_node_id_); nit: () is extraneous.
https://codereview.chromium.org/2331343005/diff/370001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc (right): https://codereview.chromium.org/2331343005/diff/370001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:201: return; On 2016/09/22 19:35:31, lazyboy wrote: > This comment needs to include a bit more details I think for clarity. > We don't want to abort the stream if the frame we were tracking was changed to > |new_host_|, right? We would clear when/if |new_host_| is deleted. > > However, couldn't IsTrackedRenderFrameHost() return false with |new_host_| and > bail out on line 200 above? I see that when FTN id is used, it will return true > with IsTrackedRenderFrameHost(), but that wouldn't work when we are tracking the > frame via process_id/routing_id pair? > > Here is the recap of what I'm talking about: > Embedder/tab opens stream from RFH1 > we start tracking RFH1 > RenderFrameHostChange(RFH1, RFH2) > new_host_ = RFH2 > RenderFrameDeleted(RFH1) > we bail out in 204 because RFH1 doesn't match new_host_ > RenderFrameDeleted(RFH2) happens somehow. > Wouldn't IsTrackedRFH(RFH2) return false if we try to match > RFH2->GetProcess()->GetID() and RFH2->GetRoutingId() with |render_frame_id_| and > |render_process_id_|? Moved the code to IsTrackedRenderFrameHost and added a comment based on what you mentioned above. https://codereview.chromium.org/2331343005/diff/370001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:256: return (render_frame_host->GetFrameTreeNodeId() == frame_tree_node_id_); On 2016/09/22 19:35:31, lazyboy wrote: > nit: () is extraneous. Done.
The CQ bit was checked by ananta@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...
lgtm https://codereview.chromium.org/2331343005/diff/410001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc (right): https://codereview.chromium.org/2331343005/diff/410001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:240: frame_tree_node_id_ = new_host_->GetFrameTreeNodeId(); Thanks. Doesn't FTN id stay the same? I'd DCHECK_EQ(old ftn id, new_host_->ftn id) in this case.
https://codereview.chromium.org/2331343005/diff/410001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc (right): https://codereview.chromium.org/2331343005/diff/410001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc:240: frame_tree_node_id_ = new_host_->GetFrameTreeNodeId(); On 2016/09/22 22:23:45, lazyboy wrote: > Thanks. > > Doesn't FTN id stay the same? > I'd DCHECK_EQ(old ftn id, new_host_->ftn id) in this case. Yes. It does. Added a DCHECK
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fenerliserdar81@gmail.com, jam@chromium.org, nasko@chromium.org, lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2331343005/#ps470001 (title: "Removed include for map")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fenerliserdar81@gmail.com, jam@chromium.org, nasko@chromium.org, lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2331343005/#ps490001 (title: "Fix DCHECK leading to browser test redness")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are as below: 1. Allow stream based requests to be handled in navigation_resource_handler.cc. 2. Fixing the way WebContents is obtained in the other places with the ResourceRequestInfoImpl::GetWebContentsGetterForRequest function. 3. The MimeHandlerStreamManager::AddStream function takes in the WebContents pointer. 4 A getter has been added to the ResourceRequestInfo class to get the frame_tree_node_id field. 5. The EmbedderObserver class now uses the WCO::RenderFrameHostChanged notification to determine if a RFH is swapped with another. This is used to determine when we should clean up the stream. The RFH being swapped out is tracked in a map. 6. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are as below: 1. Allow stream based requests to be handled in navigation_resource_handler.cc. 2. Fixing the way WebContents is obtained in the other places with the ResourceRequestInfoImpl::GetWebContentsGetterForRequest function. 3. The MimeHandlerStreamManager::AddStream function takes in the WebContents pointer. 4 A getter has been added to the ResourceRequestInfo class to get the frame_tree_node_id field. 5. The EmbedderObserver class now uses the WCO::RenderFrameHostChanged notification to determine if a RFH is swapped with another. This is used to determine when we should clean up the stream. The RFH being swapped out is tracked in a map. 6. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #26 (id:490001)
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are as below: 1. Allow stream based requests to be handled in navigation_resource_handler.cc. 2. Fixing the way WebContents is obtained in the other places with the ResourceRequestInfoImpl::GetWebContentsGetterForRequest function. 3. The MimeHandlerStreamManager::AddStream function takes in the WebContents pointer. 4 A getter has been added to the ResourceRequestInfo class to get the frame_tree_node_id field. 5. The EmbedderObserver class now uses the WCO::RenderFrameHostChanged notification to determine if a RFH is swapped with another. This is used to determine when we should clean up the stream. The RFH being swapped out is tracked in a map. 6. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Get StreamPrivate API to work. This is based off clamy's original patch https://codereview.chromium.org/1485693002/ This patch gets the StreamPrivate API which is exposed for extensions to work with PlzNavigate. This should fix top level PDF file loads and in turn some browser tests like MaterialPDFExtensionTests, PDFExtensionTests, etc. Changes in this patch are as below: 1. Allow stream based requests to be handled in navigation_resource_handler.cc. 2. Fixing the way WebContents is obtained in the other places with the ResourceRequestInfoImpl::GetWebContentsGetterForRequest function. 3. The MimeHandlerStreamManager::AddStream function takes in the WebContents pointer. 4 A getter has been added to the ResourceRequestInfo class to get the frame_tree_node_id field. 5. The EmbedderObserver class now uses the WCO::RenderFrameHostChanged notification to determine if a RFH is swapped with another. This is used to determine when we should clean up the stream. The RFH being swapped out is tracked in a map. 6. We use the WebContentsObserver::DidStartNavigation method in the EmbedderObserver to detect a navigation on the main frame and use that to cleanup the stream. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/4b7467a5a8e36f6d59cbc2df29be42e9c12ec45b Cr-Commit-Position: refs/heads/master@{#420537} ==========
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/4b7467a5a8e36f6d59cbc2df29be42e9c12ec45b Cr-Commit-Position: refs/heads/master@{#420537} |
