Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2810583006: PlzNavigate: Avoid creating WebUI for subframes. (Closed)

Created:
7 months, 1 week ago by arthursonzogni
Modified:
7 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.

Description

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/+/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)
arthursonzogni
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 ...
7 months, 1 week ago (2017-04-11 16:29:21 UTC) #21
Charlie Reis
On 2017/04/11 16:29:21, arthursonzogni wrote: > Hi Charlie, > > I am trying to solve: ...
7 months, 1 week ago (2017-04-11 21:58:31 UTC) #23
carlosk
On 2017/04/11 21:58:31, Charlie Reis (busy until Fri) wrote: > On 2017/04/11 16:29:21, arthursonzogni wrote: ...
7 months, 1 week ago (2017-04-11 23:32:59 UTC) #25
Dan Beam (no longer on Chrome)
On 2017/04/11 23:32:59, carlosk wrote: > On 2017/04/11 21:58:31, Charlie Reis (busy until Fri) wrote: ...
7 months, 1 week ago (2017-04-11 23:57:33 UTC) #26
arthursonzogni
Thanks! So there is WebUI object for subframes too. In chrome://print/, an iframe is created ...
7 months, 1 week ago (2017-04-13 09:46:56 UTC) #28
Dan Beam (no longer on Chrome)
On 2017/04/13 09:46:56, arthursonzogni wrote: > Thanks! So there is WebUI object for subframes too. ...
7 months ago (2017-04-18 02:33:18 UTC) #29
arthursonzogni
On 2017/04/18 02:33:18, Dan Beam wrote: > can you change this line to a URL ...
7 months ago (2017-04-18 12:13:09 UTC) #34
arthursonzogni
On 2017/04/18 12:13:09, arthursonzogni wrote: > On 2017/04/18 02:33:18, Dan Beam wrote: > > can ...
7 months ago (2017-04-18 16:56:01 UTC) #35
Dan Beam (no longer on Chrome)
On 2017/04/18 16:56:01, arthursonzogni wrote: > On 2017/04/18 12:13:09, arthursonzogni wrote: > > On 2017/04/18 ...
7 months ago (2017-04-18 18:25:24 UTC) #36
Dan Beam (no longer on Chrome)
lgtm though I think it's sane to make webui more truly per-frame in the near ...
7 months ago (2017-04-18 18:28:42 UTC) #37
Lei Zhang
I can confirm this fixes PlzNavigate to preserve the 1:1 relationship between PrintPreviewUI and WebContents, ...
7 months ago (2017-04-18 22:07:56 UTC) #39
Lei Zhang
Rewrote the CL description slightly.
7 months ago (2017-04-18 22:09:08 UTC) #41
Charlie Reis
On 2017/04/18 22:09:08, Lei Zhang wrote: > Rewrote the CL description slightly. Sorry, I'm still ...
7 months ago (2017-04-18 23:37:39 UTC) #42
arthursonzogni
I have rewritten the description and the CL to match what the people are thinking ...
7 months ago (2017-04-19 09:23:53 UTC) #49
Charlie Reis
Thanks, that helps a lot! I filed https://crbug.com/713313 to track the long term work. LGTM ...
7 months ago (2017-04-19 19:34:04 UTC) #52
carlosk
LGTM.
7 months ago (2017-04-19 19:46:15 UTC) #53
arthursonzogni
Thanks for the review and for the bug! https://codereview.chromium.org/2810583006/diff/140001/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2810583006/diff/140001/content/browser/frame_host/render_frame_host_manager.cc#newcode763 content/browser/frame_host/render_frame_host_manager.cc:763: // ...
7 months ago (2017-04-20 09:46:22 UTC) #59
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2810583006/160001
7 months ago (2017-04-20 09:47:17 UTC) #62
commit-bot: I haz the power
7 months ago (2017-04-20 09:51:47 UTC) #65
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/a4f64617512a9ec847024e4602ca...

Powered by Google App Engine
This is Rietveld efc10ee0f