Closed Bug 818800 Opened 11 years ago Closed 11 years ago

Remove the global private browsing service in per-window PB builds

Categories

(Firefox :: Private Browsing, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files)

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?
Depends on: 769285, 769288
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.
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.
Oh, I see.  Thanks guys!  Should I file a bug to investigate if we can remove the removed-files.in file?  :-)
(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.
(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!
Blocks: PBnGen
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.
(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)
Depends on: 823574
Attached patch Patch (v1)Splinter Review
Attachment #694434 - Flags: review?(josh)
Attachment #694434 - Flags: review?(josh) → review+
Depends on: 823580
No longer depends on: 823574
Depends on: 823725
Depends on: 823732
Depends on: 823706
Depends on: 823945
Depends on: 823949
Depends on: 801823
I expect this to only fail pdf.js tests: https://tbpl.mozilla.org/?tree=Try&rev=113526f80a2e
(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.
We have now eliminated all of the dependencies on the interface too!
Attachment #696526 - Flags: review?(josh)
Comment on attachment 696526 [details] [diff] [review]
Part 2: Remove the interface as well

\o/

Send it through try for safety?
Attachment #696526 - Flags: review?(josh) → review+
Already have...
https://hg.mozilla.org/mozilla-central/rev/0acf066babea
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Depends on: 825509
Depends on: 826037
You need to log in before you can comment on or make changes to this bug.