Reland "Streamline frame detach"

Reland "Streamline frame detach"

Share a bit more code between LocalFrame and RemoteFrame, including moving the detach-time client notification down to FrameClient.

Move the max frame counter to FrameHost instead of Page.

Move detach logic from FrameLoader to LocalFrame, unless it manipulates FrameLoader members.

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183773

[Nate Chapin, 2014-10-10 22:55:50]

japhet@chromium.org changed reviewers:
+ dcheng@chromium.org

[Nate Chapin, 2014-10-10 22:55:51]

Sorry, the diffs are a little messy.

For the substantive changes, see deltas from patch 2 for Frame.cpp and
WebFrame.cpp.

[sof, 2014-10-13 06:51:00]

https://codereview.chromium.org/645623004/diff/380001/Source/core/frame/Frame...
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/645623004/diff/380001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:233: if (m_owner) {
nit: can fuse the two "if" statements into one.

[dcheng, 2014-10-13 09:17:16]

https://codereview.chromium.org/645623004/diff/380001/Source/core/frame/Frame...
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/645623004/diff/380001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:75: // didn't decrement the frame count, so don't
increment it here.
Yikes. It would be nice to keep the swap-specific logic more local to
WebFrame::swap(), but I need to think about this more.

[Nate Chapin, 2014-10-13 17:13:58]

On 2014/10/13 09:17:16, dcheng wrote:
>
https://codereview.chromium.org/645623004/diff/380001/Source/core/frame/Frame...
> File Source/core/frame/Frame.cpp (right):
> 
>
https://codereview.chromium.org/645623004/diff/380001/Source/core/frame/Frame...
> Source/core/frame/Frame.cpp:75: // didn't decrement the frame count, so don't
> increment it here.
> Yikes. It would be nice to keep the swap-specific logic more local to
> WebFrame::swap(), but I need to think about this more.

Yeah, this was the least terrible option that occurred to me in an ~hour. Maybe
checkFrameCountConsistency() isn't worth the trouble. Or for that matter, I
wonder how important having a frame limit is, if we prevent infinite
self-embedding.

[dcheng, 2014-10-13 21:35:46]

Thinking about this more...

It seems like some of this complexity is tied to the fact that there are
multiple places trying to manage the subframe count and coordinate to keep them
consistent.

Would it make it simpler/easier to understand if WebFrame::appendChild() and
WebFrame::removeChild() were responsible for updating these counts? And then
perhaps Frame could assert that it's part of the frame tree if it has a
FrameOwner?

[Nate Chapin, 2014-10-14 17:44:09]

On 2014/10/13 21:35:46, dcheng wrote:
> Thinking about this more...
> 
> It seems like some of this complexity is tied to the fact that there are
> multiple places trying to manage the subframe count and coordinate to keep
them
> consistent.
> 
> Would it make it simpler/easier to understand if WebFrame::appendChild() and
> WebFrame::removeChild() were responsible for updating these counts? And then
> perhaps Frame could assert that it's part of the frame tree if it has a
> FrameOwner?

Done. PTAL

[dcheng, 2014-10-14 21:43:37]

https://codereview.chromium.org/645623004/diff/590001/Source/web/tests/WebFra...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/645623004/diff/590001/Source/web/tests/WebFra...
Source/web/tests/WebFrameTest.cpp:3782:
mainFrame->scopeStringMatches(kFindIdentifier, searchText, options, true);
Hmm. Is this change needed to fix a test? Or just purely cleanup?

[Nate Chapin, 2014-10-14 21:45:07]

https://codereview.chromium.org/645623004/diff/590001/Source/web/tests/WebFra...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/645623004/diff/590001/Source/web/tests/WebFra...
Source/web/tests/WebFrameTest.cpp:3782:
mainFrame->scopeStringMatches(kFindIdentifier, searchText, options, true);
On 2014/10/14 21:43:37, dcheng wrote:
> Hmm. Is this change needed to fix a test? Or just purely cleanup?

That was a long time ago, and I don't remember. :) I'll check.

[dcheng, 2014-10-15 19:57:59]

Incidentally, the rest lgtm. It'd be nice to revert the WebFrameTest.cpp change
if it's not needed, but it's not that important either way.

[Nate Chapin, 2014-10-15 21:35:36]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2014-10-15 21:35:54]

On 2014/10/15 19:57:59, dcheng wrote:
> Incidentally, the rest lgtm. It'd be nice to revert the WebFrameTest.cpp
change
> if it's not needed, but it's not that important either way.

Yeah, turns out it wasn't necessary. Thanks for catching that.

[commit-bot: I haz the power, 2014-10-15 21:36:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645623004/610001

[commit-bot: I haz the power, 2014-10-15 23:35:19]

Committed patchset #6 (id:610001) as 183773