Remove ResourceFetcher's access to a FrameFetchContext when detaching

Remove ResourceFetcher's access to a real FetchContext at frame detach time

This allows us to remove a whole bunch of frame() null-checks in FrameFetchContext.

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

[Nate Chapin, 2015-04-29 22:11:26]

japhet@chromium.org changed reviewers:
+ mkwst@chromium.org, sigbjornf@opera.com

[Nate Chapin, 2015-04-29 22:11:26]

Nth attempt at this idea.

This should be identical to a rebased
https://codereview.chromium.org/1035803002, except for the test fix in
HTMLImportTreeRoot.cpp.

[Mike West, 2015-04-30 16:21:43]

The bot failures look unrelated, so as long as the CQ is happy, LGTM to try
again.

[sof, 2015-04-30 18:18:37]

https://codereview.chromium.org/1116613003/diff/1/Source/core/html/imports/HT...
File Source/core/html/imports/HTMLImportTreeRoot.cpp (right):

https://codereview.chromium.org/1116613003/diff/1/Source/core/html/imports/HT...
Source/core/html/imports/HTMLImportTreeRoot.cpp:103:
HTMLImport::recalcTreeState(this);
Could you explain why new recalculations cannot be scheduled now? Or, what went
previously to require this change?

[Nate Chapin, 2015-04-30 18:26:08]

On 2015/04/30 18:18:37, sof wrote:
>
https://codereview.chromium.org/1116613003/diff/1/Source/core/html/imports/HT...
> File Source/core/html/imports/HTMLImportTreeRoot.cpp (right):
> 
>
https://codereview.chromium.org/1116613003/diff/1/Source/core/html/imports/HT...
> Source/core/html/imports/HTMLImportTreeRoot.cpp:103:
> HTMLImport::recalcTreeState(this);
> Could you explain why new recalculations cannot be scheduled now? Or, what
went
> previously to require this change?

Looking at the commit history, there wasn't any explanation as to why. My guess
it was an optimization to minimize timers?

It no longer works smoothly because there isn't a good way to keep the
HTMLImportTreeRoot alive, and the work done in HTMLImport::recalcTreeState() can
delete the HTMLImportTreeRoot.

[sof, 2015-04-30 19:40:32]

On 2015/04/30 18:26:08, Nate Chapin wrote:
> On 2015/04/30 18:18:37, sof wrote:
> >
>
https://codereview.chromium.org/1116613003/diff/1/Source/core/html/imports/HT...
> > File Source/core/html/imports/HTMLImportTreeRoot.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1116613003/diff/1/Source/core/html/imports/HT...
> > Source/core/html/imports/HTMLImportTreeRoot.cpp:103:
> > HTMLImport::recalcTreeState(this);
> > Could you explain why new recalculations cannot be scheduled now? Or, what
> went
> > previously to require this change?
> 
> Looking at the commit history, there wasn't any explanation as to why. My
guess
> it was an optimization to minimize timers?
> 

I don't follow, it seems to be recalculating until some fixed point is reached
for a reason.

> It no longer works smoothly because there isn't a good way to keep the
> HTMLImportTreeRoot alive, and the work done in HTMLImport::recalcTreeState()
can
> delete the HTMLImportTreeRoot.

Thanks, and HTMLImportTreeRoot being an auxillary object of the controller,
it'll be destructed when the controller is removed during any document detach.
ok, that explains why this was happening now.

So, this change makes that safe (enough), but it leaves the question about
original purpose of that loop. The cleverness of the htmlimports implementation
is beyond me; let's Cc: some people that might know/remember?

[sof, 2015-04-30 19:42:49]

sigbjornf@opera.com changed reviewers:
+ dglazkov@chromium.org, morrita@chromium.org

[sof, 2015-04-30 19:42:49]

+ dglazkov@, morrita@: re: https://codereview.chromium.org/1116613003/#msg6 --
do you know why that import recalculation loop is in place?

[sof, 2015-05-01 06:52:31]

On 2015/04/30 19:42:49, sof wrote:
> + dglazkov@, morrita@: re: https://codereview.chromium.org/1116613003/#msg6 --
> do you know why that import recalculation loop is in place?

Looking back in history a bit, it seems to come from

 https://codereview.chromium.org/141143006

so unless recalculation no longer triggers state change notifications, this
change is removing needed functionality (but without tests showing that up?)

[sof, 2015-05-01 10:15:59]

Going over the details more carefully, this is a safe & complete change to
HTMLImportTreeRoot. The loop referred to is a recalculation shortcut, and just
that. As #5 said :)

Sorry about the slight hold up, I just wanted to make sure nothing regresses to
warrant (N + 1).

lgtm.

[Nate Chapin, 2015-05-01 18:27:37]

On 2015/05/01 10:15:59, sof wrote:
> Going over the details more carefully, this is a safe & complete change to
> HTMLImportTreeRoot. The loop referred to is a recalculation shortcut, and just
> that. As #5 said :)
> 
> Sorry about the slight hold up, I just wanted to make sure nothing regresses
to
> warrant (N + 1).
> 
> lgtm.

No problem, I was not as thorough in my research as I probably should have been.
Thanks!

[Nate Chapin, 2015-05-01 18:27:45]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2015-05-01 18:28:04]

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

[commit-bot: I haz the power, 2015-05-01 20:13:33]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194827