|
|
Created:
5 years, 10 months ago by Anand Mistry (off Chromium) Modified:
5 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Mojo interfaces for out-of-process proxy resolver.
See:
https://docs.google.com/a/chromium.org/document/d/1n5hr4KJhZl2A4MicTfmyiHPdiKp7kmUoWXnRBN8SrZE/edit#
BUG=11746
Committed: https://crrev.com/073e42bb7587b90594e5799fb6a4afe08a1e1561
Cr-Commit-Position: refs/heads/master@{#314509}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Review comments. #
Total comments: 11
Patch Set 3 : More review comments. #Patch Set 4 : Remove LoadState enum. #
Messages
Total messages: 16 (4 generated)
amistry@chromium.org changed reviewers: + eroman@chromium.org, rdsmith@chromium.org, sammc@chromium.org
You might be wondering why I'm sending this out even though nothing depends on it. Well, sammc@ and I intend to implement different parts of this independently, and all parts depend on these interfaces. Submitting these now makes it much easier for us to make progress without depending on each other. As for the interfaces themselves, consider them a first-cut. The goal here is to have a min-viable working out-of-process implementation. For this reason, NetLog is missing since it's not necessary to get something working.
https://codereview.chromium.org/900433003/diff/1/net/interfaces/host_resolver... File net/interfaces/host_resolver_service.mojom (right): https://codereview.chromium.org/900433003/diff/1/net/interfaces/host_resolver... net/interfaces/host_resolver_service.mojom:19: ADDRESS_FAMILY_IPV6, I doubt this is actually going to be needed. That said no harm in having it here. https://codereview.chromium.org/900433003/diff/1/net/interfaces/host_resolver... net/interfaces/host_resolver_service.mojom:23: struct RequestInfo { RequestInfo is fairly vague. How about HostResolverRequestInfo? Is this namespaced to host_resolver_service or global for net.interfaces? https://codereview.chromium.org/900433003/diff/1/net/interfaces/host_resolver... net/interfaces/host_resolver_service.mojom:41: string canonical_name; I don't believe canonical_name is needed. https://codereview.chromium.org/900433003/diff/1/net/interfaces/host_resolver... net/interfaces/host_resolver_service.mojom:52: interface HostResolveRequestClient { I would epected HostResolverRequestClient rather than HostResolveRequestClient. https://codereview.chromium.org/900433003/diff/1/net/interfaces/proxy_resolve... File net/interfaces/proxy_resolver_service.mojom (right): https://codereview.chromium.org/900433003/diff/1/net/interfaces/proxy_resolve... net/interfaces/proxy_resolver_service.mojom:22: // Use a ProxyResolveRequestClient instead of returning a result so we can I see, you are using "ResolveClient" throughout. I am still in favor of "ResolverClient" so less changes from the service name. Or alternately naming it similar to the method that starts it "GetProxyForUrlClient" or something. Just for more consistency in deriving the names. https://codereview.chromium.org/900433003/diff/1/net/interfaces/proxy_resolve... net/interfaces/proxy_resolver_service.mojom:30: ReportResult(int32 error, string? pac_string); I can certainly understand wanting to return a PAC string as the result, and don't disagree with this decision for a first version. However long term I think it would be better for the result to be a structure ProxyList (a sequence of "scheme", "host", "port"). This way the parsing of the PAC string can also be done in the sandboxed process. https://codereview.chromium.org/900433003/diff/1/net/interfaces/proxy_resolve... net/interfaces/proxy_resolver_service.mojom:31: UpdateLoadState(LoadState load_state); Can you give some details on how this will work? Currently if memory serves the load state is used for printing the status at the bottom of the page while loading. And is determined by polling and a heuristic to determine which state takes precedence. Do you want to track it accurately and hence have this callback invoked whenever it changes? It might be simpler to instead have a request to get the load state and not track it. https://codereview.chromium.org/900433003/diff/1/net/interfaces/proxy_resolve... net/interfaces/proxy_resolver_service.mojom:40: CreateResolver(ProxyResolverService& resolver, Forgive me for not being familiar with mojo (yet!) In C++ code at least non-const references are not idiomatic. What does this translate to for C++ code?
https://codereview.chromium.org/900433003/diff/1/net/interfaces/host_resolver... File net/interfaces/host_resolver_service.mojom (right): https://codereview.chromium.org/900433003/diff/1/net/interfaces/host_resolver... net/interfaces/host_resolver_service.mojom:19: ADDRESS_FAMILY_IPV6, On 2015/02/03 04:53:29, eroman wrote: > I doubt this is actually going to be needed. That said no harm in having it > here. Acknowledged. https://codereview.chromium.org/900433003/diff/1/net/interfaces/host_resolver... net/interfaces/host_resolver_service.mojom:23: struct RequestInfo { On 2015/02/03 04:53:29, eroman wrote: > RequestInfo is fairly vague. How about HostResolverRequestInfo? done. > > Is this namespaced to host_resolver_service or global for net.interfaces? global for net.interfaces. So this translates to net::interfaces::HostResolverRequestInfo. https://codereview.chromium.org/900433003/diff/1/net/interfaces/host_resolver... net/interfaces/host_resolver_service.mojom:41: string canonical_name; On 2015/02/03 04:53:29, eroman wrote: > I don't believe canonical_name is needed. Removed. I saw the comment about crbug.com/126134, but it looks like this is still used in a few places. With this gone, does a new instance of AddressList need to call AddressList::SetDefaultCanonicalName()? https://codereview.chromium.org/900433003/diff/1/net/interfaces/host_resolver... net/interfaces/host_resolver_service.mojom:52: interface HostResolveRequestClient { On 2015/02/03 04:53:29, eroman wrote: > I would epected HostResolverRequestClient rather than HostResolveRequestClient. Done. https://codereview.chromium.org/900433003/diff/1/net/interfaces/proxy_resolve... File net/interfaces/proxy_resolver_service.mojom (right): https://codereview.chromium.org/900433003/diff/1/net/interfaces/proxy_resolve... net/interfaces/proxy_resolver_service.mojom:22: // Use a ProxyResolveRequestClient instead of returning a result so we can On 2015/02/03 04:53:29, eroman wrote: > I see, you are using "ResolveClient" throughout. > > I am still in favor of "ResolverClient" so less changes from the service name. > Or alternately naming it similar to the method that starts it > "GetProxyForUrlClient" or something. Just for more consistency in deriving the > names. Changed to "Resolver" everywhere. I agree it's better. https://codereview.chromium.org/900433003/diff/1/net/interfaces/proxy_resolve... net/interfaces/proxy_resolver_service.mojom:30: ReportResult(int32 error, string? pac_string); On 2015/02/03 04:53:29, eroman wrote: > I can certainly understand wanting to return a PAC string as the result, and > don't disagree with this decision for a first version. > > However long term I think it would be better for the result to be a structure > ProxyList (a sequence of "scheme", "host", "port"). > > This way the parsing of the PAC string can also be done in the sandboxed > process. Looks straight forward. Might as well change it right now. https://codereview.chromium.org/900433003/diff/1/net/interfaces/proxy_resolve... net/interfaces/proxy_resolver_service.mojom:31: UpdateLoadState(LoadState load_state); On 2015/02/03 04:53:29, eroman wrote: > Can you give some details on how this will work? > > Currently if memory serves the load state is used for printing the status at the > bottom of the page while loading. And is determined by polling and a heuristic > to determine which state takes precedence. > > Do you want to track it accurately and hence have this callback invoked whenever > it changes? So, I had an explanation on how this would work... until I took another look at the v8 tracing proxy implementation :(. It is possible to implement accurately by polling for state changes whenever a DNS request starts/ends (since we have no way of associating a proxy request with a DNS request). But that's O(n) on the number of pending proxy requests. Can I get by and not implement this, or just lie and always return RESOLVING_PROXY_FOR_URL? > It might be simpler to instead have a request to get the load state and not > track it. The problem there is ProxyResolver::GetLoadState() is synchronous, so we can't get it on demand. https://codereview.chromium.org/900433003/diff/1/net/interfaces/proxy_resolve... net/interfaces/proxy_resolver_service.mojom:40: CreateResolver(ProxyResolverService& resolver, On 2015/02/03 04:53:29, eroman wrote: > Forgive me for not being familiar with mojo (yet!) > > In C++ code at least non-const references are not idiomatic. What does this > translate to for C++ code? There are two ends to an interface, the client (calls methods) and service (implements the methods). When you first create an instance of an interface, both ends are in-process and you have to pass one of those ends to the other process. In every case so far where an interface is passed into a method, it's been passing the client side, so the service is local and the caller is being given it's side. In this case, the situation is reversed. The client is local and we're passing the service end to the other process to be connected. We could have done this like: CreateResolver(HostResolver hr, Observer ob) => (ProxyResolverService); This would create everything in the utility process and give us the client side. But this is asynchronous. Using & gives us the client side right away and lets us call GetProxyForUrl() right away.
https://codereview.chromium.org/900433003/diff/1/net/interfaces/proxy_resolve... File net/interfaces/proxy_resolver_service.mojom (right): https://codereview.chromium.org/900433003/diff/1/net/interfaces/proxy_resolve... net/interfaces/proxy_resolver_service.mojom:31: UpdateLoadState(LoadState load_state); On 2015/02/03 06:27:22, Anand Mistry wrote: > On 2015/02/03 04:53:29, eroman wrote: > > Can you give some details on how this will work? > > > > Currently if memory serves the load state is used for printing the status at > the > > bottom of the page while loading. And is determined by polling and a heuristic > > to determine which state takes precedence. > > > > Do you want to track it accurately and hence have this callback invoked > whenever > > it changes? > > So, I had an explanation on how this would work... until I took another look at > the v8 tracing proxy implementation :(. It is possible to implement accurately > by polling for state changes whenever a DNS request starts/ends (since we have > no way of associating a proxy request with a DNS request). But that's O(n) on > the number of pending proxy requests. Can I get by and not implement this, or > just lie and always return RESOLVING_PROXY_FOR_URL? > > > It might be simpler to instead have a request to get the load state and not > > track it. > > The problem there is ProxyResolver::GetLoadState() is synchronous, so we can't > get it on demand. I think we should punt on LoadState for now. https://codereview.chromium.org/900433003/diff/20001/net/interfaces/host_reso... File net/interfaces/host_resolver_service.mojom (right): https://codereview.chromium.org/900433003/diff/20001/net/interfaces/host_reso... net/interfaces/host_resolver_service.mojom:17: ADDRESS_FAMILY_UNSPECIFIED, Mojom-generated C++ enum values are automatically prefixed with the enum name. https://codereview.chromium.org/900433003/diff/20001/net/interfaces/host_reso... net/interfaces/host_resolver_service.mojom:43: interface HostResolverService { Can we call this HostResolver? https://codereview.chromium.org/900433003/diff/20001/net/interfaces/proxy_res... File net/interfaces/proxy_resolver_service.mojom (right): https://codereview.chromium.org/900433003/diff/20001/net/interfaces/proxy_res... net/interfaces/proxy_resolver_service.mojom:37: interface ProxyResolverService { And ProxyResolver? https://codereview.chromium.org/900433003/diff/20001/net/interfaces/proxy_res... net/interfaces/proxy_resolver_service.mojom:48: ReportResult(int32 error, array<ProxyScheme>? proxy_servers); ProxyServer.
https://codereview.chromium.org/900433003/diff/1/net/interfaces/host_resolver... File net/interfaces/host_resolver_service.mojom (right): https://codereview.chromium.org/900433003/diff/1/net/interfaces/host_resolver... net/interfaces/host_resolver_service.mojom:41: string canonical_name; On 2015/02/03 06:27:22, Anand Mistry wrote: > On 2015/02/03 04:53:29, eroman wrote: > > I don't believe canonical_name is needed. > > Removed. > > I saw the comment about crbug.com/126134, but it looks like this is still used > in a few places. With this gone, does a new instance of AddressList need to call > AddressList::SetDefaultCanonicalName()? I was thinking more in terms of "is this needed by proxy resolver". And the answer to that is "no". I would say keep cname out for now, since I don't think any clients will be needing it. https://codereview.chromium.org/900433003/diff/1/net/interfaces/proxy_resolve... File net/interfaces/proxy_resolver_service.mojom (right): https://codereview.chromium.org/900433003/diff/1/net/interfaces/proxy_resolve... net/interfaces/proxy_resolver_service.mojom:31: UpdateLoadState(LoadState load_state); On 2015/02/03 23:51:30, Sam McNally wrote: > On 2015/02/03 06:27:22, Anand Mistry wrote: > > On 2015/02/03 04:53:29, eroman wrote: > > > Can you give some details on how this will work? > > > > > > Currently if memory serves the load state is used for printing the status at > > the > > > bottom of the page while loading. And is determined by polling and a > heuristic > > > to determine which state takes precedence. > > > > > > Do you want to track it accurately and hence have this callback invoked > > whenever > > > it changes? > > > > So, I had an explanation on how this would work... until I took another look > at > > the v8 tracing proxy implementation :(. It is possible to implement accurately > > by polling for state changes whenever a DNS request starts/ends (since we have > > no way of associating a proxy request with a DNS request). But that's O(n) on > > the number of pending proxy requests. Can I get by and not implement this, or > > just lie and always return RESOLVING_PROXY_FOR_URL? > > > > > It might be simpler to instead have a request to get the load state and not > > > track it. > > > > The problem there is ProxyResolver::GetLoadState() is synchronous, so we can't > > get it on demand. > > I think we should punt on LoadState for now. Agreed (punt on LoadState for now), it can return a generic load state. We will however want it before turning this on by default. <history> The main reason for the load state was to distinguish between when the request is stalled executing javascript, vs when it is waiting for a network dependency (DNS). Since in our corp environment we would get hellish "resolving proxy..." and it wasn't clear that it was actually due to the PAC script doing synchronous DNS resolves. The load state surfaces this on the page more clearly by indicating it is a DNS request in proxy script. </history> https://codereview.chromium.org/900433003/diff/20001/net/interfaces/proxy_res... File net/interfaces/proxy_resolver_service.mojom (right): https://codereview.chromium.org/900433003/diff/20001/net/interfaces/proxy_res... net/interfaces/proxy_resolver_service.mojom:48: ReportResult(int32 error, array<ProxyScheme>? proxy_servers); On 2015/02/03 23:51:30, Sam McNally wrote: > ProxyServer. LGTM once this comment is addressed, and the LoadState stuff is removed. https://codereview.chromium.org/900433003/diff/20001/net/interfaces/proxy_res... net/interfaces/proxy_resolver_service.mojom:60: ProxyResolverErrorObserver error_observer); For simplicity you might also want to leave off ErrorObserver until later, since it isn't crucial for getting something up and running (as i recall it is just used for some extension API. if you don't report the errors noone will be the wiser :)
New patchsets have been uploaded after l-g-t-m from eroman@chromium.org
https://codereview.chromium.org/900433003/diff/1/net/interfaces/host_resolver... File net/interfaces/host_resolver_service.mojom (right): https://codereview.chromium.org/900433003/diff/1/net/interfaces/host_resolver... net/interfaces/host_resolver_service.mojom:41: string canonical_name; On 2015/02/04 01:25:19, eroman wrote: > On 2015/02/03 06:27:22, Anand Mistry wrote: > > On 2015/02/03 04:53:29, eroman wrote: > > > I don't believe canonical_name is needed. > > > > Removed. > > > > I saw the comment about crbug.com/126134, but it looks like this is still used > > in a few places. With this gone, does a new instance of AddressList need to > call > > AddressList::SetDefaultCanonicalName()? > > I was thinking more in terms of "is this needed by proxy resolver". And the > answer to that is "no". > I would say keep cname out for now, since I don't think any clients will be > needing it. Acknowledged. https://codereview.chromium.org/900433003/diff/20001/net/interfaces/host_reso... File net/interfaces/host_resolver_service.mojom (right): https://codereview.chromium.org/900433003/diff/20001/net/interfaces/host_reso... net/interfaces/host_resolver_service.mojom:17: ADDRESS_FAMILY_UNSPECIFIED, On 2015/02/03 23:51:30, Sam McNally wrote: > Mojom-generated C++ enum values are automatically prefixed with the enum name. Removed prefix. https://codereview.chromium.org/900433003/diff/20001/net/interfaces/host_reso... net/interfaces/host_resolver_service.mojom:43: interface HostResolverService { On 2015/02/03 23:51:30, Sam McNally wrote: > Can we call this HostResolver? As discussed, no. https://codereview.chromium.org/900433003/diff/20001/net/interfaces/proxy_res... File net/interfaces/proxy_resolver_service.mojom (right): https://codereview.chromium.org/900433003/diff/20001/net/interfaces/proxy_res... net/interfaces/proxy_resolver_service.mojom:37: interface ProxyResolverService { On 2015/02/03 23:51:30, Sam McNally wrote: > And ProxyResolver? Nope :P https://codereview.chromium.org/900433003/diff/20001/net/interfaces/proxy_res... net/interfaces/proxy_resolver_service.mojom:48: ReportResult(int32 error, array<ProxyScheme>? proxy_servers); On 2015/02/04 01:25:20, eroman wrote: > On 2015/02/03 23:51:30, Sam McNally wrote: > > ProxyServer. > > LGTM once this comment is addressed, and the LoadState stuff is removed. Done and removed LoadState. https://codereview.chromium.org/900433003/diff/20001/net/interfaces/proxy_res... net/interfaces/proxy_resolver_service.mojom:60: ProxyResolverErrorObserver error_observer); On 2015/02/04 01:25:20, eroman wrote: > For simplicity you might also want to leave off ErrorObserver until later, since > it isn't crucial for getting something up and running (as i recall it is just > used for some extension API. if you don't report the errors noone will be the > wiser :) Removed error observer and updated TODO.
lgtm
New patchsets have been uploaded after l-g-t-m from sammc@chromium.org
The CQ bit was checked by amistry@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/900433003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/073e42bb7587b90594e5799fb6a4afe08a1e1561 Cr-Commit-Position: refs/heads/master@{#314509}
Message was sent while issue was closed.
http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29... Broke device_unittests device_unittests SerialConnectionTest.Cancel
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/892373006/ by noel@chromium.org. The reason for reverting is: http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29... Broke device_unittests device_unittests SerialConnectionTest.Cancel it seems. . |