content: NotifyPreferencesChanged does not update oopifs |
||||
Issue descriptionFrom alexmos in https://crbug.com/829688#c6 : "Each FrameTreeNode in the FrameTree (i.e., page) might have a frame living in a different renderer process. Each process needs a RenderViewHost to track page-level state, such as the webkit prefs. This RVH is what you get by referencing ftn->current_frame_host()->GetRenderViewHost(). If there are multiple same-site frames in the same frame tree, they all share the same RenderViewHost, hence the std::set deduplication. So for a page structure like A(B(A)), we would have two RenderViewHosts for A and B, and the top and bottom frame's RVH is the same. For A(B(C),B(A)), we would have three RenderViewHosts. These RVHs are stored in FrameTree::render_view_host_map_." "Each frame also needs a RenderWidgetHost to process input events and rendering for that frame. Top-level frames have RenderWidgetHosts which are also associated with that frame's RenderViewHost, and hence for them RenderViewHost::From(ftn->current_frame_host()->GetRenderWidgetHost()) will match what rfh->current_frame_host()->GetRenderViewHost() returns. However, for subframes that have a parent in a different process, we create a new RenderWidgetHost, which isn't associated with a RenderViewHost. This is why for OOPIFs, RenderViewHost::From(ftn->current_frame_host()->GetRenderWidgetHost()) is null. In a page like A(B,B,B), each subframe will have its own separate RenderWidgetHost, but all subframes will share the same RenderViewHost (for B). The code as it's currently written will only work for top frame's RVH, though I think its intent is pretty clearly to iterate over all RVHs in the frame tree." Splitting from issue 829688 , in case we want to merge back.
,
Apr 30 2018
The content bug is now fixed. We don't need to merge for Desktop PWAs since we had a fallback. CCing afakhry in case a merge is needed for tablet mode.
,
Apr 30 2018
If the fix landed in M-68 then that should be enough for tablet mode.
,
Apr 30 2018
Thanks ortuno@! afakhry@, is tablet mode not available in M67? We're aiming to turn on Site Isolation in M67, so there will be a lot more OOPIFs there. alexmos@: Do we know of any other features that might depend on these prefs in OOPIFs, or is it safe to wait for M68?
,
Apr 30 2018
Tablet mode is available on M-67, but the page mobile-like behavior in tablet mode will be fully implemented in M-68. Hmmm, on M-67 we only have the shrink content to fit window width. I'm not sure what would happen if OOPIFs are not notified with preferences changes. In this case, a merge to M-67 seems to be needed.
,
Apr 30 2018
Just inspecting uses of NotifyPreferencesChanged(), it might also affect UpdateOverridingUserAgent() and SetHasPersistentVideo(). The latter seems to only be used on Android, so not a priority. I'm not sure what UpdateOverridingUserAgent() is used for; I see some uses in prerender and Android webview. FWIW, the DevTools UA override appears to work just fine with OOPIFs, so that apparently isn't related.
,
Apr 30 2018
alexmos@: Thanks. I think UpdateOverridingUserAgent is mainly used for Show Desktop Site on Android, so that may not be a concern. afakhry@: Sounds like we should understand more what the impact is for tablet mode. I'm not familiar with it-- is this when a ChromeOS device starts acting like a tablet (e.g., Caroline), or is it a DevTools mobile emulation mode? I'm assuming it's the former. I have a Caroline device to test on if you have repro steps to suggest. (My http://csreis.github.io/tests/cross-site-iframe-simple.html page renders correctly, but I don't think it's wide enough to trigger the behavior you describe.)
,
Apr 30 2018
creis@ It's the former. Please see the attached video. By going to google.com and searching for "testing", in non-tablet mode, see how resizing the window doesn't resize the page contents to fit the width? By switching to tablet mode, and using the same page in a narrower split screen window, see now how the contents shrinks to fit? That's the behavior we now have on M-67.
,
Apr 30 2018
Thanks, that helps! I tried injecting an iframe to https://build.chromium.org into that Google search result page (which creates an OOPIF), and it seems to be behaving correctly. In split screen tablet mode, it expands and shrinks with the page, and in normal mode it has the same 'reveal" behavior that doesn't resize anything. Not sure why it works, but I don't observe anything wrong. I'm inclined to limit merges to the cases that we know we have regressions. If you can think of a case that might not work correctly in M67, then we can get it merged today before 4 pm Pacific to let it bake in this week's beta. Otherwise, I'm inclined to wait for M68 if we don't know any concrete cases that are affected.
,
Apr 30 2018
I'm not aware of any cases, but just to make sure, when you tested, you tested on M-67, correct?
,
Apr 30 2018
Correct-- 67.0.3396.19.
,
Apr 30 2018
And just to confirm, I think we'll skip merging it to M67 since we can't find a case where it matters. There's a small chance we'll find a bug later and wish we'd merged it, but I don't think it makes sense to merge it purely out of caution. If anyone does spot a regression due to this, please mention and we can re-evaluate the merge plan. Thanks! |
||||
►
Sign in to add a comment |
||||
Comment 1 by bugdroid1@chromium.org
, Apr 30 2018