Last Comment Bug 818800 - Remove the global private browsing service in per-window PB builds
: Remove the global private browsing service in per-window PB builds
Status: RESOLVED FIXED
: dev-doc-needed
Product: Firefox
Classification: Client Software
Component: Private Browsing (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: Firefox 20
Assigned To: :Ehsan Akhgari (busy, don't ask for review please)
:
Mentors:
Depends on: 769285 769288 801823 823580 823706 823725 823732 823945 823949 825471 825509 826037
Blocks: sdk-pwpb PBnGen
  Show dependency treegraph
 
Reported: 2012-12-05 22:55 PST by :Ehsan Akhgari (busy, don't ask for review please)
Modified: 2013-01-18 16:52 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (4.15 KB, patch)
2012-12-20 09:31 PST, :Ehsan Akhgari (busy, don't ask for review please)
josh: review+
Details | Diff | Review
Part 2: Remove the interface as well (1.28 KB, patch)
2012-12-29 11:44 PST, :Ehsan Akhgari (busy, don't ask for review please)
josh: review+
Details | Diff | Review

Description :Ehsan Akhgari (busy, don't ask for review please) 2012-12-05 22:55:32 PST
This bug is about excluding everything in http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/ from per-window private browsing builds.

One thing that I'm not sure how to properly do is how to handle removed-files.in.  Basically we need everything associated with the global PB service to go away in per-window PB builds.  This means that we should add nsPrivateBrowsingService.js and nsPrivateBrowsingService.manifest to removed-files.in.  We can put that behind an #ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING, so that's not a problem.  Should I also add nsIPrivateBrowsingService.xpt on that list?  Is it kosher to put this kind of stuff behind an #ifdef in removed-files.in?  Is there anything else that I need to watch out for?

One thing to note is that we're planning to use MOZ_PER_WINDOW_PRIVATE_BROWSING as a kill switch in case of a disaster.  If we need to flip that off when this gets to Aurora/Beta, can we just backout the change here and remove the removed-files.in entries and hope that everything will work correctly?
Comment 1 Mike Hommey [:glandium] 2012-12-05 23:01:36 PST
removed-files.in is so broken that i'm tempted to tell you to ignore it. Especially since all these files live in omni.ja anyways, and the removed-files.in contents would only be useful for flat or jar chrome format builds.
Comment 2 Robert Strong [:rstrong] (use needinfo to contact me) 2012-12-05 23:10:43 PST
btw: we no longer need removed-files.in for the vast majority of cases with Firefox 5 updating to anything greater than Firefox 5... it is only needed for Firefox 4 and below. If we could require Firefox 4 users to update to a specific release before updating to anything newer then this wouldn't be a problem.

I'll talk with a few people about this in the next day or two.
Comment 3 :Ehsan Akhgari (busy, don't ask for review please) 2012-12-06 11:51:16 PST
Oh, I see.  Thanks guys!  Should I file a bug to investigate if we can remove the removed-files.in file?  :-)
Comment 4 Robert Strong [:rstrong] (use needinfo to contact me) 2012-12-06 11:56:46 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> Oh, I see.  Thanks guys!  Should I file a bug to investigate if we can
> remove the removed-files.in file?  :-)
No. Update handles the vast majority of cases now but it doesn't handle all cases. Since requiring updates to a specific version for all platforms has been difficult to get buy in for I have started to take a different approach in bug 759469 but I haven't been able to get time to finish that up.
Comment 5 :Ehsan Akhgari (busy, don't ask for review please) 2012-12-06 12:03:00 PST
(In reply to comment #4)
> (In reply to Ehsan Akhgari [:ehsan] from comment #3)
> > Oh, I see.  Thanks guys!  Should I file a bug to investigate if we can
> > remove the removed-files.in file?  :-)
> No. Update handles the vast majority of cases now but it doesn't handle all
> cases. Since requiring updates to a specific version for all platforms has been
> difficult to get buy in for I have started to take a different approach in bug
> 759469 but I haven't been able to get time to finish that up.

I see.  Thanks for the explanation!
Comment 6 Erik Vold [:erikvold] (please needinfo? me) 2012-12-18 12:21:07 PST
So it appears that pwpb UI has landed in Nightly and the feature itself as well, but the global service remains.  The plan is to remove it tho I see, but I'm wondering if there is some good way to tell that the global pb service should not be used in the meantime? which could be used after this bug has closed.

Basically I am looking for something/code that will tell me when I should no longer use nsIPrivateBrowsingService.  Something like MOZ_PER_WINDOW_PRIVATE_BROWSING but available at runtime.
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2012-12-20 09:29:45 PST
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #6)
> So it appears that pwpb UI has landed in Nightly and the feature itself as
> well, but the global service remains.  The plan is to remove it tho I see,
> but I'm wondering if there is some good way to tell that the global pb
> service should not be used in the meantime? which could be used after this
> bug has closed.

I'm planning to remove it today.  :-)

> Basically I am looking for something/code that will tell me when I should no
> longer use nsIPrivateBrowsingService.  Something like
> MOZ_PER_WINDOW_PRIVATE_BROWSING but available at runtime.

The correct test would be:

  ("@mozilla.org/privatebrowsing;1" in Components.classes)
Comment 8 :Ehsan Akhgari (busy, don't ask for review please) 2012-12-20 09:31:06 PST
Created attachment 694434 [details] [diff] [review]
Patch (v1)
Comment 9 :Ehsan Akhgari (busy, don't ask for review please) 2012-12-21 10:01:02 PST
I expect this to only fail pdf.js tests: https://tbpl.mozilla.org/?tree=Try&rev=113526f80a2e
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2012-12-21 10:28:41 PST
(In reply to comment #7)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #6)
> > So it appears that pwpb UI has landed in Nightly and the feature itself as
> > well, but the global service remains.  The plan is to remove it tho I see,
> > but I'm wondering if there is some good way to tell that the global pb
> > service should not be used in the meantime? which could be used after this
> > bug has closed.
> 
> I'm planning to remove it today.  :-)

Looks like this did not happen yesterday.  There's a couple of more patches that need to get in before we can land the patch here.  Please see the dependency list of this bug.
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2012-12-29 11:44:59 PST
Created attachment 696526 [details] [diff] [review]
Part 2: Remove the interface as well

We have now eliminated all of the dependencies on the interface too!
Comment 12 Josh Matthews [:jdm] 2012-12-29 11:48:00 PST
Comment on attachment 696526 [details] [diff] [review]
Part 2: Remove the interface as well

\o/

Send it through try for safety?
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2012-12-29 12:47:30 PST
Already have...
Comment 14 :Ehsan Akhgari (busy, don't ask for review please) 2012-12-29 12:49:17 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/0acf066babea
Comment 15 :Ehsan Akhgari (busy, don't ask for review please) 2012-12-29 17:17:24 PST
https://hg.mozilla.org/mozilla-central/rev/0acf066babea

Note You need to log in before you can comment on or make changes to this bug.