|
|
Created:
3 years, 8 months ago by arthursonzogni Modified:
3 years, 8 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, clamy, Lei Zhang, nasko Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: Avoid creating WebUI for subframes.
This CL is a short term solution. Its goal is to make the PlzNavigate
code path to have the same behavior as the non-PlzNavigate one. That is
to say, creating a WebUI for the mainframe and not for the subframes.
The WebUI was previously owned by the RenderFrameHostManager and now by
each RenderFrameHost.
Please see: https://codereview.chromium.org/1352813006
The long term solution would be to make the WebUI more truly per-frame.
There is two WebUI pages that prevent us to apply this solution. Once
these problems are gone, this CL can be reverted:
* chrome://print: It doesn't expect a WebUI to be created for its
subframe.
* chrome://chrome: It handles the creation of the WebUI by itself with
the CreateSubframeWebUI function.
BUG=704327, 713313
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
Review-Url: https://codereview.chromium.org/2810583006
Cr-Commit-Position: refs/heads/master@{#465961}
Committed: https://chromium.googlesource.com/chromium/src/+/a4f64617512a9ec847024e4602ca060e3cac4813
Patch Set 1 : git cl try #Patch Set 2 : Revert 2470823004 #Patch Set 3 : Trying to clean some code... #Patch Set 4 : @dbeam suggestion #
Total comments: 1
Patch Set 5 : Adding comments and modifying the issue. #
Total comments: 4
Patch Set 6 : @creis suggestions (+rebase) #
Messages
Total messages: 65 (46 generated)
Description was changed from ========== git cl try WebUI with PlzNavigate. BUG=None ========== to ========== git cl try WebUI with PlzNavigate. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by arthursonzogni@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 arthursonzogni@chromium.org
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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 arthursonzogni@chromium.org
The CQ bit was checked by arthursonzogni@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.
Patchset #3 (id:60001) has been deleted
Description was changed from ========== git cl try WebUI with PlzNavigate. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Make GetFrameHostForNavigation do not make WebUI for iframe. BUG=704327 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_browser_side_navigation ==========
Description was changed from ========== PlzNavigate: Make GetFrameHostForNavigation do not make WebUI for iframe. BUG=704327 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_browser_side_navigation ========== to ========== PlzNavigate: Make GetFrameHostForNavigation do not use WebUI for subframe. GetFrameHostForNavigation is called for every navigation. The problem is that it tries to call UpdatePendingWebUI for every frame, even subframe. This CL prevents it from doing this. BUG=704327 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_browser_side_navigation ==========
The CQ bit was checked by arthursonzogni@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_browser_side_navigation on master.tryserver.chromium.linux (JOB_FAILED, build hasn't started yet, builder probably lacks capacity)
arthursonzogni@chromium.org changed reviewers: + creis@chromium.org
Hi Charlie, I am trying to solve: https://bugs.chromium.org/p/chromium/issues/detail?id=704327 and maybe also: https://bugs.chromium.org/p/chromium/issues/detail?id=707482 There is still one problem. It happens when a WebUI page contains an iframe (For instance the print dialog) It looks like a WebUI is created for the main frame, but also for the iframe. GetFrameHostForNavigation() is a PlzNavigation-only function that is called for every navigations (browser-initiated/renderer-initiated, main-frame/sub-frame). It will always call UpdatePendingWebUI. I don't know anything about WebUI. It is still very blurry for me. Can you confirm me that UpdatePendingWebUI is not expected to be called for a subframe? Do you think this is the right way to fix the problem? Thanks! Arthur
creis@chromium.org changed reviewers: + carlosk@chromium.org
On 2017/04/11 16:29:21, arthursonzogni wrote: > Hi Charlie, > > I am trying to solve: > https://bugs.chromium.org/p/chromium/issues/detail?id=704327 > and maybe also: > https://bugs.chromium.org/p/chromium/issues/detail?id=707482 > > There is still one problem. It happens when a WebUI page contains an iframe (For > instance the print dialog) > It looks like a WebUI is created for the main frame, but also for the iframe. > > GetFrameHostForNavigation() is a PlzNavigation-only function that is called for > every navigations (browser-initiated/renderer-initiated, main-frame/sub-frame). > It will always call UpdatePendingWebUI. > > I don't know anything about WebUI. It is still very blurry for me. Can you > confirm me that UpdatePendingWebUI is not expected to be called for a subframe? > Do you think this is the right way to fix the problem? > > Thanks! > Arthur Thanks for checking-- this doesn't look right to me. I don't remember the details (other than WebUI objects being pretty confusing), but I think it was intentional that we gave WebUI objects to subframes. That said, I would have expected some tests to fail, which surprises me. (Maybe the linux_browser_side_navigation try job would have failed some if it hadn't gone purple?) carlosk@, do you remember why we needed WebUI objects on subframes (from when you refactored this to be RenderFrame-specific)?
carlosk@chromium.org changed reviewers: + dbeam@chromium.org
On 2017/04/11 21:58:31, Charlie Reis (busy until Fri) wrote: > On 2017/04/11 16:29:21, arthursonzogni wrote: > > Hi Charlie, > > > > I am trying to solve: > > https://bugs.chromium.org/p/chromium/issues/detail?id=704327 > > and maybe also: > > https://bugs.chromium.org/p/chromium/issues/detail?id=707482 > > > > There is still one problem. It happens when a WebUI page contains an iframe > (For > > instance the print dialog) > > It looks like a WebUI is created for the main frame, but also for the iframe. > > > > GetFrameHostForNavigation() is a PlzNavigation-only function that is called > for > > every navigations (browser-initiated/renderer-initiated, > main-frame/sub-frame). > > It will always call UpdatePendingWebUI. > > > > I don't know anything about WebUI. It is still very blurry for me. Can you > > confirm me that UpdatePendingWebUI is not expected to be called for a > subframe? > > Do you think this is the right way to fix the problem? > > > > Thanks! > > Arthur > > Thanks for checking-- this doesn't look right to me. I don't remember the > details (other than WebUI objects being pretty confusing), but I think it was > intentional that we gave WebUI objects to subframes. That said, I would have > expected some tests to fail, which surprises me. (Maybe the > linux_browser_side_navigation try job would have failed some if it hadn't gone > purple?) > > carlosk@, do you remember why we needed WebUI objects on subframes (from when > you refactored this to be RenderFrame-specific)? +dbeam@ who used to be the main reviewer of WebUI changes. I have very vague memories myself but we should have kept the existing WebUI behavior with the PlzNavigate changes. That being said, it seems to make sense that we have specific WebUIs for subframes. For instance, when one opens the desktop Chrome settings page and clicks on the different links on the top left (Extensions, Settings, About) I think the frame to the right switches among different WebUIs. Also, I am not surprised that tests didn't fail: IIRC WebUIs had very low test coverage.
On 2017/04/11 23:32:59, carlosk wrote: > On 2017/04/11 21:58:31, Charlie Reis (busy until Fri) wrote: > > On 2017/04/11 16:29:21, arthursonzogni wrote: > > > Hi Charlie, > > > > > > I am trying to solve: > > > https://bugs.chromium.org/p/chromium/issues/detail?id=704327 > > > and maybe also: > > > https://bugs.chromium.org/p/chromium/issues/detail?id=707482 > > > > > > There is still one problem. It happens when a WebUI page contains an iframe > > (For > > > instance the print dialog) > > > It looks like a WebUI is created for the main frame, but also for the > iframe. > > > > > > GetFrameHostForNavigation() is a PlzNavigation-only function that is called > > for > > > every navigations (browser-initiated/renderer-initiated, > > main-frame/sub-frame). > > > It will always call UpdatePendingWebUI. > > > > > > I don't know anything about WebUI. It is still very blurry for me. Can you > > > confirm me that UpdatePendingWebUI is not expected to be called for a > > subframe? > > > Do you think this is the right way to fix the problem? > > > > > > Thanks! > > > Arthur > > > > Thanks for checking-- this doesn't look right to me. I don't remember the > > details (other than WebUI objects being pretty confusing), but I think it was > > intentional that we gave WebUI objects to subframes. That said, I would have > > expected some tests to fail, which surprises me. (Maybe the > > linux_browser_side_navigation try job would have failed some if it hadn't gone > > purple?) > > > > carlosk@, do you remember why we needed WebUI objects on subframes (from when > > you refactored this to be RenderFrame-specific)? > > +dbeam@ who used to be the main reviewer of WebUI changes. > > I have very vague memories myself but we should have kept the existing WebUI > behavior with the PlzNavigate changes. > > That being said, it seems to make sense that we have specific WebUIs for > subframes. For instance, when one opens the desktop Chrome settings page and > clicks on the different links on the top left (Extensions, Settings, About) I > think the frame to the right switches among different WebUIs. > > Also, I am not surprised that tests didn't fail: IIRC WebUIs had very low test > coverage. actually, this particularly method is tested a bunch: https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/log_web_ui_url_b... https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/log_web_ui_url_u... please add a test case for chrome://settings or some high-level URL (not a *frame* URL) if you'd like to catch regressions in the future.
Description was changed from ========== PlzNavigate: Make GetFrameHostForNavigation do not use WebUI for subframe. GetFrameHostForNavigation is called for every navigation. The problem is that it tries to call UpdatePendingWebUI for every frame, even subframe. This CL prevents it from doing this. BUG=704327 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_browser_side_navigation ========== to ========== PlzNavigate: Make GetFrameHostForNavigation do not use WebUI for subframe. GetFrameHostForNavigation is called for every navigation. The problem is that it tries to call UpdatePendingWebUI for every frame, even subframe. This CL prevents it from doing this. BUG=704327 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ==========
Thanks! So there is WebUI object for subframes too. In chrome://print/, an iframe is created by a javascript function, which open chrome://print/pdf_preview.html?chrome://print/1/0/print.pdf in an iframe. The subframe navigation doesn't take the RenderFrameHostManager::Navigate() path and therefore doesn't call UpdatePendingWebUI. With PlzNavigate, UpdatePendingWebUi is called for every navigations. That is the difference. Maybe the PlzNavigate behavior is the one we want. In this case, I will have to modify the print preview dialog such that having 2 WebUI is not a problem. I relaunched the linux_browser_side_navigation try bot. It is green. It was also green in the two previous patchset.
On 2017/04/13 09:46:56, arthursonzogni wrote: > Thanks! So there is WebUI object for subframes too. > > In chrome://print/, an iframe is created by a javascript function, which open > chrome://print/pdf_preview.html?chrome://print/1/0/print.pdf in an iframe. > The subframe navigation doesn't take the RenderFrameHostManager::Navigate() path > and therefore doesn't call UpdatePendingWebUI. > With PlzNavigate, UpdatePendingWebUi is called for every navigations. That is > the difference. > > Maybe the PlzNavigate behavior is the one we want. In this case, I will have to > modify the print preview dialog such that having 2 WebUI is not a problem. > > I relaunched the linux_browser_side_navigation try bot. It is green. It was also > green in the two previous patchset. can you change this line to a URL other than chrome://history? that page no longer uses subframes, so isn't a good option. https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/log_web_ui_url_b... your best bet would probably be to change the top-level URL to chrome://extensions and check for visits to chrome://chrome and chrome://extensions-frame.
The CQ bit was checked by arthursonzogni@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 2017/04/18 02:33:18, Dan Beam wrote: > can you change this line to a URL other than chrome://history? that page no > longer uses subframes, so isn't a good option. > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/log_web_ui_url_b... > > your best bet would probably be to change the top-level URL to > chrome://extensions and check for visits to chrome://chrome and > chrome://extensions-frame. @dbeam I applied your suggestions in the latest patchset #4. Is it what you had in mind? It passes with/with PlzNavigate and with/without the patchset #1,#2,#3. I will probably have to test more things to make it fail.
On 2017/04/18 12:13:09, arthursonzogni wrote: > On 2017/04/18 02:33:18, Dan Beam wrote: > > can you change this line to a URL other than chrome://history? that page no > > longer uses subframes, so isn't a good option. > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/log_web_ui_url_b... > > > > your best bet would probably be to change the top-level URL to > > chrome://extensions and check for visits to chrome://chrome and > > chrome://extensions-frame. > > @dbeam I applied your suggestions in the latest patchset #4. Is it what you had > in mind? > It passes with/with PlzNavigate and with/without the patchset #1,#2,#3. > I will probably have to test more things to make it fail. I have taken a look at chrome://extensions. With PlzNavigate, two WebUIControllers are created for all the iframes in chrome://extensions. It is bad. According to me, we should not call UpdatePendingWebUI for the subframes in GetRenderFrameHostForNavigation. It looks like, when it is needed, the WebUIController subframes are created by the mainframe WebUIController with the CreateSubFrameWebUI function. @dbeam: WDYT? There is WebUI page that doesn't expect a WebUIController to be created for its iframes (e.g. chrome://print) There is WebUI page that creates the WebUIController objects by themselves. (See CreateSubFrameWebUI)
On 2017/04/18 16:56:01, arthursonzogni wrote: > On 2017/04/18 12:13:09, arthursonzogni wrote: > > On 2017/04/18 02:33:18, Dan Beam wrote: > > > can you change this line to a URL other than chrome://history? that page no > > > longer uses subframes, so isn't a good option. > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/log_web_ui_url_b... > > > > > > your best bet would probably be to change the top-level URL to > > > chrome://extensions and check for visits to chrome://chrome and > > > chrome://extensions-frame. > > > > @dbeam I applied your suggestions in the latest patchset #4. Is it what you > had > > in mind? > > It passes with/with PlzNavigate and with/without the patchset #1,#2,#3. > > I will probably have to test more things to make it fail. > > I have taken a look at chrome://extensions. > With PlzNavigate, two WebUIControllers are created for all the iframes in > chrome://extensions. It is bad. > > According to me, we should not call UpdatePendingWebUI for the subframes in > GetRenderFrameHostForNavigation. > It looks like, when it is needed, the WebUIController subframes are created by > the mainframe WebUIController with the CreateSubFrameWebUI function. > > @dbeam: WDYT? > > There is WebUI page that doesn't expect a WebUIController to be created for its > iframes (e.g. chrome://print) > There is WebUI page that creates the WebUIController objects by themselves. (See > CreateSubFrameWebUI) I think that the "uber page" is going away and you should do fairly minimal work on it. Settings, About, and History are all their own pages, with Extensions coming soon. We'll then retire this page. For what it's worth: UberUI was written in a time where webuis still lived on RenderViews and having sub-frame WebUIs or controllers/handlers was very hard to do, so we had to make our own routing / subframe creation system. It simply has not received many updates lately.
lgtm though I think it's sane to make webui more truly per-frame in the near future (especially after we remove the uber page) https://codereview.chromium.org/2810583006/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/log_web_ui_url_browsertest.cc (right): https://codereview.chromium.org/2810583006/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/log_web_ui_url_browsertest.cc:48: features::kMaterialDesignHistory); i'll have to remove this code soon... I'm deleting the old history code shortly
thestig@chromium.org changed reviewers: + thestig@chromium.org
I can confirm this fixes PlzNavigate to preserve the 1:1 relationship between PrintPreviewUI and WebContents, which is what we had without PlzNavigate and matches my mental picture of how they relate.
Description was changed from ========== PlzNavigate: Make GetFrameHostForNavigation do not use WebUI for subframe. GetFrameHostForNavigation is called for every navigation. The problem is that it tries to call UpdatePendingWebUI for every frame, even subframe. This CL prevents it from doing this. BUG=704327 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: Change GetFrameHostForNavigation to not use WebUI for subframes. GetFrameHostForNavigation is called for every navigation. The problem is that it tries to call UpdatePendingWebUI for every frame, even subframe. This CL prevents it from doing this. BUG=704327 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ==========
Rewrote the CL description slightly.
On 2017/04/18 22:09:08, Lei Zhang wrote: > Rewrote the CL description slightly. Sorry, I'm still finding this very confusing, because there's no clear reason why we would skip WebUIs for subframes when reading the code. It sounds like Dan is saying that the CreateSubFrameWebUI thing is leftover from the RenderView days and we should be moving more towards making it per-frame in general (i.e., the opposite of what this CL is doing?). But it also sounds like that causes problems for chrome://print (which is sticking around) and not UberUI (which is going away). What's the path towards cleaning this code up? Will we be able to do something cleaner when UberUI is gone and chrome://print is the only user of subframe WebUI objects? Or is this CL the desired long term outcome? Either way, I think we need some comments in the code and CL description saying why we're skipping UpdatePendingWebUI for subframes, since it would be very hard to piece together from reading the code. Then I could be ok with this as a short term step if we have a path towards something that makes sense down the line.
Description was changed from ========== PlzNavigate: Change GetFrameHostForNavigation to not use WebUI for subframes. GetFrameHostForNavigation is called for every navigation. The problem is that it tries to call UpdatePendingWebUI for every frame, even subframe. This CL prevents it from doing this. BUG=704327 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: Avoid creating WebUI for subframes. This CL is a short term solution. Its goal is to make the PlzNavigate code path to have the same behavior as the non-PlzNavigate one. That is to say, creating a WebUI for the mainframe and not for the subframes. The WebUI was previously owned by the RenderFrameHostManager and now by each RenderFrameHost. Please see: https://codereview.chromium.org/1352813006 The long term solution would be to make the WebUI more truly per-frame. There is two WebUI pages that prevent us to apply this solution. Once these problems are gone, this CL can be reverted: * chrome://print: It doesn't expect a WebUI to be created for its subframe. * chrome://chrome: It handles the creation of the WebUI by itself with the CreateSubframeWebUI function. BUG=704327 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ==========
The CQ bit was checked by arthursonzogni@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 arthursonzogni@chromium.org to run a CQ dry run
Patchset #5 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I have rewritten the description and the CL to match what the people are thinking here. To summarize, in the near future, a WebUI will be created for each frame, including the subframes. This CL does the opposite and we will be able to revert part of it when PlzNavigate will be shipped (the old code path doesn't support creating WebUI for subframe) and when the chrome://chrome is gone and chrome://print is fixed (maybe something close to https://crrev.com/2817883003/).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks, that helps a lot! I filed https://crbug.com/713313 to track the long term work. LGTM with nits. https://codereview.chromium.org/2810583006/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2810583006/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:763: // support it. See http://crrev.com/2810583006/. Thanks, that helps. Let's change the last line of each of these comments to reference the bug with a TODO instead of the CL. Something like: TODO(crbug.com/713313): Make WebUI objects always be per-frame instead. https://codereview.chromium.org/2810583006/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:866: // support it. See http://crrev.com/2810583006/. nit: Let's move this comment above line 863, since it's unusual to indent it within the middle of the conditional.
LGTM.
The CQ bit was checked by arthursonzogni@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: Avoid creating WebUI for subframes. This CL is a short term solution. Its goal is to make the PlzNavigate code path to have the same behavior as the non-PlzNavigate one. That is to say, creating a WebUI for the mainframe and not for the subframes. The WebUI was previously owned by the RenderFrameHostManager and now by each RenderFrameHost. Please see: https://codereview.chromium.org/1352813006 The long term solution would be to make the WebUI more truly per-frame. There is two WebUI pages that prevent us to apply this solution. Once these problems are gone, this CL can be reverted: * chrome://print: It doesn't expect a WebUI to be created for its subframe. * chrome://chrome: It handles the creation of the WebUI by itself with the CreateSubframeWebUI function. BUG=704327 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: Avoid creating WebUI for subframes. This CL is a short term solution. Its goal is to make the PlzNavigate code path to have the same behavior as the non-PlzNavigate one. That is to say, creating a WebUI for the mainframe and not for the subframes. The WebUI was previously owned by the RenderFrameHostManager and now by each RenderFrameHost. Please see: https://codereview.chromium.org/1352813006 The long term solution would be to make the WebUI more truly per-frame. There is two WebUI pages that prevent us to apply this solution. Once these problems are gone, this CL can be reverted: * chrome://print: It doesn't expect a WebUI to be created for its subframe. * chrome://chrome: It handles the creation of the WebUI by itself with the CreateSubframeWebUI function. BUG=704327,713313 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the review and for the bug! https://codereview.chromium.org/2810583006/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2810583006/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:763: // support it. See http://crrev.com/2810583006/. On 2017/04/19 19:34:04, Charlie Reis wrote: > Thanks, that helps. Let's change the last line of each of these comments to > reference the bug with a TODO instead of the CL. Something like: > > TODO(crbug.com/713313): Make WebUI objects always be per-frame instead. Done. https://codereview.chromium.org/2810583006/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:866: // support it. See http://crrev.com/2810583006/. On 2017/04/19 19:34:04, Charlie Reis wrote: > nit: Let's move this comment above line 863, since it's unusual to indent it > within the middle of the conditional. Done.
The CQ bit was checked by arthursonzogni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, creis@chromium.org, carlosk@chromium.org Link to the patchset: https://codereview.chromium.org/2810583006/#ps160001 (title: "@creis suggestions (+rebase)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 160001, "attempt_start_ts": 1492681619368760, "parent_rev": "74ad4fde9304fc6f71a165462360662c5bfe7924", "commit_rev": "a4f64617512a9ec847024e4602ca060e3cac4813"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Avoid creating WebUI for subframes. This CL is a short term solution. Its goal is to make the PlzNavigate code path to have the same behavior as the non-PlzNavigate one. That is to say, creating a WebUI for the mainframe and not for the subframes. The WebUI was previously owned by the RenderFrameHostManager and now by each RenderFrameHost. Please see: https://codereview.chromium.org/1352813006 The long term solution would be to make the WebUI more truly per-frame. There is two WebUI pages that prevent us to apply this solution. Once these problems are gone, this CL can be reverted: * chrome://print: It doesn't expect a WebUI to be created for its subframe. * chrome://chrome: It handles the creation of the WebUI by itself with the CreateSubframeWebUI function. BUG=704327,713313 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: Avoid creating WebUI for subframes. This CL is a short term solution. Its goal is to make the PlzNavigate code path to have the same behavior as the non-PlzNavigate one. That is to say, creating a WebUI for the mainframe and not for the subframes. The WebUI was previously owned by the RenderFrameHostManager and now by each RenderFrameHost. Please see: https://codereview.chromium.org/1352813006 The long term solution would be to make the WebUI more truly per-frame. There is two WebUI pages that prevent us to apply this solution. Once these problems are gone, this CL can be reverted: * chrome://print: It doesn't expect a WebUI to be created for its subframe. * chrome://chrome: It handles the creation of the WebUI by itself with the CreateSubframeWebUI function. BUG=704327,713313 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel Review-Url: https://codereview.chromium.org/2810583006 Cr-Commit-Position: refs/heads/master@{#465961} Committed: https://chromium.googlesource.com/chromium/src/+/a4f64617512a9ec847024e4602ca... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://chromium.googlesource.com/chromium/src/+/a4f64617512a9ec847024e4602ca... |