The global PB service is going away. Ideally, the engine should be querying relevant docshells for this information instead.

Josh, do you have a timeline for this, or any plans for the transition? We're pretty swamped in Q1 (and probably Q2, too), so the more we know and the easier the transition, the less trouble we'll be in. Thanks!

The global service will not go away until every dependent bug in bug 463027 is fixed. The pb-per-window project is convenient because it can happen incrementally and in parallel with the existing global PB implementation. Don't worry about allocating resources towards this at the moment; I'll let you know if there's a burning need, and the usage here looks small enough that I can probably get a community volunteer to tackle it with your assistance.

Sounds perfect. Thanks, Josh!

It looks to me like the check in getAllIDs can be removed. Instead, onTab will need to be modified to check the source of the event - if it's a private window, then we should just return. Likewise, getAllTabs should skip any window that's private. I don't know what createRecord does; perhaps Richard can explain what needs to happen there.

Greg, is comment 4 something you can answer?

(In reply to Josh Matthews [:jdm] from comment #5) > Greg, is comment 4 something you can answer? Yes, but I'll answer it when I have a spare ten minutes :D

OK. If we're transitioning from a world in which private browsing is global (Sync's behavior: pretend there are no open tabs) to a world in which it's per-window (new behavior: ignore any window that's marked as private), we probably need to do something like this: * onTab(): check source of event. * Adjust listeners in observe(). * getAllIDs(): return empty if all windows are in private browsing mode. * getAllTabs(): return non-private-browsing tabs. * createRecord(): builds on top of getAllTabs().

(In reply to Richard Newman [:rnewman] from comment #7) > * onTab(): check source of event. This needs to call PrivateBrowsingUtils.isWindowPrivate on the window object belonging to the tab. We should also replace the call to PBPrefs.get("autostart") with PrivateBrowsingUtils.permanentPrivateBrowsing (here and elsewhere in this code). > * Adjust listeners in observe(). observe() will only need to do something #ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING. > * getAllIDs(): return empty if all windows are in private browsing mode. We can check whether all windows are private by enumerating the existing windows using Services.wm.getEnumerator("navigator:browser") and calling PrivateBrowsingUtils.isWindowPrivate on all of them. > * getAllTabs(): return non-private-browsing tabs. It's enough to check whether each window is private, and skip it if it is. No need to delve into each tab really. > * createRecord(): builds on top of getAllTabs(). Exactly. The PB check here can be removed.

(In reply to Ehsan Akhgari [:ehsan] from comment #8) > (In reply to Richard Newman [:rnewman] from comment #7) > > * onTab(): check source of event. > > This needs to call PrivateBrowsingUtils.isWindowPrivate on the window object > belonging to the tab. We should also replace the call to > PBPrefs.get("autostart") with PrivateBrowsingUtils.permanentPrivateBrowsing > (here and elsewhere in this code). I'm not really sure on how to get the window of the tab from the event. I tried with |event.originalTarget.ownerDocument.defaultView| and |event.originalTarget.linkedBrowser.contentWindow|, but neither seems to work.

Comment on attachment 677918 [details] [diff] [review] Patch v1 >+ if (!PrivateBrowsingUtils.isWindowPrivate(window)) { >+ window.tabs.forEach(function(tab) { If you invert the condition and return instead, you can avoid all of the indentation changes here.

Comment on attachment 677918 [details] [diff] [review] Patch v1 Review of attachment 677918 [details] [diff] [review]: ----------------------------------------------------------------- Looks good in general. I have some comments below. ::: services/sync/modules/engines/tabs.js @@ +123,5 @@ > let currentState = JSON.parse(Svc.Session.getBrowserState()); > let tabLastUsed = this.tabLastUsed; > currentState.windows.forEach(function(window) { > + if (!PrivateBrowsingUtils.isWindowPrivate(window)) { > + window.tabs.forEach(function(tab) { What Josh said. @@ +192,5 @@ > + let allWindowsArePrivate = true; > + let wins = Services.wm.getEnumerator("navigator:browser"); > + while (wins.hasMoreElements()) { > + if (!PrivateBrowsingUtils.isWindowPrivate(wins.getNext())) { > + allWindowsArePrivate = false; You can break out of the loop here, since all it takes for allWindowsArePrivate to be false is one non-private window. @@ +320,5 @@ > } > }, > > onTab: function onTab(event) { > + let win = event.originalTarget.ownerDocument.defaultView; event.originalTarget is the tab object. In order to get the window corresponding to it, you can use event.originalTarget.linkedBrowser.contentWindow.

I updated the suggested changes, however this patch still not work because: > let currentState = JSON.parse(Svc.Session.getBrowserState()); > let tabLastUsed = this.tabLastUsed; > currentState.windows.forEach(function(window) { > if (!PrivateBrowsingUtils.isWindowPrivate(window)) { > return; > } The window object here is the session store window data object, not the DOM object. So, I think we need a way to get the window object from this data or add new flag 'isPrivate' to the stored session data. Any idea?

(In reply to Andres Hernandez [:andreshm] from comment #12) > Created attachment 678635 [details] [diff] [review] > Patch v2 > > I updated the suggested changes, however this patch still not work because: > > > let currentState = JSON.parse(Svc.Session.getBrowserState()); > > let tabLastUsed = this.tabLastUsed; > > currentState.windows.forEach(function(window) { > > if (!PrivateBrowsingUtils.isWindowPrivate(window)) { > > return; > > } > > The window object here is the session store window data object, not the DOM > object. So, I think we need a way to get the window object from this data or > add new flag 'isPrivate' to the stored session data. Any idea? Hmm, you could use the flag you're adding in bug 722985 here, I guess.

Comment on attachment 680093 [details] [diff] [review] Patch v3 Sync patch requires r+ from Sync peer.

Comment on attachment 680093 [details] [diff] [review] Patch v3 Review of attachment 680093 [details] [diff] [review]: ----------------------------------------------------------------- I'm happy with the code. addl-rev gps for build peery kind of things.

Headsup to Tracy: can we make sure that tabs sync in private browsing mode makes sense going forward? I will watch which flags you set with keen interest! :D Thanks!

Comment on attachment 680093 [details] [diff] [review] Patch v3 Review of attachment 680093 [details] [diff] [review]: ----------------------------------------------------------------- Makefile changes look good. For the record, I don't like having preprocessor magic for per window private browsing. It seems very fragile and prone to bugs. Is there a plan to phase out having different types of behavior governed by a build-time variable?

Oh, and here's a follow-up comment: please run services/sync/tests/tps/test_privbrw_tabs.js (indeed, the whole TPS suite) before you land this, and ask myself and jgriffin for review for any necessary changes. TPS itself might need revising in a similar way, in which case we can wait for a follow-up bug.

(In reply to comment #18) > Makefile changes look good. For the record, I don't like having preprocessor > magic for per window private browsing. It seems very fragile and prone to bugs. > Is there a plan to phase out having different types of behavior governed by a > build-time variable? The flag is necessary to have a flip switch in case per-window PB can't make it the first time we try to ship it. I will remove the flag and all of the code hidden behind it on the next train which ships per-window PB first.

(In reply to comment #19) > Oh, and here's a follow-up comment: please run > > services/sync/tests/tps/test_privbrw_tabs.js > > (indeed, the whole TPS suite) before you land this, and ask myself and jgriffin > for review for any necessary changes. TPS itself might need revising in a > similar way, in which case we can wait for a follow-up bug. Hmm, you probably need to convert that test into a per-window PB test as well.

Push backed out for one of the changesets causing xpcshell failures of form: https://tbpl.mozilla.org/php/getParsedLog.php?id=17110009&tree=Mozilla-Inbound Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d168b55e74ba

I just updated the patch to fix the issue reported in Bug 811490 Comment 6. Now all xpcshell tests are working fine locally.