762717 - Tab close should animate the counter

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sriram Ramasubramanian [:sriram]]

The tab closing had a nice little animation on the counter.
This is lost due to re-ordering of code some time back.
This wasn't noticed on phones, as the tabs-UI covered the url-bar.
This is pretty evident after the new refresh for tabs-UI.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch reorders the code.

Comment 2

[:Margaret Leibovic]

Comment on attachment 631212 [details] [diff] [review]
Patch

As I understand it, this fix works because it relies on the fact that this runnable runs before the runnable in selectTab that calls mBrowserToolbar.update(). I know we're not going to be able to fix the problems of all these racing runnables all over the place in this bug, but I think we should try to make things better if possible while we're messing around in here. This code is too fragile.

Comment 3

[Sriram Ramasubramanian [:sriram]]

So what makes this code fragile? I would like more explanation on that!
Long long ago, so long ago, there were crashes because
1. Tabs weren't recycled properly.
2. There was confusing over selecting the next tab

Now, next-tab is properly selected.
Ideally order for closing a tab is
1. get the tab that we want to select next.
2. close the tab
3. select the nextTab that's choosen.

This is exactly what the code does.

And regarding runnables. We don't have priority for runnables. Hence, there won't be races between runnables. closeTab's runnables completes before selectTab's runnable!

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

A better to patch to do the cleanup in the list before posting the runnable.
This still preserves my order -- closeTab's runnable followed by selectTab's runnable.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 631237 [details] [diff] [review]
Patch


>         GeckoApp.mAppContext.mMainHandler.post(new Runnable() { 

Add a comment above posting the runnable:

// Allow the browser toolbar to display the tab count before we select the new tab

>+        selectTab(nextTab.getId());

Add a comment here to let us know selectTab uses a runnable too:

// Runnable in selectTab will refresh the browser toolbar and must be queued after the tab count animation


The queuing of the runnables seems crucial for the animation to work. Why not just do this?

>       GeckoApp.mAppContext.mMainHandler.post(new Runnable() { 
>            public void run() {
>                notifyListeners(tab, TabEvents.CLOSED);
>                GeckoApp.mBrowserToolbar.updateTabCountAndAnimate(Tabs.getInstance().getCount());
>                GeckoApp.mDoorHangerPopup.updatePopup();
>                GeckoApp.mAppContext.hidePlugins(tab);
>                tab.onDestroy();
>+
>+               selectTab(nextTab.getId());
>            }
>        });

Comment 6

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #5)
> Comment on attachment 631237 [details] [diff] [review]
> Patch
> The queuing of the runnables seems crucial for the animation to work. Why
> not just do this?
> 
> >       GeckoApp.mAppContext.mMainHandler.post(new Runnable() { 
> >            public void run() {
> >                notifyListeners(tab, TabEvents.CLOSED);
> >                GeckoApp.mBrowserToolbar.updateTabCountAndAnimate(Tabs.getInstance().getCount());
> >                GeckoApp.mDoorHangerPopup.updatePopup();
> >                GeckoApp.mAppContext.hidePlugins(tab);
> >                tab.onDestroy();
> >+
> >+               selectTab(nextTab.getId());
> >            }
> >        });

The runnables are going to be queued, unless we post one of them to the front. I thought of moving this inside the runnable. However, it will be like,
Background thread -> Attach a runnable -> Attach a runnable.
Instead my code does the attaching of runnables in the background thread one after another, which feels nicer to me. (Basically runnables are holding the code that are not handled in this file).

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> (In reply to Mark Finkle (:mfinkle) from comment #5)
> > Comment on attachment 631237 [details] [diff] [review]
> > Patch
> > The queuing of the runnables seems crucial for the animation to work. Why
> > not just do this?
> > 
> > >       GeckoApp.mAppContext.mMainHandler.post(new Runnable() { 
> > >            public void run() {
> > >                notifyListeners(tab, TabEvents.CLOSED);
> > >                GeckoApp.mBrowserToolbar.updateTabCountAndAnimate(Tabs.getInstance().getCount());
> > >                GeckoApp.mDoorHangerPopup.updatePopup();
> > >                GeckoApp.mAppContext.hidePlugins(tab);
> > >                tab.onDestroy();
> > >+
> > >+               selectTab(nextTab.getId());
> > >            }
> > >        });
> 
> The runnables are going to be queued, unless we post one of them to the
> front.

Yes, as long as they are posted to the same Handler. In this case, they are, but you don't know that without looking at the impl of selectTab. It's implicit. That was the main point of the discussion on IRC.

> I thought of moving this inside the runnable. However, it will be like,
> Background thread -> Attach a runnable -> Attach a runnable.
> Instead my code does the attaching of runnables in the background thread one
> after another, which feels nicer to me. (Basically runnables are holding the
> code that are not handled in this file).

Looking nicer is certainly something I use as a reason to code, but in this case my preference is to be more explicit. Putting the call to selectTab in the runnable makes it very plain to see that the code will execute _after_ the browser toolbar animation.

Comment 8

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #3)
> So what makes this code fragile? I would like more explanation on that!

This is fragile for exactly the reasons mfinkle talked about above. This fix depends on knowing the implementation of selectTab, which could easily lead to other mistakes down the road. I think what this really indicates is that it would be optimal to decouple the browser toolbar updates from the places where we're actually managing the tabs. Unfortunately, it would require more invasive surgery to actually make a change like that, so in the meantime, I agree with mfinkle that we should try to be as explicit as we can be, and put a comment above the selectTab call to explain why order matters here.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Let me summarize the points that I had mentioned to mfinkle:
1. Runnables are blocks of coding that needs to be executed in another thread. We use them basically for UI thread, as most of the processing is in background thread.
2. There is only one UI thread for an activity, and we post it to that.
3. Any thread spawn is a background thread.
4. When runnables are posted, they are added to a queue one after another -- unless postAtFrontOfQueue() is used.

5.
> > >       GeckoApp.mAppContext.mMainHandler.post(new Runnable() { 
> > >            public void run() {
> > >                notifyListeners(tab, TabEvents.CLOSED);
> > >                GeckoApp.mBrowserToolbar.updateTabCountAndAnimate(Tabs.getInstance().getCount());
> > >                GeckoApp.mDoorHangerPopup.updatePopup();
> > >                GeckoApp.mAppContext.hidePlugins(tab);
> > >                tab.onDestroy();
> > >+
> > >+               selectTab(nextTab.getId());
> > >            }
> > >        });

This is wrong implementation. selectTab() might have some code to be run in the background thread, and some needs to be posted to UI thread. In this logic, closeTab() does something in background thread, posts a runnable to UI thread. Now calling selectTab() on the UI thread will make "that little piece of code that needs to be run on background thread" to be run on UI thread -- which is wrong. Hence, selectTab() should be outside the runnable.

6. General way to use runnables.
some_method() {
  .. code in this thread ..;
  post a runnable to another thread;
  .. code in this thread ..;
  post a runnable to another thread;
  .. code in this thread;
}
Any of these code chunks can be combined together if they are in same thread, provided it doesnt affect anything.

-----
7. The implementation I proposed is not fragile. It was there long long ago. It got changed, and I wanted to revert back.
8. The order of execution matters. When closing a tab,
  a. find the next tab to switch to.
  b. close the tab.
  c. select the found tab.
  Ideally we could do this:

  proxyCloseTab(whichTab) {
     nextTab = getNextTab(whichTab);
     closeTab(whichTab);
     selectTab(nextTab);
  }

  But there is some piece of code lurking around in GeckoApp, that forces to use parent as the nextTab. Hence,

   closeTab(whichTab, nextTab) {
      .. perform close tab operations.. ;
      selectTab(nextTab);
   }

This is straightforward to me.

9. "This fix depends on knowing the implementation of selectTab, which could easily lead to other mistakes down the road." - Definitely not. The implementation of selectTab() is independent of closeTab() and they should never depend on each other. That's how abstraction should be. Here the problem was in "sequence of steps", and not on the "logic performed in each method". If the selectTab() was still animating, things might be working now, but the "sequence of steps" is still wrong.
To quote an example:
Let's say Ian comes back and says we want a glass shattering animation for close tab.
The existing sequence will select a tab and then will try to close the tab. No one will be able to see the animation.
But my sequence will show the animation of close tab, and then select a tab.
In either case, the implementation doesn't matter. But the sequence of steps is important.

----

In short:
* Sequence of steps in this logic was wrong, which is what I changed.
* closeTab() doesn't need to know what selectTab() is going to do.
* Better to keep UI runnables and background logic separate. There was a proposal to use a custom class that does things clearly (runOnBackground() and runOnUI()). But it got dropped sometime back.

I don't want to fix this now. I'll wait for wes's patches to land, and post a patch after all the browser toolbar, tabs logic have been separated out.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Doorhangers needs to be cleared to let the GC feel there is nothing.
onDestroy() (basically reverse of instantiation) call be called in background thread.

Comment 11

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: selectTab() after closing

Patch rebased on top of wes's patches.

Comment 12

[:Margaret Leibovic]

Comment on attachment 632924 [details] [diff] [review]
Patch

Good catch.

Comment 13

[:Margaret Leibovic]

Comment on attachment 632925 [details] [diff] [review]
Patch: selectTab() after closing

Thanks for the detailed explanation above. Could you add to the comment here to indicate that we want to make sure selectTab is always called after we're done closing the tab? I wouldn't want the order here to get mixed up again by accident.

Comment 14

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/6257b5d30881
http://hg.mozilla.org/integration/mozilla-inbound/rev/37a3ca3fd518

Comment 15

[Matt Brubeck (:mbrubeck)]

Sorry, I backed this out because something in the push caused various failures in mochitest-1, 2, 3, and 8.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c820098cb62d

Comment 16

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.