876485 - Refactor GeckoAppShell's Clipboard code into a new util/Clipboard.java file

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Peterson [:cpeterson]]

GeckoAppShell.java is a huge class and its Clipboard code is getting increasingly complicated by version-specific and device-specific workarounds. We should extract those methods into a new class.

The current Clipboard code is here:

  https://hg.mozilla.org/mozilla-central/file/2ca0856eb5ee/mobile/android/base/GeckoAppShell.java#l1218

That code should move into a util/Clipboard.java file that is a static singleton, like the other util/*Utils.java classes. We should move the existing code as-is, the in a second patch simplify the method names to something like:

  Clipboard.getText()
  Clipboard.setText()
  Clipboard.clearText() // which would just call setText("")


!!! An important reminder: Fennec's widget code calls the Clipboard code from C++/JNI and must be updated with the new class and method names:

  https://hg.mozilla.org/mozilla-central/file/2ca0856eb5ee/widget/android/AndroidBridge.cpp#l123

Comment 1

[Federico Paolinelli]

Hi there, can I work on this one?

Comment 2

[Chris Peterson [:cpeterson]]

hi Federico, thanks for your help!

Have you compiled and installed Firefox for Android before? Here are the build instructions:

https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec

If you have any questions, the #mobile IRC channel on irc.mozilla.com is the best way to get quick answers to your questions. You can also email me directly at cpeterson at mozilla dot com.

Comment 3

[Federico Paolinelli]

Attachment: First submission of the patch

If this is ok, I will proceed with refactoring the name of the methods.

Comment 4

[Chris Peterson [:cpeterson]]

Comment on attachment 756942 [details] [diff] [review]
First submission of the patch

Review of attachment 756942 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! If you post a revised patch with the following small fixes, I'll r+ and check it in.

::: mobile/android/base/GeckoApplication.java
@@ -10,4 @@
>  import org.mozilla.gecko.util.HardwareUtils;
>  import org.mozilla.gecko.util.ThreadUtils;
>  
> -import android.app.Application;

You can leave this `import android...` line here. Our Java coding style, we order our imports by blocks separated by empty line: org.mozilla.*, com.*, net.*, org.*, android.*, then java.*.

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Java_practices

::: mobile/android/base/util/Clipboard.java
@@ +1,1 @@
> +package org.mozilla.gecko.util;

Please add this MPLv2 comment block to the top of this new file:

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

@@ +8,5 @@
> +
> +public class Clipboard {
> +    private static Context mContext;
> +    private final static String LOG_TAG = "Clipboard";
> +

Please add a private Clipboard constructor (that does nothing) and mark the class as `final`. Since Clipboard is a singleton utility class, we don't want anyone to allocate a Clipboard instance or subclass it.

@@ +9,5 @@
> +public class Clipboard {
> +    private static Context mContext;
> +    private final static String LOG_TAG = "Clipboard";
> +
> +    public static void init(Context c) {

I think we should add a new Context parameter to the getClipboardText() and setClipboardText() methods. I'm a little nervous about holding a static reference to our Context. Note that adding a new parameter will require the C++ JNI code to be updated.

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 756942 [details] [diff] [review]
First submission of the patch

Review of attachment 756942 [details] [diff] [review]:
-----------------------------------------------------------------

Here are some suggestions for additional code cleanup you might want to make in your patch to rename the Clipboard methods.

::: mobile/android/base/GeckoInputConnection.java
@@ +270,5 @@
>              case R.id.cut:
>                  // If selection is empty, we'll select everything
>                  if (selStart == selEnd) {
>                      // Fill the clipboard
> +                    Clipboard.setClipboardText(editable.toString());

`setClipboardText(editable.toString())` can be streamlined to `setText(editable)` with my CharSequence suggestion below.

::: mobile/android/base/util/Clipboard.java
@@ +21,5 @@
> +    /* On some devices, access to the clipboard service needs to happen
> +     * on a thread with a looper, so this function requires a looper is
> +     * present on the thread. */
> +    @SuppressWarnings("deprecation")
> +    private static String getClipboardTextImpl() {

Let's move getClipboardTextImpl() to the bottom of the class definition since it's a private method.

@@ +22,5 @@
> +     * on a thread with a looper, so this function requires a looper is
> +     * present on the thread. */
> +    @SuppressWarnings("deprecation")
> +    private static String getClipboardTextImpl() {
> +        if (android.os.Build.VERSION.SDK_INT >= 11) {

Let's import android.os.Build at the top of the file and just use Build.VERSION.SDK_INT here.

@@ +32,5 @@
> +                    return item.coerceToText(mContext).toString();
> +                }
> +            }
> +        } else {
> +            android.text.ClipboardManager cm = (android.text.ClipboardManager)mContext.getSystemService(Context.CLIPBOARD_SERVICE);

These getSystemService(Context.CLIPBOARD_SERVICE) lines are really long and repeated in two methods. Can we split these lines into private helper methods like these?

android.text.ClipboardManager getClipboardManager(Context context) {
    return (android.text.ClipboardManager) context.getSystemService(Context.CLIPBOARD_SERVICE);
}

android.content.ClipboardManager getClipboardManager11(Context context) {
    // In API Level 11 and above, CLIPBOARD_SERVICE returns android.content.ClipboardManager,
    // which is a subclass of android.text.ClipboardManager.
    return (android.content.ClipboardManager) getClipboardManager(context);
}

@@ +40,5 @@
> +        }
> +        return null;
> +    }
> +
> +    private static SynchronousQueue<String> sClipboardQueue = new SynchronousQueue<String>();

Even though sClipboardQueue's declaration was in the middle of GeckoAppShell class, let's move it to the top of the Clipboard class, our usual Java coding style. We can make it `final`, too.

@@ +67,5 @@
> +            return "";
> +        }
> +    }
> +
> +    public static void setClipboardText(final String text) {

Let's change set[Clipboard]Text() to take a CharSequence instead of a String. Android's ClipboardManager classes take CharSequences. This will allow us to streamline GeckoInputConnection.java's `setClipboardText(editable.toString())` call to `setText(editable)` (because Editable implements the CharSequence interface). But I think get[Clipboard]Text() should still return a regular String.

@@ +72,5 @@
> +        ThreadUtils.postToBackgroundThread(new Runnable() {
> +            @Override
> +            @SuppressWarnings("deprecation")
> +            public void run() {
> +                if (android.os.Build.VERSION.SDK_INT >= 11) {

Just use Build.VERSION.SDK_INT here, too.

Comment 6

[Federico Paolinelli]

Attachment: Second attempt

I also had to change the jclass called from jni from geckoappshell to this new clipboardclass, which I did not change in the previous version. Hoping everything is fine..

Comment 7

[Chris Peterson [:cpeterson]]

Comment on attachment 758055 [details] [diff] [review]
Second attempt

Review of attachment 758055 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! If you post a revised patch with the suggestions below, I can r+ your patch and land in on mozilla-central.

::: mobile/android/base/util/Clipboard.java
@@ +60,5 @@
> +            @SuppressWarnings("deprecation")
> +            public void run() {
> +                if (Build.VERSION.SDK_INT >= 11) {
> +                    android.content.ClipboardManager cm = getClipboardManager11(mContext);
> +                    cm.setPrimaryClip(ClipData.newPlainText("Text", text));

I think your copy of the mozilla-central code is a little out of date. Can you please add the following bug fix from GeckoAppShell.java's setClipboardText() to Clipboard.java's setText()?

https://hg.mozilla.org/mozilla-central/rev/1ad0d609ccd1

@@ +85,5 @@
> +     * present on the thread. */
> +    @SuppressWarnings("deprecation")
> +    private static String getClipboardTextImpl() {
> +        if (Build.VERSION.SDK_INT >= 11) {
> +            android.content.ClipboardManager cm = (android.content.ClipboardManager)mContext.getSystemService(Context.CLIPBOARD_SERVICE);

Can you please use the new getClipboardManager() and getClipboardManager11() helper functions in getClipboardTextImpl(), too?

Comment 8

[Federico Paolinelli]

Attachment: Third version

Added the last changes you suggested.

Comment 9

[Chris Peterson [:cpeterson]]

green try build:
https://tbpl.mozilla.org/?tree=Try&rev=7b57a51eed28

Comment 10

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33a3d0e4bc18

Thanks for fixing this bug, Federico! :)

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/33a3d0e4bc18