Closed Bug 792496 Opened 8 years ago Closed 8 years ago

Remove functions that just delegate to ScreenshotHandler

Categories

(Firefox for Android :: Toolbar, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 18

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Delete stuff (obsolete) — Splinter Review
Follow-up from bug 791264. Try run pending at https://tbpl.mozilla.org/?tree=Try&rev=44c06397b3b0
Attachment #662629 - Flags: review?(cpeterson)
Attached patch Delete stuffSplinter Review
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
Attachment #662629 - Attachment is obsolete: true
Attachment #662629 - Flags: review?(cpeterson)
Attachment #662636 - Flags: review?(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.
Attachment #662636 - Flags: review?(cpeterson) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/b1497c3230a5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Depends on: 794296
You need to log in before you can comment on or make changes to this bug.