Update geolocation infobar to handle Android system settings

chrome/browser/android/google_location_settings_helper.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 CHROME_BROWSER_ANDROID_GOOGLE_LOCATION_SETTINGS_HELPER_H_
+#define CHROME_BROWSER_ANDROID_GOOGLE_LOCATION_SETTINGS_HELPER_H_
+
+#include "base/android/jni_helper.h"
+#include "base/android/scoped_java_ref.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/values.h"
+#include "chrome/browser/android/google_location_settings_helper_factory.h"
+
+/*
+ * This class is needed to fetch the current system location
+ * setting and update the infobar button label based on that information i.e,
+ * display "Allow" if google apps setting is set as enabled else, display
+ * "Settings" with a link to open the system location settings activity.
+ */

[bulach, 2012/10/23 13:06:26]

nit: C++ comments are // instead of /* */

[Ramya, 2012/10/23 21:57:14]

Done.

+class GoogleLocationSettingsHelper {
+ public:
+  GoogleLocationSettingsHelper();
+  ~GoogleLocationSettingsHelper() {}

[bulach, 2012/10/23 13:06:26]

nit: add \n after this.
also, the destructor shouldn't be inline, as this class contains non-POD member
(clang bot will complain..)

[Ramya, 2012/10/23 21:57:14]

Done.

+  static void SetGoogleLocationSettingsHelperFactory(
+      GoogleLocationSettingsHelperFactory* factory);
+  std::string GetAcceptButtonLabel();
+  void ShowGoogleLocationSettings();
+  bool IsPlatformSettingEnabled();
+  static bool Register(JNIEnv* env);
+
+ private:
+  base::android::ScopedJavaGlobalRef<jobject>
+      java_google_location_settings_helper_;
+  static GoogleLocationSettingsHelperFactory* helper_factory_;

[bulach, 2012/10/23 13:06:26]

this will leak.. :(

how about we invert the dependency?
rather than this class requiring the embedder to call "Set...Factory", we could:

- make this class a pure c++ interface.
- add "static GoogleLocationSettingsHelper* Create()"
|__ this method will be implemented by content shell, or the android app. in
tests, we could then implement a mock

- if you agree with the above, we could completely remove all the java files
from this patch... that is, chromium requires a very thin C++ interface, and in
the app side in android it's an implementation detail that it happens to be
implemented in java. both content shell and tests can then provide their own
implementations..

[Jay Civelli, 2012/10/23 17:37:53]

It should not be too bad a leak though, we only have one
GoogleLocationSettingsHelperFactory per run of the application.
I believe this is the approach that Ramya had originally taken but then review
comments suggested her to go the factory route, so she reimplemented it that
way.
Do you think she could revisit that at a later point?

+
+  DISALLOW_COPY_AND_ASSIGN(GoogleLocationSettingsHelper);
+};
+
+#endif  // CHROME_BROWSER_ANDROID_GOOGLE_LOCATION_SETTINGS_HELPER_H_