[Cronet] Enable persistence mode for Sdch

components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java

+// Copyright 2015 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.
+
+package org.chromium.net;
+
+import android.os.ConditionVariable;
+
+import static junit.framework.Assert.assertNull;
+
+import org.chromium.base.CalledByNative;
+import org.chromium.base.JNINamespace;
+
+/**
+ * Test utilities related to Sdch.
+ */
+@JNINamespace("cronet")
+public final class SdchTestUtil {
+    // Blocks until the native SdchObserver is added.
+    private static final ConditionVariable sAddBlock = new ConditionVariable();

[mmenke, 2015/05/26 19:34:40]

Rather than use globals, could we just instantiate an object instead, and have a
single object correspond to a single wait for a dictionary?

[xunjieli, 2015/05/26 20:28:28]

Done. I have moved this condition variable to be a member variable of
SdchObserverCallback.
Just to make sure I understand, do you mean we change sCallback to be a
non-global as well? That will involve a bit more work on the C++ to pass java
pointers to network thread. Our current tests don't need multiple callback
objects at the same time, but I will try if you think we should do that. 

[mmenke, 2015/05/27 14:40:43]

That's right.  It's not the ability to have multiple callbacks I'm worried about
- globals in general are ugly, and are best avoided unless really needed.  They
tend to lead to unanticipated problems, so it's best to just not use them in the
first place, if you reasonably can do so.

There are actually two approaches to get rid of the static variables, I'm fine
with either one:

1)  Keep this class static, and pass the callback through C++, and then make it
a parameter of onDictionaryAdded.

2)  Or rename this to OneshotSdchDictionaryObserver, and make the callback a
member variable (Actually...Can just get rid of it and integrate the logic into
OneshotSdchDictionaryObserver itself).  Then pass the class itself through C++,
and invoke onDictionaryAdded on it directly from C++.

I think 2) is a little cleaner, and not much more complicated, but it's up to
you.

+
+    /**
+     * Abstract class to listen for callbacks of the native SdchObserver.
+     */
+    public abstract static class SdchObserverCallback {
+        /**
+         * Called when a dictionary is added to the SdchManager.
+         * @param url the url of the dictionary added.
+         */
+        public abstract void onDictionaryAdded(String url);
+    }
+
+    private static SdchObserverCallback sCallback;
+    private static boolean sRegisterSdchObserverSucceeded = false;

[mmenke, 2015/05/26 19:34:40]

We're generally putting variables before methods in Java files.

[xunjieli, 2015/05/26 20:28:28]

Done.

+
+    /**
+     * Returns whether the native SdchObserver has been registered. The native
+     * SdchObserver will not be registered when dictionaries for the
+     * {@code targetUrl} are already loaded, for example.
+     */
+    public static boolean registerSdchObserverCallback(String targetUrl, long contextAdapter,
+            SdchObserverCallback callback, boolean isLegacyAPI) {

[mmenke, 2015/05/26 19:34:40]

I thought we weren't adding legacy API support for this?

[xunjieli, 2015/05/26 20:28:28]

We aren't adding legacy support for sdch meta data persistence. However, we have
already added sdch support (without the persistence bits) for the legacy API, so
we need a more reliable way to test it as well. (Before this CL, I put an
arbitrary wait in the test in order to make sure the dictionary is fetched).

+        assertNull(sCallback);
+        sCallback = callback;
+        if (isLegacyAPI) {
+            nativeAddSdchObserverLegacyAPI(targetUrl, contextAdapter);
+        } else {
+            nativeAddSdchObserver(targetUrl, contextAdapter);
+        }
+        sAddBlock.block();
+        sAddBlock.close();
+        return sRegisterSdchObserverSucceeded;
+    }
+
+    @CalledByNative
+    private static void onDictionaryAdded(String dictionaryURL) {
+        if (sCallback != null) {
+            sCallback.onDictionaryAdded(dictionaryURL);
+            sCallback = null;
+        }
+    }
+
+    @CalledByNative
+    private static void onAddSdchObserverCompleted() {
+        sRegisterSdchObserverSucceeded = true;
+        sAddBlock.open();
+    }
+
+    @CalledByNative
+    private static void onDictionaryAlreadyPresent() {
+        sAddBlock.open();
+    }
+
+
+    private static native void nativeAddSdchObserver(String targetUrl, long contextAdapter);
+    private static native void nativeAddSdchObserverLegacyAPI(
+            String targetUrl, long contextAdapter);
+}