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

RESOLVED FIXED in Firefox 20

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({dev-doc-needed})

unspecified
Firefox 20
x86
macOS
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
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?
(Assignee)

Updated

6 years ago
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.
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 5

6 years ago
(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!
(Assignee)

Updated

6 years ago
Blocks: 463027
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.
(Assignee)

Comment 7

6 years ago
(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
(Assignee)

Comment 8

6 years ago
Created attachment 694434 [details] [diff] [review]
Patch (v1)
Attachment #694434 - Flags: review?(josh)
Attachment #694434 - Flags: review?(josh) → review+
(Assignee)

Updated

6 years ago
Depends on: 823580
(Assignee)

Updated

6 years ago
No longer depends on: 823574
(Assignee)

Updated

6 years ago
Depends on: 823725
(Assignee)

Updated

6 years ago
Depends on: 823732
(Assignee)

Updated

6 years ago
Depends on: 823706
(Assignee)

Updated

6 years ago
Depends on: 823945
(Assignee)

Updated

6 years ago
Depends on: 823949
(Assignee)

Updated

6 years ago
Depends on: 801823
(Assignee)

Comment 9

6 years ago
I expect this to only fail pdf.js tests: https://tbpl.mozilla.org/?tree=Try&rev=113526f80a2e
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
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!
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+
(Assignee)

Comment 13

6 years ago
Already have...
(Assignee)

Comment 15

6 years ago
https://hg.mozilla.org/mozilla-central/rev/0acf066babea
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Depends on: 825471

Updated

6 years ago
Depends on: 825509
(Assignee)

Updated

6 years ago
Depends on: 826037
You need to log in before you can comment on or make changes to this bug.