Upstream Android proxy config service.

net/proxy/proxy_config_service_android.h

+// Copyright (c) 2012 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.
+
+#ifndef NET_PROXY_PROXY_CONFIG_SERVICE_ANDROID_H_
+#define NET_PROXY_PROXY_CONFIG_SERVICE_ANDROID_H_
+#pragma once
+
+#include <string>
+
+#include "base/android/scoped_java_ref.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/observer_list.h"
+#include "net/base/net_export.h"
+#include "net/proxy/proxy_config.h"
+#include "net/proxy/proxy_config_service.h"
+
+namespace base {
+class MessageLoopProxy;
+}
+
+namespace net {
+
+class NET_EXPORT ProxyConfigServiceAndroid : public ProxyConfigService {
+ public:
+  // Delegate abstracts access to the platform.
+  // Owned by ProxyConfigServiceAndroid.
+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+    virtual void Start(ProxyConfigServiceAndroid* service) = 0;
+    virtual void Stop() = 0;
+    virtual std::string GetProperty(const std::string& property) = 0;

[Ryan Sleevi, 2012/05/07 17:12:20]

nit: Document what these methods do.

For example, with Start(), you're passing around a naked pointer. Is it an
ownership transferred pointer? It's not clear from just the header.

Similar, with GetProperty, if a property does not exist, what does it return? An
empty string? Is it valid for a property to be present but have an empty string?
If so, should they be distinguished?

[Philippe, 2012/05/09 11:48:40]

Good points.

Currently, ProxyConfigServiceAndroid does not use this distinction. In this
particular case, I don't really see how we can benefit from making the
distinction.

+  };
+
+  ProxyConfigServiceAndroid(
+      scoped_refptr<base::MessageLoopProxy> observer_loop);

[Ryan Sleevi, 2012/05/07 17:12:20]

Instead of using a MessageLoopProxy, I believe the new hotness is to pass around
a base::TaskRunner, of which a MLP derives from.

Also, this is a by-value copy, whereas given that it's ref-counted, it makes
more sense to either pass as a const-ref or a naked pointer. AIUI, only the
non-refcounted scoped classes (scoped_ptr/ptr_malloc/_array) support the C++11
move emulation.

Same with line 60

[Philippe, 2012/05/09 11:48:40]

Thanks for the tip :) I used SingleThreadTaskRunner so that we can keep calling
BelongsToCurrentThread().

I'm not sure I see your point about const-ref/naked pointer passing. I don't
take (unique) ownership of the pointer (which a unique_ptr or scoped_ptr with
move emulation would have  expressed nicely). I still need the guarantee that
the message loop proxy won't be deleted before me which implies the use of a
shared pointer. What am I missing?

While we are talking about ownership, is that common in the Chromium code base
to use a scoped_ptr (with Pass()) in place of a unique_ptr? I see that
scoped_ptr "supports move semantic" through Pass(). I'm just wondering if people
use it since it might be confusing to have a scoped_ptr used like a unique_ptr
knowing that scoped_ptr is also part of C++ 11.

[Ryan Sleevi, 2012/05/14 18:00:25]

Just pass the naked pointer and store a scoped_refptr<> as a member. Don't pass
a scoped_refptr<> by-val.

Passing it by-val just adds an extra AddRef()/Release() pair - first to copy it
into the temporary passed as the argument, second to pass it to the actual
stored data member. Passing the naked pointer but storing in a scoped_refptr<>
eliminates that by only calling AddRef() once - and while the pointer is still
valid.
We don't use C++11 in Chromium code, and likely won't for some time yet. As far
as unique_ptr<> vs scoped_ptr<>, scoped_ptr<> is used (almost? totally?)
exclusively, in part due to shared heritage in google3 land.

[Philippe, 2012/05/15 16:12:47]

I see now. I guess that I was not used to having the refcount part of the object
itself as opposed to the shared pointer (like std::shared_ptr). The good point
with that is that you can pass the object through a raw pointer without loosing
any refcount information.

+  virtual ~ProxyConfigServiceAndroid();
+
+  // Register JNI bindings.
+  static bool Init(JNIEnv* env);
+
+  // ProxyConfigService:
+  // Observer-related operations must be called on the observer thread.
+  virtual void AddObserver(Observer* observer) OVERRIDE;
+  virtual void RemoveObserver(Observer* observer) OVERRIDE;
+
+  virtual ConfigAvailability GetLatestProxyConfig(ProxyConfig* config) OVERRIDE;
+
+  void ProxySettingsChanged();
+
+  // Called from Java to signal that the proxy settings have changed.
+  void ProxySettingsChanged(JNIEnv*, jobject) { ProxySettingsChanged(); }
+
+ private:
+  friend class ProxyConfigServiceAndroidTest;
+
+  // For tests; takes ownership of |delegate|.
+  ProxyConfigServiceAndroid(scoped_refptr<base::MessageLoopProxy> observer_loop,
+                            Delegate* delegate);
+
+  // Structure holding the state used by both this class and the asynchronous
+  // callback running on the observer thread. Instances of SharedState are
+  // referenced through a shared pointer so that we handle the case where
+  // ProxyConfigServiceAndroid gets deleted before the asynchronous callback
+  // (using this state) runs.
+  struct SharedState;
+
+  // Called on observer thread.
+  static void ProxySettingsChangedCallback(
+      scoped_refptr<SharedState> callback_state);
+  bool OnObserverThread() const;
+
+  const scoped_refptr<SharedState> shared_state_;

[Ryan Sleevi, 2012/05/07 17:12:20]

This means the scoped_refptr itself is const, but the SharedState is not.

Did you mean scoped_refptr<const SharedState> (which I don't know if it will
even work)

[Philippe, 2012/05/09 11:48:40]

It only prevents us from reassigning the pointer. It documents the code by
expressing that once we constructed the instance (and DCHECKed), we can "safely"
dereference the pointer.
That way it is the semantic equivalent of non-const reference except that we
keep a syntax that makes mutations explicit (which is good).
"const SharedState" would have been best unfortunately we still need to perform
mutations on the ObserverList which is in SharedState.

[Ryan Sleevi, 2012/05/14 18:00:25]

But I mean, that's not really true. Once you construct the object, the
SharedState remains mutable and so any decision made at time of construction may
be wrong. Yes, it prevents reassignment of pointers, but as far as const
assumptions go, or about the validity of the members, it doesn't really old
much.
Right, which we explicitly don't allow (const or non-const references).

[Philippe, 2012/05/15 16:12:47]

I removed the const here and below since it might be confusing for the reader.

+
+  // Message loop of the thread on which observers are notified whenever proxy
+  // settings change.
+  const scoped_refptr<base::MessageLoopProxy> observer_loop_;

[Ryan Sleevi, 2012/05/07 17:12:20]

Drop the const?

[Philippe, 2012/05/09 11:48:40]

I would like to keep it for the same reason as above if you don't mind.

[Ryan Sleevi, 2012/05/14 18:00:25]

I don't feel terribly strongly about this, but I still don't think it buys all
that much =) Protecting members from re-assignment, ok - but eh, I've yet to see
a Chromium bug caused by that, as opposed to doing some sort of mutative action
on the underlying object.

[Philippe, 2012/05/15 16:12:47]

Done.

+};
+
+} // namespace net
+
+#endif // NET_PROXY_PROXY_CONFIG_SERVICE_ANDROID_H_