Tease apart ServiceWorkerNetworkProvider from other stuff.

Tease apart ServiceWorkerNetworkProvider from other stuff.

BUG=285976
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255324

[michaeln, 2014-02-28 01:59:58]

ptal

adam, i'd like your thoughts on the webkit api change suggested in there

nothing done to the get requestextradata baked into resourcerequest ipcs, but
once we nail down this bit we can flesh out ipc plumbing gunkware

https://codereview.chromium.org/183993004/diff/20001/content/child/request_ex...
File content/child/request_extra_data.h (right):

https://codereview.chromium.org/183993004/diff/20001/content/child/request_ex...
content/child/request_extra_data.h:35: int service_worker_provider_id);
I'll have to chase down places where this ctor is used to fix compile errors.

[michaeln, 2014-02-28 02:04:02]

https://codereview.chromium.org/183993004/diff/20001/content/child/service_wo...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/183993004/diff/20001/content/child/service_wo...
content/child/service_worker/service_worker_network_provider.cc:27:
scoped_ptr<ServiceWorkerNetworkProvider> network_provider) {
could use some DCHECKS about datasource_userdata not being null

[kinuko, 2014-02-28 04:30:59]

https://codereview.chromium.org/183993004/diff/20001/content/child/service_wo...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/183993004/diff/20001/content/child/service_wo...
content/child/service_worker/service_worker_network_provider.cc:41:
thread_safe_sender_->Send(
nit: ChildThread::current()->Send is probably enough.

https://codereview.chromium.org/183993004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/183993004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1456: }
Couldn't we just use document_state retrieved at line 1382 above?

[michaeln, 2014-02-28 20:48:26]

https://codereview.chromium.org/183993004/diff/20001/content/child/service_wo...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/183993004/diff/20001/content/child/service_wo...
content/child/service_worker/service_worker_network_provider.cc:41:
thread_safe_sender_->Send(
On 2014/02/28 04:31:00, kinuko wrote:
> nit: ChildThread::current()->Send is probably enough.

Good point, Done.

https://codereview.chromium.org/183993004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/183993004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1456: }
On 2014/02/28 04:31:00, kinuko wrote:
> Couldn't we just use document_state retrieved at line 1382 above?

Nope, that's from the wrong document. That corresponds to what's topmost in the
frame hierarchy. We need the document that's issuing the request.

[kinuko, 2014-03-03 03:26:33]

https://codereview.chromium.org/183993004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/183993004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1456: }
On 2014/02/28 20:48:27, michaeln wrote:
> On 2014/02/28 04:31:00, kinuko wrote:
> > Couldn't we just use document_state retrieved at line 1382 above?
> 
> Nope, that's from the wrong document. That corresponds to what's topmost in
the
> frame hierarchy. We need the document that's issuing the request.

I see, thanks. Some of the tests look failing probably due to null-ptr access
around here-- can we fix it?

[michaeln, 2014-03-03 23:34:55]

I'll take a closer look at the resource fetcher tests. I'm sure we can make it
not crash pretty easily, but i'd like to see why those are different.
ResourceFetcher is a content class that uses webcore's loading machinery from
the chromium side, so these requests don't go thru the full webcore loader
stack.

[michaeln, 2014-03-04 00:13:55]

https://codereview.chromium.org/183993004/diff/70001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/183993004/diff/70001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1752: if (frame->provisionalDataSource())
{  // May be null is some tests.
The ResourceFetcher tests all do this,
  fetcher->Start(frame, WebURLRequest::TargetIsMainFrame,...
so this logic see 'mainframe' but its not really so.

[michaeln, 2014-03-04 04:23:21]

There is an actual use case where ResourceFetcher is used with a target type of
TargetIsMainFrame (for AlternateErrorDisplay) In that case, the provisional data
source is null because it's not really going thru the usual processing
(navigation to frameloader to docloader).

I think the logic in snapshot 6 is good for that oddball case (so sw will be
completely hands off this resource load).

The other ResourceFetcher use cases are around ImageLoaders on the chromium
side. I'm not entirely sure what those all are used for, favicons for one.

Also think the logic in #6 works for these too, these requests will be routed
thru sw for the current doc.

[michaeln, 2014-03-04 04:27:48]

https://codereview.chromium.org/183993004/diff/90001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/183993004/diff/90001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1749: // this ambiguity about which
datasource can be removed.
i don't plan on leaving this comment here

https://codereview.chromium.org/183993004/diff/90001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1753: if (frame->provisionalDataSource())
{  // May be null is some tests.
i'll remove the 'tests' comment since this can apply in other cases too

[kinuko, 2014-03-04 12:53:07]

lgtm if you update comments & try bots are happy

https://codereview.chromium.org/183993004/diff/90001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/183993004/diff/90001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1753: if (frame->provisionalDataSource())
{  // May be null is some tests.
On 2014/03/04 04:27:49, michaeln wrote:
> i'll remove the 'tests' comment since this can apply in other cases too

When you remove the comment can you add a brief comment when it can be null?
(Like the notes you made in the issue comments)

[michaeln, 2014-03-04 19:28:39]

bah... merge conflict again? i wonder if the patch that i merged with between
snapshot 5 and 6 has since been reverted?

[michaeln, 2014-03-04 21:05:05]

> When you remove the comment can you add a brief comment when it can be null?
> (Like the notes you made in the issue comments)

done

[michaeln, 2014-03-04 21:16:51]

@creis, ptal as owner

[michaeln, 2014-03-04 22:57:48]

modulo sick win builders, bots look pretty happy now :)

[Charlie Reis, 2014-03-04 23:02:16]

Are there tests that cover this ID somewhere?  Otherwise looks ready with nits
below.

https://chromiumcodereview.appspot.com/183993004/diff/130001/content/child/se...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://chromiumcodereview.appspot.com/183993004/diff/130001/content/child/se...
content/child/service_worker/service_worker_network_provider.cc:26:
base::SupportsUserData* datasource_userdata,
Wrong indent.

https://chromiumcodereview.appspot.com/183993004/diff/130001/content/child/se...
content/child/service_worker/service_worker_network_provider.cc:52: 
Remove extra blank line.

https://chromiumcodereview.appspot.com/183993004/diff/130001/content/child/se...
File content/child/service_worker/service_worker_network_provider.h (right):

https://chromiumcodereview.appspot.com/183993004/diff/130001/content/child/se...
content/child/service_worker/service_worker_network_provider.h:15: // Instanced
prior to the main resource load being started and remains
Instantiated
(Sorry, I had trouble reading this with the previous sentence ending in
instance.)

https://chromiumcodereview.appspot.com/183993004/diff/130001/content/child/se...
content/child/service_worker/service_worker_network_provider.h:16: // allocated
until after the last subresoure load has occured.
subresource, occurred

https://chromiumcodereview.appspot.com/183993004/diff/130001/content/child/se...
content/child/service_worker/service_worker_network_provider.h:18: // and
dispose the ServiceWorkerProviderHost in the browser process
Presumably the browser process also disposes of the host if this child process
dies, right?  (No change needed here; just wanted to sanity check.)

https://chromiumcodereview.appspot.com/183993004/diff/130001/content/child/se...
content/child/service_worker/service_worker_network_provider.h:22: // Its a
scoped integer that sends an ipc upon ctor'tion and dtor'tion.
It's

Also, you might say either "via ctor/dtor" or "upon construction and deletion,"
since it currently reads like "upon contortion and distortion."  :)

https://chromiumcodereview.appspot.com/183993004/diff/130001/content/child/se...
content/child/service_worker/service_worker_network_provider.h:25: // Ownership
is trasferred to the DocumentState.
transferred

[michaeln, 2014-03-04 23:53:45]

I'll make the changes in the next upload.

> Are there tests that cover this ID somewhere?

Not yet.

This is not fleshed out enough to do system testing yet, and RenderFrameImpl is
too ginormous with crazy dependencies to unittest. Witness there are 0 unittests
that construct an instance of RenderFrameImpl in render_frame_impl_unittest.cc.
I'm assuming others have tried and failed.

https://chromiumcodereview.appspot.com/183993004/diff/130001/content/child/se...
File content/child/service_worker/service_worker_network_provider.h (right):

https://chromiumcodereview.appspot.com/183993004/diff/130001/content/child/se...
content/child/service_worker/service_worker_network_provider.h:15: // Instanced
prior to the main resource load being started and remains
On 2014/03/04 23:02:17, Charlie Reis wrote:
> Instantiated
> (Sorry, I had trouble reading this with the previous sentence ending in
> instance.)

Done.

https://chromiumcodereview.appspot.com/183993004/diff/130001/content/child/se...
content/child/service_worker/service_worker_network_provider.h:16: // allocated
until after the last subresoure load has occured.
On 2014/03/04 23:02:17, Charlie Reis wrote:
> subresource, occurred

Done.

https://chromiumcodereview.appspot.com/183993004/diff/130001/content/child/se...
content/child/service_worker/service_worker_network_provider.h:18: // and
dispose the ServiceWorkerProviderHost in the browser process
> Presumably the browser process also disposes of the host if this child process
> dies, right?  (No change needed here; just wanted to sanity check.)

oh, we hadn't thought of that ;)

JK, yes, cleanup happens by way of that form of exit too. This is the basis for
how we know which serviceworkers are in use and such.

https://chromiumcodereview.appspot.com/183993004/diff/130001/content/child/se...
content/child/service_worker/service_worker_network_provider.h:22: // Its a
scoped integer that sends an ipc upon ctor'tion and dtor'tion.
> since it currently reads like "upon contortion and distortion."  :)

Done (but... i kinda liked that pun)

https://chromiumcodereview.appspot.com/183993004/diff/130001/content/child/se...
content/child/service_worker/service_worker_network_provider.h:25: // Ownership
is trasferred to the DocumentState.
On 2014/03/04 23:02:17, Charlie Reis wrote:
> transferred

Done.

[Charlie Reis, 2014-03-05 00:24:51]

On 2014/03/04 23:53:45, michaeln wrote:
> I'll make the changes in the next upload.
> 
> > Are there tests that cover this ID somewhere?
> 
> Not yet.
> 
> This is not fleshed out enough to do system testing yet, and RenderFrameImpl
is
> too ginormous with crazy dependencies to unittest. Witness there are 0
unittests
> that construct an instance of RenderFrameImpl in
render_frame_impl_unittest.cc.
> I'm assuming others have tried and failed.
> 

We haven't migrated any tests over to render_frame_unittest.cc yet, but I was
thinking more like render_view_browsertest.cc (which is really more of a unit
test).  DontIgnoreBackAfterNavEntryLimit is a good example, where it loads HTML
and watches for an IPC message with particular values.  In this case, you could
create one of the providers and make sure it stamps the ID on the
ResourceRequest.  (Not 100% sure it will be easy to trigger the resource request
and read it, but it's not as tough as it seems.)

[michaeln, 2014-03-05 00:49:53]

> We haven't migrated any tests over to render_frame_unittest.cc yet, but I was
> thinking more like render_view_browsertest.cc (which is really more of a unit
> test).  DontIgnoreBackAfterNavEntryLimit is a good example, where it loads
HTML
> and watches for an IPC message with particular values.  In this case, you
could
> create one of the providers and make sure it stamps the ID on the
> ResourceRequest.  (Not 100% sure it will be easy to trigger the resource
request
> and read it, but it's not as tough as it seems.)

i'll take a look at the browser tests

[michaeln, 2014-03-05 03:01:39]

thnx for pointing out how unittest like those browser_tests are

[Charlie Reis, 2014-03-05 18:32:56]

Great!  Thanks for the test.  LGTM.

https://codereview.chromium.org/183993004/diff/170001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/183993004/diff/170001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2257: RequestExtraData*  extra_data
= NULL;
nit: Remove extra space after *.

https://codereview.chromium.org/183993004/diff/170001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2260: // that the main request is
tagged with our id.
nit: our -> the provider's

(I got confused about whether "our id" stuck around for the second document.)

[michaeln, 2014-03-05 21:19:25]

ty!

> nit: Remove extra space after *.

done

> nit: our -> the provider's

done

> (I got confused about whether "our id" stuck around for the second document.)

hope this expectation helped EXPECT_NE(provider1_id, provider->provider_id())
point you in the right direction

[Charlie Reis, 2014-03-05 21:21:00]

On 2014/03/05 21:19:25, michaeln wrote:
> > (I got confused about whether "our id" stuck around for the second
document.)
> 
> hope this expectation helped EXPECT_NE(provider1_id, provider->provider_id())
> point you in the right direction

Yep, that's how I figured it out.  :)

[michaeln, 2014-03-05 22:51:13]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-03-05 22:52:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/183993004/200001

[commit-bot: I haz the power, 2014-03-05 23:10:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-05 23:10:07]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[michaeln, 2014-03-05 23:22:24]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-03-05 23:30:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/183993004/200001

[commit-bot: I haz the power, 2014-03-06 00:12:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/183993004/200001

[commit-bot: I haz the power, 2014-03-06 00:30:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 00:30:52]

Try jobs failed on following builders: mac_chromium_compile_dbg

[alecflett, 2014-03-06 00:43:25]

The CQ bit was checked by alecflett@chromium.org

[commit-bot: I haz the power, 2014-03-06 00:45:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/183993004/200001

[commit-bot: I haz the power, 2014-03-06 02:27:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 02:27:13]

Try jobs failed on following builders: mac_chromium_rel

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[kinuko, 2014-03-06 03:44:01]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-03-06 03:44:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/183993004/200001

[commit-bot: I haz the power, 2014-03-06 04:59:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 04:59:10]

Retried try job too often on win_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[kinuko, 2014-03-06 07:41:35]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-03-06 07:41:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/183993004/200001

[commit-bot: I haz the power, 2014-03-06 13:10:23]

Change committed as 255324