The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 20

Status

()

Firefox
Private Browsing
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
Firefox 20
x86
Mac OS X
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 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

4 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

4 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

4 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!
Blocks: 748604
(Assignee)

Updated

4 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

4 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

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

Updated

4 years ago
Attachment #694434 - Flags: review?(josh) → review+
(Assignee)

Updated

4 years ago
Depends on: 823580
(Assignee)

Updated

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

Updated

4 years ago
Depends on: 823725
(Assignee)

Updated

4 years ago
Depends on: 823732
(Assignee)

Updated

4 years ago
Depends on: 823706
(Assignee)

Updated

4 years ago
Depends on: 823945
(Assignee)

Updated

4 years ago
Depends on: 823949
(Assignee)

Updated

4 years ago
Depends on: 801823
(Assignee)

Comment 9

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

Comment 10

4 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

4 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

4 years ago
Already have...
(Assignee)

Comment 14

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0acf066babea
(Assignee)

Comment 15

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

Updated

4 years ago
Depends on: 825471

Updated

4 years ago
Depends on: 825509
(Assignee)

Updated

4 years ago
Depends on: 826037

Comment 16

4 years ago
Ehsan, where's the developer doc for the new API, and the warning that the old API is removed?

I can't find anything at all on
https://developer.mozilla.org/en-US/docs/Firefox_20_for_developers
http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/public/nsIPrivateBrowsingService.idl
http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/public/nsIPrivateBrowsingServiceObsolete.idl
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIPrivateBrowsingService
https://developer.mozilla.org/en-US/docs/Supporting_private_browsing_mode

All of these pages, and others, should point to a page that explains how to use the new API. But I can't find anything.
Keywords: dev-doc-needed
I'll try to update those. Meanwhile: https://developer.mozilla.org/en-US/docs/Supporting_per-window_private_browsing and https://developer.mozilla.org/en-US/docs/Updating_addons_broken_by_private_browsing_changes are where it's at.
You need to log in before you can comment on or make changes to this bug.