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 "base/android/scoped_java_ref.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/ref_counted.h"

[Ryan Sleevi, 2012/04/24 18:20:04]

nit: Make sure the headers are sorted

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

Done.

+#include "base/observer_list.h"
+#include "base/string_piece.h"
+#include "base/synchronization/lock.h"
+#include "net/base/net_export.h"
+#include "net/proxy/proxy_config.h"
+#include "net/proxy/proxy_config_service.h"
+
+#include <string>

[Ryan Sleevi, 2012/04/24 18:20:04]

nit: C++ system headers should appear before any Chromium headers

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

Done.

+
+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;
+  };
+
+  ProxyConfigServiceAndroid();  // Default
+  ProxyConfigServiceAndroid(Delegate* delegate);  // For tests; takes ownership.

[Ryan Sleevi, 2012/04/24 18:20:04]

nit: explicit ProxyConfigServiceAndroid(Delegate* delegate)

And if it's just for tests, make it private and use either friend or
FRIEND_TEST_ALL_PREFIXES to really make clear it's just for testing.

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

Done.

+  virtual ~ProxyConfigServiceAndroid();
+
+  // Register JNI bindings.
+  static bool Init(JNIEnv* env);
+
+  // ProxyConfigService:
+  virtual void AddObserver(Observer* observer) OVERRIDE;
+  virtual void RemoveObserver(Observer* observer) OVERRIDE;
+  virtual ConfigAvailability GetLatestProxyConfig(ProxyConfig* config) OVERRIDE;
+
+  // Called from the platform on any thread to signal that proxy settings have
+  // changed. AddObserver() must have been called at least once before.

[Ryan Sleevi, 2012/04/24 18:20:04]

I must admit, I'm not sure your threading guarantee - does this mean it can be
called from worker threads?

Should this be "on the network thread" ?

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

This method is actually called from the Java side which runs on the UI thread. I
removed this comment since the one below already provides enough information.

+  void ProxySettingsChanged();
+
+  // Called from Java to signal that the proxy settings have changed.
+  void ProxySettingsChanged(JNIEnv*, jobject) { ProxySettingsChanged(); }
+
+ private:
+  class NotifyTask;
+
+  void ProxySettingsChangedCallback();
+  void CancelNotifyTask();
+  void ClearNotifyTask();
+  bool OnObserverThread();
+  void AssignObserverThread();
+
+  // Accessed by the constructor, and from then on from the Observer thread.
+  scoped_ptr<Delegate> delegate_;
+  ObserverList<Observer> observers_;
+
+

[Ryan Sleevi, 2012/04/24 18:20:04]

nit: Delete extra line

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

Done.

+  base::Lock notify_task_lock_;
+  NotifyTask* notify_task_;  // Accessed from any thread, guarded by lock_.

[Ryan Sleevi, 2012/04/24 18:20:04]

Using Locks, particularly for threading, is generally seen as an anti-pattern
within the code.

Are you just trying to do ObserverListThreadSafe ?

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

NotifyTask was mainly used to handle the case where AndroidProxyConfigService
gets deleted before the callback runs on the network thread.

We removed that and used a shared pointer instead.

+
+  // The thread that this object lives on; set on first call to AddObserver().
+  // Notifications from the platform (ProxySettingsChanged) will be bounced
+  // over to this thread before calling back to the Observer. All other
+  // methods should be called on this thread.

[Ryan Sleevi, 2012/04/24 18:20:04]

This behaviour strikes me as a little surprising, in that it's one of those
things where it can be subtle to figure out what the 'true' thread this
observer_loop_ refers to (eg: it's dependent on whether or not someone 'else'
has called AddObserver)

Can you make this explicit from the ctor, rather than implicit?

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

The message loop proxy is now injected through the constructor.

+  scoped_refptr<base::MessageLoopProxy> observer_loop_;
+};
+
+} // namespace net
+
+#endif // NET_PROXY_PROXY_CONFIG_SERVICE_ANDROID_H_