Closed Bug 1142034 Opened 5 years ago Closed 4 years ago

[e10s] Don't show "Restore All Crashed Tabs" when only one tab open

Categories

(Firefox :: General, defect, minor)

35 Branch
x86_64
Linux
defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
e10s m8+ ---
firefox42 --- fixed

People

(Reporter: jonathan, Assigned: ursula)

Details

Attachments

(2 files, 1 obsolete file)

The button is out of place when only one tab is open and crashed.

[2nd case, if not fixed with this needs new bug report - dom.ipc.processCount>1 when only one tab crashes.]
tracking-e10s: --- → ?
Assignee: nobody → ursulasarracini
Bug 1142034 - Don't show 'Restore All Crashed Tabs' when only one tab open
Attachment #8638072 - Flags: review?(mconley)
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

https://reviewboard.mozilla.org/r/14033/#review12547

::: browser/base/content/browser.js:1145
(Diff revision 1)
> +      if (gBrowser.tabs.length == 1) {

Ursula and I just talked about this line in IRC - this only checks to see if there are other tabs in this window, and doesn't take into account crashed tabs in other windows (nor does it take into account that there might be non-crashed tabs in this window).

What I've suggested she do instead is to expose a crashedTabCount on SessionStore that she can check here.
Attachment #8638072 - Flags: review?(mconley)
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

Bug 1142034 - Don't show 'Restore All Crashed Tabs' when only one tab open
Attachment #8638072 - Flags: review?(mconley)
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

https://reviewboard.mozilla.org/r/14033/#review12687

This is definitely the right idea. I'm pretty stoked about this. Just two minor suggestions before we're good to go.

::: browser/components/sessionstore/SessionStore.jsm:1448
(Diff revision 2)
> +    if (this._crashedBrowsers.has(browser.permanentKey)) {
> +      this._crashedBrowsersCount--;
> +    }

I actually think we probably want to do this in onTabClose. I believe onTabRemove is called when a tab is removed from a window - even when that tab is being dragged into a new window.

::: browser/base/content/browser.js:1142
(Diff revision 2)
>        TabCrashReporter.onAboutTabCrashedLoad(gBrowser.getBrowserForDocument(event.target));
> +
> +      // If there was only one tab open that crashed, do not show the "restore all tabs" button
> +      if (SessionStore.crashedTabCount == 1) {
> +        TabCrashReporter.hideRestoreAllButton(gBrowser.getBrowserForDocument(event.target));
> +      }

Now that I think about it, we might as well just make this part of onAboutTabCrashedLoad. Perhaps something like:

```
TabCrashReporter.onAboutTabCrashedLoad(gBrowser.getBrowserForDcoument(event.target), {
  crashedTabCount: SessionStore.crashedTabCount,
});
```

and then in onAboutTabCrashedLoad:

```
onAboutTabCrashedLoad: function (aBrowser, aParams) {
  // ...
  if (aParams.crashedTabCount == 1) {
    // If there was only one tab open that crashed, do not show the "restore all tabs" button 
    ...
  }
},
```

That way, we leave it up to TabCrashReporter to decide on how it wants to deal with the number of crashed tabs.
Attachment #8638072 - Flags: review?(mconley)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #4)
> Comment on attachment 8638072 [details]
> MozReview Request: Bug 1142034 - Don't show 'Restore All Crashed Tabs' when
> only one tab open
> 
> https://reviewboard.mozilla.org/r/14033/#review12687
> 
> This is definitely the right idea. I'm pretty stoked about this. Just two
> minor suggestions before we're good to go.
> 
> ::: browser/components/sessionstore/SessionStore.jsm:1448
> (Diff revision 2)
> > +    if (this._crashedBrowsers.has(browser.permanentKey)) {
> > +      this._crashedBrowsersCount--;
> > +    }
> 
> I actually think we probably want to do this in onTabClose. I believe
> onTabRemove is called when a tab is removed from a window - even when that
> tab is being dragged into a new window.
> 

The reason I put this in onTabRemove was because if you close the entire window at once with multiple crashed tabs open, onClose will call onTabRemove for as many tabs as were opened, and so the crashed browsers count will be decremented by that number whereas onTabClose won't be called when the window closes.
Attachment #8638072 - Flags: review?(mconley)
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

Bug 1142034 - Don't show 'Restore All Crashed Tabs' when only one tab open
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

https://reviewboard.mozilla.org/r/14033/#review12705

::: browser/base/content/browser.js:1176
(Diff revision 3)
>          for (let browserWin of browserWindows()) {
> -          for (let tab of browserWin.gBrowser.tabs) {
> +          SessionStore.reviveAllCrashedTabs(browserWin.gBrowser.tabs);
> -            SessionStore.reviveCrashedTab(tab);
> -          }
>          }

Sorry, I should have been more clear when we talked in person; I meant that we should have SessionStore do the looping over the tabs _and_ the windows as well.

So this should just be:

```
SessionStore.reviveAllCrashedTabs();
```

And then reviveAllCrashedTabs would be:

```
reviveAllCrashedTabs: function() {
    let windowsEnum = Services.wm.getEnumerator("navigator:browser");

    while (windowsEnum.hasMoreElements()) {
      let window = windowsEnum.getNext();
      // restore each tab in win.gBrowser.tabs

    }
  },
}
Attachment #8638072 - Flags: review?(mconley)
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

Bug 1142034 - Don't show 'Restore All Crashed Tabs' when only one tab open
Attachment #8638072 - Flags: review?(mconley)
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

https://reviewboard.mozilla.org/r/14033/#review12713

This looks good - just one nit to remove.

We should get some tests for this - we can probably add them to this file: https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser_crashedTabs.js

::: browser/components/sessionstore/SessionStore.jsm:2210
(Diff revision 4)
> +    // Reset the number of crashed browsers.

I don't think this comment adds much, tbh
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

Bug 1142034 - Regression tests for new tabcrashed page
Attachment #8638072 - Attachment description: MozReview Request: Bug 1142034 - Don't show 'Restore All Crashed Tabs' when only one tab open → MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page
Attachment #8638072 - Flags: review+ → review?(mconley)
Bug 1142034 - Don't show 'Restore All Crashed Tabs' when only one tab open
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

Bug 1142034 - Regression tests for new tabcrashed page
Comment on attachment 8639551 [details]
MozReview Request: Bug 1142034 - Don't show 'Restore All Crashed Tabs' when only one tab open

https://reviewboard.mozilla.org/r/14213/#review12885
Attachment #8639551 - Flags: review+
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

https://reviewboard.mozilla.org/r/14033/#review12887

This looks great! Just some suggestions.

::: browser/components/sessionstore/test/browser_crashedTabs.js:349
(Diff revision 6)
> +add_task(function test_hide_restore_all_button() {

Because this function yields, it's a "generator", and we need to denote it with a * in the declaration, like this:

```
add_task(function* test_hide_restore_allButton() {
 // ...
});
```

::: browser/components/sessionstore/test/browser_crashedTabs.js:364
(Diff revision 6)
> +  let restoreAllButton = content.document.getElementById("restoreAll");
> +  let restoreOneButton = content.document.getElementById("restoreTab");

Because "content" is a shortcut to the current browser content window, and because that's a CPOW in e10s mode, it wouldn't surprise me if this will go away in the future.

Let's try to future proof, and use this instead of "content":

```
let doc = browser.contentDocument;

let restoreAllButton = doc.getElementById("restoreAll");
let restoreOneButton = doc.getElementBy("restoreTab");
```

::: browser/components/sessionstore/test/browser_crashedTabs.js:368
(Diff revision 6)
> +  is(restoreOneButton.getAttribute("class"), "primary", "Restore Tab button should be of class primary");

I think checking the classList with "has" would probably be more resilient to things like theme fiddling in the future, like:

ok(restoreOneButton.classList.has("primary"),
   "Restore Tab button should have the primary class");

::: browser/components/sessionstore/test/browser_crashedTabs.js:380
(Diff revision 6)
> +  is(!(restoreOneButton.getAttribute("class"), "primary"), "Restore Tab button should not be of class primary");

ok(!(restoreOneButton.classList.has("primary")), "Restore Tab button should not have the primary class");
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

Bug 1142034 - Regression tests for new tabcrashed page
Attachment #8638072 - Flags: review?(mconley)
Attachment #8639551 - Attachment is obsolete: true
I'm afraid you knocked out your fix patch again in this push. Remember that you don't need to use -c unless it's just a single changeset that's being reviewed / landed.
Flags: needinfo?(ursulasarracini)
Bug 1142034 - Don't show 'Restore All Crashed Tabs' when only one tab open
Note to self: MozReview /= Bugzilla
Flags: needinfo?(ursulasarracini)
Comment on attachment 8639944 [details]
MozReview Request: Bug 1142034 - Don't show 'Restore All Crashed Tabs' when only one tab open

https://reviewboard.mozilla.org/r/14257/#review12895
Attachment #8639944 - Flags: review+
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

https://reviewboard.mozilla.org/r/14033/#review12897

Looks good! Let's get a green try run, and then checkin-needed. Thanks!
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

https://reviewboard.mozilla.org/r/14033/#review12901

::: browser/components/sessionstore/test/browser_crashedTabs.js:369
(Diff revision 7)
> +  ok(restoreOneButton.classList.has("primary"), "Restore Tab button should have the primary class");

Argh - I goofed. According to your try run, classList.has doesn't exist. And it's right - should be classList.contains.

Can you make those changes, ensure it runs and passes locally, and then trigger another try job? You can also cancel the rest of your current try job.
Attachment #8638072 - Flags: review+
Attachment #8638072 - Flags: review?(mconley)
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

Bug 1142034 - Regression tests for new tabcrashed page
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

https://reviewboard.mozilla.org/r/14033/#review12909

r=me with green try run. Great job!
Attachment #8638072 - Flags: review?(mconley) → review+
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

Bug 1142034 - Regression tests for new tabcrashed page
Attachment #8638072 - Flags: review+ → review?(mconley)
Errr - I had to change a test here's the new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9ea228bb2a3
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

Bug 1142034 - Regression tests for new tabcrashed page
Comment on attachment 8638072 [details]
MozReview Request: Bug 1142034 - Regression tests for new tabcrashed page

https://reviewboard.mozilla.org/r/14033/#review12921

r=me with a green try run. Thanks!
Attachment #8638072 - Flags: review?(mconley) → review+
url:        https://hg.mozilla.org/integration/fx-team/rev/ec8ea44bdb05ee4b0efcb8d55fb888b206a179b5
changeset:  ec8ea44bdb05ee4b0efcb8d55fb888b206a179b5
user:       Ursula <usarracini@mozilla.com>
date:       Thu Jul 23 14:47:35 2015 -0400
description:
Bug 1142034 - Don't show 'Restore All Crashed Tabs' when only one tab has crashed. r=mconley

url:        https://hg.mozilla.org/integration/fx-team/rev/c69643cd3502c2b24987890954959594bb7ad0e9
changeset:  c69643cd3502c2b24987890954959594bb7ad0e9
user:       Ursula <usarracini@mozilla.com>
date:       Fri Jul 24 17:56:02 2015 -0400
description:
Bug 1142034 - Regression test. r=mconley

Makes sure we show the "Restore All Tabs" button at the right time.
https://hg.mozilla.org/mozilla-central/rev/ec8ea44bdb05
https://hg.mozilla.org/mozilla-central/rev/c69643cd3502
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.