Closed Bug 727307 Opened 8 years ago Closed 6 years ago

Use handler messages in GeckoApp

Categories

(Firefox for Android :: General, defect, P3)

ARM
Android
defect

Tracking

()

RESOLVED WONTFIX
Tracking Status
fennec + ---

People

(Reporter: sriram, Assigned: sriram)

Details

Attachments

(2 files, 3 obsolete files)

As a part of bug 725932, I add handling messages in GeckoApp's Handler. We can probably move some of the Runnables we have as messages.

Upside:
No | new Runnable() | popping in in the middle.
| Handler.sendMessage(someMessage) | will replace them.

Downside:
Creating messages are cumbersome! :(
Bundle bundle = new Bundle();
bundle.putInt(x, y);

Message message = handler.obtainMessage();
message.setData(bundle);

handler.sendMessage(message);

If we can come up with an approach to reuse messages, as they are going to be sent again and again (say BrowserToolbar's title update), this is a really clean approach.

Can static messages solve the issue? If so, how much of space are we going to hold for all these "messages"?
OS: Mac OS X → Android
Hardware: x86 → ARM
Attached patch Patch (obsolete) — Splinter Review
This is the approach I'm thinking of. If this feels good, I can continue cleaning up GeckoApp with such messages.

I tried doing a message.recycle() in handleMessage(), but that seems to cause the App to hang. I don't know why.
Attachment #598451 - Flags: feedback?(mark.finkle)
Attachment #598451 - Flags: feedback?(blassey.bugs)
Comment on attachment 598451 [details] [diff] [review]
Patch

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

f- because you're asking about the approach and using the bundles is unnecessarily expensive. But really this is very close to what we want.

::: mobile/android/base/GeckoApp.java
@@ +1026,5 @@
>                  setLaunchState(GeckoApp.LaunchState.GeckoRunning);
>                  GeckoAppShell.sendPendingEventsToGecko();
>                  connectGeckoLayerClient();
>              } else if (event.equals("ToggleChrome:Hide")) {
> +                mMainHandler.sendMessage(HandlerMessage.BROWSER_TOOLBAR_HIDE);

I think you want to use mMainHandler.sendEmptyMessage(BROWSER_TOOLBAR_HIDE) here.

or, you could do:

Message.obtain(mMainHanlder, BROWSER_TOOLBAR, HIDE, 0).sendToTarget();


and a third option would be:
mBrowserToolbarHideMsg.sendToTarget();

where mMainToolbarHideMsg is constructed in onCreate() with:
mBrowserToolbarHideMsg = 
Message.obtain(mMainHanlder, BROWSER_TOOLBAR, HIDE, 0);

I think in most cases the third option is preferable because it avoids the cost of constructing a message every time we send it. But, the overall point here is to not use the bundle if possible.

@@ +2744,2 @@
>  
>              switch (type) {

And here you want to switch on message.what (what is the int field that gets set by the argument to sendEmptyMessage()). This allows us to not deal with the bundles at all.

@@ +2751,5 @@
> +                    mBrowserToolbar.show();
> +                    break;
> +
> +                case BROWSER_TOOLBAR_HIDE:
> +                    mBrowserToolbar.hide();

you could potentially combine these into:

case BROWSER_TOOLBAR:
    switch (message.arg1) {
        case SHOW:
            mBrowserToolbar.show();
            break;
        case HIDE:
            mBrowserToolbar.hide();
            break;
    }
Attachment #598451 - Flags: feedback?(blassey.bugs) → feedback-
Attached patch Patch (obsolete) — Splinter Review
I didn't check the documentation for other options of Messages :( and that made me use Bundles unnecessarily.

I've changed the approach to use different ways of obtaining Messages based on need.

I have few doubts with this patch:
1. I've used a class HandlerMessage with constants declared in it. Enum in java is a pain. Is it ok to use it this way?

2. I've used .obtainMessage(what, arg1, arg2) for BrowserToolbar, and .obtainMessage(what, new Boolean(value)) for FullScreen. Which of these is more desire-able? I can create two constant SHOW and HIDE and use them instead of 1's and 0's in arguments. (in which case arg2 would be UNUSED).
  -- Can these constants be inside HandlerMessage, if needed?
Attachment #598451 - Attachment is obsolete: true
Attachment #599763 - Flags: feedback?(blassey.bugs)
Attachment #598451 - Flags: feedback?(mark.finkle)
tracking-fennec: --- → +
Priority: -- → P3
Assignee: nobody → sriram
Comment on attachment 599763 [details] [diff] [review]
Patch

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

couple nits, but overall this the right approach

::: mobile/android/base/GeckoApp.java
@@ +1013,5 @@
>                  setLaunchState(GeckoApp.LaunchState.GeckoRunning);
>                  GeckoAppShell.sendPendingEventsToGecko();
>                  connectGeckoLayerClient();
>              } else if (event.equals("ToggleChrome:Hide")) {
> +                handlerMsg = mMainHandler.obtainMessage(HandlerMessage.BROWSER_TOOLBAR, 0, 0);

how about:

if (sToolbarHideMsg == null)
    sToolbarHideMsg = mMainHandler.obtainMessage(HandlerMessage.BROWSER_TOOLBAR, 0, 0);
sToolbarHideMsg.sendToTarget();

@@ +1019,3 @@
>              } else if (event.equals("ToggleChrome:Show")) {
> +                handlerMsg = mMainHandler.obtainMessage(HandlerMessage.BROWSER_TOOLBAR, 1, 0);
> +                mMainHandler.sendMessage(handlerMsg);

again, as above, I'd prefer not to recreate these messages everytime.

Also, I think it is cleaner to do msg.sendToTarget() than handler.sendMessage(msg);

@@ +2732,5 @@
>                      break;
>  
> +                case HandlerMessage.BROWSER_TOOLBAR:
> +                    int show = message.arg1;
> +                    if (show == 1)

no need for the show temporary, just:
if (message.arg1 == 1)
Attachment #599763 - Flags: feedback?(blassey.bugs) → feedback+
Messages are obtained from global pool and are recycled by android periodically.
Now say there are 2 requests for "BrowserToolbar - Show" (with a BrowserToolbar hide in between), when we try sending the message, our app will crash saying the message is already added to the queue. Hence it is highly not recommended to store messages in local variables as they cannot be reused. Also, it is an overload to store messages in the app -- given that we are going to go the Message way for doing something on UI thread.

I am happy removing the temporary variable. I just felt it would be self explanatory what the argument stands for.
Attached patch PatchSplinter Review
This cleans up the Runnables with Messages sent to the handler. I've removed quite a lot of "final" variables we used. They were need so that they can be referenced from another thread. Now that we know everything is running in UI thread, there is no need for them.
Attachment #599763 - Attachment is obsolete: true
Attachment #602109 - Flags: review?(mark.finkle)
Comment on attachment 602109 [details] [diff] [review]
Patch

From a code structure point of view, I don't like this approach. We are adding yet another "big-ass switch/if" block to the code. Instead of keeping the runnable code blocks with the logical code that uses them, we move the code to a far away place. This makes tracking the code-flow harder.

This is especially sad when most of the code is started from a JSON message in GeckoApp.handleMessage, calls a handler function and then ends up doing more work in GeckoAppHandler.handleMessage - just seems messier to me.

I think we need to discuss and think about this approach some more.
Attachment #602109 - Flags: review?(mark.finkle) → review-
Also, this is not blocking beta or release.
I don't think this goes back to GeckoAppHandler.handleMessage.

Currently the code is separated into this:
Whatever message we get from Gecko, is handled in handleMessage(). This runs in GeckoAppShell's thread.
Once all the background processing is done, we send a "message" to UI handler to ask it to do necessary UI changes for the message. We don't go back to GeckoAppShell here.

Any new message that we will add in the future will be straight forward. Do whatever we want to do in background thread in handleMessage(). Once done, send a message to UI thread to do the UI work.

I feel this is a better approach then doing everything in handleMessage().

Also, creating a new instance of Runnable is costly. Instead, we have one single static function taking care of the messages. Given that there are a lot of messages from Gecko, and lot of Runnables instantiated, its better to use a single static function and avoid instantiation.
Attached patch WIP (obsolete) — Splinter Review
This is the approach I had in mind. This is pretty much same as GeckoAsyncTask though. Both the background and UI tasks are enclosed closer. The method names are self explanatory. There is no need for messaging. This would be like creating an object and executing it right there.
Attachment #609447 - Flags: feedback?(mark.finkle)
Attachment #609447 - Flags: feedback?(blassey.bugs)
Attached patch WIPSplinter Review
Had uploaded a wrong patch earlier (lacking the GeckoMessageHandler file). This has been fixed in this patch.
Attachment #609447 - Attachment is obsolete: true
Attachment #609495 - Flags: feedback?(mark.finkle)
Attachment #609495 - Flags: feedback?(blassey.bugs)
Attachment #609447 - Flags: feedback?(mark.finkle)
Attachment #609447 - Flags: feedback?(blassey.bugs)
Comment on attachment 609495 [details] [diff] [review]
WIP

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

::: mobile/android/base/GeckoMessageHandler.java
@@ +10,5 @@
> +       onUiThread();
> +    }
> +
> +    public final void execute() {
> +        onBackground();

we probably need a guarantee that this is being run on the background thread
Attachment #609495 - Flags: feedback?(blassey.bugs) → feedback+
Comment on attachment 609495 [details] [diff] [review]
WIP

Is this still worthwhile in the current code?
Attachment #609495 - Flags: feedback?(mark.finkle)
Not much enthusiasm in comment 7, and this would probably need a new patch built from scratch anyway, so closing.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.