Web Panel sidebars are meant to be used by extensions to display remote content in the sidebar, and when I create one then open a private window, it is displayed there, and when I open a non-private window from a private one, the web panel is opened in the new window too. I don't think that the web panel, and even the sidebar should be reopened when the parent window has an opposite privacy state.

Note: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#833

Not sure how best to test this, I do have a jetpack test for it already though. This patch could probably use more testing tho, and I'm not sure if it breaks any tests..

Comment on attachment 770527 [details] [diff] [review] Sidebars should not migrate across private / non-private window Review of attachment 770527 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense, but I'd like Gavin to also take a look at this. For testing this patch, you can write a browser-chrome test which opens a sidebar, and then opens a private window, and makes sure that the sidebar is not open in that window. The test can also open a regular window and ensure that the sidebar is present in that window.

Comment on attachment 770527 [details] [diff] [review] Sidebars should not migrate across private / non-private window Can you just add this new condition to the existing one, to avoid re-indenting everything? You could also lose the comment since it's pretty self-documenting. I doubt we have existing test coverage for this, but it seems like something that would be relatively easy to add as a browser chrome test. See e.g. browser/base/content/test/browser_private_browsing_window.js for a test that tests similar situations.

Just in case you are wondering why I changed the Make file, this is why..

Comment on attachment 772847 [details] [diff] [review] patch and tests >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >- if (window.opener && !window.opener.closed) { >+ if (window.opener && !window.opener.closed >+ && PrivateBrowsingUtils.isWindowPrivate(window) == PrivateBrowsingUtils.isWindowPrivate(window.opener)) { nit: we keep operators at the end of the line when wrapping conditions >- else { >+ else if (!window.opener || window.opener.closed) { I don't think you need to add this condition - if we're not restoring the opener's sidebox because of PB differences, we might as well open any default persisted ones, I figure. In any case this is an edge case so we shouldn't worry too much about it. >diff --git a/browser/components/privatebrowsing/test/browser/Makefile.in b/browser/components/privatebrowsing/test/browser/Makefile.in The reformatting should probably happen in a separate changeset. >diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_sidebar.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_sidebar.js >+ let window = Cc['@mozilla.org/appshell/window-mediator;1']. >+ getService(Ci.nsIWindowMediator). >+ getMostRecentWindow('navigator:browser'); Services.wm.getMostRecentWindow >+ ok(!!window, 'there is a window open at the start of these tests'); Though actually, this is kind of a silly check - you can just use "window" directly, and you don't need to assert its existence (browser chrome tests run in a chrome window scope). >+ function openSidebar(win) { >+ let { promise, resolve } = defer(); >+ let { document: doc } = win; I really hate this style - destructuring assignment for a single property is way harder to read than a simple property access (let doc = win.document) :)

Comment on attachment 773592 [details] [diff] [review] patch, tests, review fixes >diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_sidebar.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_sidebar.js >+function test() { >+ let window = Services.wm.getMostRecentWindow('navigator:browser'); As I tried to explain in my review comment, this line isn't necessary - this test code is running in a chrome browser window scope, so |window| already refers to the browser chrome window (and this variable is just shadowing the existing reference that has the same value). So you can just remove it. >+ function closeCachedWindows () { >+ if (windowCache.length == 0) { >+ return; >+ } >+ return closeWindow(windowCache.pop()).then(closeCachedWindows); Didn't notice this before but this is a rather inefficient way of closing windows. I suppose it doesn't matter too much for a test file, but I would skip the "wait for unload" bits and just put a windowCache.forEach(w => w.close()) at the end of the test.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #12) > Comment on attachment 773592 [details] [diff] [review] > patch, tests, review fixes > > >diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_sidebar.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_sidebar.js > > >+function test() { > > >+ let window = Services.wm.getMostRecentWindow('navigator:browser'); > > As I tried to explain in my review comment, this line isn't necessary - this > test code is running in a chrome browser window scope, so |window| already > refers to the browser chrome window (and this variable is just shadowing the > existing reference that has the same value). So you can just remove it. hmm weird, I am sure I tried that.. > >+ function closeCachedWindows () { > >+ if (windowCache.length == 0) { > >+ return; > >+ } > >+ return closeWindow(windowCache.pop()).then(closeCachedWindows); > > Didn't notice this before but this is a rather inefficient way of closing > windows. I suppose it doesn't matter too much for a test file, but I would > skip the "wait for unload" bits and just put a windowCache.forEach(w => > w.close()) at the end of the test.

I don't have commit privileges to push the change, so someone will need to do that for me, assuming they won't take authorship with mercurial.

Thanks for the help everyone!