Add types.private.ChromeDirectSetting and Connect it to preferencesPrivate.googleGeolocationAccessE…

chrome/browser/extensions/api/preference/chrome_direct_setting.h

+// Copyright (c) 2013 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 CHROME_BROWSER_EXTENSIONS_API_PREFERENCE_CHROME_DIRECT_SETTING_H__
+#define CHROME_BROWSER_EXTENSIONS_API_PREFERENCE_CHROME_DIRECT_SETTING_H__
+
+#include "base/lazy_instance.h"
+#include "chrome/browser/extensions/extension_function.h"
+
+class PrefService;
+
+namespace extensions {
+namespace chromedirectsetting {
+
+// Base class to host instance method helpers.
+class DirectSettingFunctionBase : public SyncExtensionFunction {
+ protected:
+  DirectSettingFunctionBase() {}

[miket_OOO, 2013/07/10 18:34:47]

No inline constructors/destructors in non-virtual classes.

http://www.chromium.org/developers/coding-style/cpp-dos-and-donts (I have to
re-read this every time I encounter this in a review. If I've missed an
exception, please push back.)

[robliao, 2013/07/10 20:20:09]

I think the intent behind this rule is for libraries consumed by other
components. Given that no one directly constructs these objects (instantiation
is handled via RegisterFunction), I think we are okay here.

This was modeled after the Preference API Functions (e.g. GetPreferenceFunction)
with a twist on adding DISALLOW_COPY_AND_ASSIGN, which requires an explicit
construction declaration and implementation.

If there is a strong desire to move the {}'s, I can do so. Unfortunately, it
clutters the implementation file.

Alternatively, I can remove DISALLOW_COPY_AND_ASSIGN.
On 2013/07/10 18:34:47, miket wrote:

[robliao, 2013/07/10 20:33:33]

Furthermore, these classes shouldn't store state due to the transient nature of
functions, so I think we should definitely be okay here.
On 2013/07/10 20:20:09, Robert Liao wrote:

[Bernhard Bauer, 2013/07/10 22:11:26]

No, the main purpose of the rule is to prevent unnecessary code; see the
background section at
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in....

[Bernhard Bauer, 2013/07/10 22:11:26]

Stateless or not is not the issue here, but whether the constructor/destructor
constains a non-trivial amount of code. In this case, it does.

[robliao, 2013/07/10 22:18:45]

What non-trivial code is present here? The compiler would have generated a
default constructor if I didn't have DISALLOW_COPY_AND_ASSIGN.
On 2013/07/10 22:11:26, Bernhard Bauer wrote:

[Bernhard Bauer, 2013/07/10 22:37:24]

Exactly, and it still does generate the default constructor (which consists of
calling the base constructor). The guide explicitly mentions "no base class" as
one of the conditions for allowing inline constructors. Without the
DISALLOW_COPY_AND_ASSIGN and the explicitly declared constructor, I probably
wouldn't have caught it, but it would have been wrong nevertheless. 

[robliao, 2013/07/11 00:28:13]

I went ahead and removed the inline constructors and destructors.

I feel it comes at the expense of readability in chrome_direct_setting.cc for a
scenario that won't occur in the style guideline.
On 2013/07/10 22:37:24, Bernhard Bauer wrote:

+  virtual ~DirectSettingFunctionBase() {}
+
+  // Returns the user pref service.
+  PrefService* GetPrefService();
+
+  // Returns true if the caller is a component extension.
+  bool IsCalledFromComponentExtension();
+
+  // Returns true if the preference is on the whitelist.
+  bool IsPreferenceOnWhitelist(const std::string& pref_key);
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DirectSettingFunctionBase);
+};
+
+class GetDirectSettingFunction : public DirectSettingFunctionBase {
+ public:
+  DECLARE_EXTENSION_FUNCTION("types.private.ChromeDirectSetting.get",
+                             TYPES_PRIVATE_CHROMEDIRECTSETTING_GET)
+
+  GetDirectSettingFunction() {}
+
+ protected:
+  // ExtensionFunction:
+  virtual bool RunImpl() OVERRIDE;
+
+ private:
+  virtual ~GetDirectSettingFunction() {}
+  DISALLOW_COPY_AND_ASSIGN(GetDirectSettingFunction);
+};
+
+class SetDirectSettingFunction : public DirectSettingFunctionBase {
+ public:
+  DECLARE_EXTENSION_FUNCTION("types.private.ChromeDirectSetting.set",
+                             TYPES_PRIVATE_CHROMEDIRECTSETTING_SET)
+
+  SetDirectSettingFunction() {}
+
+ protected:
+  // ExtensionFunction:
+  virtual bool RunImpl() OVERRIDE;
+
+ private:
+  virtual ~SetDirectSettingFunction() {}
+  DISALLOW_COPY_AND_ASSIGN(SetDirectSettingFunction);
+};
+
+class ClearDirectSettingFunction : public DirectSettingFunctionBase {
+ public:
+  DECLARE_EXTENSION_FUNCTION("types.private.ChromeDirectSetting.clear",
+                             TYPES_PRIVATE_CHROMEDIRECTSETTING_CLEAR)
+
+  ClearDirectSettingFunction() {}
+
+ protected:
+  // ExtensionFunction:
+  virtual bool RunImpl() OVERRIDE;
+
+ private:
+  virtual ~ClearDirectSettingFunction() {}
+  DISALLOW_COPY_AND_ASSIGN(ClearDirectSettingFunction);
+};
+
+}  // namespace chromedirectsetting
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_API_PREFERENCE_CHROME_DIRECT_SETTING_H__
+