[ServiceWorker] Fix RenderFrameImpl::isControlledByServiceWorker()

[ServiceWorker] Fix RenderFrameImpl::isControlledByServiceWorker()

In current code, we check the main frame's data_source in RenderFrameImpl.
But even if the main frame is not controlled by the ServiceWorker the iframe on it could be controlled by the ServiceWorker.
So we must check their own frame_'s data_source.

BUG=411151
Committed: https://crrev.com/bf24e5ea0df583420cb420330efef7de12801875
Cr-Commit-Position: refs/heads/master@{#297394}

[horo, 2014-09-29 09:08:25]

horo@chromium.org changed reviewers:
+ falken@chromium.org

[horo, 2014-09-29 09:08:26]

falken@
Could you please review this?

[falken, 2014-09-29 09:24:18]

On 2014/09/29 09:08:26, horo wrote:
> falken@
> Could you please review this?

I'm not sure I can competently review this, I haven't touched render_frame_impl
before.

One suggestion is to make the CL description explain why you're making this
change, rather than just describing the change. I read the bug but don't see how
it relates to main_frame vs frame_.

[horo, 2014-09-30 03:38:25]

On 2014/09/29 09:24:18, falken wrote:
> On 2014/09/29 09:08:26, horo wrote:
> > falken@
> > Could you please review this?
> 
> I'm not sure I can competently review this, I haven't touched
render_frame_impl
> before.
> 
> One suggestion is to make the CL description explain why you're making this
> change, rather than just describing the change. I read the bug but don't see
how
> it relates to main_frame vs frame_.

Changed the description.

[falken, 2014-09-30 05:42:49]

this lgtm. of course an OWNER must look at it too.

thanks for revising the description

[horo, 2014-09-30 05:46:11]

horo@chromium.org changed reviewers:
+ jochen@chromium.org

[horo, 2014-09-30 05:46:11]

jochen@
Could you please review this?

[Mike West, 2014-09-30 06:17:30]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2014-09-30 06:17:31]

The change looks perfectly reasonable, but it's not clear where it's actually
used. Quickly grepping through code, I don't see where this is ever called:
FrameLoaderClient::isControlledByServiceWorker is defined, but doesn't seem to
be executed.

Will there be any visible change after landing this patch?

https://codereview.chromium.org/609283002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/609283002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:3185: return false;
Can this ever be false?

[horo, 2014-09-30 06:48:39]

Patchset #2 (id:20001) has been deleted

[horo, 2014-09-30 06:55:59]

> The change looks perfectly reasonable, but it's not clear where it's actually
> used. Quickly grepping through code, I don't see where this is > ever called:
> FrameLoaderClient::isControlledByServiceWorker is defined, but doesn't seem to
> be executed.
>
> Will there be any visible change after landing this patch?

This method is not called yet.
I'm planing to call this method from the constructor of DocumentThreadableLoader
and ResourceLoader::start() to create a fallback request.
This fallback request will be used when FetchEent.respondWith() is not called in
the ServiceWorker and the browser process send back the was_fallback_required
response.
https://codereview.chromium.org/602073002/

This is a work in progress patch.
https://codereview.chromium.org/600393004/

https://codereview.chromium.org/609283002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/609283002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:3185: return false;
On 2014/09/30 06:17:31, Mike West wrote:
> Can this ever be false?


DCHECK(frame_) seems be better.

[Mike West, 2014-09-30 07:08:32]

Thanks. LGTM. You'll still need an OWNER's review, but hopefully this will speed
that up.

[jochen (gone - plz use gerrit), 2014-09-30 08:44:48]

lgtm

[horo, 2014-09-30 08:59:52]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-09-30 09:00:39]

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

[commit-bot: I haz the power, 2014-09-30 09:51:28]

Committed patchset #2 (id:40001) as 42f7159727d5073dab5f33dd47db723e415768ec

[commit-bot: I haz the power, 2014-09-30 09:52:09]

Patchset 2 (id:??) landed as
https://crrev.com/bf24e5ea0df583420cb420330efef7de12801875
Cr-Commit-Position: refs/heads/master@{#297394}