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

Issue 1128403004: Fix Frame::isMainFrame() to be compatible with provisional frames (Closed)

Created:
5 years, 7 months ago by dcheng
Modified:
5 years, 7 months ago
Reviewers:
Nate Chapin
CC:
blink-reviews, mlamouri+watch-blink_chromium.org, site-isolation-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Fix Frame::isMainFrame() to be compatible with provisional frames In OOPI, there can be provisional local frames. These frames are only partially linked into the frame tree. A provisional child frame has a parent set, but it does not appear in its parent's list of children. Similarly, a provisional main frame has a null parent set, but is never set as the main frame of a Page. Frame::isMainFrame() originally checked if Page::mainFrame() == this to determine if the frame is a main frame. However, code in navigation asserts that if Frame::isMainFrame() is false, the frame has a parent, which fails for provisional local main frames. They have no parent frame, but they are not yet set as the main frame of the Page: they only become the Page's main frame if the load commits. Since the parent is properly set for provisional local frames, change Frame::isMainFrame() to use that to determine main frame status. BUG=357747 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195211

Patch Set 1 #

Patch Set 2 : Update stuff #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -2 lines) Patch
M Source/core/frame/Frame.cpp View 1 chunk +1 line, -2 lines 0 comments Download
M Source/web/WebRemoteFrameImpl.cpp View 1 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (3 generated)
dcheng
5 years, 7 months ago (2015-05-08 23:16:34 UTC) #2
Nate Chapin
LGTM The case you're describing only holds if the provisional frame is the main frame, ...
5 years, 7 months ago (2015-05-08 23:22:23 UTC) #3
dcheng
On 2015/05/08 at 23:22:23, japhet wrote: > LGTM > > The case you're describing only ...
5 years, 7 months ago (2015-05-09 17:23:16 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128403004/20001
5 years, 7 months ago (2015-05-12 00:13:36 UTC) #7
commit-bot: I haz the power
5 years, 7 months ago (2015-05-12 00:16:50 UTC) #8
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195211

Powered by Google App Engine
This is Rietveld 408576698