792496 - Remove functions that just delegate to ScreenshotHandler

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Delete stuff

Follow-up from bug 791264. Try run pending at https://tbpl.mozilla.org/?tree=Try&rev=44c06397b3b0

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Delete stuff

Whoops, forgot to qref. New patch also updates the functions in AndroidBridge that invoke the notify calls to use jScreenshotHandlerClass instead of mGeckoAppShellClass. New try run at https://tbpl.mozilla.org/?tree=Try&rev=4f7c1acb747e

Comment 2

[Chris Peterson [:cpeterson]]

Comment on attachment 662636 [details] [diff] [review]
Delete stuff

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

LGTM with a couple nits.

::: mobile/android/base/ScreenshotHandler.java
@@ +22,5 @@
>  import java.util.Iterator;
>  import java.util.LinkedList;
>  import java.util.Queue;
>  
> +public class ScreenshotHandler implements Runnable {

You should make the ScreenshotHandler class final since we only access it through ScreenshotHandler.getInstance().

::: widget/android/AndroidBridge.cpp
@@ +174,5 @@
>      jLockScreenOrientation = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "lockScreenOrientation", "(I)V");
>      jUnlockScreenOrientation = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "unlockScreenOrientation", "()V");
>  
> +    jScreenshotHandlerClass = (jclass) jEnv->NewGlobalRef(jEnv->FindClass("org/mozilla/gecko/ScreenshotHandler"));
> +    jNotifyScreenShot = jEnv->GetStaticMethodID(jScreenshotHandlerClass, "notifyScreenShot", "(Ljava/nio/ByteBuffer;IIIIIIII)V");

IIIIIIII! :)

(I'm just pointing out the extreme method signature. No change needed, though this is a code smell.)

@@ +2529,1 @@
>                                buffer, tabId, dstX, dstY, dstX + dstW, dstY + dstH, bufW, bufH, token);

C++ code should be wrapped at 80 columns.

@@ +2537,5 @@
>      if (!env)
>          return;
>  
>      AutoLocalJNIFrame jniFrame(env, 0);
> +    env->CallStaticVoidMethod(AndroidBridge::Bridge()->jScreenshotHandlerClass, AndroidBridge::Bridge()->jNotifyPaintedRect, top, left, bottom, right);

C++ code should be wrapped at 80 columns.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Peterson (:cpeterson) from comment #2)
> > +public class ScreenshotHandler implements Runnable {
> 
> You should make the ScreenshotHandler class final since we only access it
> through ScreenshotHandler.getInstance().

Good point, thanks.

jEnv->NewGlobalRef(jEnv->FindClass("org/mozilla/gecko/ScreenshotHandler"));
> > +    jNotifyScreenShot = jEnv->GetStaticMethodID(jScreenshotHandlerClass, "notifyScreenShot", "(Ljava/nio/ByteBuffer;IIIIIIII)V");
> 
> IIIIIIII! :)
> 
> (I'm just pointing out the extreme method signature. No change needed,
> though this is a code smell.)
> 

Yeah... the alternative at the time was to create more JNI goop to wrap more classes.

> C++ code should be wrapped at 80 columns.
> 

Done, but for the record, I object to this style point since we have big screens now. Also the rest of this file blatantly ignores this wrapping too.

I'll land the patch once the try run is done.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1497c3230a5

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/b1497c3230a5