648045 - Mark the active tab in minimized windows as inactive

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

We do all sorts of aggressive throttling for background tabs, but we define "background" as "not currently selected in the tabbrowser".  We should consider extending this definition to all tabs in minimized windows.

Gavin, whom should I cc or where should I put this bug to get some traction?

Comment 1

[[Baboo]]

I guess this and bug 648046 should be platform-independent?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Mark the currently selected tab in a minimized window as inactive.

I considered exposing nsGlobalWindow::DispatchCustomEvent instead (on nsPIDOMWindow).  Please let me know if you would prefer that, ok?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 543045 [details] [diff] [review]
Mark the currently selected tab in a minimized window as inactive.

Should be pretty easy to add a test, right?

Comment 4

[Boris Zbarsky [:bzbarsky]]

I actually have no idea how to add a test for this... which part of it would be testable?  That the event is fired on minimize?  Or that the docshell is set to not be active on minimize?  How can I even trigger minimize in a test?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

I was thinking just testing that the tabbrowser's tab's docShell state is consistent and correct after a chromeWindow.minimize()/focus() etc.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.  Let me try that.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with test

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 543066 [details] [diff] [review]
Now with test

stick a PD license header on the test too?
https://www.mozilla.org/MPL/boilerplate-1.1/pd-c

Comment 9

[Boris Zbarsky [:bzbarsky]]

> stick a PD license header on the test too?

Done.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 543066 [details] [diff] [review]
Now with test

nsPIDOMWindow::DispatchCustomEvent could be useful also in other cases.

So r+ either way.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 12

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3abc0dbbf710

Comment 13

[Boris Zbarsky [:bzbarsky]]

And also http://hg.mozilla.org/integration/mozilla-inbound/rev/9e4ab3907b29 to merge the test...

Comment 14

[Marco Bonardo [:mak]]

backed out due to permaorange in the test, sounds like something needs better merging

Comment 15

[Marco Bonardo [:mak]]

this is after the push in comment 13:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_minimize.js | Docshell should be active - Got undefined, expected true
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_minimize.js | Docshell should be inactive - Got undefined, expected false
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_minimize.js | Docshell should be active again - Got undefined, expected true

Comment 16

[Boris Zbarsky [:bzbarsky]]

OK, the second changeset is not really needed, since that's working with tabbrowser, not browser.

The first changeset passes tests on Mac and Windows, but on Linux unminimizing seems to not mark as active?  Testing that now.

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK, on Linux minimize/restore are very async and sort of not in sync with window state changes.  I'll rewrite the test to handle that.

Comment 18

[Karl Tomlinson (:karlt)]

I'm not sure about this, but the sizemode attribute on the document element (if it even gets set) may be more in sync with window state changes than windowState.

Comment 19

[Boris Zbarsky [:bzbarsky]]

We don't set sizemode dynamically (except on very small screens during browser startup) as far as I can tell.

Comment 20

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/annotate/dd9a2ec82f68/dom/tests/mochitest/chrome/sizemode_attribute.xul
tests dynamic sizemode changes on WINNT.

It is apparently broken on Mac (bug 409095).

There seems to be some dynamic change happening on X11.  sizemode gets set to maximized at least, but part of the test fails, perhaps due to timing (fullscreen is not the correct event to wait for) or perhaps due to incomplete implementation.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> tests dynamic sizemode changes on WINNT.

Ah, interesting.  Looks like we do update it in some cases (e.g. nsXULWindow::SavePersistentAttributes), but we don't do that for minimized sizemode.

In any case, I just made the test poll for the right docshell active state; if that fails to get set the test will time out.

http://hg.mozilla.org/integration/mozilla-inbound/rev/5decde435e4a

Comment 22

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/5decde435e4a

Comment 23

[[e-mail]]

Need to document the new sizemodechange event, no?

Comment 24

[Eric Shepherd [:sheppy]]

Updated documentation:

https://developer.mozilla.org/en/Gecko/Gecko_event_reference
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDocShell

Also updated Firefox 8 for developers.

Comment 25

[Nickolay_Ponomarev]

I moved the docs for the sizemodechange event to https://developer.mozilla.org/en/XUL/Events#Window_events , since that's where other top-level-window events were documented (even though it probably works with HTML-as-the-top-level-doc).

Note that the event is fired way too often at least on Mac (bug 715867).