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

Issue 1125243002: Add a member boolean to indicate whether RenderFrameImpl is subframe. (Closed)

Created:
5 years, 7 months ago by nasko
Modified:
5 years, 7 months ago
Reviewers:
ncarter (slow), dcheng
CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, site-isolation-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add a member boolean to indicate whether RenderFrameImpl is subframe. Whether a RenderFrameImpl is a subframe or the main frame is known at initialization time and doesn't change for the lifetime of the object. This CL adds a boolean that keeps that state and removes the few local variables used for checking whether it is a subframe. The reason is that as we invert the ownership of RenderViewImpl and RenderFrameImpl, the main frame will have to pass ownership of the RenderViewImpl to the RenderFrameProxy during swapping local to remote. Since the frame will be destroyed, it needs to instruct RenderViewImpl to clear its main_render_frame_ pointer, because it is going to be invalid. BUG=357747 Committed: https://crrev.com/8b9d9bd3f88f4ea830a15120d7833557193843de Cr-Commit-Position: refs/heads/master@{#328579}

Patch Set 1 #

Total comments: 1

Patch Set 2 : Fix compile break. #

Patch Set 3 : Improve comment. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -7 lines) Patch
M content/renderer/render_frame_impl.h View 1 2 2 chunks +6 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 6 chunks +5 lines, -7 lines 0 comments Download
M content/renderer/render_frame_impl_browsertest.cc View 1 2 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (3 generated)
nasko
Hey Nick, Can you review this CL for me? It adds a boolean member to ...
5 years, 7 months ago (2015-05-06 16:36:35 UTC) #2
dcheng
Is this an optimization? I'm curious why we can't do something like !!frame_->parent() for is_subframe()?
5 years, 7 months ago (2015-05-06 16:42:29 UTC) #4
ncarter (slow)
Concept seems fine, will be ready to approve once it compiles. https://codereview.chromium.org/1125243002/diff/1/content/renderer/render_frame_impl_browsertest.cc File content/renderer/render_frame_impl_browsertest.cc (right): ...
5 years, 7 months ago (2015-05-06 16:47:40 UTC) #5
nasko
On 2015/05/06 16:47:40, ncarter wrote: > Concept seems fine, will be ready to approve once ...
5 years, 7 months ago (2015-05-06 16:54:50 UTC) #6
nasko
On 2015/05/06 16:42:29, dcheng wrote: > Is this an optimization? I'm curious why we can't ...
5 years, 7 months ago (2015-05-06 16:57:38 UTC) #7
dcheng
On 2015/05/06 at 16:57:38, nasko wrote: > On 2015/05/06 16:42:29, dcheng wrote: > > Is ...
5 years, 7 months ago (2015-05-06 17:01:33 UTC) #8
ncarter (slow)
On 2015/05/06 17:01:33, dcheng wrote: > On 2015/05/06 at 16:57:38, nasko wrote: > > On ...
5 years, 7 months ago (2015-05-06 17:08:35 UTC) #9
nasko
On 2015/05/06 17:08:35, ncarter wrote: > On 2015/05/06 17:01:33, dcheng wrote: > > On 2015/05/06 ...
5 years, 7 months ago (2015-05-06 17:18:04 UTC) #10
nasko
Improved the comment as per Nick's suggestion.
5 years, 7 months ago (2015-05-06 17:39:56 UTC) #11
dcheng
On 2015/05/06 at 17:18:04, nasko wrote: > On 2015/05/06 17:08:35, ncarter wrote: > > On ...
5 years, 7 months ago (2015-05-06 17:44:40 UTC) #12
ncarter (slow)
On 2015/05/06 17:44:40, dcheng wrote: > On 2015/05/06 at 17:18:04, nasko wrote: > > On ...
5 years, 7 months ago (2015-05-06 18:05:29 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1125243002/40001
5 years, 7 months ago (2015-05-06 18:09:02 UTC) #15
commit-bot: I haz the power
Committed patchset #3 (id:40001)
5 years, 7 months ago (2015-05-06 19:23:30 UTC) #16
commit-bot: I haz the power
5 years, 7 months ago (2015-05-06 19:24:32 UTC) #17
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/8b9d9bd3f88f4ea830a15120d7833557193843de
Cr-Commit-Position: refs/heads/master@{#328579}

Powered by Google App Engine
This is Rietveld 408576698