<dbaron> CTho, right now tabbrowser doesn't really communicate to Gecko which tab is active and which isn't <dbaron> CTho, it probably should, formally <CTho> dbaron: oh. is there an API for that already? <dbaron> CTho, no, there isn't an API

So if you look at GetVisibility (on nsIBaseWindow), that seems to be pretty much what we want. We could try to teach the tree owner to be smarter or teach tabbrowser to communicate more with the tree owner. Or something... Note that in an actual embedding situation, with an embeddor-implemented tree owner, this is all a non-issue -- just teach your tree owner about your tab implementation. This bug is about the xul treeowner implementation in particular, right?

I've thought GetVisibility would be better if there were a SetVisibility rather than trying to poke around and figure it out.

Making an observation with MS's VBA API vs FF. VBA doesn't provide enough information for this type of coding to check active/visible/locked/enabled tabs. For reference: a VBA tab control which might be what IE's tab browser feature is based on (and all of IE's options windows), you only get the current tab's index number (0->?) as a reference for the active tab which is set = to main control's value. Only problem is that there no simple code to check 'if IsActive(mytabName or mytabIndexNumber) Then yyy' to check active status. It stores the active tab # after a tab change. You cannot just check to see if something is visible by a function, but you have to do some unnecessary logic like 'if tab.index(tab.index).Name = xxx then tab.index(tab.index).Visible = False' and you have to check all index and logic possibilities. And, If you need logic based on that specific tab vs another, then you have to know the logic about which tab is active by checking something like the title of the tab before you can decide what action to take. I imagine both the Tools>Options and Tools>Add-on's panels, and now the CTRL-TABs Panel as well as tabbrowser are coded differently, but they are all basically similar UI constructs to the user. [and the common API is probably the reason MS was so quick to add Tab-Browsing because everything in IE uses the same API for all UI interfaces] I would think having a more powerful target goal API for this could be more useful down the road for future generations of FF, since so many pieces of FF are tab/panel/window type contructs, [ie. coded different multiple times, but similar logic ;-)].

As a note, somewhere we probably want to handle nsIDocShell::isOffscreenBrowser http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShell.idl#485 Since it seems to be a flag saying "offscreen, but treat it as onscreen"

WIP patch. This seems to work. However, one problem is that, when opening a link in a background tab, the presshell gets instantiated before the docshell is set to be inactive. This is a problem, because it means that we'll request decode on everything in bug 512260. I'm hoping bz can tell me a way for presshells to start out inactive if they're opened in a background tab.

Er, CCing bz, since I just asked him a question in the previous comment.

I'm confused. The presshell is instantiated but there is no DOM or anything, right? So there's nothing to request decode on.... Then you set it inactive. Or am I missing something?

Oh, and this still needs some way for non-current-firefox embeddings (e.g. e10s) to set things up correctly via nsIWebBrowser-and-friends.

(In reply to comment #7) > I'm confused. The presshell is instantiated but there is no DOM or anything, > right? So there's nothing to request decode on.... Then you set it inactive. > Or am I missing something? Shrug - as long as you think there's not much possibility for race-y behavior, then it's fine by me.

I doubt there really is.

working on this again. Unbitrotted the patch. Seems to work as before.

Added a patch that handles embedding. Flagging gavin for review, bz for sr. Pushed to try as c97faaddb0b0.

Comment on attachment 459483 [details] [diff] [review] patch v3 Why do you need to qualify the GetPresShell call? You shouldn't need that. Please add the new attribute at the end of the interface. You need to rev the docshell iid. For the presshell's mIsActive, just put it on nsIPresShell, please. That will avoid the virtual function calls, and we'll need a getter for it anyway. Or will SetIsActive on the presshell need access to PresShell members (as opposed to nsIPresShell ones)?

Updated patch addressing bz's review comments. Reflagging.

pushed to try as c97faaddb0b0, since the old try push apparently got cancelled anyway.

Comment on attachment 459528 [details] [diff] [review] patch v4 >+++ b/layout/base/nsIPresShell.h >+ virtual nsresult SetIsActive(PRBool aIsActive) >+ { >+ mIsActive = aIsActive; >+ return NS_OK; >+ } >+ >+ virtual nsresult GetIsActive(PRBool *rv) >+ { >+ *rv = mIsActive; >+ return NS_OK; >+ } >+ I was thinking more like: void SetIsActive(PRBool aIsActive) { mIsActive = aIsActive; } PRBool IsActive() { return mIsActive; } with none of that COM-alike jazz. ;) Though the setter might need to be virtual and be implemented as nsIPresShell::SetIsActive in nsPresShell.cpp for the stuff we want to do with refresh drivers. But we can worry about that when we're hooking those up. Inline for now. >+++ b/layout/base/nsPresShell.cpp >+ // Get our activeness from the docShell. That chunk of code should probably move into a helper function and run both in Init and in Thaw. Otherwise if the active state of a docshell changes while the presshell is in bfcache and then you hit the back button the presshell's state will be wrong. We could use tests for that if possible...

addressed bz's new comments. Flagging.

Comment on attachment 460086 [details] [diff] [review] patch v5 sr=bzbarsky

Comment on attachment 460086 [details] [diff] [review] patch v5 >--- a/browser/base/content/tabbrowser.xml >+++ b/browser/base/content/tabbrowser.xml >@@ -722,28 +722,31 @@ > > <method name="updateCurrentBrowser"> > <parameter name="aForceUpdate"/> > <body> > <![CDATA[ > var newBrowser = this.getBrowserAtIndex(this.tabContainer.selectedIndex); > if (this.mCurrentBrowser == newBrowser && !aForceUpdate) > return; >+ newBrowser.docShell.isActive = true; Can you do this right below or above newBrowser.setAttribute("type", "content-primary");?

I just realized something. For subframes, shouldn't we be inheriting activeness from the parent?

Added a patch addressing bz and dao's latest comments.

This blocks bug 512260, which blocks a blocker (bug 563088). Nominating.

The way comment 20 was addressed is wrong in several ways: 1) You probably don't want to cross type boundaries when setting active on the kids. 2) It doesn't handle the bfcache restoration case correctly, as far as I can see. 3) It doesn't handle navigation from one page with subframes to another correctly...

(In reply to comment #23) > The way comment 20 was addressed is wrong in several ways: > > 1) You probably don't want to cross type boundaries when setting active on the > kids. What type boundaries are being crossed? It's an nsIDocShell method calling other nsIDocShell methods, no? > 2) It doesn't handle the bfcache restoration case correctly, as far as I can > see. > 3) It doesn't handle navigation from one page with subframes to another > correctly... I confess to not understand these comments. This code is more or less the same as before, all we're doing now is passing along the call to any descendants we might have rather than leaving them in the dark. What is it about having descendants that made this work before but not now? Maybe you can explain it to me on IRC if bmo is too cumbersome for that.

> It's an nsIDocShell method calling other nsIDocShell methods, no? Yes. So if someone calls SetActive(PR_TRUE) on the root chrome docshell for a Firefox window, it will happily call SetActive(PR_TRUE) on every single tab in that window. That seems wrong to me... but maybe it's ok. > I confess to not understand these comments. Here's an example. Say I have a tab. It's not the currently active tab, so its docshell is marked inactive. Now the page in that tab executes |window.location.href = "some url"|. The new url has a subframe. This creates a new docshell which is a child of our docshell. This docshell defaults to being active, and no one tells it otherwise. Its presshell likewise thinks its active. > What is it about having descendants that made this work before but not now? Nothing. Descendants didn't work at all correctly before.... I we're ok with crossing the chrome/content boundary, I think the right way to handle this is to propagate the active state to kids in SetDocLoaderParent and RestoreFromHistory (see where we make the SetAllowJavascript calls).

Updated patch, handling bz's comments the right way.

Tests, which triple-check that we're doing this properly. ;-)

So this bug should be in good shape now. I spent part of yesterday and all of today sorting through some issues in order to test this stuff properly, but now it's handled and we have tests that pass. I pushed this to try as 6be97fbeeca9, after which point I'll reflag for reviews and then hopefully land all this stuff.

Comment on attachment 461077 [details] [diff] [review] patch v7 Looks green on try with the tests included. Flagging for review. Hopefully this is the last time. ;-)

So for docshell, in RestoreFromHistory I think the code is wrong. What it should do instead is walk the list of kids and explicitly call SetIsActive(mIsActive) on them all. Otherwise subframes of bfcached pages will end up with the active state from before they went into bfcache.

Oh, and the rest of the patch looks great.

Comment on attachment 461077 [details] [diff] [review] patch v7 So with that change, sr=me

Comment on attachment 461079 [details] [diff] [review] tests v1 I modified the test to test the path bz brought up, and it still passes when it should fail. I'm investigating this now, but for the mean time I'm canceling review.

I just stepped through the part of the test I added that should fail but doesn't. What seems to be happening is that we call GoForward(), get to nsDocShell::RestorePresentation(), and then bail because we don't have a saved presentation: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#6555 I'll investigate this more tomorrow, but if anyone has any insight on what might cause this to be not saved, that would be very helpful. The page has two iframes, one of which has its own iframe (so 3 levels deep), if that makes a difference.

That part shouldn't matter. You want to step into the CanSavePresentation called under the location set that navigates to pg3 to see what's going on there...

Also, a chrome test might have been more appropriate for the docshell bits than a browser test. Probably ok either way, though.

Uploaded new tests. These correctly detect the error in the previous patch (and thus fail). These took me a long time to get right. First of all, mIsDocumentLoaded gets set in the post-fire load handler, so "load" event handlers get called when it's still false. This was a problem, because it meant that we were trying to navigate to a new page before the old one was completely done, meaning that we threw away the presentation instead of stashing it in the bfcache. So now we call executeSoon() on all the callbacks we use to progress through the test. Furthermore, I was getting screwed up by "load" not firing on back/forward. Thanks to gavin for pointing me towards "pageshow" and "pagehide". Reflagging dolske for review on these.

Added a patch that fixes the bug bz pointed out, however it doesn't quite follow his instructions. > What it should do instead is walk the list of kids and explicitly call > SetIsActive(mIsActive) on them all. I'm pretty sure this isn't necessary. Right before the code in question, we call AddChild(): http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#6908 This calls AddChildLoader(): http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2997 Which calls SetDocLoaderParent(): http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsDocLoader.cpp#721 This is code that the patch already modifies to set activeness correctly. bz - can you confirm this? Is there any reason to walk the children explicitly?

> Which calls SetDocLoaderParent(): Ah, which calls SetIsActive on self, and hence will call it on descendants too. OK, good. I don't know what I was thinking when I made that comment. ;) I do think it's worth adding a comment in RestorePresentation that explains that we're purposefully inheriting parent's active state. Other than that, this looks great.

Added the comment that bz wanted. This patch should be 100% ready to go now, pending gavin's review. Flagging.

Comment on attachment 461367 [details] [diff] [review] tests v2 >diff --git a/docshell/test/navigation/Makefile.in b/docshell/test/navigation/Makefile.in >+_BROWSER_TEST_FILES = \ These will fail in Fennec until we implement isActive setting there, right? Need a ifneq (mobile,$(MOZ_BUILD_APP)) here and a followup filed on getting this into Fennec as well, if so.

Comment on attachment 461585 [details] [diff] [review] patch v9 r=me on the browser/ part.

This blocks beta 5, the feature freeze.

Pushed the code bits to mc: http://hg.mozilla.org/mozilla-central/rev/b7836c3a63db I'll land the tests later when I get review.

Comment on attachment 461367 [details] [diff] [review] tests v2 I'm just rs+ing this on grounds that I don't see anything obviously terrible. If there was something you specifically wanted sanity checked, flag me^H^Hjoe for review again. Side note: probably better to either preemptively check in with tests or wait for full reviews... code without tests in the tree is just asking for trouble. :)

I filed bug 585771 and bug 585780 to get support for this into fennec and e10s.

Fixed the fennec build problems gavin pointed out. This is green on try.

Pushed the tests to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/f1e252898bca

Documentation updated: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIWebBrowser https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDocShell And mentioned on Firefox 4 for developers page.