1331290 - Refactor the code about closing all opening tabs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nevin Chen(Not active on Bugzilla)]

We have this snippet in a couple of places now. Add a Tabs.getInstance().closeAll() method for them

                final Iterable<Tab> tabs = Tabs.getInstance().getTabsInOrder();

                for (final Tab tab : tabs) {

                    Tabs.getInstance().closeTab(tab, false);

                }

Comment 1

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1331290 - Refactor the code about closing all opening tabs.

Review commit: https://reviewboard.mozilla.org/r/182050/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182050/

Comment 2

[Jan Henning [:JanH]]

Comment on attachment 8910618 [details]
Bug 1331290 - Refactor the code about closing all opening tabs.

https://reviewboard.mozilla.org/r/182050/#review187654

Seems fine, although I'd switch the condition variable around to use a more descriptive argument name.

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:107
(Diff revision 1)
>      private int mPrivateClearColor;
>  
> -    public void closeAll() {
> +    /**
> +     * @param isAllTabs true: close all tabs. false: only close private tabs
> +     */
> +    public void closeAllTabs(boolean isAllTabs) {

You're adding the param description, which is helpful, but I'd still think that the argument name itself could be made more descriptive in the first place, e.g. using something like `onlyPrivateTabs`.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/BaseTest.java:617
(Diff revision 1)
>              closeTab(tabID);
>          }
>      }
>  
>      public void closeAllTabs() {
> -        Tabs.getInstance().closeAll();
> +        Tabs.getInstance().closeAllTabs(true);

Because you're touching this - `closeAllTabs` should be annotated with `@RobocopTarget`.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8910618 [details]
Bug 1331290 - Refactor the code about closing all opening tabs.

https://reviewboard.mozilla.org/r/182050/#review187756

A quick drive-by review; apologies for the intrusion!

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:107
(Diff revision 1)
>      private int mPrivateClearColor;
>  
> -    public void closeAll() {
> +    /**
> +     * @param isAllTabs true: close all tabs. false: only close private tabs
> +     */
> +    public void closeAllTabs(boolean isAllTabs) {

Three points.

Firstly, in an instance method of `Tabs` you're calling `Tabs.getInstance()`. That's wrong; use `this`.

That you have this here means that there are no unit tests for this code!

Secondly, for a method this small, in a public API, it's way better to not use a boolean parameter at all — you can name it here, but in the caller it's just `closeAllTabs(isNormal())` or `closeAllTabs(false)`.

There are two good ways to do this kind of thing.

The first is to use an enum, so this becomes something like `closeTabsMatching(TabKind.Any)` or `closeTabsMatching(TabKind.Private)`. The caller is forced to be explicit about specifying the behavior. Java doesn't make this quite as neat as Rust or Swift, but it's better than a boolean.

The second is to split this into two methods:

```
public void closeAllTabs() {
    for (final Tab tab: this.mOrder) {
        this.closeTab(tab);
    }
}

public void closePrivateTabs() {
    for (final Tab tab: this.mOrder) {
        if (tab.isPrivate()) {
            this.closeTab(tab);
        }
    }
}
```

Explicit, simple, probably inlined in bytecode, and everyone's happy.

Thirdly, looping over `closeTab` in this way is really expensive, and this code is also wrong!

`closeTab` is synchronized per-call, it sends a message to Gecko, it stacks Undo toasts (that if tapped will do the wrong thing — it'll restore just the last tab, not all of them!), it modifies a `CopyOnWriteArrayList` once per tab, etc. etc.

`closeAllTabs` is probably worth implementing at the same 'level' as `closeTab`, with correct handling of all of those concerns — filter the tab list, then 'set' it and update state and send messages accordingly.

You don't have to do it in this bug, but it should be done.

Comment 4

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8910618 [details]
Bug 1331290 - Refactor the code about closing all opening tabs.

https://reviewboard.mozilla.org/r/182050/#review192550

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:107
(Diff revision 1)
>      private int mPrivateClearColor;
>  
> -    public void closeAll() {
> +    /**
> +     * @param isAllTabs true: close all tabs. false: only close private tabs
> +     */
> +    public void closeAllTabs(boolean isAllTabs) {

Thanks for the notice. I'm less concerned about the toast since we have 'showUndoToast' since bug 701725. But others are things I could improve. I'll use bug 1406671 to fix them.

Comment 5

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8910618 [details]
Bug 1331290 - Refactor the code about closing all opening tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182050/diff/1-2/

Comment 6

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8910618 [details]
Bug 1331290 - Refactor the code about closing all opening tabs.

Cancel the request cause previously forwarded to janH

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8910618 [details]
Bug 1331290 - Refactor the code about closing all opening tabs.

https://reviewboard.mozilla.org/r/182050/#review192832

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:109
(Diff revision 2)
>  
> -    public void closeAll() {
> +    // Close all tabs including normal and private tabs.
> +    @RobocopTarget
> +    public void closeAllTabs() {
> +        for (final Tab tab : mOrder) {
> +            this.closeTab(tab, false);

From my last set of comments, this change addresses:

- Incorrect use of `getInstance` here.
- Incorrect display of a stack of toasts.
- Naming.

This implementation still has the following limitations:

- Synchronization: each `closeTab` call is synchronized, but the whole operation isn't. That's needlessly expensive, and might be unsafe.
- Unnecessary messages sent to Gecko, and unnecessary work done in each individual `closeTab` call.
- No ability to undo closing multiple tabs.

This patch is better than what's already there, but IMO the work isn't finished; please file a follow-up to rewrite `closeAllTabs` and `closeAllPrivateTabs` to not call `closeTab`.

Comment 8

[Richard Newman [:rnewman]]

Cross-linking to the follow-up bug.

Comment 9

[Nevin Chen(Not active on Bugzilla)]

I'll land this next week

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50fbdae0bcd6
Refactor the code about closing all opening tabs. r=JanH,rnewman

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/50fbdae0bcd6