Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
669703003:
Plugin Power Saver: Restrict Power Saver to cross-origin. (Closed)
|
Created:
6 years, 2 months ago by tommycli Modified:
6 years, 2 months ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, Lei Zhang, groby-ooo-7-16, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionPlugin Power Saver: Restrict Power Saver to cross-origin.
Design doc: http://goo.gl/iDix3p
BUG=403800
Committed: https://crrev.com/55196a2b4a303ddbc46f2b63b58c725f80710c9c
Cr-Commit-Position: refs/heads/master@{#300998}
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : much simplify #Patch Set 4 : #
Total comments: 13
Patch Set 5 : add comments #
Total comments: 5
Patch Set 6 : Add path for remote top level frames. #
Total comments: 9
Patch Set 7 : #Patch Set 8 : #
Total comments: 2
Patch Set 9 : #
Messages
Total messages: 45 (9 generated)
tommycli@chromium.org changed reviewers: + jamesr@chromium.org, raymes@chromium.org
raymes, jamesr: PTAL. This limits the experimental Power Saver to operate on cross-site plugins only.
+cc thestig, groby
jamesr@chromium.org changed reviewers: - jamesr@chromium.org
I don't know what this is, removing myself from reviewer list.
tommycli@chromium.org changed reviewers: + jamesr@chromium.org
jamesr: Thanks for the quick response. This feature is meant to reduce peripheral plugin power consumption on laptops/Chromebooks. I put you as a reviewer, as you're the only owner under content/renderer. Is there someone else I should use as a reviewer?
jamesr@chromium.org changed reviewers: - jamesr@chromium.org
Anybody who's familiar with this area and is a content/ owner
tommycli@chromium.org changed reviewers: + piman@chromium.org
piman: Do you mind taking a look at content/renderer/render_frame_impl.cc ? Thanks!
groby@chromium.org changed reviewers: + groby@chromium.org
https://codereview.chromium.org/669703003/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:590: // is peripheral here. I think now would be a good time to add a heuristic function to set is_peripheral_content :) We'll add more heuristics over time, and I'd rather have this factored out. https://codereview.chromium.org/669703003/diff/20001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/669703003/diff/20001/content/renderer/render_... content/renderer/render_frame_impl.cc:1539: frame->top()->document().url()); Question: We already pass in the RenderFrameImpl (3rd param), which should give us access to frame via GetWebFrame(). Do we really need to loop this extra parameter all the way through?
I don't think you need to do as much plumbing for this. container()->element().document() should be the document which the plugin is embedded in and it should be accessible from within PepperPluginInstanceImpl. You should be able to grab the main frame from there if you really want to. But my guess is that you actually just want to get the document the plugin is embedded in.
https://codereview.chromium.org/669703003/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:590: // is peripheral here. Yes, please pull all this out into a function :)
tommycli@chromium.org changed reviewers: - piman@chromium.org
groby/raymes: thanks guys, much simplified now https://codereview.chromium.org/669703003/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:590: // is peripheral here. On 2014/10/23 00:17:21, groby wrote: > I think now would be a good time to add a heuristic function to set > is_peripheral_content :) > > We'll add more heuristics over time, and I'd rather have this factored out. Done. https://codereview.chromium.org/669703003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:590: // is peripheral here. On 2014/10/23 00:21:28, raymes wrote: > Yes, please pull all this out into a function :) Done. https://codereview.chromium.org/669703003/diff/20001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/669703003/diff/20001/content/renderer/render_... content/renderer/render_frame_impl.cc:1539: frame->top()->document().url()); On 2014/10/23 00:17:22, groby wrote: > Question: We already pass in the RenderFrameImpl (3rd param), which should give > us access to frame via GetWebFrame(). Do we really need to loop this extra > parameter all the way through? You are totally right. I totally missed the existence of that method.
Awesome - much better, so simplify! - http://memegenerator.net/instance/45468865 https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3302: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); Using GetContainer()->element().document().url() would simplify even more :) (Sorry, I didn't know _that_ path was available either, but see raymes comment above)
https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3302: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); Are you sure you want the main frame? Consider the following case: I have a site that contains an iframe which contains a plugin. Which URL do you want to compare the plugin URL to? I think it makes more sense to compare to the URL which the plugin is actually embedded in.
https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3302: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); On 2014/10/23 01:34:07, raymes wrote: > Are you sure you want the main frame? > > Consider the following case: > I have a site that contains an iframe which contains a plugin. Which URL do you > want to compare the plugin URL to? I think it makes more sense to compare to the > URL which the plugin is actually embedded in. That'd be the iframe's URL, right? We definitely don't want that. I.e if mydomain.com includes flash _hosted_ on mydomain.com, we consider that not peripheral, no matter if iframed or not.
https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3302: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); I was thinking about a situation where origin X has an iframe at origin Y which embeds a plugin at origin Y. In that case, the origin of the plugin (Y) would not match the top level frame (X) so it would be considered peripheral, even though it may be the main focus of the website. Imagine a site which contains an iframe which is navigated to youtube. Is this the expected behavior? I may be completely missing something so sorry if this sounds wrong!
piman@chromium.org changed reviewers: + creis@chromium.org, piman@chromium.org
https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3302: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); If we're in an out-of-process iframe, I think this will crash (WebRemoteFrameImpl::document() will return an empty WebDocument with a NULL wrapped Document, and calling url() on it will crash). +creis I'm not sure if this is supposed to eventually work or not (I'm assuming a frame could know the URL of its parent since a navigation in the parent would remove the child iframe, but I may be missing edge cases).
https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3302: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); On 2014/10/23 03:53:38, raymes wrote: > I was thinking about a situation where origin X has an iframe at origin Y which > embeds a plugin at origin Y. In that case, the origin of the plugin (Y) would > not match the top level frame (X) so it would be considered peripheral, even > though it may be the main focus of the website. Imagine a site which contains an > iframe which is navigated to youtube. Is this the expected behavior? > > I may be completely missing something so sorry if this sounds wrong! This is indeed expected behavior - this CL is not the complete behavior. As long as the plugin at origin Y exceeds certain size metrics, it will be still relevant. The current CL just catches the case of small helper-plugins from the same domain. https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3302: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); On 2014/10/23 03:56:32, piman (Very slow to review) wrote: > If we're in an out-of-process iframe, I think this will crash > (WebRemoteFrameImpl::document() will return an empty WebDocument with a NULL > wrapped Document, and calling url() on it will crash). > > +creis I'm not sure if this is supposed to eventually work or not (I'm assuming > a frame could know the URL of its parent since a navigation in the parent would > remove the child iframe, but I may be missing edge cases). Will GetContainer()->element().document().url( still work in the OOPIF case?
On Wed, Oct 22, 2014 at 10:51 PM, <groby@chromium.org> wrote: > > https://codereview.chromium.org/669703003/diff/60001/ > content/renderer/pepper/pepper_plugin_instance_impl.cc > File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): > > https://codereview.chromium.org/669703003/diff/60001/ > content/renderer/pepper/pepper_plugin_instance_impl.cc#newcode3302 > content/renderer/pepper/pepper_plugin_instance_impl.cc:3302: GURL > top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); > On 2014/10/23 03:53:38, raymes wrote: > >> I was thinking about a situation where origin X has an iframe at >> > origin Y which > >> embeds a plugin at origin Y. In that case, the origin of the plugin >> > (Y) would > >> not match the top level frame (X) so it would be considered >> > peripheral, even > >> though it may be the main focus of the website. Imagine a site which >> > contains an > >> iframe which is navigated to youtube. Is this the expected behavior? >> > > I may be completely missing something so sorry if this sounds wrong! >> > > This is indeed expected behavior - this CL is not the complete behavior. > As long as the plugin at origin Y exceeds certain size metrics, it will > be still relevant. The current CL just catches the case of small > helper-plugins from the same domain. > > https://codereview.chromium.org/669703003/diff/60001/ > content/renderer/pepper/pepper_plugin_instance_impl.cc#newcode3302 > content/renderer/pepper/pepper_plugin_instance_impl.cc:3302: GURL > top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); > On 2014/10/23 03:56:32, piman (Very slow to review) wrote: > >> If we're in an out-of-process iframe, I think this will crash >> (WebRemoteFrameImpl::document() will return an empty WebDocument with >> > a NULL > >> wrapped Document, and calling url() on it will crash). >> > > +creis I'm not sure if this is supposed to eventually work or not (I'm >> > assuming > >> a frame could know the URL of its parent since a navigation in the >> > parent would > >> remove the child iframe, but I may be missing edge cases). >> > > Will GetContainer()->element().document().url( still work in the OOPIF > case? > That should, since we're talking about the local DOM (PepperPluginInstanceImpl::GetDocumentURL does exactly this anyway, and we expose that API to plugins, so we would need a solution no matter what if it was in question). > > https://codereview.chromium.org/669703003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3302: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); On 2014/10/23 05:51:19, groby wrote: > On 2014/10/23 03:56:32, piman (Very slow to review) wrote: > > If we're in an out-of-process iframe, I think this will crash > > (WebRemoteFrameImpl::document() will return an empty WebDocument with a NULL > > wrapped Document, and calling url() on it will crash). > > > > +creis I'm not sure if this is supposed to eventually work or not (I'm > assuming > > a frame could know the URL of its parent since a navigation in the parent > would > > remove the child iframe, but I may be missing edge cases). > > Will GetContainer()->element().document().url( still work in the OOPIF case? We need render_frame_->GetWebFrame()->top()->document().url(), as GetContainer()->element().document().url() gets the URL of the IFRAME's source, not the top level document. And I can confirm that render_frame_->GetWebFrame()->top()->document().url() does crash for cross-site iframes...
[+site-isolation-reviews to CC] Thanks for noticing that, piman@! A few thoughts on the OOPIF case below. https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3302: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); On 2014/10/23 17:23:27, tommycli wrote: > On 2014/10/23 05:51:19, groby wrote: > > On 2014/10/23 03:56:32, piman (Very slow to review) wrote: > > > If we're in an out-of-process iframe, I think this will crash > > > (WebRemoteFrameImpl::document() will return an empty WebDocument with a NULL > > > wrapped Document, and calling url() on it will crash). > > > > > > +creis I'm not sure if this is supposed to eventually work or not (I'm > > assuming > > > a frame could know the URL of its parent since a navigation in the parent > > would > > > remove the child iframe, but I may be missing edge cases). > > > > Will GetContainer()->element().document().url( still work in the OOPIF case? > > We need render_frame_->GetWebFrame()->top()->document().url(), as > GetContainer()->element().document().url() gets the URL of the IFRAME's source, > not the top level document. > > And I can confirm that render_frame_->GetWebFrame()->top()->document().url() > does crash for cross-site iframes... It seems like it's unclear to multiple reviewers what policy you're trying enforce. I'd recommend documenting the intent in a comment, even if you plan to evolve it later. That way we can check whether the code matches the intent. As for OOPIF, it's guaranteed that a RemoteFrame will always be cross-origin from a LocalFrame. Thus, if the main frame is a RemoteFrame (i.e., render_frame_->GetWebFrame()->top()->isWebRemoteFrame()), we know it has a different origin than render_frame_ and don't need to check. Of course, that might not match your intent if you have a main frame on A, with an iframe on B, hosting a plugin from A. We wouldn't notice that the plugin and the main frame come from the same origin. If you need that, you might be able to use Alex's upcoming origin replication for RemoteFrames, or you could do this check in the browser process and just let the renderer know. https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.h:690: bool IsPeripheralContent() const; I haven't heard the term peripheral content before, so maybe we can clarify what it means in a comment?
https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3302: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); On 2014/10/23 18:23:27, Charlie Reis wrote: > On 2014/10/23 17:23:27, tommycli wrote: > > On 2014/10/23 05:51:19, groby wrote: > > > On 2014/10/23 03:56:32, piman (Very slow to review) wrote: > > > > If we're in an out-of-process iframe, I think this will crash > > > > (WebRemoteFrameImpl::document() will return an empty WebDocument with a > NULL > > > > wrapped Document, and calling url() on it will crash). > > > > > > > > +creis I'm not sure if this is supposed to eventually work or not (I'm > > > assuming > > > > a frame could know the URL of its parent since a navigation in the parent > > > would > > > > remove the child iframe, but I may be missing edge cases). > > > > > > Will GetContainer()->element().document().url( still work in the OOPIF case? > > > > We need render_frame_->GetWebFrame()->top()->document().url(), as > > GetContainer()->element().document().url() gets the URL of the IFRAME's > source, > > not the top level document. > > > > And I can confirm that render_frame_->GetWebFrame()->top()->document().url() > > does crash for cross-site iframes... > > It seems like it's unclear to multiple reviewers what policy you're trying > enforce. I'd recommend documenting the intent in a comment, even if you plan to > evolve it later. That way we can check whether the code matches the intent. > > As for OOPIF, it's guaranteed that a RemoteFrame will always be cross-origin > from a LocalFrame. Thus, if the main frame is a RemoteFrame (i.e., > render_frame_->GetWebFrame()->top()->isWebRemoteFrame()), we know it has a > different origin than render_frame_ and don't need to check. > > Of course, that might not match your intent if you have a main frame on A, with > an iframe on B, hosting a plugin from A. We wouldn't notice that the plugin and > the main frame come from the same origin. If you need that, you might be able > to use Alex's upcoming origin replication for RemoteFrames, or you could do this > check in the browser process and just let the renderer know. I added some comments. It is indeed a confusing intent. Any time the plugin and the top level frame have the same origin, I do need to know, so it can be marked as essential content. SOME cross-origin plugin content is peripheral, but ALL same-origin plugin content is essential. I'll almost certainly have to bounce to the browser process to handle the cross-origin case, but I'd like to avoid IPC for plugins from the some origin as the top level frame, as those are essential and should be played without delay. Do you have any more info on Alex's origin replication for RemoteFrames? https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.h:690: bool IsPeripheralContent() const; On 2014/10/23 18:23:27, Charlie Reis wrote: > I haven't heard the term peripheral content before, so maybe we can clarify what > it means in a comment? Added comments.
Thanks! That's much clearer. We can probably use Alex's WebFrame::securityContext to fix this for RemoteFrames. In the meantime, it might be better to avoid the crash in --site-per-process mode by returning early. https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3302: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); On 2014/10/23 19:32:23, tommycli wrote: > Do you have any more info on Alex's origin replication for RemoteFrames? Yes, see http://crbug.com/426512. It's a fairly big change that he's working on. https://codereview.chromium.org/669703003/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3307: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); Maybe we can throw a temporary check in until Alex fixes 426512, which returns early if the main frame is remote. (This will only affect --site-per-process mode, breaking case 3 but avoiding a crash.) Something like: // TODO(alexmos): Update this to use the origin of the RemoteFrame when 426512 is fixed. // For now, case 3 in the comment above doesn't work in --site-per-process mode. if (render_frame_->GetWebFrame()->top()->isWebRemoteFrame()) return true;
https://codereview.chromium.org/669703003/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3303: // content's origin differs from the top level frame's origin. For example: Technically, there are more reasons to consider content peripheral. Would you mind changing the comment to "Content is considered to be cross-origin...."? Followed by "currently, all cross-origin content is considered peripheral" or something? https://codereview.chromium.org/669703003/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3307: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); On 2014/10/23 19:51:08, Charlie Reis wrote: > Maybe we can throw a temporary check in until Alex fixes 426512, which returns > early if the main frame is remote. (This will only affect --site-per-process > mode, breaking case 3 but avoiding a crash.) Something like: > > // TODO(alexmos): Update this to use the origin of the RemoteFrame when 426512 > is fixed. > // For now, case 3 in the comment above doesn't work in --site-per-process mode. > if (render_frame_->GetWebFrame()->top()->isWebRemoteFrame()) > return true; > +1 on this. I'd be surprised if there's a large number of sites that hosts the content on the same domain but iframes through a different one, anyways.
https://codereview.chromium.org/669703003/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3307: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); On 2014/10/23 19:51:08, Charlie Reis wrote: > Maybe we can throw a temporary check in until Alex fixes 426512, which returns > early if the main frame is remote. (This will only affect --site-per-process > mode, breaking case 3 but avoiding a crash.) Something like: > > // TODO(alexmos): Update this to use the origin of the RemoteFrame when 426512 > is fixed. > // For now, case 3 in the comment above doesn't work in --site-per-process mode. > if (render_frame_->GetWebFrame()->top()->isWebRemoteFrame()) > return true; > Done. https://codereview.chromium.org/669703003/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:3307: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); On 2014/10/23 20:00:45, groby wrote: > On 2014/10/23 19:51:08, Charlie Reis wrote: > > Maybe we can throw a temporary check in until Alex fixes 426512, which returns > > early if the main frame is remote. (This will only affect --site-per-process > > mode, breaking case 3 but avoiding a crash.) Something like: > > > > // TODO(alexmos): Update this to use the origin of the RemoteFrame when 426512 > > is fixed. > > // For now, case 3 in the comment above doesn't work in --site-per-process > mode. > > if (render_frame_->GetWebFrame()->top()->isWebRemoteFrame()) > > return true; > > > > +1 on this. I'd be surprised if there's a large number of sites that hosts the > content on the same domain but iframes through a different one, anyways. Done.
https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:3314: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); Can you save the result of render_frame_->GetWebFrame()->top()? It does a tree walk, it'd be nice not to do twice.
https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:3311: if (render_frame_->GetWebFrame()->top()->isWebRemoteFrame()) creis@: As an aside, we have lots of places in this file that access the main frame through render_frame_->GetWebFrame()->view()->mainFrame(). Since all of those are presumably going to be problematic as well, is it better just to omit this check for now and fix them all later in one fell swoop? https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:3314: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); nit: In the rest of this file we use render_frame_->GetWebFrame()->view()->mainFrame() to get to the main frame. I assume this is the same as the "top()" frame. Could we use it here for consistency too? That will make it easier for someone who comes and wants to refactor this file for site isolation (for example).
https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:3311: if (render_frame_->GetWebFrame()->top()->isWebRemoteFrame()) On 2014/10/23 21:09:03, raymes wrote: > creis@: As an aside, we have lots of places in this file that access the main > frame through render_frame_->GetWebFrame()->view()->mainFrame(). Since all of > those are presumably going to be problematic as well, is it better just to omit > this check for now and fix them all later in one fell swoop? Per my announcement to chromium-dev on Tuesday, I'd like to get new code to start working with OOPIF. This one can be an example for updating the rest of this file in a future CL. https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:3314: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); On 2014/10/23 21:09:03, raymes wrote: > nit: In the rest of this file we use > render_frame_->GetWebFrame()->view()->mainFrame() to get to the main frame. I > assume this is the same as the "top()" frame. Could we use it here for > consistency too? That will make it easier for someone who comes and wants to > refactor this file for site isolation (for example). > Assuming render_frame_->GetWebFrame() is a LocalFrame, that should work fine. (view() is null on a RemoteFrame for the time being.)
raymes/piman/creis: Thanks for the review. Comments below. https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:3311: if (render_frame_->GetWebFrame()->top()->isWebRemoteFrame()) On 2014/10/23 21:51:33, Charlie Reis wrote: > On 2014/10/23 21:09:03, raymes wrote: > > creis@: As an aside, we have lots of places in this file that access the main > > frame through render_frame_->GetWebFrame()->view()->mainFrame(). Since all of > > those are presumably going to be problematic as well, is it better just to > omit > > this check for now and fix them all later in one fell swoop? > > Per my announcement to chromium-dev on Tuesday, I'd like to get new code to > start working with OOPIF. This one can be an example for updating the rest of > this file in a future CL. Testing this, I've discovered that cross-origin plugins don't work with OOPIF yet. Before it can reach this point, it crashes in https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/renderer/c... https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:3314: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); On 2014/10/23 21:07:34, piman (Very slow to review) wrote: > Can you save the result of render_frame_->GetWebFrame()->top()? It does a tree > walk, it'd be nice not to do twice. Done. https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:3314: GURL top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); On 2014/10/23 21:09:03, raymes wrote: > nit: In the rest of this file we use > render_frame_->GetWebFrame()->view()->mainFrame() to get to the main frame. I > assume this is the same as the "top()" frame. Could we use it here for > consistency too? That will make it easier for someone who comes and wants to > refactor this file for site isolation (for example). > Done. Looks like the other places of this file use this technique: container()->element().document().frame()->view()->mainFrame() If this is the same as the top() frame, that's fine also. Do you know why you guys use the frame derived from container() instead of render_frame_->GetWebFrame()?
The concept of a render frame never used to exist so that would be my guess why (we used to get the render view). I think it's ok to use render_frame_->GetWebFrame() to get to the frame instead of going through the element. I don't think they could ever be different. On Fri Oct 24 2014 at 9:21:10 AM <tommycli@chromium.org> wrote: > raymes/piman/creis: Thanks for the review. Comments below. > > > https://codereview.chromium.org/669703003/diff/100001/ > content/renderer/pepper/pepper_plugin_instance_impl.cc > File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): > > https://codereview.chromium.org/669703003/diff/100001/ > content/renderer/pepper/pepper_plugin_instance_impl.cc#newcode3311 > content/renderer/pepper/pepper_plugin_instance_impl.cc:3311 > <https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper...>: > if > (render_frame_->GetWebFrame()->top()->isWebRemoteFrame()) > On 2014/10/23 21:51:33, Charlie Reis wrote: > > On 2014/10/23 21:09:03, raymes wrote: > > > creis@: As an aside, we have lots of places in this file that access > the main > > > frame through render_frame_->GetWebFrame()->view()->mainFrame(). > Since all of > > > those are presumably going to be problematic as well, is it better > just to > > omit > > > this check for now and fix them all later in one fell swoop? > > > Per my announcement to chromium-dev on Tuesday, I'd like to get new > code to > > start working with OOPIF. This one can be an example for updating the > rest of > > this file in a future CL. > > Testing this, I've discovered that cross-origin plugins don't work with > OOPIF yet. Before it can reach this point, it crashes in > https://code.google.com/p/chromium/codesearch/#chromium/ > src/chrome/renderer/chrome_content_renderer_client.cc&q= > OverrideCreatePlugin&sq=package:chromium&type=cs&l=572 > > https://codereview.chromium.org/669703003/diff/100001/ > content/renderer/pepper/pepper_plugin_instance_impl.cc#newcode3314 > content/renderer/pepper/pepper_plugin_instance_impl.cc:3314 > <https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper...>: > GURL > top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); > On 2014/10/23 21:07:34, piman (Very slow to review) wrote: > > Can you save the result of render_frame_->GetWebFrame()->top()? It > does a tree > > walk, it'd be nice not to do twice. > > Done. > > https://codereview.chromium.org/669703003/diff/100001/ > content/renderer/pepper/pepper_plugin_instance_impl.cc#newcode3314 > content/renderer/pepper/pepper_plugin_instance_impl.cc:3314 > <https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper...>: > GURL > top_frame_url = render_frame_->GetWebFrame()->top()->document().url(); > On 2014/10/23 21:09:03, raymes wrote: > > nit: In the rest of this file we use > > render_frame_->GetWebFrame()->view()->mainFrame() to get to the main > frame. I > > assume this is the same as the "top()" frame. Could we use it here for > > consistency too? That will make it easier for someone who comes and > wants to > > refactor this file for site isolation (for example). > > > Done. > > Looks like the other places of this file use this technique: > > container()->element().document().frame()->view()->mainFrame() > > If this is the same as the top() frame, that's fine also. > > Do you know why you guys use the frame derived from container() instead > of render_frame_->GetWebFrame()? > > https://codereview.chromium.org/669703003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm https://codereview.chromium.org/669703003/diff/140001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:3311: WebFrame* top_frame = render_frame_->GetWebFrame()->view()->mainFrame(); nit: might as well call this "main_frame" and "main_frame_url" to match the language used elsewhere
lgtm
thanks guys! https://codereview.chromium.org/669703003/diff/140001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:3311: WebFrame* top_frame = render_frame_->GetWebFrame()->view()->mainFrame(); On 2014/10/23 22:33:19, raymes wrote: > nit: might as well call this "main_frame" and "main_frame_url" to match the > language used elsewhere Done.
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/669703003/160001
LGTM as well https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/669703003/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:3311: if (render_frame_->GetWebFrame()->top()->isWebRemoteFrame()) On 2014/10/23 22:21:09, tommycli wrote: > On 2014/10/23 21:51:33, Charlie Reis wrote: > > On 2014/10/23 21:09:03, raymes wrote: > > > creis@: As an aside, we have lots of places in this file that access the > main > > > frame through render_frame_->GetWebFrame()->view()->mainFrame(). Since all > of > > > those are presumably going to be problematic as well, is it better just to > > omit > > > this check for now and fix them all later in one fell swoop? > > > > Per my announcement to chromium-dev on Tuesday, I'd like to get new code to > > start working with OOPIF. This one can be an example for updating the rest of > > this file in a future CL. > > Testing this, I've discovered that cross-origin plugins don't work with OOPIF > yet. Before it can reach this point, it crashes in > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/renderer/c... Nice find. Can you file a bug with the Cr-Internals-Sandbox-SiteIsolation label for that? We'll get it triaged.
I suppose I should technically say LGTM as well :)
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/55196a2b4a303ddbc46f2b63b58c725f80710c9c Cr-Commit-Position: refs/heads/master@{#300998} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
