Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2919313004:
Get rid of URLLoaderFactory in browser-side case (Closed)
|
Created:
3 years, 6 months ago by kinuko Modified:
3 years, 6 months ago CC:
chromium-reviews, asanka, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, kinuko+watch, jsbell+serviceworker_chromium.org, tzik, abarth-chromium, darin-cc_chromium.org, loading-reviews_chromium.org, blink-worker-reviews_chromium.org, nhiroki, rdsmith+watch_chromium.org, michaeln, shimazu+serviceworker_chromium.org, serviceworker-reviews, Aaron Boodman, kinuko+serviceworker, horo+watch_chromium.org, darin (slow to review), mmenke Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionGet rid of URLLoaderFactory in browser-side case
BUG=729503
Review-Url: https://codereview.chromium.org/2919313004
Cr-Commit-Position: refs/heads/master@{#477568}
Committed: https://chromium.googlesource.com/chromium/src/+/d8b13e21c0c676d7f8e075236948983d09b42bbe
Patch Set 1 #Patch Set 2 : . #
Total comments: 5
Patch Set 3 : . #
Total comments: 5
Patch Set 4 : . #
Total comments: 1
Messages
Total messages: 42 (22 generated)
The CQ bit was checked by kinuko@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...
Description was changed from ========== Get rid of URLLoaderFactory in browser-side case and add URLLoader::Start BUG=729503 ========== to ========== Get rid of URLLoaderFactory in browser-side case and add URLLoader::Start BUG=729503 ==========
kinuko@chromium.org changed reviewers: + jam@chromium.org, yhirano@chromium.org
The CQ bit was checked by kinuko@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...
https://codereview.chromium.org/2919313004/diff/20001/content/common/url_load... File content/common/url_loader.mojom (right): https://codereview.chromium.org/2919313004/diff/20001/content/common/url_load... content/common/url_loader.mojom:40: Start(associated URLLoader& loader, URLLoaderClient client); Having a method like this in mojom feels a bit weird, while for C++ use cases this makes things handier. What do you think? Any alternative suggestions also really welcome.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2919313004/diff/20001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2919313004/diff/20001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:33: virtual void MaybeCreateLoader(const ResourceRequest& resource_request, I'm probably missing something, but why can't this interface always return null or a started URLLoader?
michaeln@chromium.org changed reviewers: + michaeln@chromium.org
https://codereview.chromium.org/2919313004/diff/20001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2919313004/diff/20001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:33: virtual void MaybeCreateLoader(const ResourceRequest& resource_request, On 2017/06/05 19:39:24, jam wrote: > I'm probably missing something, but why can't this interface always return null > or a started URLLoader? There is a use case for a non-started loaders. Looking forward to appcache redirect and response fallback handling, an appcache fallback should only be used if certain conditions arise. Upon return from this function, we know if there's a fallback resource or not. We could return a loader that has not be started and a bit that indicates its a fallback. The caller would only start the fallback loader if it was needed.
https://codereview.chromium.org/2919313004/diff/20001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2919313004/diff/20001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:33: virtual void MaybeCreateLoader(const ResourceRequest& resource_request, On 2017/06/05 21:52:39, michaeln wrote: > On 2017/06/05 19:39:24, jam wrote: > > I'm probably missing something, but why can't this interface always return > null > > or a started URLLoader? > > There is a use case for a non-started loaders. > > Looking forward to appcache redirect and response fallback handling, an appcache > fallback should only be used if certain conditions arise. Upon return from this > function, we know if there's a fallback resource or not. We could return a > loader that has not be started and a bit that indicates its a fallback. The > caller would only start the fallback loader if it was needed. This could also be implemented by using a callback that creates a started loader? i.e. i'm just wondering if we absolutely need this new interface in the mojo interface or if we can avoid it.
https://codereview.chromium.org/2919313004/diff/20001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2919313004/diff/20001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:33: virtual void MaybeCreateLoader(const ResourceRequest& resource_request, On 2017/06/05 19:39:24, jam wrote: > I'm probably missing something, but why can't this interface always return null > or a started URLLoader? (I'm guessing I'm missing the obvious reason here, or else Kinuko would have just done this). If we do need a factory, can it be a synchronous C++ factory as we briefly brainstormed in-person? That gets rid of the pain points of using mojom::URLLoaderFactory (no extra hops, no need to worry about associated pipes that we can't forward, no need to worry about the Sync call).
On 2017/06/06 00:55:40, jam wrote: > https://codereview.chromium.org/2919313004/diff/20001/content/browser/loader/... > File content/browser/loader/url_loader_request_handler.h (right): > > https://codereview.chromium.org/2919313004/diff/20001/content/browser/loader/... > content/browser/loader/url_loader_request_handler.h:33: virtual void > MaybeCreateLoader(const ResourceRequest& resource_request, > On 2017/06/05 19:39:24, jam wrote: > > I'm probably missing something, but why can't this interface always return > null > > or a started URLLoader? > > (I'm guessing I'm missing the obvious reason here, or else Kinuko would have > just done this). > > If we do need a factory, can it be a synchronous C++ factory as we briefly > brainstormed in-person? That gets rid of the pain points of using > mojom::URLLoaderFactory (no extra hops, no need to worry about associated pipes > that we can't forward, no need to worry about the Sync call). Yep it's possible to achieve the same by having a C++ interface around that as well, I just did it on mojo::URLLoader as it looked simplest, but I can see its downside as well (i.e. another confusion, as is seen in this conversation). Let me do something else purely in C++ side and re-upload, so you can see how it'd look. Thanks!
Description was changed from ========== Get rid of URLLoaderFactory in browser-side case and add URLLoader::Start BUG=729503 ========== to ========== Get rid of URLLoaderFactory in browser-side case BUG=729503 ==========
The CQ bit was checked by kinuko@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 checked by kinuko@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Got rid of additional mojo method, and simply replaced the LoaderFactory with an abstract callback. I think this is probably suffice for our use cases and still gives us some flexibility and simplicity-- wdyt?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/2919313004/diff/60001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2919313004/diff/60001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:19: base::OnceCallback<void(mojom::URLLoaderAssociatedRequest request, actually, can this now be URLLoaderRequest instead of URLLoaderAssociatedRequest?
On 2017/06/06 17:45:32, jam wrote: > https://codereview.chromium.org/2919313004/diff/60001/content/browser/loader/... > File content/browser/loader/url_loader_request_handler.h (right): > > https://codereview.chromium.org/2919313004/diff/60001/content/browser/loader/... > content/browser/loader/url_loader_request_handler.h:19: > base::OnceCallback<void(mojom::URLLoaderAssociatedRequest request, > actually, can this now be URLLoaderRequest instead of > URLLoaderAssociatedRequest? Hm, yeah I think that's possible.
On 2017/06/06 23:11:59, kinuko wrote: > On 2017/06/06 17:45:32, jam wrote: > > > https://codereview.chromium.org/2919313004/diff/60001/content/browser/loader/... > > File content/browser/loader/url_loader_request_handler.h (right): > > > > > https://codereview.chromium.org/2919313004/diff/60001/content/browser/loader/... > > content/browser/loader/url_loader_request_handler.h:19: > > base::OnceCallback<void(mojom::URLLoaderAssociatedRequest request, > > actually, can this now be URLLoaderRequest instead of > > URLLoaderAssociatedRequest? > > Hm, yeah I think that's possible. Ok that'd be preferable since associated pointers can't be unbound and sent around. Ananta ran into this where the appcache handler had to always be bound to the URLLoaderClient pipe even when appcache wasn't being used.
lgtm https://codereview.chromium.org/2919313004/diff/60001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2919313004/diff/60001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:31: // Calls non-null |callback| if this handler can handle the request, calls "Calls |callback| with a non-null StartLoaderCallback"
https://codereview.chromium.org/2919313004/diff/60001/content/browser/loader/... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2919313004/diff/60001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:147: void MaybeStartLoader(StartLoaderCallback start_loader_callback) { it's turtles all the way down! MaybeCreateLoader cb_MaybeStartLoader cb_StartLoader finally create and start a loader works for me, lgtm 2
(Rebasing, inline response only yet) https://codereview.chromium.org/2919313004/diff/60001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2919313004/diff/60001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:19: base::OnceCallback<void(mojom::URLLoaderAssociatedRequest request, On 2017/06/06 17:45:31, jam wrote: > actually, can this now be URLLoaderRequest instead of > URLLoaderAssociatedRequest? Hmm, down the chain we still pass this request to the network service's factory so it seems to need some tweak to achieve that. Let me land this as is for now and see if it can be done separately. https://codereview.chromium.org/2919313004/diff/60001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:31: // Calls non-null |callback| if this handler can handle the request, calls On 2017/06/07 01:35:10, yhirano wrote: > "Calls |callback| with a non-null StartLoaderCallback" Done. (Will be updated in the patch to be uploaded)
On 2017/06/07 02:28:22, michaeln wrote:
> it's turtles all the way down!
_
.-./*)
_/___\/
U U
Updated except for associated part, landing this for now.
The CQ bit was checked by kinuko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org, jam@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2919313004/#ps80001 (title: ".")
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": 80001, "attempt_start_ts": 1496812824351670,
"parent_rev": "3ff6ed64df1419714cfa65f287f107a934f519f4", "commit_rev":
"d8b13e21c0c676d7f8e075236948983d09b42bbe"}
Message was sent while issue was closed.
Description was changed from ========== Get rid of URLLoaderFactory in browser-side case BUG=729503 ========== to ========== Get rid of URLLoaderFactory in browser-side case BUG=729503 Review-Url: https://codereview.chromium.org/2919313004 Cr-Commit-Position: refs/heads/master@{#477568} Committed: https://chromium.googlesource.com/chromium/src/+/d8b13e21c0c676d7f8e075236948... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/d8b13e21c0c676d7f8e075236948...
Message was sent while issue was closed.
yzshen@chromium.org changed reviewers: + yzshen@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2919313004/diff/80001/content/browser/loader/... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2919313004/diff/80001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:153: .Run(std::move(url_loader_request_), I noticed that this still uses associated URLLoader. Because it is not passed over a message pipe (i.e., a non-associated interface or an already-associated interface), it is in a pending-association state -- it doesn't know on which message pipe to send messages. In this state, calling the associated URLLoader interface will crash. We will need to use something like mojo::MakeIsolatedRequest() to create a dedicated message pipe for this associated interface. One annoying thing is that the corresponding URLLoaderAssociatedPtr belongs to NavigationURLLoaderNetworkService and in use now. So mojo::MakeIsolatedRequest() doesn't fit. I should be able to fix this with my browser-side SB check CL. Alternatively, the problem will go away if we switch to non-associated URLLoader.
Message was sent while issue was closed.
On 2017/06/07 23:28:25, yzshen1 wrote: > https://codereview.chromium.org/2919313004/diff/80001/content/browser/loader/... > File content/browser/loader/navigation_url_loader_network_service.cc (right): > > https://codereview.chromium.org/2919313004/diff/80001/content/browser/loader/... > content/browser/loader/navigation_url_loader_network_service.cc:153: > .Run(std::move(url_loader_request_), > I noticed that this still uses associated URLLoader. Because it is not passed > over a message pipe (i.e., a non-associated interface or an already-associated > interface), it is in a pending-association state -- it doesn't know on which > message pipe to send messages. In this state, calling the associated URLLoader > interface will crash. > > We will need to use something like mojo::MakeIsolatedRequest() to create a > dedicated message pipe for this associated interface. One annoying thing is that > the corresponding URLLoaderAssociatedPtr belongs to > NavigationURLLoaderNetworkService and in use now. So mojo::MakeIsolatedRequest() > doesn't fit. > > I should be able to fix this with my browser-side SB check CL. Alternatively, > the problem will go away if we switch to non-associated URLLoader. Hi Yuzhu, I have a patch that tries to work around this https://codereview.chromium.org/2931623002/, would that work? I'm adding you as a reviewer to the change (I'm not super familiar with how associated pipe works). |
