Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2848493007:
Reduce/Remove URLRequest dependencies from AppCacheRequestHandler (Closed)
DescriptionReduce/Remove URLRequest dependencies from AppCacheRequestHandler
The AppCacheRequestHandler class provides functionality for serving network requests from
the AppCache if applicable. This class is instantiated by the AppCacheInterceptor when a network request
is initiated. Its lifetime depends on the associated URLRequest.
The plan is to reuse this class in the network service world where we won't be intercepting
network requests in the browser process. This effectively means that there won't be any URLRequest as well
To achieve this the proposal is to provide an abstraction for a request called AppCachRequest. This class
will have two subclasses at the moment. AppCacheURLRequest and AppCacheURLLoaderRequest. The AppCacheURLRequest
class will be used by the current network implementation which relies on intercepting n/w requests.
The AppCacheURLLoaderRequest class will be used by the network service mojo implementation.
Next step is to provide an abstraction for the AppCacheURLRequestJob class.
BUG=715632
TBR=jam
Review-Url: https://codereview.chromium.org/2848493007
Cr-Commit-Position: refs/heads/master@{#469254}
Committed: https://chromium.googlesource.com/chromium/src/+/4e8a529c38afa4770486d8033cb37af0d6caf311
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address review comments #Patch Set 3 : Fix compile failures #
Total comments: 6
Patch Set 4 : Address comments #
Total comments: 2
Patch Set 5 : Renamed AppCacheURLLoader to AppCacheURLLoaderRequest #Patch Set 6 : format changes #
Total comments: 4
Patch Set 7 : Use unique_ptr to make ownership of the AppCacheRequest class explicit #Patch Set 8 : git cl format #Patch Set 9 : Add comments #Messages
Total messages: 65 (35 generated)
ananta@chromium.org changed reviewers: + michaeln@chromium.org
The CQ bit was checked by ananta@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Description was changed from ========== Reduce/Remove URLRequest dependencies from AppCacheRequestHandler The AppCacheRequestHandler class provides functionality for serving network requests from the AppCache if applicable. This class is instantiated by the AppCacheInterceptor when a network request is initiated. Its lifetime depends on the associated URLRequest. The plan is to reuse this class in the network service world where we won't be intercepting network requests in the browser process. This effectively means that there won't be any URLRequest as well To achieve this the proposal is to provide an abstraction for a request called AppCachRequest. This class will have two subclasses at the moment. AppCacheURLRequest and AppCacheResourceRequest. The AppCacheURLRequest class will be used by the current network implementation which relies on intercepting n/w requests. The AppCacheResourceRequest class will be used by the network service mojo implementation. Next step is to provide an abstraction for the AppCacheURLRequestJob class. BUG=715632 ========== to ========== Reduce/Remove URLRequest dependencies from AppCacheRequestHandler The AppCacheRequestHandler class provides functionality for serving network requests from the AppCache if applicable. This class is instantiated by the AppCacheInterceptor when a network request is initiated. Its lifetime depends on the associated URLRequest. The plan is to reuse this class in the network service world where we won't be intercepting network requests in the browser process. This effectively means that there won't be any URLRequest as well To achieve this the proposal is to provide an abstraction for a request called AppCachRequest. This class will have two subclasses at the moment. AppCacheURLRequest and AppCacheResourceRequest. The AppCacheURLRequest class will be used by the current network implementation which relies on intercepting n/w requests. The AppCacheResourceRequest class will be used by the network service mojo implementation. Next step is to provide an abstraction for the AppCacheURLRequestJob class. BUG=715632 ==========
lgtm (also cc'd kinuko and scott for synergy with sw work) https://codereview.chromium.org/2848493007/diff/1/content/browser/appcache/ap... File content/browser/appcache/appcache_request.h (right): https://codereview.chromium.org/2848493007/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_request.h:34: virtual const GURL& GetFirstPartyForCookies() const = 0; style nit: you could remove some blank lines and smush these getters closer together https://codereview.chromium.org/2848493007/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_request.h:55: // Getters for the request types we currently support. It might be nice to make these private to dissuade their use general code? Of course it makes sense for the AppCacheUrlRequestJob to be able to access GetURLRequest(), but we need to avoid the dependency in general. https://codereview.chromium.org/2848493007/diff/1/content/browser/appcache/ap... File content/browser/appcache/appcache_request_handler.h (right): https://codereview.chromium.org/2848493007/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_request_handler.h:196: std::unique_ptr<AppCacheRequest> request_; i was wondering how object ownership was arranged, to make this more clear, can you used std::unique_ptr<> when passing it to the AppCacheRequestHandler ctor (and std::move it around as needed).
https://codereview.chromium.org/2848493007/diff/1/content/browser/appcache/ap... File content/browser/appcache/appcache_resource_request.h (right): https://codereview.chromium.org/2848493007/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_resource_request.h:42: ResourceRequest request_; Wait a sec, I think the 'handler' relies on request_.url() updating to reflect the current url when following a change of redirects. // The original url is the url used to initialize the request, and it may // differ from the url if the request was redirected. const GURL& original_url() const { return url_chain_.front(); } // The chain of urls traversed by this request. If the request had no // redirects, this vector will contain one element. const std::vector<GURL>& url_chain() const { return url_chain_; } const GURL& url() const { return url_chain_.back(); } We'll need to do something to update the value returned by GetURL for each successive call to MaybeLoadResource while walking down a redirect chain. Maybe this class can maintain the url_chain_ outside of the ResourceRequest struct.
drive-by: the two names of the subclases are a bit confusing. I could understand that AppCacheURLRequest means that it's using net::URLRequest. but what does AppCacheResourceRequest mean? That one is using URLLoader, so should it be AppCacheURLLoader? ResourceRequest is just a struct that contains some info, and isn't what the implementation uses.
Thanks michaeln. jam i renamed the files and the class. https://codereview.chromium.org/2848493007/diff/1/content/browser/appcache/ap... File content/browser/appcache/appcache_request.h (right): https://codereview.chromium.org/2848493007/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_request.h:34: virtual const GURL& GetFirstPartyForCookies() const = 0; On 2017/05/01 20:53:58, michaeln wrote: > style nit: you could remove some blank lines and smush these getters closer > together The functions were supposed to have comments. Fixed now https://codereview.chromium.org/2848493007/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_request.h:55: // Getters for the request types we currently support. On 2017/05/01 20:53:58, michaeln wrote: > It might be nice to make these private to dissuade their use general code? Of > course it makes sense for the AppCacheUrlRequestJob to be able to access > GetURLRequest(), but we need to avoid the dependency in general. Moved them to the protected section. https://codereview.chromium.org/2848493007/diff/1/content/browser/appcache/ap... File content/browser/appcache/appcache_resource_request.h (right): https://codereview.chromium.org/2848493007/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_resource_request.h:42: ResourceRequest request_; On 2017/05/01 23:41:31, michaeln wrote: > Wait a sec, I think the 'handler' relies on request_.url() updating to reflect > the current url when following a change of redirects. > > // The original url is the url used to initialize the request, and it may > // differ from the url if the request was redirected. > const GURL& original_url() const { return url_chain_.front(); } > // The chain of urls traversed by this request. If the request had no > // redirects, this vector will contain one element. > const std::vector<GURL>& url_chain() const { return url_chain_; } > const GURL& url() const { return url_chain_.back(); } > > We'll need to do something to update the value returned by GetURL for each > successive call to MaybeLoadResource while walking down a redirect chain. Maybe > this class can maintain the url_chain_ outside of the ResourceRequest struct. Sure. I was planning to do that in a later patchset while adding more functionality to this class
The CQ bit was checked by ananta@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by ananta@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...
kinuko@chromium.org changed reviewers: + kinuko@chromium.org
drive-by, just wanted to see how appcache side's going. (Thanks Michael for cc-ing) Interesting so you'll be reusing existing URLRequestJob related code too? For that part for SW we'll be probably doing it differently as doing it in the renderer will make enough amount of differences there. https://codereview.chromium.org/2848493007/diff/40001/content/browser/appcach... File content/browser/appcache/appcache_url_loader.h (right): https://codereview.chromium.org/2848493007/diff/40001/content/browser/appcach... content/browser/appcache/appcache_url_loader.h:13: // AppCacheRequest wrapper for the ResourceRequest class. ... for the cases that uses URLLoader. ? Could you also change the class name in the description? https://codereview.chromium.org/2848493007/diff/40001/content/browser/appcach... content/browser/appcache/appcache_url_loader.h:41: AppCacheURLLoader(const ResourceRequest& request); nit: explicit
Description was changed from ========== Reduce/Remove URLRequest dependencies from AppCacheRequestHandler The AppCacheRequestHandler class provides functionality for serving network requests from the AppCache if applicable. This class is instantiated by the AppCacheInterceptor when a network request is initiated. Its lifetime depends on the associated URLRequest. The plan is to reuse this class in the network service world where we won't be intercepting network requests in the browser process. This effectively means that there won't be any URLRequest as well To achieve this the proposal is to provide an abstraction for a request called AppCachRequest. This class will have two subclasses at the moment. AppCacheURLRequest and AppCacheResourceRequest. The AppCacheURLRequest class will be used by the current network implementation which relies on intercepting n/w requests. The AppCacheResourceRequest class will be used by the network service mojo implementation. Next step is to provide an abstraction for the AppCacheURLRequestJob class. BUG=715632 ========== to ========== Reduce/Remove URLRequest dependencies from AppCacheRequestHandler The AppCacheRequestHandler class provides functionality for serving network requests from the AppCache if applicable. This class is instantiated by the AppCacheInterceptor when a network request is initiated. Its lifetime depends on the associated URLRequest. The plan is to reuse this class in the network service world where we won't be intercepting network requests in the browser process. This effectively means that there won't be any URLRequest as well To achieve this the proposal is to provide an abstraction for a request called AppCachRequest. This class will have two subclasses at the moment. AppCacheURLRequest and AppCacheURLLoader. The AppCacheURLRequest class will be used by the current network implementation which relies on intercepting n/w requests. The AppCacheURLLoader class will be used by the network service mojo implementation. Next step is to provide an abstraction for the AppCacheURLRequestJob class. BUG=715632 ==========
On 2017/05/03 01:28:21, kinuko wrote: > drive-by, just wanted to see how appcache side's going. (Thanks Michael for > cc-ing) > > Interesting so you'll be reusing existing URLRequestJob related code too? For > that part for SW we'll be probably doing it differently as doing it in the > renderer will make enough amount of differences there. > > https://codereview.chromium.org/2848493007/diff/40001/content/browser/appcach... > File content/browser/appcache/appcache_url_loader.h (right): > > https://codereview.chromium.org/2848493007/diff/40001/content/browser/appcach... > content/browser/appcache/appcache_url_loader.h:13: // AppCacheRequest wrapper > for the ResourceRequest class. > ... for the cases that uses URLLoader. > > ? > > Could you also change the class name in the description? > > https://codereview.chromium.org/2848493007/diff/40001/content/browser/appcach... > content/browser/appcache/appcache_url_loader.h:41: AppCacheURLLoader(const > ResourceRequest& request); > nit: explicit Not planning to reuse the URLRequestJob code for AppCache. AppCache has a class AppCacheResponseReader which provides the functionality we need. However I plan to add an abstraction for the job in the AppCacheRequestHandler class.
https://codereview.chromium.org/2848493007/diff/40001/content/browser/appcach... File content/browser/appcache/appcache_url_loader.h (right): https://codereview.chromium.org/2848493007/diff/40001/content/browser/appcach... content/browser/appcache/appcache_url_loader.h:13: // AppCacheRequest wrapper for the ResourceRequest class. On 2017/05/03 01:28:21, kinuko wrote: > ... for the cases that uses URLLoader. > > ? > > Could you also change the class name in the description? Done. https://codereview.chromium.org/2848493007/diff/40001/content/browser/appcach... content/browser/appcache/appcache_url_loader.h:13: // AppCacheRequest wrapper for the ResourceRequest class. On 2017/05/03 01:28:21, kinuko wrote: > ... for the cases that uses URLLoader. > > ? > > Could you also change the class name in the description? Done. https://codereview.chromium.org/2848493007/diff/40001/content/browser/appcach... content/browser/appcache/appcache_url_loader.h:41: AppCacheURLLoader(const ResourceRequest& request); On 2017/05/03 01:28:21, kinuko wrote: > nit: explicit Done. https://codereview.chromium.org/2848493007/diff/40001/content/browser/appcach... content/browser/appcache/appcache_url_loader.h:41: AppCacheURLLoader(const ResourceRequest& request); On 2017/05/03 01:28:21, kinuko wrote: > nit: explicit Done.
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org Link to the patchset: https://codereview.chromium.org/2848493007/#ps60001 (title: "Address comments")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Reduce/Remove URLRequest dependencies from AppCacheRequestHandler The AppCacheRequestHandler class provides functionality for serving network requests from the AppCache if applicable. This class is instantiated by the AppCacheInterceptor when a network request is initiated. Its lifetime depends on the associated URLRequest. The plan is to reuse this class in the network service world where we won't be intercepting network requests in the browser process. This effectively means that there won't be any URLRequest as well To achieve this the proposal is to provide an abstraction for a request called AppCachRequest. This class will have two subclasses at the moment. AppCacheURLRequest and AppCacheURLLoader. The AppCacheURLRequest class will be used by the current network implementation which relies on intercepting n/w requests. The AppCacheURLLoader class will be used by the network service mojo implementation. Next step is to provide an abstraction for the AppCacheURLRequestJob class. BUG=715632 ========== to ========== Reduce/Remove URLRequest dependencies from AppCacheRequestHandler The AppCacheRequestHandler class provides functionality for serving network requests from the AppCache if applicable. This class is instantiated by the AppCacheInterceptor when a network request is initiated. Its lifetime depends on the associated URLRequest. The plan is to reuse this class in the network service world where we won't be intercepting network requests in the browser process. This effectively means that there won't be any URLRequest as well To achieve this the proposal is to provide an abstraction for a request called AppCachRequest. This class will have two subclasses at the moment. AppCacheURLRequest and AppCacheURLLoader. The AppCacheURLRequest class will be used by the current network implementation which relies on intercepting n/w requests. The AppCacheURLLoader class will be used by the network service mojo implementation. Next step is to provide an abstraction for the AppCacheURLRequestJob class. BUG=715632 TBR=jam ==========
The CQ bit was checked by ananta@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/03 01:28:21, kinuko wrote: > drive-by, just wanted to see how appcache side's going. (Thanks Michael for > cc-ing) > > Interesting so you'll be reusing existing URLRequestJob related code too? For > that part for SW we'll be probably doing it differently as doing it in the > renderer will make enough amount of differences there. We're planning to reuse the AppCacheRequestHandler and the AppCacheResponseReader, and the URLRequestJob derivative is cut out, but something that wraps the 'reader' and has a 'Start' method for the 'handler' to call will be in. Basically factor out all dependencies on net::URLRequest constructs and otherwise don't change a thing, at least for now. We think that makes sense to reuse the max amount of code while they have to co-exist. Eventually, for subresource loads, pushing the contents of class AppCache (map of urls to respsonseids, list of namespaces) into the renderer and having the renderer decide whether to use the network service or load a resource from the appcache might make sense. I think that's may be more in line with the serviceworker plans.
https://codereview.chromium.org/2848493007/diff/60001/content/browser/appcach... File content/browser/appcache/appcache_url_loader.h (right): https://codereview.chromium.org/2848493007/diff/60001/content/browser/appcach... content/browser/appcache/appcache_url_loader.h:15: class CONTENT_EXPORT AppCacheURLLoader : public AppCacheRequest { Sorry for the naming nit, but this isn't the 'loader' either? This should be some kind of 'request'. maybe call this one AppCacheLoaderRequest and the other is AppCacheURLReqeust // some auto-chromium-class-name-generating :) AppCacheLoaderRequest (this one might be ok) AppCacheURLLoadRequest AppCacheResourceLoaderRequest AppCacheLoadRequest and the other new class job'ish class could be AppCacheLoaderJob (akin to AppCacheURLRequestJob)
jam@chromium.org changed reviewers: + jam@chromium.org
https://codereview.chromium.org/2848493007/diff/60001/content/browser/appcach... File content/browser/appcache/appcache_url_loader.h (right): https://codereview.chromium.org/2848493007/diff/60001/content/browser/appcach... content/browser/appcache/appcache_url_loader.h:15: class CONTENT_EXPORT AppCacheURLLoader : public AppCacheRequest { On 2017/05/03 02:32:12, michaeln wrote: > Sorry for the naming nit, but this isn't the 'loader' either? This should be > some kind of 'request'. > > maybe call this one AppCacheLoaderRequest and the other is AppCacheURLReqeust > > // some auto-chromium-class-name-generating :) > AppCacheLoaderRequest (this one might be ok) > AppCacheURLLoadRequest > AppCacheResourceLoaderRequest > AppCacheLoadRequest > > and the other new class job'ish class could be AppCacheLoaderJob (akin to > AppCacheURLRequestJob) > My suggestion was because one subclass is implemented with net::URLRequest and it's named AppCacheURLRequest, while the other one is implemented with mojom::URLLoader so AppCacheURLLoader seemed the consistent choice.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Reduce/Remove URLRequest dependencies from AppCacheRequestHandler The AppCacheRequestHandler class provides functionality for serving network requests from the AppCache if applicable. This class is instantiated by the AppCacheInterceptor when a network request is initiated. Its lifetime depends on the associated URLRequest. The plan is to reuse this class in the network service world where we won't be intercepting network requests in the browser process. This effectively means that there won't be any URLRequest as well To achieve this the proposal is to provide an abstraction for a request called AppCachRequest. This class will have two subclasses at the moment. AppCacheURLRequest and AppCacheURLLoader. The AppCacheURLRequest class will be used by the current network implementation which relies on intercepting n/w requests. The AppCacheURLLoader class will be used by the network service mojo implementation. Next step is to provide an abstraction for the AppCacheURLRequestJob class. BUG=715632 TBR=jam ========== to ========== Reduce/Remove URLRequest dependencies from AppCacheRequestHandler The AppCacheRequestHandler class provides functionality for serving network requests from the AppCache if applicable. This class is instantiated by the AppCacheInterceptor when a network request is initiated. Its lifetime depends on the associated URLRequest. The plan is to reuse this class in the network service world where we won't be intercepting network requests in the browser process. This effectively means that there won't be any URLRequest as well To achieve this the proposal is to provide an abstraction for a request called AppCachRequest. This class will have two subclasses at the moment. AppCacheURLRequest and AppCacheURLLoaderRequest. The AppCacheURLRequest class will be used by the current network implementation which relies on intercepting n/w requests. The AppCacheURLLoaderRequest class will be used by the network service mojo implementation. Next step is to provide an abstraction for the AppCacheURLRequestJob class. BUG=715632 TBR=jam ==========
On 2017/05/03 03:54:45, jam wrote: > https://codereview.chromium.org/2848493007/diff/60001/content/browser/appcach... > File content/browser/appcache/appcache_url_loader.h (right): > > https://codereview.chromium.org/2848493007/diff/60001/content/browser/appcach... > content/browser/appcache/appcache_url_loader.h:15: class CONTENT_EXPORT > AppCacheURLLoader : public AppCacheRequest { > On 2017/05/03 02:32:12, michaeln wrote: > > Sorry for the naming nit, but this isn't the 'loader' either? This should be > > some kind of 'request'. > > > > maybe call this one AppCacheLoaderRequest and the other is AppCacheURLReqeust > > > > // some auto-chromium-class-name-generating :) > > AppCacheLoaderRequest (this one might be ok) > > AppCacheURLLoadRequest > > AppCacheResourceLoaderRequest > > AppCacheLoadRequest > > > > and the other new class job'ish class could be AppCacheLoaderJob (akin to > > AppCacheURLRequestJob) > > > > My suggestion was because one subclass is implemented with net::URLRequest and > it's named AppCacheURLRequest, while the other one is implemented with > mojom::URLLoader so AppCacheURLLoader seemed the consistent choice. Thanks guys. Renamed to AppCacheURLLoaderRequest :)
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org Link to the patchset: https://codereview.chromium.org/2848493007/#ps100001 (title: "format changes")
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 jam@chromium.org
sorry for the noise, I ran into Michael and he explained his reasoning. probably best to go back to appcache_resource_request.h as the first patchset. I thought that this class would be the one implementing URLLoader interface in the future but Michael corrected me that it would be in a different class. So the original name is best.
So long as the handler takes as input some kind of 'request' that describes what resource to retrieve, i'm happy. https://codereview.chromium.org/2848493007/diff/100001/content/browser/appcac... File content/browser/appcache/appcache_request_handler.cc (right): https://codereview.chromium.org/2848493007/diff/100001/content/browser/appcac... content/browser/appcache/appcache_request_handler.cc:185: return job.release(); The 'request' and the 'job' objects should be two different objects. https://codereview.chromium.org/2848493007/diff/100001/content/browser/appcac... File content/browser/appcache/appcache_request_handler.h (right): https://codereview.chromium.org/2848493007/diff/100001/content/browser/appcac... content/browser/appcache/appcache_request_handler.h:79: AppCacheRequest* request); The 'handler' takes as input a request object that describes what resource to retrieve. And it produces as output objects that can retrieve resources from the appcache's diskcache, we're calling those objects 'jobs'.
https://codereview.chromium.org/2848493007/diff/100001/content/browser/appcac... File content/browser/appcache/appcache_request_handler.cc (right): https://codereview.chromium.org/2848493007/diff/100001/content/browser/appcac... content/browser/appcache/appcache_request_handler.cc:185: return job.release(); On 2017/05/03 20:05:47, michaeln wrote: > The 'request' and the 'job' objects should be two different objects. > > Yes https://codereview.chromium.org/2848493007/diff/100001/content/browser/appcac... File content/browser/appcache/appcache_request_handler.h (right): https://codereview.chromium.org/2848493007/diff/100001/content/browser/appcac... content/browser/appcache/appcache_request_handler.h:79: AppCacheRequest* request); On 2017/05/03 20:05:47, michaeln wrote: > The 'handler' takes as input a request object that describes what resource to > retrieve. And it produces as output objects that can retrieve resources from the > appcache's diskcache, we're calling those objects 'jobs'. Noted. Thanks
The CQ bit was checked by ananta@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Request thing being a request makes better sense to me, so cool. (lgtm/2, fwiw) On 2017/05/03 02:20:23, michaeln wrote: > On 2017/05/03 01:28:21, kinuko wrote: > > drive-by, just wanted to see how appcache side's going. (Thanks Michael for > > cc-ing) > > > > Interesting so you'll be reusing existing URLRequestJob related code too? For > > that part for SW we'll be probably doing it differently as doing it in the > > renderer will make enough amount of differences there. > > We're planning to reuse the AppCacheRequestHandler and the > AppCacheResponseReader, and the URLRequestJob derivative is cut out, but > something that wraps the 'reader' and has a 'Start' method for the 'handler' to > call will be in. > > Basically factor out all dependencies on net::URLRequest constructs and > otherwise don't change a thing, at least for now. We think that makes sense to > reuse the max amount of code while they have to co-exist. Sounds reasonable. > Eventually, for subresource loads, pushing the contents of class AppCache (map > of urls to respsonseids, list of namespaces) into the renderer and having the > renderer decide whether to use the network service or load a resource from the > appcache might make sense. I think that's may be more in line with the > serviceworker plans. Could we chat a bit about the rough design? I'm trying to capture how these interaction should look in this new world. I'll be separately sending you an email/or talking to you on chat.
On 2017/05/03 22:05:47, kinuko wrote: > Request thing being a request makes better sense to me, so cool. (lgtm/2, fwiw) > > On 2017/05/03 02:20:23, michaeln wrote: > > On 2017/05/03 01:28:21, kinuko wrote: > > > drive-by, just wanted to see how appcache side's going. (Thanks Michael for > > > cc-ing) > > > > > > Interesting so you'll be reusing existing URLRequestJob related code too? > For > > > that part for SW we'll be probably doing it differently as doing it in the > > > renderer will make enough amount of differences there. > > > > We're planning to reuse the AppCacheRequestHandler and the > > AppCacheResponseReader, and the URLRequestJob derivative is cut out, but > > something that wraps the 'reader' and has a 'Start' method for the 'handler' > to > > call will be in. > > > > Basically factor out all dependencies on net::URLRequest constructs and > > otherwise don't change a thing, at least for now. We think that makes sense to > > reuse the max amount of code while they have to co-exist. > > Sounds reasonable. > > > Eventually, for subresource loads, pushing the contents of class AppCache (map > > of urls to respsonseids, list of namespaces) into the renderer and having the > > renderer decide whether to use the network service or load a resource from the > > appcache might make sense. I think that's may be more in line with the > > serviceworker plans. > > Could we chat a bit about the rough design? I'm trying to capture how these > interaction should look in this new world. I'll be separately sending you an > email/or talking to you on chat. (will read the design doc first :))
On 2017/05/03 22:07:18, kinuko wrote: > On 2017/05/03 22:05:47, kinuko wrote: > > Request thing being a request makes better sense to me, so cool. (lgtm/2, > fwiw) > > > > On 2017/05/03 02:20:23, michaeln wrote: > > > On 2017/05/03 01:28:21, kinuko wrote: > > > > drive-by, just wanted to see how appcache side's going. (Thanks Michael > for > > > > cc-ing) > > > > > > > > Interesting so you'll be reusing existing URLRequestJob related code too? > > For > > > > that part for SW we'll be probably doing it differently as doing it in the > > > > renderer will make enough amount of differences there. > > > > > > We're planning to reuse the AppCacheRequestHandler and the > > > AppCacheResponseReader, and the URLRequestJob derivative is cut out, but > > > something that wraps the 'reader' and has a 'Start' method for the 'handler' > > to > > > call will be in. > > > > > > Basically factor out all dependencies on net::URLRequest constructs and > > > otherwise don't change a thing, at least for now. We think that makes sense > to > > > reuse the max amount of code while they have to co-exist. > > > > Sounds reasonable. > > > > > Eventually, for subresource loads, pushing the contents of class AppCache > (map > > > of urls to respsonseids, list of namespaces) into the renderer and having > the > > > renderer decide whether to use the network service or load a resource from > the > > > appcache might make sense. I think that's may be more in line with the > > > serviceworker plans. > > > > Could we chat a bit about the rough design? I'm trying to capture how these > > interaction should look in this new world. I'll be separately sending you an > > email/or talking to you on chat. > > (will read the design doc first :)) Sounds good. We can discuss offline or in a meeting if that works.
I'll be out of town for the next couple weeks (till 5/17). I'll bring a laptop with me, but I'm not going to be around much. > > (will read the design doc first :)) > > Sounds good. We can discuss offline or in a meeting if that works.
Gotcha. I'll be in town on May 16-19 (hopefully, I need to confirm the final status from my travel agency yet), so I hope we can chat after that then! (Will likely send some emails to you/ananta before then too, but no need to be pressured to reply soon) On Thu, May 4, 2017 at 7:54 AM, <michaeln@chromium.org> wrote: > I'll be out of town for the next couple weeks (till 5/17). I'll bring a > laptop > with me, but I'm not going to be around much. > > > > (will read the design doc first :)) > > > > Sounds good. We can discuss offline or in a meeting if that works. > > > > https://codereview.chromium.org/2848493007/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ananta@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...
On 2017/05/03 23:48:51, kinuko wrote: > Gotcha. I'll be in town on May 16-19 (hopefully, I need to confirm the > final status from my travel agency yet), so I hope we can chat after that > then! Great, I'll see you then then :)
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 ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2848493007/#ps120001 (title: "Use unique_ptr to make ownership of the AppCacheRequest class explicit")
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": 1493866971455620,
"parent_rev": "2d7e67c3c84d2f11e12d34bdb71fa1b2153e38a5", "commit_rev":
"4e8a529c38afa4770486d8033cb37af0d6caf311"}
Message was sent while issue was closed.
Description was changed from ========== Reduce/Remove URLRequest dependencies from AppCacheRequestHandler The AppCacheRequestHandler class provides functionality for serving network requests from the AppCache if applicable. This class is instantiated by the AppCacheInterceptor when a network request is initiated. Its lifetime depends on the associated URLRequest. The plan is to reuse this class in the network service world where we won't be intercepting network requests in the browser process. This effectively means that there won't be any URLRequest as well To achieve this the proposal is to provide an abstraction for a request called AppCachRequest. This class will have two subclasses at the moment. AppCacheURLRequest and AppCacheURLLoaderRequest. The AppCacheURLRequest class will be used by the current network implementation which relies on intercepting n/w requests. The AppCacheURLLoaderRequest class will be used by the network service mojo implementation. Next step is to provide an abstraction for the AppCacheURLRequestJob class. BUG=715632 TBR=jam ========== to ========== Reduce/Remove URLRequest dependencies from AppCacheRequestHandler The AppCacheRequestHandler class provides functionality for serving network requests from the AppCache if applicable. This class is instantiated by the AppCacheInterceptor when a network request is initiated. Its lifetime depends on the associated URLRequest. The plan is to reuse this class in the network service world where we won't be intercepting network requests in the browser process. This effectively means that there won't be any URLRequest as well To achieve this the proposal is to provide an abstraction for a request called AppCachRequest. This class will have two subclasses at the moment. AppCacheURLRequest and AppCacheURLLoaderRequest. The AppCacheURLRequest class will be used by the current network implementation which relies on intercepting n/w requests. The AppCacheURLLoaderRequest class will be used by the network service mojo implementation. Next step is to provide an abstraction for the AppCacheURLRequestJob class. BUG=715632 TBR=jam Review-Url: https://codereview.chromium.org/2848493007 Cr-Commit-Position: refs/heads/master@{#469254} Committed: https://chromium.googlesource.com/chromium/src/+/4e8a529c38afa4770486d8033cb3... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/4e8a529c38afa4770486d8033cb3... |
