User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre There are various attributes and properties - from both the browser and the tab - that are lost when a tab is dragged into a different window. This includes favicon (bug 449730), feed details (icon in urlbar), link to search provider (button glow) and, I suspect, others like fastfind and added by extensions. But most importantly of all, the busy status of the tab - which affects the title of the tab, the throbber, and the stop button. My patch (attachment 360942 [details]) in bug 449730 works in simple tests but looked dangerous to add after the final beta as (a) This is used all over the interface and I don't know the code well enough to know if it would cope with adding a "busy" tab onto the tabbar without warning, and (b) I am not convinced that there couldn't be a state change after the listeners of the old tab have been removed but before the listeners for the new tab have been added. Reproducible: Always

Boris, Mano: are we losing these attributes at the browser or platform level?

The "busy" attribute is maintained entirely inside tabbrowser.xml. So swapBrowsersAndCloseOther should probably be copying it or something.

AFAICT the tab element itself is not duplicated in the new window, see http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?mark=936-946#932 so it would be at the browser level. Concerning (b) in comment 0, I don't think that is possible as dnd effectively blocks _any_ other events from being sent/processed. There also seems to be code in the swap that ensures that progressListeners are not fired, although I haven't read through it all.

And probably updating mIsBusy for the two tabbrowsers as needed...

Mano: can you take this? See comment 2, comment 3 and comment 4.

(In reply to comment #4) > And probably updating mIsBusy for the two tabbrowsers as needed... There is certainly an hour glass left in the first window if you detach a loading tab from it.

(In reply to comment #0) > This includes favicon (bug 449730), feed details (icon in urlbar), link to > search provider (button glow) and, I suspect, others like fastfind and added by > extensions. favicon works by swapping "mIconURL", and I can make good progress on RSS and search if I copy "feeds" and "engines" when I detach a tab; It looks as if fastfind was already handled and I suspect that there is no correct answer for those added by extensions.

John: dunno if Mano's taking this or not, but sounds like you're working up a patch. Are you?

(In reply to comment #8) > John: dunno if Mano's taking this or not, but sounds like you're working up a > patch. Are you? No! I was just investigating what worked if I swapped the fields by adding them to fieldstoswap and discovered that the RSS and search seemed to work. I am pretty sure that this is not the correct place to add the code - as I don't think that all browsers have the "feeds" and "engines" fields - but I don't know where is. As to the effect of adding/detaching a "busy" tab, this definitely needs someone more familiar with the code.

I'm not sure I like the idea of changing to a blacklist rather than a whitelist here... are we really missing that many attributes/properties at the moment? Explicitly having to opt-out of attribute copying seems more dangerous than having to opt-in, and we could make fieldsToSwap a public member (with a better name) to allow extensions to add stuff to it if that capability is really needed, no?

I don't think that copying custom js properties from one tab to another can harm anything. Once adoptNode is implemented for xbl, we would have to use a blacklist either way, and remove the whitelist. The tab-attributes part is, indeed, a little more dangerous. That said, I looked over all attributes which are set in tabbrowser.xml, and the blacklist is pretty much complete as far as I can tell.

And no, we shouldn't force extension authors to do any work on their side, its our bug and thus our job.

My point is that I think we should choose between whitelist vs. blacklist for our known attributes/properties based on whichever one would be smaller. Is the blacklist really smaller than the whitelist? We can't really know whether copying extension-added attributes is safe. You're assuming that copying them is more likely to fix bugs than it is to cause them, but I'm not sure that is a safe assumption.

(In reply to comment #10) > Created an attachment (id=363488) [details] For me this copies 'engines' but doesn't copy 'feeds'. This still doesn't handle the status bar text properly; I suspect that properties from the mTabProgreessListener (esp. mMessage) also need to be copied; Those in the section marked: // cache flags for correct status bar update after tab switching

(In reply to comment #12) > The tab-attributes part is, indeed, a little more dangerous. That said, I > looked over all attributes which are set in tabbrowser.xml, and the blacklist > is pretty much complete as far as I can tell. It seems to want to copy things like 'first-tab' and 'last-tab' which are presumably set in tabbox.xml.

Yeah, I should black-list this too. Maybe we should use a whiteless-model for the tab attributes, and a no-list model for the browser-js-properties. Werid, it did copy "feeds" for me. Can you post STR or debug this on your build?

(In reply to comment #17) > Werid, it did copy "feeds" for me. Can you post STR or debug this on your > build? This is on Windows 1.9.1 branch. 1. Open window 2. Open second tab 3. Go to https://www.addons.mozilla.org/ 4. Detach tab to new window No RSS icon. DomI and alert show 'feeds' as null. 'feeds' was on list of copied attributes. Looking further the value is correct before the swapFrameLoaders and null after; Presumably you will need to keep otherFieldValues and, optionally, ourFieldValues as in the original code to preserve the values around that swap.

Comment on attachment 363488 [details] [diff] [review] patch After chatter on #developers, mano's gonna be coming up with a new patch here that will require review from bz and mconnor.

The backend changes are on bug 480149.

Bug 480149 has been backed out of the branch; once that's fixed, AIUI, we'll also need a new patch here, right?

--> P1, as this bug will require the wider feedback of a beta release or is of sufficient complexity that we should be looking at it sooner, not later.

Bug 480149 is now fixed1.9.1 - when can we expect progress here?

Changing summary to reflect the current goal of this bug.

Also semi-fix the icon-patch now that the backend issued is semi-fixed.

Comment on attachment 368697 [details] [diff] [review] patch >diff --git a/toolkit/content/widgets/browser.xml b/toolkit/content/widgets/browser.xml > <method name="swapDocShells"> >+ // We need to swap fields that are tied to our docshell or related to >+ // the loaded page >+ // Fields which are built as a result of notifactions (pageshow/hide, >+ // DOMLinkAdded/Removed, onStateChange) should not be swapped here, >+ // becuase these notifcations are dispatched again once the docshells >+ // are swapped. Tha said, see bug 458697 for onStateChange, we >+ // work around that in tabbrowser. typo: notifications, because, That This reference to bug 458697 doesn't really seem relevant here. Should be possible to write a test for this (both icon and busy state), right? Ideally we'd test the non-DOMLinkAdded favicon case too, but I suppose it might not be currently possible to get a favicon.ico in the server root.

Just dump the favicon in testing/mochitest and add it to _SERV_FILES in the sibling Makefile.in; I don't see a reason why we couldn't have one. Only thing I can think of (not an issue, just an are-we-covering-potential-bases thought) that might be odd is if we ever want favicon.ico to not be idempotent, or if we want it to vary across proxy-faked servers, but if we need it it's easy enough to hack that up in testing/mochitest/server.js when we have a demonstrable need.

Don't we still need the other part of attachment 361480 [details] [diff] [review] (bug 449730)? That is: - this.setTabTitle(aOurTab); + if (aOurTab.hasAttribute("busy")) + this.setTabTitleLoading(aOurTab); + else + this.setTabTitle(aOurTab);

Ah yes, we probably do. good catch!

Mano: what's the update on getting the patch w/test ready for checkin?

Test upcoming. Note that I'm using gBrowser.setIcon directly (same goes for the busy state), meaning I'm testing neither DOMLinkAdded nor favicon.ico. That's out of the scope of this bug. I'll file a bug on testing favicons in general though.

Comment on attachment 371259 [details] [diff] [review] patch with tests looks good here.

Using gBrowser.setIcon directly isn't an ideal test... why not just load a page with a <link rel="favicon"> to actually test that we don't clobber those values? Also, why are you using objects with handleEvent members rather than just passing in functions? Makes the test rather confusing...

Given That I cannot actually test the busy state even, and that favicon.ico is the only other option (which I'm not even sure on how to set given the async behavior), I decided to go ahead and just use setIcon. I don't think using setIcon makes it easier to clobber. As for using handleEvent within an object, that's the only way I know of which allows to actually remove the listener in its function.

event.currentTarget.removeEventListener(event.type, arguments.callee, true/false);

Backed out; this caused Mac unit test timeouts, as far as I can tell (at the very least, it was timing out reliably until this was backed out).

Mano: do we know what caused the unit test failures?

Here is what happens: 1. My test didn't close the detached window 2. The test that is failing (private browsing) opens a window, then tries to open a context menu, but for some reason, due to (1), that windows is not focused, therefore opening the context menu fails, therefore the test timed out. Thus I'm landing my patch again with the following fixed: 1. Close the detached window 2. Alter the private browsing test to focus its window, before it tries to open a context menu. I don't think this test can or should assume that the window is focused.

Mano: sounds like a solid plan. Get that together and submit to tryserver so we can do a test run?

Uh, Mano?

I tried landing it twice on the try server, but the mac unit test box there doesn't work most of the time :-/

Can you please post the WIP patch here so that someone else can try it? We might be able to get someone to run the tests locally on their Mac, for example.

OK, this didn't break the private browsing test on MozillaTry, but the mac machine there seems to be very unreliable. I'll try to land this now and see how it goes

Passed. http://hg.mozilla.org/mozilla-central/rev/7f08e631236b http://hg.mozilla.org/mozilla-central/rev/1fe4e8cab24a

Verified fixed on trunk and 1.9.1 with builds on OS X and Windows: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090420 Minefield/3.6a1pre ID:20090420031158 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090420 Shiretoko/3.5b4pre ID:20090420031111