New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 758879 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Factor out common part between ServiceWorkerSubresourceLoader and ServiceWorkerURLLoaderJob

Project Member Reported by falken@chromium.org, Aug 25 2017

Issue description

ServiceWorkerSubresourceLoader and ServiceWorkerURLLoader have a lot of common code. We should factor out the common code so we don't have so much copy/paste coding.

Note that ServiceWorkerSubresourceLoader lives in content/child (code that's executed in a renderer process), while ServiceWorkerURLLoader lives in content/browser (code that's executed in the browser process). The common code will have to live in the common place, which is called content/common.

There are two rough approaches to take:
1) Create a common base class that the two classes inherit from.
2) Create a common helper class (or classes) that the two classes can use.

In this case, I would probably attempt both approaches and see which looks to fit better, but my instinct is usually to go with 2).
 

Comment 1 by kinuko@chromium.org, Aug 25 2017

Yeah my instinct also says 2) would look reasonably good, but we can try a few things too.

Comment 2 by emim@google.com, Aug 28 2017

Status: Started (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/381696320505027cff278d61d029cc71e56143eb

commit 381696320505027cff278d61d029cc71e56143eb
Author: Emi Morikawa <emim@google.com>
Date: Tue Sep 05 15:02:37 2017

Factor out common part between ServiceWorkerSubresourceLoader and ServiceWorkerURLLoaderJob

Make new class "ServiceWorkerLoaderHelpers". This contains helper functions for service worker classes that use URLLoader. 

Bug:  758879 
Change-Id: I286336903b6b750ee094947264b163e6debd4995
Reviewed-on: https://chromium-review.googlesource.com/644627
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499632}
[modify] https://crrev.com/381696320505027cff278d61d029cc71e56143eb/content/browser/service_worker/service_worker_url_loader_job.cc
[modify] https://crrev.com/381696320505027cff278d61d029cc71e56143eb/content/browser/service_worker/service_worker_url_loader_job.h
[modify] https://crrev.com/381696320505027cff278d61d029cc71e56143eb/content/child/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/381696320505027cff278d61d029cc71e56143eb/content/child/service_worker/service_worker_subresource_loader.h
[modify] https://crrev.com/381696320505027cff278d61d029cc71e56143eb/content/common/BUILD.gn
[modify] https://crrev.com/381696320505027cff278d61d029cc71e56143eb/content/common/DEPS
[add] https://crrev.com/381696320505027cff278d61d029cc71e56143eb/content/common/service_worker/service_worker_loader_helpers.cc
[add] https://crrev.com/381696320505027cff278d61d029cc71e56143eb/content/common/service_worker/service_worker_loader_helpers.h

Comment 4 by emim@google.com, Sep 11 2017

Status: Fixed (was: Started)

Sign in to add a comment