Add LoadState reporting to the mojo proxy resolver.

net/proxy/mojo_proxy_resolver_impl.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/proxy/mojo_proxy_resolver_impl.h"
 
 #include "base/stl_util.h"
 #include "mojo/common/common_type_converters.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/proxy/mojo_type_converters.h"
 #include "net/proxy/proxy_info.h"
 #include "net/proxy/proxy_resolver.h"
 #include "net/proxy/proxy_resolver_script_data.h"
 
 namespace net {
 
 class MojoProxyResolverImpl::Job : public mojo::ErrorHandler {
  public:
   Job(interfaces::ProxyResolverRequestClientPtr client,
       MojoProxyResolverImpl* resolver,
       const GURL& url);
   ~Job() override;
 
   void Start();
 
+  // Invoked when the LoadState for this job changes.
+  void LoadStateChanged(LoadState load_state);
+
  private:
   // mojo::ErrorHandler override.
   // This is invoked in response to the client disconnecting, indicating
   // cancellation.
   void OnConnectionError() override;
 
   void GetProxyDone(int error);
 
   MojoProxyResolverImpl* resolver_;
 
@@ skipping 22 matching lines @@
     : resolver_(resolver.Pass()) {
   DCHECK(resolver_->expects_pac_bytes());
 }
 
 MojoProxyResolverImpl::~MojoProxyResolverImpl() {
   if (!set_pac_script_requests_.empty())
     resolver_->CancelSetPacScript();
   STLDeleteElements(&resolve_jobs_);
 }
 
+void MojoProxyResolverImpl::LoadStateChanged(
+    net::ProxyResolver::RequestHandle handle,
+    LoadState load_state) {
+  auto it = request_handle_to_job_.find(handle);
+  if (it == request_handle_to_job_.end())
+    return;
+
+  it->second->LoadStateChanged(load_state);
+}
+
 void MojoProxyResolverImpl::SetPacScript(
     const mojo::String& data,
     const mojo::Callback<void(int32_t)>& callback) {
   DVLOG(1) << "SetPacScript(" << data << ")";
   set_pac_script_requests_.push(
       SetPacScriptRequest(ProxyResolverScriptData::FromUTF8(data), callback));
   if (set_pac_script_requests_.size() == 1)
     StartSetPacScript();
 }
 
 void MojoProxyResolverImpl::GetProxyForUrl(
     const mojo::String& url,
     interfaces::ProxyResolverRequestClientPtr client) {
   DVLOG(1) << "GetProxyForUrl(" << url << ")";
   Job* job = new Job(client.Pass(), this, url.To<GURL>());
   bool inserted = resolve_jobs_.insert(job).second;
   DCHECK(inserted);
   job->Start();
 }
 
-void MojoProxyResolverImpl::DeleteJob(Job* job) {
+void MojoProxyResolverImpl::DeleteJob(
+    Job* job,
+    net::ProxyResolver::RequestHandle handle) {

[eroman, 2015/02/25 00:08:05]

Does this need another parameter? the handle is already part of the Job

[Sam McNally, 2015/02/26 04:00:00]

Done.

+  if (handle)
+    request_handle_to_job_.erase(handle);
   size_t num_erased = resolve_jobs_.erase(job);
   DCHECK(num_erased);
   delete job;
 }
 
 void MojoProxyResolverImpl::StartSetPacScript() {
   DCHECK(!set_pac_script_requests_.empty());
   int result = resolver_->SetPacScript(
       set_pac_script_requests_.front().script_data,
       base::Bind(&MojoProxyResolverImpl::SetPacScriptDone,
@@ skipping 28 matching lines @@
 
 void MojoProxyResolverImpl::Job::Start() {
   int result = resolver_->resolver_->GetProxyForURL(
       url_, &result_, base::Bind(&Job::GetProxyDone, base::Unretained(this)),
       &request_handle_, BoundNetLog());
   if (result != ERR_IO_PENDING) {
     GetProxyDone(result);
     return;
   }
   client_.set_error_handler(this);
+  resolver_->request_handle_to_job_.insert(
+      std::make_pair(request_handle_, this));
+}
+
+void MojoProxyResolverImpl::Job::LoadStateChanged(LoadState load_state) {
+  client_->LoadStateChanged(load_state);

[eroman, 2015/02/25 00:08:05]

What is the implication of calling this? Other callbacks on the
TracingProxyResolver are resilient to cancellation/deletion happening while
dispatching the callback, but it doesn't look like that is the case for this
loadstate change one.

What may client->LoadStateChanged() do in response?

[Anand Mistry (off Chromium), 2015/02/25 04:42:27]

As an optimisation, I wonder if we should try to coalesce state changes. The
case I'm thinking of is when a DNS resolution finishes. Then, one of two cases
is likely to happen:
1. The script executes for a short amount of time and fires another DNS request.
It's likely that the script is only executing for 10s of us before starting the
next request. In that case, the state changes and then changes back within a
very short amount of time.
2. The script executes for a short amount of time and finishes, returning the
result. In this case, the state is changed, but since the request has finished,
the state is then no longer valid.

In both cases, the intermediate state is unlikely to be observed. So instead of
changing the state and sending messages for updates, maybe set a 1ms timer
before sending the state change message, and cancel that timer if one of the two
events happen. That way, sending the Mojo messages is avoided entirely for these
cases. My suspicion is that case #2 is likely to be very common.

[Sam McNally, 2015/02/26 04:00:00]

client_ is the mojo binding so it only sends a message to the
ProxyResolverMojo::Job. Cancellation is signaled by closing the message pipe so
if the Job has been cancelled on the other end but this end hasn't noticed yet,
the message will be discarded.

 }
 
 void MojoProxyResolverImpl::Job::GetProxyDone(int error) {
-  request_handle_ = nullptr;
   DVLOG(1) << "GetProxyForUrl(" << url_ << ") finished with error " << error
            << ". " << result_.proxy_list().size() << " Proxies returned:";
   for (const auto& proxy : result_.proxy_list().GetAll()) {
     DVLOG(1) << proxy.ToURI();
   }
   mojo::Array<interfaces::ProxyServerPtr> result;
   if (error == OK) {
     result = mojo::Array<interfaces::ProxyServerPtr>::From(
         result_.proxy_list().GetAll());
   }
   client_->ReportResult(error, result.Pass());
-  resolver_->DeleteJob(this);
+  auto request_handle = request_handle_;
+  request_handle_ = nullptr;
+  resolver_->DeleteJob(this, request_handle);
 }
 
 void MojoProxyResolverImpl::Job::OnConnectionError() {
-  resolver_->DeleteJob(this);
+  resolver_->DeleteJob(this, request_handle_);
 }
 
 MojoProxyResolverImpl::SetPacScriptRequest::SetPacScriptRequest(
     const scoped_refptr<ProxyResolverScriptData>& script_data,
     const mojo::Callback<void(int32_t)>& callback)
     : script_data(script_data), callback(callback) {
 }
 
 }  // namespace net