Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(98)

Issue 1352813006: Move WebUI ownership from the RenderFrameHostManager to the RenderFrameHost. (Closed)

Created:
5 years, 3 months ago by carlosk
Modified:
5 years, 1 month ago
CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, creis+watch_chromium.org, darin-cc_chromium.org, Evan Stade
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Move WebUI ownership from the RenderFrameHostManager to the RenderFrameHost. This change refactors the current ownership of the WebUI instance, moving it from the RenderFrameHostManager to the RenderFrameHost which is directly associated with the WebUI. The design document for this change is here: https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3JXcWm8_zWBhEjaJtI BUG=508850 Committed: https://crrev.com/e5c678bf5b084988a60a4636342414f2831c1695 Cr-Commit-Position: refs/heads/master@{#358050}

Patch Set 1 #

Total comments: 12

Patch Set 2 : Latest round of fixes. #

Patch Set 3 : More test fixes and improvements but the code is stil a WIP! #

Total comments: 8

Patch Set 4 : Rebase (post Blink repo merge) #

Patch Set 5 : Rebase #

Patch Set 6 : Still in a WIP state but wanted to upload the latest changes. #

Patch Set 7 : Got to an almost working state and addressed reviewer comments. #

Total comments: 27

Patch Set 8 : Removed InitializeWebUI #

Total comments: 53

Patch Set 9 : Latest code review changes. #

Patch Set 10 : Fixed uninitialized variable and missing call to WebUIImpl::RenderViewReused. #

Total comments: 2

Patch Set 11 : Rebase. #

Total comments: 11

Patch Set 12 : Doesn't try to create a WebUI if URL is invalid (fixes unit_tests). #

Total comments: 10

Patch Set 13 : Rebase (that seems to fix last week's renderer crash). #

Patch Set 14 : Reverted to having RFHI::InitializeWebUI and other minor changes. #

Patch Set 15 : Trying to fix the Mac test errors. #

Patch Set 16 : Fixed ChromeOS errors caused by improper timing of RenderView(Created|Reused) calls. #

Total comments: 27

Patch Set 17 : Changes to comments and small code fixes from nasko@'s comments. #

Total comments: 1

Patch Set 18 : Simplified the code mainly by consolidating WebUIImpl::RenderView* calls. #

Total comments: 17

Patch Set 19 : Rebased and fixed conflicts with http://crbug.com/1403343002 #

Patch Set 20 : Minor fixes and changes from CR comments. #

Total comments: 16

Patch Set 21 : One more round of minor fixes and changes from CR comments. #

Patch Set 22 : Rebased and fixed conflicts of the revert of http://crbug.com/1403343002 :/ #

Total comments: 36

Patch Set 23 : Rebased and fixed conflicts. #

Patch Set 24 : Addressing some of the current CR comments (before moving on to eliminate the pending WebUI). #

Total comments: 1

Patch Set 25 : Rebase. #

Patch Set 26 : WIP: removing the pending WebUI (failing tests; no PlzNavigate support). #

Patch Set 27 : WIP: removed the pending WebUI from RFHI (did not yet remove the pending WebUI API from RFHM). #

Patch Set 28 : Rebase to fix patch errors. #

Total comments: 17

Patch Set 29 : Addressing lastest and previously not responded comments. #

Patch Set 30 : Added one more active WebUI transition special case. #

Patch Set 31 : Rename method RFHM::pending_web_ui to GetNavigatingWebUI. #

Patch Set 32 : Rebase to fix patch issues. #

Patch Set 33 : Fixed header comments. #

Total comments: 10

Patch Set 34 : Rebase. #

Patch Set 35 : Minor changes to adress latest CR comments. #

Total comments: 19

Patch Set 36 : Addressed latest comments by nasko@. #

Total comments: 2

Patch Set 37 : Minor comment updates. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+343 lines, -321 lines) Patch
M content/browser/frame_host/frame_tree.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 1 chunk +1 line, -2 lines 0 comments Download
M content/browser/frame_host/render_frame_host_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 2 chunks +5 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +5 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 5 chunks +25 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 7 chunks +72 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 13 chunks +42 lines, -86 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 28 chunks +166 lines, -196 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +5 lines, -3 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 3 chunks +13 lines, -15 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 2 chunks +1 line, -1 line 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 5 chunks +8 lines, -18 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 62 (14 generated)
clamy
Thanks! A few comments on this version. https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/render_frame_host_delegate.h File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/render_frame_host_delegate.h#newcode170 content/browser/frame_host/render_frame_host_delegate.h:170: virtual scoped_ptr<WebUIImpl> ...
5 years, 3 months ago (2015-09-17 17:04:38 UTC) #2
carlosk
Adding interested people for voluntary early feedback. Mind this is still a WIP so there ...
5 years, 3 months ago (2015-09-18 07:50:41 UTC) #4
nasko
Some perfcrastination and few small nits. Most of my comments are in the design doc. ...
5 years, 3 months ago (2015-09-18 18:40:19 UTC) #5
carlosk
Thanks! I think this is looking way better now but there's still failing tests (only ...
5 years, 2 months ago (2015-09-30 19:37:27 UTC) #8
clamy
Thanks! I really like the direction the patch is taking: the logic for WebUIs really ...
5 years, 2 months ago (2015-10-01 13:15:13 UTC) #9
carlosk
Thanks! Still working on the failing tests. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_host/render_frame_host_impl.cc#newcode1979 content/browser/frame_host/render_frame_host_impl.cc:1979: web_ui_->RenderViewCreated(render_view_host_); On ...
5 years, 2 months ago (2015-10-01 16:00:22 UTC) #10
nasko
I really like how RFHM is becoming simpler with this move. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): ...
5 years, 2 months ago (2015-10-01 20:05:16 UTC) #11
clamy
Thanks! A few more comments. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_host/render_frame_host_impl.cc#newcode1979 content/browser/frame_host/render_frame_host_impl.cc:1979: web_ui_->RenderViewCreated(render_view_host_); On 2015/10/01 16:00:22, ...
5 years, 2 months ago (2015-10-02 14:20:02 UTC) #12
carlosk
Thanks! Replied to all comments but there's still tests to fix and other questions I'll ...
5 years, 2 months ago (2015-10-05 16:59:58 UTC) #13
carlosk
I fixed a couple of issues that made browser_tests finally pass. Still unit_tests to go... ...
5 years, 2 months ago (2015-10-06 17:03:21 UTC) #14
Dan Beam
https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_host/render_frame_host_impl.cc#newcode226 content/browser/frame_host/render_frame_host_impl.cc:226: web_ui_->RenderViewCreated(render_view_host_); On 2015/10/01 20:05:16, nasko (slow to review) wrote: ...
5 years, 2 months ago (2015-10-06 17:50:33 UTC) #15
nasko
I'm sorry for the delays, I should be back to more regular reviews at the ...
5 years, 2 months ago (2015-10-07 23:49:33 UTC) #16
Dan Beam
https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_host/render_frame_host_impl.cc#newcode226 content/browser/frame_host/render_frame_host_impl.cc:226: web_ui_->RenderViewCreated(render_view_host_); On 2015/10/07 23:49:32, nasko (slow to review) wrote: ...
5 years, 2 months ago (2015-10-07 23:55:11 UTC) #17
Dan Beam
https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_host/render_frame_host_impl.cc#newcode226 content/browser/frame_host/render_frame_host_impl.cc:226: web_ui_->RenderViewCreated(render_view_host_); On 2015/10/07 23:55:11, Dan Beam wrote: > On ...
5 years, 2 months ago (2015-10-08 00:01:35 UTC) #18
carlosk
Thanks for all comments. dbeam@ and creis@: there's a questions for you in these comments ...
5 years, 2 months ago (2015-10-09 17:29:10 UTC) #19
Dan Beam
whenever I see var_name = ComplexType(); var_name_extra = p_o_d; I think "why doesn't ComplexType::extra_ exist?" ...
5 years, 2 months ago (2015-10-10 00:02:08 UTC) #20
carlosk
Thanks! On 2015/10/10 00:02:08, Dan Beam wrote: > whenever I see > > var_name = ...
5 years, 2 months ago (2015-10-13 14:40:26 UTC) #21
Dan Beam
https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_host/render_frame_host_impl.cc#newcode226 content/browser/frame_host/render_frame_host_impl.cc:226: web_ui_->RenderViewCreated(render_view_host_); On 2015/10/09 17:29:10, carlosk wrote: > On 2015/10/08 ...
5 years, 2 months ago (2015-10-13 16:24:21 UTC) #22
Charlie Reis
https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_host/render_frame_host_manager.cc#newcode1667 content/browser/frame_host/render_frame_host_manager.cc:1667: int bindings, On 2015/10/13 14:40:26, carlosk wrote: > On ...
5 years, 2 months ago (2015-10-13 17:03:27 UTC) #23
carlosk
Thanks. On 2015/10/13 16:24:21, Dan Beam wrote: > https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_host/render_frame_host_impl.cc > File content/browser/frame_host/render_frame_host_impl.cc (right): > > ...
5 years, 2 months ago (2015-10-13 17:35:06 UTC) #24
Dan Beam
On 2015/10/13 17:35:06, carlosk wrote: > Thanks. > > On 2015/10/13 16:24:21, Dan Beam wrote: ...
5 years, 2 months ago (2015-10-13 17:50:16 UTC) #25
nasko
https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_host/render_frame_host_impl.h File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_host/render_frame_host_impl.h#newcode480 content/browser/frame_host/render_frame_host_impl.h:480: // Initializes the first WebUI for a recently created ...
5 years, 2 months ago (2015-10-14 23:54:15 UTC) #26
carlosk
Thanks! Based on nasko@'s latest comments I made some simplifications regarding the moment the WebUI ...
5 years, 2 months ago (2015-10-15 16:34:08 UTC) #27
nasko
https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_host/render_frame_host_impl.h File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_host/render_frame_host_impl.h#newcode492 content/browser/frame_host/render_frame_host_impl.h:492: // pending WebUI instance is set. On 2015/10/15 16:34:07, ...
5 years, 2 months ago (2015-10-15 17:40:10 UTC) #28
carlosk
Thanks. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_host/render_frame_host_manager.cc#newcode1984 content/browser/frame_host/render_frame_host_manager.cc:1984: dest_web_ui->RenderViewCreated(render_view_host); On 2015/10/15 17:40:09, nasko (slow to review) ...
5 years, 2 months ago (2015-10-19 17:21:01 UTC) #29
Dan Beam
i think this CL is becoming a cluster https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_host/render_frame_host_manager.cc#newcode1984 content/browser/frame_host/render_frame_host_manager.cc:1984: dest_web_ui->RenderViewCreated(render_view_host); ...
5 years, 2 months ago (2015-10-19 19:22:10 UTC) #30
nasko
Mostly nits about using DCHECK instead of CHECK. Couple of questions to answer, but I ...
5 years, 2 months ago (2015-10-20 01:13:22 UTC) #31
Dan Beam
On 2015/10/20 01:13:22, nasko (slow to review) wrote: > I'm not sure what dbeam@'s comment ...
5 years, 2 months ago (2015-10-20 02:21:25 UTC) #32
nasko
On 2015/10/20 02:21:25, Dan Beam wrote: > On 2015/10/20 01:13:22, nasko (slow to review) wrote: ...
5 years, 2 months ago (2015-10-20 03:58:46 UTC) #33
carlosk
Thanks! On 2015/10/20 03:58:46, nasko (slow to review) wrote: > On 2015/10/20 02:21:25, Dan Beam ...
5 years, 2 months ago (2015-10-20 12:58:52 UTC) #34
Dan Beam
most of this zooms over my head, but lgtm if it helps! https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_host/render_frame_host_delegate.h File content/browser/frame_host/render_frame_host_delegate.h ...
5 years, 2 months ago (2015-10-22 20:56:32 UTC) #35
Charlie Reis
Thanks-- I'm thrilled to see the WebUI state move out of RFHM! A few questions ...
5 years, 2 months ago (2015-10-23 06:39:25 UTC) #36
carlosk
Thanks for the latest round of comments. Given the change in RFHM that make navigations ...
5 years, 1 month ago (2015-10-27 14:35:45 UTC) #38
nasko
Few comments. https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_host/render_frame_host_impl.cc#newcode1044 content/browser/frame_host/render_frame_host_impl.cc:1044: ResetWebUI(); This should probably be done when ...
5 years, 1 month ago (2015-10-29 22:13:15 UTC) #40
carlosk
Thanks. I just wanted to reply to all open comments before moving on with the ...
5 years, 1 month ago (2015-10-30 10:35:23 UTC) #41
carlosk
In the interest of having a checkpoint on this long lasting refactor work I made ...
5 years, 1 month ago (2015-10-30 15:18:17 UTC) #42
nasko
Few more comments. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_host/render_frame_host_impl.cc#newcode1040 content/browser/frame_host/render_frame_host_impl.cc:1040: web_ui_type_ = WebUI::kNoWebUI; On 2015/10/27 14:35:44, ...
5 years, 1 month ago (2015-11-02 17:00:36 UTC) #43
Fabrice (no longer in Chrome)
5 years, 1 month ago (2015-11-02 17:05:28 UTC) #44
Fabrice (no longer in Chrome)
5 years, 1 month ago (2015-11-02 17:05:57 UTC) #46
carlosk
Thanks! https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_host/render_frame_host_impl.cc#newcode1040 content/browser/frame_host/render_frame_host_impl.cc:1040: web_ui_type_ = WebUI::kNoWebUI; On 2015/11/02 17:00:35, nasko (slow ...
5 years, 1 month ago (2015-11-03 09:50:30 UTC) #47
nasko
I think it is mostly there, just a few minor fixes. https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): ...
5 years, 1 month ago (2015-11-04 17:16:10 UTC) #50
carlosk
Thanks. Good comments that made me identify a few more issues that needed fixing. https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_host/render_frame_host_impl.cc ...
5 years, 1 month ago (2015-11-04 21:43:59 UTC) #54
nasko
LGTM with a couple of nits. https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_host/render_frame_host_manager.cc#newcode1082 content/browser/frame_host/render_frame_host_manager.cc:1082: speculative_render_frame_host_->web_ui()->RenderViewCreated( On 2015/11/04 ...
5 years, 1 month ago (2015-11-05 00:36:38 UTC) #55
carlosk
Thanks. Final changes are in and checking Commit now... https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_host/render_frame_host_manager.cc#newcode2647 content/browser/frame_host/render_frame_host_manager.cc:2647: ...
5 years, 1 month ago (2015-11-05 13:19:06 UTC) #56
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1352813006/900001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1352813006/900001
5 years, 1 month ago (2015-11-05 13:19:45 UTC) #59
commit-bot: I haz the power
Committed patchset #37 (id:900001)
5 years, 1 month ago (2015-11-05 14:18:46 UTC) #60
commit-bot: I haz the power
Patchset 37 (id:??) landed as https://crrev.com/e5c678bf5b084988a60a4636342414f2831c1695 Cr-Commit-Position: refs/heads/master@{#358050}
5 years, 1 month ago (2015-11-05 14:19:31 UTC) #61
carlosk
5 years, 1 month ago (2015-11-06 10:33:07 UTC) #62
Message was sent while issue was closed.
A revert of this CL (patchset #37 id:900001) has been created in
https://codereview.chromium.org/1422043007/ by carlosk@chromium.org.

The reason for reverting is: Due to crashes in
RenderFrameHostManager::CheckWebUITransition..

Powered by Google App Engine
This is Rietveld 408576698