[ImportantSites] JNI bindings for CBD filtering and Important Sites.

chrome/browser/android/preferences/pref_service_bridge.cc

Index: chrome/browser/android/preferences/pref_service_bridge.cc
diff --git a/chrome/browser/android/preferences/pref_service_bridge.cc b/chrome/browser/android/preferences/pref_service_bridge.cc
index 6675341cd65bf26a19c256161904bce2b5f6cbed..7eba312fe35a38d93c899c818c6b791ccc15d8ae 100644
--- a/chrome/browser/android/preferences/pref_service_bridge.cc
+++ b/chrome/browser/android/preferences/pref_service_bridge.cc
@@ -24,6 +24,7 @@
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
 #include "chrome/browser/browsing_data/browsing_data_remover.h"
 #include "chrome/browser/browsing_data/browsing_data_remover_factory.h"
+#include "chrome/browser/browsing_data/origin_filter_builder.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/net/prediction_options.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
@@ -49,6 +50,7 @@
 #include "content/public/browser/user_metrics.h"
 #include "jni/PrefServiceBridge_jni.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "url/origin.h"
 
 using base::android::AttachCurrentThread;
 using base::android::CheckException;
@@ -496,6 +498,36 @@ class ClearBrowsingDataObserver : public BrowsingDataRemover::Observer {
   ScopedObserver<BrowsingDataRemover, BrowsingDataRemover::Observer> observer_;
 };
 
+// Redirects a BrowsingDataRemover completion callback back into the java
+// callback.
+class ClearBrowsingDataObserverWithCallback

[newt (away), 2016/03/09 05:40:02]

There's no reason to add this new class; just modify ClearBrowsingDataObserver
above (which is currently unused in this patch set).

Also, please remove mClearBrowsingDataListener and browsingDataCleared() from
PrefServiceBridge, since they won't be used after this patch.

+    : public BrowsingDataRemover::Observer {
+ public:
+  ClearBrowsingDataObserverWithCallback(
+      JNIEnv* env,
+      jobject java_callback,
+      BrowsingDataRemover* browsing_data_remover)
+      : weak_java_callback_(env, java_callback), observer_(this) {
+    observer_.Add(browsing_data_remover);
+  }
+
+  void OnBrowsingDataRemoverDone() override {
+    // We delete ourselves when done.
+    scoped_ptr<ClearBrowsingDataObserverWithCallback> auto_delete(this);
+
+    JNIEnv* env = AttachCurrentThread();
+    if (weak_java_callback_.get(env).is_null())

[newt (away), 2016/03/09 05:40:02]

Incidentally, this use of the weak pointer is broken and prone to NPEs. If
garbage collection runs after line 519, but before line 522, then we'll get a
null pointer exception when calling "weak_java_callback_.get(env).obj()" on line
523.

As explained here
https://www.ibm.com/support/knowledgecenter/SSYKE2_8.0.0/com.ibm.java.lnx.80....
:

"It is not safe to call most JNI functions with a weak global reference, even if
you have tested that the reference is not null, because the weak global
reference could become a null reference after it has been tested or even during
the JNI function. Instead, a weak global reference should always be promoted to
a strong reference before it is used. You can promote a weak global reference
using the NewLocalRef or NewGlobalRef JNI functions."

Anyway, avoid weak references when possible for reasons mentioned below.

+      return;
+
+    Java_OnClearBrowsingDataListener_onBrowsingDataCleared(
+        env, weak_java_callback_.get(env).obj());
+  }
+
+ private:
+  JavaObjectWeakGlobalRef weak_java_callback_;

[newt (away), 2016/03/09 05:40:02]

This shouldn't be weak; it should just be a regular ScopedJavaGlobalRef. Use
weak pointers sparingly, since they can introduce nondeterministic behavior that
depends on the whims of the garbage collector. For example, imagine this call

  PrefServiceBridge.clearBrowsingDataExcludingOrigins(new
ClearBrowsingDataObserver() {
      @Override
      public void onBrowsingDataCleared() {
          hideProgressDialog();
      }
  }, ...);

What happens if the garbage collector runs before the browsing data has been
cleared? In that case, the otherwise unreferenced ClearBrowsingDataObserver
would be gc'd and would never receive a callback and the progress dialog would
never be dismissed. This is definitely not what we want. A normal non-weak
reference should be used here.

+  ScopedObserver<BrowsingDataRemover, BrowsingDataRemover::Observer> observer_;
+};
+
 }  // namespace
 
 static jboolean GetBrowsingDataDeletionPreference(
@@ -557,7 +589,7 @@ static void ClearBrowsingData(JNIEnv* env,
       BrowsingDataRemoverFactory::GetForBrowserContext(GetOriginalProfile());
   // ClearBrowsingDataObserver deletes itself when |browsing_data_remover| is
   // done.
-  new ClearBrowsingDataObserver(env, obj, browsing_data_remover);
+  new ClearBrowsingDataObserverWithCallback(env, obj, browsing_data_remover);
 
   std::vector<int> data_types_vector;
   base::android::JavaIntArrayToIntVector(env, data_types, &data_types_vector);
@@ -597,6 +629,68 @@ static void ClearBrowsingData(JNIEnv* env,
       remove_mask, BrowsingDataHelper::UNPROTECTED_WEB);
 }
 
+static void ClearBrowsingDataExcludingOrigins(

[newt (away), 2016/03/09 05:40:02]

As Martin mentioned, please combine this method with the method above. If we're
clearing all browsing data (and excluding any origins), you could pass "null"
instead of an array of excluded origins.

+    JNIEnv* env,
+    const JavaParamRef<jclass>& clazz,
+    const JavaParamRef<jobject>& java_callback,
+    const JavaParamRef<jintArray>& data_types,
+    const JavaParamRef<jobjectArray>& excluding_origins) {

[newt (away), 2016/03/09 05:40:02]

call this "excluded_origins" or just "origins"

+  BrowsingDataRemover* browsing_data_remover =
+      BrowsingDataRemoverFactory::GetForBrowserContext(GetOriginalProfile());
+  // ClearBrowsingDataObserver deletes itself when |browsing_data_remover| is
+  // done.
+  new ClearBrowsingDataObserver(env, java_callback.obj(),
+                                browsing_data_remover);
+
+  std::vector<int> data_types_vector;
+  base::android::JavaIntArrayToIntVector(env, data_types, &data_types_vector);
+
+  int remove_mask = 0;
+  for (const int data_type : data_types_vector) {
+    switch (static_cast<BrowsingDataType>(data_type)) {
+      case HISTORY:
+        remove_mask |= BrowsingDataRemover::REMOVE_HISTORY;
+        break;
+      case CACHE:
+        remove_mask |= BrowsingDataRemover::REMOVE_CACHE;
+        break;
+      case COOKIES:
+        remove_mask |= BrowsingDataRemover::REMOVE_COOKIES;
+        remove_mask |= BrowsingDataRemover::REMOVE_SITE_DATA;
+        break;
+      case PASSWORDS:
+        remove_mask |= BrowsingDataRemover::REMOVE_PASSWORDS;
+        break;
+      case FORM_DATA:
+        remove_mask |= BrowsingDataRemover::REMOVE_FORM_DATA;
+        break;
+      case BOOKMARKS:
+        // Bookmarks are deleted separately on the Java side.
+        NOTREACHED();
+        break;
+      case NUM_TYPES:
+        NOTREACHED();
+    }
+  }
+
+  OriginFilterBuilder filter_builder(OriginFilterBuilder::BLACKLIST);
+  jsize origin_count = env->GetArrayLength(excluding_origins.obj());
+  for (jsize i = 0; i < origin_count; ++i) {
+    jstring url =
+        (jstring)env->GetObjectArrayElement(excluding_origins.obj(), i);
+    filter_builder.AddOrigin(url::Origin(ConvertJavaStringToUTF8(env, url)))
+  }
+
+  /* Commented out, as we don't have the filterbuilder hookup yet.
+    int period_selected =
+    GetPrefService()->GetInteger(prefs::kDeleteTimePeriod);
+    browsing_data_remover->Remove(
+        BrowsingDataRemover::Period(
+            static_cast<BrowsingDataRemover::TimePeriod>(period_selected)),
+        remove_mask, filter_builder, BrowsingDataHelper::UNPROTECTED_WEB);
+  */
+}
+
 static jboolean CanDeleteBrowsingHistory(JNIEnv* env,
                                          const JavaParamRef<jobject>& obj) {
   return GetPrefService()->GetBoolean(prefs::kAllowDeletingBrowserHistory);