Native Android accessibility.

content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibility.java

Index: content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibility.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibility.java b/content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibility.java
new file mode 100644
index 0000000000000000000000000000000000000000..da59218749558ee47b73165b32eb949fef725132
--- /dev/null
+++ b/content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibility.java
@@ -0,0 +1,63 @@
+// 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.
+
+package org.chromium.content.browser.accessibility;
+
+import android.graphics.Rect;
+
+import org.chromium.base.CalledByNative;
+import org.chromium.base.JNINamespace;
+
+/**
+ * This object is never instantiated, it's just used to define the interface Java code
+ * can use to access a BrowserAccessibilityAndroid C++ class, plus static helper methods
+ * for use by the C++ class.
+ */
+@JNINamespace("content")
+public class BrowserAccessibility {
+    private BrowserAccessibility() { }
+
+    @CalledByNative
+    private static Rect createRect(int x, int y, int right, int bottom) {
+        return new Rect(x, y, right, bottom);
+    }
+
+    // Actions.
+    static native void nativeFocusJNI(int nativeBrowserAccessibilityAndroid);

[bulach, 2013/06/19 10:26:53]

a few issues here:
1. naming: there's never a JNI suffix, please remove.
2. visibility: the native interface is almost always private, i.e., an
implementation detail of the java class, rather than part of the interface
exposed to other java objects.
3. type safety: because these methods are not private, anybody can pass this
magic int.. normally, there's a java class containing it, so we know it can be
safely casted in the C++ side to a BrowserAccessibilityAndroid*... here, it's
crossing boundaries "diagonally" :)
that is: BrowserAccessibilityManager in java, gets the native pointer for an
entirely different class, then calls a native interface from a different class..

it's normally cleaner to cross boundaries only "vertically" and "horizonally":
- in the java side, BrowserAccessibilityManager only talks to
BrowserAccessibility in java.
- BrowserAccessibility java then talks to its native counter part..

This leads to better decoupling, and all the classes are safer to further
refactorings....

I know it's more boiler plate, but it'll save future headache:
- Make it a single factory function, BrowserAccessibility.createFromId(int
bamaPtr), with a comment saying that bamaPtr has to be a pointer to
BrowserAccessibilityManagerAndroid.... this will be single point of contact
where type safety is not guaranteed, but that's easier to track... this factory
function would call into native, cast the bamaPtr to
BrowserAccessibilityManagerAndroid* and return
reinterpret_cast<jint>(GetFromRendererID(id)).. then back in java, there'd be a
private constructor taking an int and stashing it in a member of this object.

- have all the methods non-static and private.

- have java methods that just delegate to the native counterparts: so we can
ensure they'll always be called with the right pointer obtained from above..


how does it sound?

[dmazzoni, 2013/06/19 14:00:16]

Understood.

Actually initially I had all of these as methods on BrowserAccessibilityManager,
but taking a first argument as an int led to all sorts of bugs, because there
were a couple of functions that took an ID as an argument and the rest took
an int that represented a pointer.

Switching to this pattern made the code extremely clear and prevented
accidental programming errors, without changing performance.

I'm a bit concerned about the performance implications of making
BrowserAccessibility an actual object that gets instantiated.
Creating this object would be very hot, we'd have several thousand
of them sometimes, and the same one might be accessed dozens of times
in a row from different call stacks.

I wonder if there's another way we could get a bit more safety / clarity
without constructing a bunch of Java objects we don't actually need.

+    static native void nativeClickJNI(int nativeBrowserAccessibilityAndroid);
+
+    // Accessors.
+    static native String nativeGetNameJNI(int nativeBrowserAccessibilityAndroid);
+    static native Object nativeGetAbsoluteRectJNI(int nativeBrowserAccessibilityAndroid);
+    static native Object nativeGetRectInParentJNI(int nativeBrowserAccessibilityAndroid);
+    static native boolean nativeIsFocusableJNI(int nativeBrowserAccessibilityAndroid);
+    static native boolean nativeIsFocusedJNI(int nativeBrowserAccessibilityAndroid);
+    static native boolean nativeIsEditableTextJNI(int nativeBrowserAccessibilityAndroid);
+    static native boolean nativeGetClickableJNI(int nativeBrowserAccessibilityAndroid);
+    static native int nativeGetParentJNI(int nativeBrowserAccessibilityAndroid);
+    static native int nativeGetChildCountJNI(int nativeBrowserAccessibilityAndroid);
+    static native int nativeGetChildIdAtJNI(int nativeBrowserAccessibilityAndroid, int childIndex);
+    static native boolean nativeIsCheckableJNI(int nativeBrowserAccessibilityAndroid);
+    static native boolean nativeIsCheckedJNI(int nativeBrowserAccessibilityAndroid);
+    static native String nativeGetClassNameJNI(int nativeBrowserAccessibilityAndroid);
+    static native boolean nativeIsEnabledJNI(int nativeBrowserAccessibilityAndroid);
+    static native boolean nativeIsPasswordJNI(int nativeBrowserAccessibilityAndroid);
+    static native boolean nativeIsScrollableJNI(int nativeBrowserAccessibilityAndroid);
+    static native boolean nativeIsSelectedJNI(int nativeBrowserAccessibilityAndroid);
+    static native boolean nativeIsVisibleJNI(int nativeBrowserAccessibilityAndroid);
+    static native int nativeGetItemIndexJNI(int nativeBrowserAccessibilityAndroid);
+    static native int nativeGetItemCountJNI(int nativeBrowserAccessibilityAndroid);
+    static native int nativeGetScrollXJNI(int nativeBrowserAccessibilityAndroid);
+    static native int nativeGetScrollYJNI(int nativeBrowserAccessibilityAndroid);
+    static native int nativeGetMaxScrollXJNI(int nativeBrowserAccessibilityAndroid);
+    static native int nativeGetMaxScrollYJNI(int nativeBrowserAccessibilityAndroid);
+    static native String nativeGetAriaLiveJNI(int nativeBrowserAccessibilityAndroid);
+    static native int nativeGetSelectionStartJNI(int nativeBrowserAccessibilityAndroid);
+    static native int nativeGetSelectionEndJNI(int nativeBrowserAccessibilityAndroid);
+    static native int nativeGetEditableTextLengthJNI(int nativeBrowserAccessibilityAndroid);
+    static native int nativeGetTextChangeFromIndexJNI(int nativeBrowserAccessibilityAndroid);
+    static native int nativeGetTextChangeAddedCountJNI(int nativeBrowserAccessibilityAndroid);
+    static native int nativeGetTextChangeRemovedCountJNI(int nativeBrowserAccessibilityAndroid);
+    static native String nativeGetTextChangeBeforeTextJNI(int nativeBrowserAccessibilityAndroid);
+};