658010 - doc shell visibility check should check if the doc shell is active

Status: RESOLVED INVALID

(Core :: Layout, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

What situations is this function actually called in?  That is, why are we changing it to look at active at all?

And if it _should_ look at active, why does it care about being an offscreen browser if !active?

Comment 3

[Timothy Nikkel (:tnikkel)]

We check the visibility of doc shells for event and focus stuff (http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#4541 http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6691 http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsXULPopupManager.cpp#1446 http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1370 http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#4837) and at http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#3017 which only seems to be concerned with hidden tabs.

I'm not sure we _need_ to check IsActive in GetVisibility (after my other changes for removing views from decks, specifically the changes to nsIFrame::AreAncestorViewsVisible in bug 658005) but I think we should and I don't think it will hurt.

The offscreen browser stuff was added for the mobile browser back when it used the tile browser. My understanding is that the tile browser is basically dead code, but it is still in our tree. The tile browser painted web content to the screen using a canvas via drawWindow, so the docshell wasn't actually on screen, so they needed something to say that the browser was actually being drawn to the screen. See bug 454235. I'm basically just propagating the same check for consistency but ideally getting rid of the offscreen browser stuff and the unused tile browser is what should happen.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I'm basically just propagating the same check for consistency

I think my point is that an inactive offscreen browser should still be considered invisible... that's what inactive means.

Comment 5

[Timothy Nikkel (:tnikkel)]

My understanding of IsOffScreenBrowser is that it is an override of the normal ways of determining if a document is visible. So that if it is true you consider the docshell visible and ignore the normal methods of determining visibility. Is that not the case?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.  I don't know...

I can live with assuming it means that, I guess.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 533365 [details] [diff] [review]
patch

r=me

Comment 8

[Timothy Nikkel (:tnikkel)]

Due to other changes in how I'm doing this we don't need this so I'd rather not add the additional complexity. (It was always more of insurance, but I don't need the insurance now.)