Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2874073004:
network service: Add job wrapper to SWControlleeRequestHandler (Closed)
|
Created:
3 years, 7 months ago by scottmg Modified:
3 years, 7 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, jam, nhiroki, kinuko+serviceworker, horo+watch_chromium.org, darin-cc_chromium.org, kinuko+watch, tzik, blink-worker-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionnetwork service: Add job wrapper to SWControlleeRequestHandler
Removes direct use of URLRequestJob from
ServiceWorkerControlleeRequestHandler, by wrapping URLRequestJob* in a
new class, ServiceWorkerJobWrapper. This is in service of having
SWControlleeRequestHandler support making requests via
mojom::URLLoaderFactory in the future. The mojo path is neither called,
nor currently implemented.
This is split off of https://codereview.chromium.org/2843043002/.
NO_DEPENDENCY_CHECKS=true
BUG=715640
Review-Url: https://codereview.chromium.org/2874073004
Cr-Commit-Position: refs/heads/master@{#472744}
Committed: https://chromium.googlesource.com/chromium/src/+/30fc2c9848ad8c09c8de1242bea524b5e1856f5b
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #
Total comments: 6
Patch Set 5 : . #
Total comments: 4
Patch Set 6 : improve name #
Total comments: 2
Patch Set 7 : fix dchecks #
Messages
Total messages: 48 (37 generated)
Description was changed from ========== network service: Add job wrapper to SWControlleeRequestHandler BUG=715640 ========== to ========== network service: Add job wrapper to SWControlleeRequestHandler Removes direct use of URLRequestJob from ServiceWorkerControlleeRequestHandler, by wrapping URLRequestJob* in a new class, ServiceWorkerJobWrapper. This is in service of subsequently having SWControlleeRequestHandler support request via mojom::URLLoaderFactory in the future. This is split off of https://codereview.chromium.org/2843043002/. BUG=715640 ==========
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Description was changed from ========== network service: Add job wrapper to SWControlleeRequestHandler Removes direct use of URLRequestJob from ServiceWorkerControlleeRequestHandler, by wrapping URLRequestJob* in a new class, ServiceWorkerJobWrapper. This is in service of subsequently having SWControlleeRequestHandler support request via mojom::URLLoaderFactory in the future. This is split off of https://codereview.chromium.org/2843043002/. BUG=715640 ========== to ========== network service: Add job wrapper to SWControlleeRequestHandler Removes direct use of URLRequestJob from ServiceWorkerControlleeRequestHandler, by wrapping URLRequestJob* in a new class, ServiceWorkerJobWrapper. This is in service of subsequently having SWControlleeRequestHandler support request via mojom::URLLoaderFactory in the future. The mojo path is neither called, nor currently implemented. This is split off of https://codereview.chromium.org/2843043002/. BUG=715640 ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== network service: Add job wrapper to SWControlleeRequestHandler Removes direct use of URLRequestJob from ServiceWorkerControlleeRequestHandler, by wrapping URLRequestJob* in a new class, ServiceWorkerJobWrapper. This is in service of subsequently having SWControlleeRequestHandler support request via mojom::URLLoaderFactory in the future. The mojo path is neither called, nor currently implemented. This is split off of https://codereview.chromium.org/2843043002/. BUG=715640 ========== to ========== network service: Add job wrapper to SWControlleeRequestHandler Removes direct use of URLRequestJob from ServiceWorkerControlleeRequestHandler, by wrapping URLRequestJob* in a new class, ServiceWorkerJobWrapper. This is in service of having SWControlleeRequestHandler support making requests via mojom::URLLoaderFactory in the future. The mojo path is neither called, nor currently implemented. This is split off of https://codereview.chromium.org/2843043002/. BUG=715640 ==========
Description was changed from ========== network service: Add job wrapper to SWControlleeRequestHandler Removes direct use of URLRequestJob from ServiceWorkerControlleeRequestHandler, by wrapping URLRequestJob* in a new class, ServiceWorkerJobWrapper. This is in service of having SWControlleeRequestHandler support making requests via mojom::URLLoaderFactory in the future. The mojo path is neither called, nor currently implemented. This is split off of https://codereview.chromium.org/2843043002/. BUG=715640 ========== to ========== network service: Add job wrapper to SWControlleeRequestHandler Removes direct use of URLRequestJob from ServiceWorkerControlleeRequestHandler, by wrapping URLRequestJob* in a new class, ServiceWorkerJobWrapper. This is in service of having SWControlleeRequestHandler support making requests via mojom::URLLoaderFactory in the future. The mojo path is neither called, nor currently implemented. This is split off of https://codereview.chromium.org/2843043002/. BUG=715640 ==========
scottmg@chromium.org changed reviewers: + kinuko@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by scottmg@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2868133003 Patch 40001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
Description was changed from ========== network service: Add job wrapper to SWControlleeRequestHandler Removes direct use of URLRequestJob from ServiceWorkerControlleeRequestHandler, by wrapping URLRequestJob* in a new class, ServiceWorkerJobWrapper. This is in service of having SWControlleeRequestHandler support making requests via mojom::URLLoaderFactory in the future. The mojo path is neither called, nor currently implemented. This is split off of https://codereview.chromium.org/2843043002/. BUG=715640 ========== to ========== network service: Add job wrapper to SWControlleeRequestHandler Removes direct use of URLRequestJob from ServiceWorkerControlleeRequestHandler, by wrapping URLRequestJob* in a new class, ServiceWorkerJobWrapper. This is in service of having SWControlleeRequestHandler support making requests via mojom::URLLoaderFactory in the future. The mojo path is neither called, nor currently implemented. This is split off of https://codereview.chromium.org/2843043002/. NO_DEPENDENCY_CHECKS=true BUG=715640 ==========
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2874073004/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2874073004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler.cc:467: job_.reset(); This and job_->WasCanceled() don't look to work together https://codereview.chromium.org/2874073004/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_job_wrapper.h (right): https://codereview.chromium.org/2874073004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_job_wrapper.h:30: ServiceWorkerControlleeURLLoaderFactory* factory); I wonder if this should rather wrap urlloader? https://codereview.chromium.org/2874073004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_job_wrapper.h:54: size_t GetURLChainSize() const; I feel in the new code the job wouldn't know these... but we can look into these later.
Thanks, sorry for the long delay. https://codereview.chromium.org/2874073004/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2874073004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler.cc:467: job_.reset(); On 2017/05/11 12:48:53, kinuko wrote: > This and job_->WasCanceled() don't look to work together Done. https://codereview.chromium.org/2874073004/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_job_wrapper.h (right): https://codereview.chromium.org/2874073004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_job_wrapper.h:30: ServiceWorkerControlleeURLLoaderFactory* factory); On 2017/05/11 12:48:53, kinuko wrote: > I wonder if this should rather wrap urlloader? I'm not 100% sure if that will work yet, but that does seem closer to wrapping ServiceWorkerURLRequestJob, so seems like a good idea for now. Done. https://codereview.chromium.org/2874073004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_job_wrapper.h:54: size_t GetURLChainSize() const; On 2017/05/11 12:48:53, kinuko wrote: > I feel in the new code the job wouldn't know these... but we can look into these > later. Yeah, probably right. Since they're "only" for metrics maybe we can report them elsewhere later.
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
falken@chromium.org changed reviewers: + falken@chromium.org
https://codereview.chromium.org/2874073004/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2874073004/diff/80001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler.cc:192: // The job may have been canceled and then destroyed before this was invoked. I guess "and then destroyed" is no longer needed or true, here and throughout. https://codereview.chromium.org/2874073004/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_job_wrapper.h (right): https://codereview.chromium.org/2874073004/diff/80001/content/browser/service... content/browser/service_worker/service_worker_job_wrapper.h:22: class ServiceWorkerJobWrapper { nit: ServiceWorkerJobWrapper isn't a great name since we already have ServiceWorkerJobCoordinator and ServiceWorkerJobTest for a totally different type of job. I'd suggest ServiceWorkerURLRequestWrapper but it'd require changing all the "job" terminology in ServiceWorkerControlleeRequestHandler which could be burdensome. Maybe just ServiceWorkerURLRequestJobWrapper? Originally I thought it's too specific since the class handles both URLRequestJob and URLLoaderFactory, but maybe it's OK as the class should be temporary once the URLRequestJOb support is no longer needed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2874073004/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2874073004/diff/80001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler.cc:192: // The job may have been canceled and then destroyed before this was invoked. On 2017/05/17 01:29:05, falken wrote: > I guess "and then destroyed" is no longer needed or true, here and throughout. Done. https://codereview.chromium.org/2874073004/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_job_wrapper.h (right): https://codereview.chromium.org/2874073004/diff/80001/content/browser/service... content/browser/service_worker/service_worker_job_wrapper.h:22: class ServiceWorkerJobWrapper { On 2017/05/17 01:29:05, falken wrote: > nit: ServiceWorkerJobWrapper isn't a great name since we already have > ServiceWorkerJobCoordinator and ServiceWorkerJobTest for a totally different > type of job. > > I'd suggest ServiceWorkerURLRequestWrapper but it'd require changing all the > "job" terminology in ServiceWorkerControlleeRequestHandler which could be > burdensome. > > Maybe just ServiceWorkerURLRequestJobWrapper? Originally I thought it's too > specific since the class handles both URLRequestJob and URLLoaderFactory, but > maybe it's OK as the class should be temporary once the URLRequestJOb support is > no longer needed. Agreed, the name is poor. How about ServiceWorkerURLJobWrapper to disambiguate from the generic "job" of ServiceWorkerJobCoordinator and ServiceWorkerJobTest?
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
lgtm https://codereview.chromium.org/2874073004/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2874073004/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_controllee_request_handler.cc:404: DCHECK(url_job_.get()); This needs to be DCHECK(!JobWasCanceled()) I think
https://codereview.chromium.org/2874073004/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2874073004/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_controllee_request_handler.cc:404: DCHECK(url_job_.get()); On 2017/05/18 01:59:29, kinuko wrote: > This needs to be DCHECK(!JobWasCanceled()) I think Thank you! Done.
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by scottmg@chromium.org
The CQ bit was checked by scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, falken@chromium.org Link to the patchset: https://codereview.chromium.org/2874073004/#ps120001 (title: "fix dchecks")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495078779484240,
"parent_rev": "956654f51851fc1a7d26ac064d04f025374b2b93", "commit_rev":
"6cb3ef148aca24747b87b53e1ae5f92464e39067"}
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495078779484240,
"parent_rev": "9f1c33a3a158a560bbddf899bfc4b09f8d4ef009", "commit_rev":
"30fc2c9848ad8c09c8de1242bea524b5e1856f5b"}
Message was sent while issue was closed.
Description was changed from ========== network service: Add job wrapper to SWControlleeRequestHandler Removes direct use of URLRequestJob from ServiceWorkerControlleeRequestHandler, by wrapping URLRequestJob* in a new class, ServiceWorkerJobWrapper. This is in service of having SWControlleeRequestHandler support making requests via mojom::URLLoaderFactory in the future. The mojo path is neither called, nor currently implemented. This is split off of https://codereview.chromium.org/2843043002/. NO_DEPENDENCY_CHECKS=true BUG=715640 ========== to ========== network service: Add job wrapper to SWControlleeRequestHandler Removes direct use of URLRequestJob from ServiceWorkerControlleeRequestHandler, by wrapping URLRequestJob* in a new class, ServiceWorkerJobWrapper. This is in service of having SWControlleeRequestHandler support making requests via mojom::URLLoaderFactory in the future. The mojo path is neither called, nor currently implemented. This is split off of https://codereview.chromium.org/2843043002/. NO_DEPENDENCY_CHECKS=true BUG=715640 Review-Url: https://codereview.chromium.org/2874073004 Cr-Commit-Position: refs/heads/master@{#472744} Committed: https://chromium.googlesource.com/chromium/src/+/30fc2c9848ad8c09c8de1242bea5... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/30fc2c9848ad8c09c8de1242bea5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
