Mostly manual refactoring of ServiceWorkerRequestHandler into a base class and two derived classes,…

Mostly manual refactoring of ServiceWorkerRequestHandler into a base class and two derived classes, one for use when processing requests from a controllee document, and the other for use with request from a service worker context.

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

[michaeln, 2014-04-24 01:59:44]

i'm sure i'll have to update some tests for this refactor (running some try jobs
to see), but  ptal a this split in the production code

[kinuko, 2014-04-24 05:15:00]

Looks reasonable to me (LGTM) if bots become happy.

I assume we're going to have multiple URLRequestJobs too, right?

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_context_request_handler.h
(right):

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_context_request_handler.h:26:
net::NetworkDelegate* network_delegate);
OVERRIDE

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_controllee_request_handler.cc:74:
PrepareForMainResource(const GURL& url) {
Ah... are these indents made by git cl format I guess?

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_controllee_request_handler.h
(right):

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_controllee_request_handler.h:34:
net::NetworkDelegate* network_delegate);
OVERRIDE

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_provider_host.h:86:
ResourceType::Type resource_type);
nit: this could probably return already scoped_ptr'ed one

[michaeln, 2014-04-24 19:01:19]

> I assume we're going to have multiple URLRequestJobs too, right?

I think so. For the here and now, I expect I'll have one job type for script
recording to first populate the cache and another for playback on subsequent
loads.

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_context_request_handler.h
(right):

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_context_request_handler.h:26:
net::NetworkDelegate* network_delegate);
On 2014/04/24 05:15:00, kinuko wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_controllee_request_handler.cc:74:
PrepareForMainResource(const GURL& url) {
On 2014/04/24 05:15:00, kinuko wrote:
> Ah... are these indents made by git cl format I guess?

i did that by hand, the class name is sooooo long :( any better suggestion for
wrapping this?

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_controllee_request_handler.h
(right):

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_controllee_request_handler.h:34:
net::NetworkDelegate* network_delegate);
On 2014/04/24 05:15:00, kinuko wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_provider_host.h:86:
ResourceType::Type resource_type);
On 2014/04/24 05:15:00, kinuko wrote:
> nit: this could probably return already scoped_ptr'ed one

Done

[jsbell, 2014-04-24 21:22:23]

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_controllee_request_handler.cc:74:
PrepareForMainResource(const GURL& url) {
On 2014/04/24 19:01:19, michaeln wrote:
> On 2014/04/24 05:15:00, kinuko wrote:
> > Ah... are these indents made by git cl format I guess?
> 
> i did that by hand, the class name is sooooo long :( any better suggestion for
> wrapping this?

I'd just let `git-cl format` do it's thing. It breaks after the ( in this case.
Breaking after the :: appears to be a last resort for nested classes.

https://codereview.chromium.org/255413005/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/255413005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:121: if
(hosted_version_) {
Whoah, just cluing into the fact that ServiceWorkerProviderHost hosts eithe a
document or a SW itself.

If I'm not misunderstanding that, can we (here or another CL) (1) update the
docs in the header file (which just talk about the document case), and (2) maybe
do this test via a helper e.g. isContextHost() / isControlleeHost() ?

[michaeln, 2014-04-24 21:44:07]

The CQ bit was checked by michaeln@chromium.org

[michaeln, 2014-04-24 21:44:23]

The CQ bit was unchecked by michaeln@chromium.org

[michaeln, 2014-04-24 21:49:16]

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/255413005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_controllee_request_handler.cc:74:
PrepareForMainResource(const GURL& url) {
On 2014/04/24 21:22:24, jsbell wrote:
> On 2014/04/24 19:01:19, michaeln wrote:
> > On 2014/04/24 05:15:00, kinuko wrote:
> > > Ah... are these indents made by git cl format I guess?
> > 
> > i did that by hand, the class name is sooooo long :( any better suggestion
for
> > wrapping this?
> 
> I'd just let `git-cl format` do it's thing. It breaks after the ( in this
case.
> Breaking after the :: appears to be a last resort for nested classes.

Done, taking git-cl format's suggestions across the board.

https://codereview.chromium.org/255413005/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/255413005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:121: if
(hosted_version_) {
On 2014/04/24 21:22:24, jsbell wrote:
> Whoah, just cluing into the fact that ServiceWorkerProviderHost hosts eithe a
> document or a SW itself.
> 
> If I'm not misunderstanding that, can we (here or another CL) (1) update the
> docs in the header file (which just talk about the document case), and (2)
maybe
> do this test via a helper e.g. isContextHost() / isControlleeHost() ?

Good idea, I'll do both in this CL.

[michaeln, 2014-04-24 21:59:18]

https://codereview.chromium.org/255413005/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/255413005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:121: if
(hosted_version_) {
> Good idea, I'll do both in this CL.

Having a bit of a hard time with terminology, hows this.

// Note this class can also host a running service worker, in which
// case it will observe resource loads made directly by the service worker.
class CONTENT_EXPORT ServiceWorkerProviderHost

bool IsHostToRunningServiceWorker();

[michaeln, 2014-04-24 23:54:20]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-04-24 23:55:29]

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

[commit-bot: I haz the power, 2014-04-24 23:59:48]

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

[commit-bot: I haz the power, 2014-04-24 23:59:48]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[michaeln, 2014-04-25 00:53:46]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-04-25 00:54:13]

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

[commit-bot: I haz the power, 2014-04-25 08:56:37]

Change committed as 266166