582244 - (pb) Implement Private Browsing

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement, P2)

Description

[Irina Sandu]

Implement Private Browsing and add button for it.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Is private browsing needed on a mobile device?

Comment 2

[Aakash Desai [:aakashd]]

Moving this to an enhancement.

Comment 3

[Aakash Desai [:aakashd]]

Looks like users are requesting this as a feature as well: http://input.mozilla.com/en-US/search/?product=mobile&sentiment=suggestions&date_end=&date_start=&version=4.0b3&q=history+

Comment 4

[Thomas Arend [:tarend]]

Ragavan: "Sync already respects Private Browsing mode in desktop Firefox. If you are running in Private Browsing mode on desktop, Sync will not sync anything other than stuff you explicitly bookmark or store. So, if Fennec implements Private Browsing, I'd imagine we'd do the same thing."

Madhava: could you add a design proposal to this bug? Thanks!!

Comment 5

[Thomas Arend [:tarend]]

Madhava: could you take a stab at UI design for private browsing? Thanks

Comment 6

[Madhava Enros [:madhava]]

I'll think about this a little for phones; the framing is a little different, as phones aren't shared much -- this is more about things ending up synced to other places.

On tablets, the traditional framing around device sharing is definitely there; in fact, I think tablets are going to be me _more_ shared than laptops or desktop computers (the tablet on the coffeetable case). In some ways, what we want, rather than a private browsing mode, is better multi-user support including a "guest" login that would act like PBM.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

We can use multiple profiles on mobile. The MobileProfiles add-on allows this now.

Comment 8

[Thomas Arend [:tarend]]

Users described their desire for a "private browsing mode" as: "pause" sync and also stop recording browsing history temporarily, so that some web pages visited while in "private browsing mode" would neither show up in the Awesome Screen nor on their other computers via sync.

Comment 9

[Daniel C]

Comment 1 (mfinkle) and comment 6 (madhava) have a false assumption that private browsing is only useful on a shared device.

Firefox Mobile works best when using Sync. Therefore any rationale that applies to using private browsing on the desktop / laptop also applies to mobile devices, otherwise all private sites visited on the phone / tablet will suddenly appear on all the other devices as well.

Even on desktop Firefox, people use private browsing without sharing the computer. It means you can log in to a site with different credentials without logging out of the current session (or use the site without being logged in - useful for Google searches that you don't want saved to Web History), or just generally preventing things like this happening when someone looks over your shoulder or you want to show someone something: http://img101.imageshack.us/img101/9743/fileyn0.jpg

Currently, the options are to use Firefox Mobile without Sync, or switch to another browser each time you want to visit something that you don't want synced.

Comment 10

[Ian Melven :imelven]

Comment 9 from Daniel Cater reinforces my personal view there's a case for two different models of private browsing on mobile (after a brief discussion with mbrubeck). 

One use case is the one he describes, private browsing identical to the desktop. This preserves the users addons/bookmarks etc. and lets them browse in a mode where nothing is recorded.

The other is something like a 'guest mode', ie you hand your tablet to a friend. In this case, the user possibly would NOT want their guest to have their existing history in the awesome bar, bookmarks etc. This could be implemented by switching to a new blank profile and could possible be implemented in an add on (based on mfinkle's profile switching add on). 

I think these two modes both have valid use cases and ideally, both would end up being implemented.

Comment 11

[Ian Melven :imelven]

i tried out the MobileProfiles addon, with some UI/UX work and a change to always create a new blank profile when entering a 'guest mode' and switch back to the previous one when exiting, it seems like it would satifisy that use case. the only caveat is that switching profiles requires restarting the browser.

Comment 12

[Daniel C]

(In reply to Ian Melven :imelven from comment #11)
> i tried out the MobileProfiles addon, with some UI/UX work and a change to
> always create a new blank profile when entering a 'guest mode' and switch
> back to the previous one when exiting, it seems like it would satifisy that
> use case. the only caveat is that switching profiles requires restarting the
> browser.

Could the "guest mode" feature be separated out into another bug? It has a separate use case and requires a different solution. Desktop Firefox doesn't have it, whereas it does have Private Browsing so that is a parity-desktop thing. I believe that people expect Private Browsing to exist on Firefox Mobile (I certainly did) and I worry that if you conflate that with "Guest Mode" then it will just delay getting this bug fixed.

Comment 13

[Philip Clarke]

A general query though, the desktop version requires closing down all windows/ tabs, will the mobile version be implementing this or will it pop a new private window while still having access to the non-private ones ? When I swipe right, how would a private window be highlighted ? a padlock could be confused with SSL (a ghost icon ?) would PB automatically send do not track headers ? I think it probably should since mobile was ahead of the game in implementing it. With a guest mode, is one getting to the stage of locking the browser with a login screen since it's primary purpose is to stop the device's history etc... being visible. Probably guest mode is better as a menu option as in one is explicitly handing one's device over but selecting the mode first, a guest would still with the current proposals be able to see history etc,,, by just turning off their guest session, so the you go back to the locking profiles with a password option..

Comment 14

[Ian Melven :imelven]

(In reply to Philip Clarke from comment #13)
> A general query though, the desktop version requires closing down all
> windows/ tabs, will the mobile version be implementing this or will it pop a
> new private window while still having access to the non-private ones ? When
> I swipe right, how would a private window be highlighted ? a padlock could
> be confused with SSL (a ghost icon ?) 

these are all good questions, the UI/UX for private browsing on mobile is so far undetermined, but i think it's pretty key to the feature (just like it was on the desktop). additionally, it probably makes sense to have a different UX on a tablet where you have more room for UI/indicators etc than on the phone where by necessity there isn't much browser chrome (it even scrolls off the screen when scrolling down a page). 

> would PB automatically send do not
> track headers ? I think it probably should since mobile was ahead of the
> game in implementing it. 

i'm not totally sure and need to think about it more but right now my initial feeling is that DNT and private browsing are orthogonal and address different user needs. 

> With a guest mode, is one getting to the stage of
> locking the browser with a login screen since it's primary purpose is to
> stop the device's history etc... being visible. Probably guest mode is
> better as a menu option as in one is explicitly handing one's device over
> but selecting the mode first, a guest would still with the current proposals
> be able to see history etc,,, by just turning off their guest session, so
> the you go back to the locking profiles with a password option..

fennec already has master password support for locking profiles.

in a discussion earlier, another spin on 'guest mode' was 'i don't care if a guest can see my browsing history etc. i just don't want them to be able to use my saved passwords', which actually would different it even further from private browsing (and not necessarily require a full profile switch and consequent restart). 

also i agree with comment #12, this bug is about private browsing in mobile Firefox
as it exists in desktop Firefox today. while we might discuss guest mode and other ideas as tangents in here, any work along those lines belongs in a new, different bug.

Comment 15

[Paul [pwd]]

Desktop is currently redesigning it's PB UX in bug 463027, it may be a good opportunity to discuss the plans between the various UX teams involved in bringing PB to fruition and see if we can go for a unified experience across all platforms?

Comment 16

[Ian Melven :imelven]

one obstacle to porting desktop private browsing to mobile private browsing is that desktop private browsing is not e10s-ready at the current time. i am hoping (and will possibly work on) that this will be addressed as part of the changes to make private browsing per window (bug 463027)

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Ian Melven :imelven from comment #16)
> one obstacle to porting desktop private browsing to mobile private browsing
> is that desktop private browsing is not e10s-ready at the current time. i am
> hoping (and will possibly work on) that this will be addressed as part of
> the changes to make private browsing per window (bug 463027)

Firefox Mobile on Android (Native) is not e10s anymore, so that should lower the obstacle a bit.

Comment 18

[Daniel C]

For what it's worth, it would appear that the stock browser on Android Ice Cream Sandwich (4.0) has a private browsing mode:

"View multiple open pages, swipe to get rid of those you're finished with, and enter incognito mode for private browsing." - http://www.google.co.uk/nexus/#/features

Comment 20

[Alex Keybl [:akeybl]]

Just wanted to reiterate why I think this is important (as filed in bug 716734)

* we now have flash support
* the crash-stats URLs we receive across the board point to certain private sites being very popular among our users
* the main Android demographic is young men (61% male according to http://www.intomobile.com/2011/08/23/study-says-61-of-android-app-users-male-59-of-blackberry-female/ and 60% under 34 according to http://blog.mobclix.com/2010/11/17/mobclix-index-android-marketplace/)

Comment 22

[Chris Lord [:cwiiis]]

Adding uiwanted, we'd like to have this in Firefox Mobile 17.

Comment 23

[Brian Nicholson (:bnicholson)]

Chris is busy with other bugs, so I'll be taking a look at this.

Comment 24

[(no longer active)]

(In reply to comment #23)
> Chris is busy with other bugs, so I'll be taking a look at this.

Brian, as I had previously mentioned to Chris and mfinkle, I think we should first get the per-window private browsing implementation into a good shape before attempting to port this feature to mobile, in order to avoid having to rewrite the implementation when desktop Firefox switches to per-window PB.  Are you in the loop on those discussions?

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Ehsan Akhgari [:ehsan] from comment #24)
> (In reply to comment #23)
> > Chris is busy with other bugs, so I'll be taking a look at this.
> 
> Brian, as I had previously mentioned to Chris and mfinkle, I think we should
> first get the per-window private browsing implementation into a good shape
> before attempting to port this feature to mobile, in order to avoid having
> to rewrite the implementation when desktop Firefox switches to per-window
> PB.  Are you in the loop on those discussions?

We are starting with just porting over a nsIPrivateBrowsingService impl to start. We currently have none. The desktop impl is not 100% portable. Once we get the service working, we want to see what pieces of mobile have zero PB support. For example, we impl our own history service. We also have Reader Mode, which creates a reading list and caches data. Our session store might need work.

Also, UX wants mobile to move to per-tab PB so we'll need to look at what other platform PB checks need to move to the docshell approach.

Lastly, I don't know if Product wants to wait for per-window to be functional before shipping PB on mobile. It's one of their higher priorities.

Comment 26

[(no longer active)]

(In reply to comment #25)
> (In reply to Ehsan Akhgari [:ehsan] from comment #24)
> > (In reply to comment #23)
> > > Chris is busy with other bugs, so I'll be taking a look at this.
> > 
> > Brian, as I had previously mentioned to Chris and mfinkle, I think we should
> > first get the per-window private browsing implementation into a good shape
> > before attempting to port this feature to mobile, in order to avoid having
> > to rewrite the implementation when desktop Firefox switches to per-window
> > PB.  Are you in the loop on those discussions?
> 
> We are starting with just porting over a nsIPrivateBrowsingService impl to
> start. We currently have none. The desktop impl is not 100% portable. Once we
> get the service working, we want to see what pieces of mobile have zero PB
> support. For example, we impl our own history service. We also have Reader
> Mode, which creates a reading list and caches data. Our session store might
> need work.

OK, it makes sense.  Note that a large number of the platform pieces have been moved over to the per-window API, and at this point we should avoid adding usages of nsIPrivateBrowsingService to read the current global value.  I can help you guys to figure out how to use the per-window API if you need to.

Also, I would appreciate if the mobile folks ask for my r? on the PB patches.

> Also, UX wants mobile to move to per-tab PB so we'll need to look at what other
> platform PB checks need to move to the docshell approach.

Oh OK.  You can look at the list of dependency list of bug 463027 to find all of the platform pieces (by just ignoring the browser/ pieces.)  I think that should be a comprehensive list of all of the platform stuff.

Oh, and there is now the dependency list of bug 787743 which I am working on to fix -- but you can consider that work done, since that's my highest immediate priority at this point.

> Lastly, I don't know if Product wants to wait for per-window to be functional
> before shipping PB on mobile. It's one of their higher priorities.

I was not talking about implementing per-window PB on desktop -- I was talking about finishing the platform bits, since at this point there is very few of them left, and it just doesn't make sense to ship something based on the current global service (speaking with my module owner hat on :-)

At any rate, I'd be glad to help you guys out whenever needed.

Comment 27

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Import private browsing component

Imports the PrivateBrowsing component from desktop (no changes).

Comment 28

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Implement about:privatebrowsing

Comment 29

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Mobile private browsing changes

I removed view-source and page info window code. I also removed about:blank transition; instead, this just closes all open tabs and uses new ones. I didn't touch removeDataFromDomain, but it looks like that's only used by about:permissions (which we don't have yet).

I pretty much had these patches done before Ehsan suggested not using the global private browsing flags, so I'm just uploading what I have now. I can make these changes now if necessary, or I can take care of them in a follow-up so we can land this ASAP (just to get some PB feedback).

Comment 30

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Private browsing native UI

Simple menu UI for entering/exiting private browsing. We can improve the UI in a follow-up.

Comment 31

[Brian Nicholson (:bnicholson)]

Attachment: Part 5: Don't update history with private browsing enabled

Prevents history, favicons, and thumbnails from being updated or added. History can still be deleted. Bookmarks can still be added/edited/deleted.

Comment 32

[Brian Nicholson (:bnicholson)]

Attachment: Part 6: Save private browsing session to Android bundle

Since Android can kill us any time we're in the background, we need to be able to restore the session state when we resume. This is normally done via the SessionStore component, but since we can't save the session to disk in private browsing mode, we need to save the state into the Android bundle. This patch pulls it from Gecko whenever onSaveInstanceState() is called. When we restore, a PrivateBrowsing:SetState message is sent back to Gecko, where setBrowserState() is called.

One problem I ran into was that killing Gecko causes the _savedBrowserState in the private browsing component to be lost. I worked around this by creating the readLastSession() method to get the state from sessionstore.js, which then notifies "resume-private-browsing-session" observers with the state. PrivateBrowsingService listens for this message and sets _savedBrowserState to the data.

Comment 33

[Brian Nicholson (:bnicholson)]

Private browsing test build: http://dl.dropbox.com/u/35559547/fennec-private-browsing.apk

Comment 34

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 659407 [details] [diff] [review]
Part 5: Don't update history with private browsing enabled

nice

Comment 35

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 659403 [details] [diff] [review]
Part 3: Mobile private browsing changes

>diff --git a/mobile/android/components/PrivateBrowsingService.js b/mobile/android/components/PrivateBrowsingService.js

These changes seem fine to me. The code removals makes sense for Mobile.

>-          browserWindow.getInterface(Ci.nsIWebNavigation)
>-                       .QueryInterface(Ci.nsIDocShellTreeItem)
>-                       .treeOwner
>-                       .QueryInterface(Ci.nsIInterfaceRequestor)
>-                       .getInterface(Ci.nsIXULWindow)
>-                       .docShell.contentViewer.resetCloseWindow();
>-        }
>-      }
>     }
>     else
>       this._saveSession = false;

while you are in here, can you cuddle the else and add {} since the if block has them

>diff --git a/mobile/android/components/SessionStore.js b/mobile/android/components/SessionStore.js

>+      case "private-browsing":
>+        switch (aData) {
>+          case "enter":
>+            this._inPrivateBrowsing = true;
>+            break;
>+          case "exit":
>+            this._inPrivateBrowsing = false;
>+            break;
>+        }
>+        break;

Can we simplify this to:
this._inPrivateBrowsing = (aData == "enter");

The SessionStore changes look sane, but please get QA to do some focused testing on OOM and other restore modes.

Comment 36

[(no longer active)]

I talked about these patches with Brian on IRC.  I believe that here is what we need to do for this bug:

1. Fix bug 774963.  The removeDataForDomain API is a performance foot-gun, and it would be a terrible idea to introduce it to mobile.  We should just move that to a different service and don't port it to Fennec.
2. Start by implementing the PB service from scratch.  The desktop service is a hairy piece of code which has tons of hacks and work-arounds for desktop specific problems, and we don't need to carry all of that baggage over to mobile.  I can help Brian to spot critical things that the desktop service does, but I believe we can have a much simpler implementation on mobile.
3. For the UX piece, the about:pb port will only make sense if that is the user experience we want to provide on mobile.  Brian will speak to the UX folks about that before we spend time to port that over to mobile.

I'll clear the review requests for now.

Comment 37

[(no longer active)]

Comment on attachment 659437 [details] [diff] [review]
Part 6: Save private browsing session to Android bundle

A question about this patch.  What is the Android bundle?  Does it persist its data somewhere?

Comment 38

[Brian Nicholson (:bnicholson)]

(In reply to Ehsan Akhgari [:ehsan] from comment #37)
> Comment on attachment 659437 [details] [diff] [review]
> Part 6: Save private browsing session to Android bundle
> 
> A question about this patch.  What is the Android bundle?  Does it persist
> its data somewhere?

In Android, background apps can be killed at any time (e.g., opening Fennec, pushing home, and launching another app can cause Fennec to be killed). When this happens, apps can save any state data to a bundle, which is held in memory. When the app is resumed, it has access to the bundle, and it uses the bundle to restore to the state from before the app was killed.

Comment 39

[(no longer active)]

(In reply to comment #38)
> (In reply to Ehsan Akhgari [:ehsan] from comment #37)
> > Comment on attachment 659437 [details] [diff] [review]
> > Part 6: Save private browsing session to Android bundle
> > 
> > A question about this patch.  What is the Android bundle?  Does it persist
> > its data somewhere?
> 
> In Android, background apps can be killed at any time (e.g., opening Fennec,
> pushing home, and launching another app can cause Fennec to be killed). When
> this happens, apps can save any state data to a bundle, which is held in
> memory. When the app is resumed, it has access to the bundle, and it uses the
> bundle to restore to the state from before the app was killed.

Is this guaranteed to live in the memory?  If yes, then this approach is fine.

Comment 40

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Ehsan Akhgari [:ehsan] from comment #36)
> I talked about these patches with Brian on IRC.  I believe that here is what
> we need to do for this bug:
> 
> 1. Fix bug 774963.  The removeDataForDomain API is a performance foot-gun,
> and it would be a terrible idea to introduce it to mobile.  We should just
> move that to a different service and don't port it to Fennec.

I don't think we should block on this bug. We can land a NOT_IMPL'd version for the initial milestone.

> 2. Start by implementing the PB service from scratch.  The desktop service
> is a hairy piece of code which has tons of hacks and work-arounds for
> desktop specific problems, and we don't need to carry all of that baggage
> over to mobile.  I can help Brian to spot critical things that the desktop
> service does, but I believe we can have a much simpler implementation on
> mobile.

I thought Brian's "part 3" patch removed a lot of the non-mobile chunks of code. If we remove removeDataForDomain as well, that should result in a minimal service impl. Are there other bits you'd want to remove too?

> 3. For the UX piece, the about:pb port will only make sense if that is the
> user experience we want to provide on mobile.  Brian will speak to the UX
> folks about that before we spend time to port that over to mobile.

Mobile UX is going to wait on about:pb for now and see what kind of experience we want to support. We can always add it back later, in a new bug, if it's needed.

> I'll clear the review requests for now.

Can we get you to review part 1, 3 and 5?

Comment 41

[(no longer active)]

(In reply to comment #40)
> (In reply to Ehsan Akhgari [:ehsan] from comment #36)
> > I talked about these patches with Brian on IRC.  I believe that here is what
> > we need to do for this bug:
> > 
> > 1. Fix bug 774963.  The removeDataForDomain API is a performance foot-gun,
> > and it would be a terrible idea to introduce it to mobile.  We should just
> > move that to a different service and don't port it to Fennec.
> 
> I don't think we should block on this bug. We can land a NOT_IMPL'd version for
> the initial milestone.

I guess, but why not fix bug 774963 properly?  It's not really that hard at all!  :-)  Just to prove that I'm trying to help things move forward more quickly here, I can do that one evening if Brian doesn't want to do that for any reason.

> > 2. Start by implementing the PB service from scratch.  The desktop service
> > is a hairy piece of code which has tons of hacks and work-arounds for
> > desktop specific problems, and we don't need to carry all of that baggage
> > over to mobile.  I can help Brian to spot critical things that the desktop
> > service does, but I believe we can have a much simpler implementation on
> > mobile.
> 
> I thought Brian's "part 3" patch removed a lot of the non-mobile chunks of
> code. If we remove removeDataForDomain as well, that should result in a minimal
> service impl. Are there other bits you'd want to remove too?

Yes.  From just a quick look, all or most of the cruft about saved browser state, _saveSession, _autoStarted can go away; the logic of _onBefore/AfterPrivateBrowsingModeChange can be simplified to a great extent, the transition code can also be simplified/dropped depending on the sessionstore implementation on mobile, the _ensureCanCloseWindows thing can die, the command line handling pieces will be unnecessary, and probably a bunch of stuff that I'm forgetting right now...

> > 3. For the UX piece, the about:pb port will only make sense if that is the
> > user experience we want to provide on mobile.  Brian will speak to the UX
> > folks about that before we spend time to port that over to mobile.
> 
> Mobile UX is going to wait on about:pb for now and see what kind of experience
> we want to support. We can always add it back later, in a new bug, if it's
> needed.

OK.  I will also try to talk to Ian soon to get a sense of what he's thinking about, and to provide him with some of the experience that we had on the desktop with PB.

> > I'll clear the review requests for now.
> 
> Can we get you to review part 1, 3 and 5?

Part 1 and 3 pretty much have to be rewritten.  I talked to Brian about that on IRC and he agreed.  I took a look at part 5, and I don't know a lot about that code.  Does that part just uses a no-op history database object for private browsing mode?

Comment 42

[(no longer active)]

Also, just to make my position clear on the way that mobile is approaching this problem, I still do not agree to comment 25 at all.  I think this is totally the wrong approach for prototyping PB support for mobile.  Most of the platform pieces are already there and almost all of the code that Brian is working on in this bug needs to be thrown out whole-sale once I remove nsIPrivateBrowsingService.  The thing that I hate to see happen is us moving forward with what is basically an implementation of a deprecated interface which will hopefully die soon, which would put mobile in a terrible situation once that happens.

It is a much better approach to use the new APIs for the purpose of prototyping.  And just to be clear, the portions of the new API which are now on mozilla-central are entirely enough for the initial implementation, although a few issues still need to be addressed before the new API is in a shippable state.

I really hope that you would reconsider the approach here, Mark.

Comment 43

[Paul [pwd]]

How much work needs to be done to get the new APIs in a usable state for RTM and what's a near-definitive timescale?

Comment 44

[(no longer active)]

(In reply to comment #43)
> How much work needs to be done to get the new APIs in a usable state for RTM
> and what's a near-definitive timescale?

There is currently nobody working on them full-time.  If we had somebody who would focus on them, I would guess it would take them a couple of weeks assuming they are not familiar with any of the areas of code they will need to modify.

Comment 45

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Bare bones PB implementation

Adds support for private tabs, which can be opened with the "New Private Browsing Tab" menu item.

Comment 46

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Add private browsing checks to native UI

Prevents thumbnails and favicons from being saved to the DB. Also adds checks to prevent history from being written to the DB (not fully working yet because of bug 723005).

Comment 47

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Split writing of private and non-private data

When saving the session, this patch makes sure that only non-private data is written to disk, and private data is sent to Java where it is written to the Android bundle.

Comment 48

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Change URL bar color for PB tabs

Changes the URL bar to purple for private tabs. This is a terribly ugly UI we can use for testing PB functionality until the real UI has been spec'd.

Comment 49

[Brian Nicholson (:bnicholson)]

Test build with these per-tab patches applied: http://dl.dropbox.com/u/35559547/fennec-private-browsing-per-tab.apk

Comment 50

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Change URL bar color for PB tabs, v2

Fixed to support all layouts.

Comment 51

[Brian Nicholson (:bnicholson)]

Requesting secreview for mobile private browsing. For our implementation, we're using the per-tab approach from bug 463027, and the mobile-specific components are being tracked in bug 794502.

Comment 52

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Brian Nicholson (:bnicholson) from comment #49)
> Test build with these per-tab patches applied:
> http://dl.dropbox.com/u/35559547/fennec-private-browsing-per-tab.apk

I tried the test build.
I feel that a tab opened from other private browse tabs should become a private browse one. I think it is natural behavior.

I'm looking forward to landing this feature!

Comment 53

[Brian Nicholson (:bnicholson)]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #52)
> I tried the test build.
> I feel that a tab opened from other private browse tabs should become a
> private browse one. I think it is natural behavior.

Yeah, we'll likely end up doing this. I think we'll make this change when we add a real private browsing UI.

Comment 54

[(no longer active)]

Comment on attachment 664284 [details] [diff] [review]
Part 1: Bare bones PB implementation

Review of attachment 664284 [details] [diff] [review]:
-----------------------------------------------------------------

Looks sane.

Comment 55

[(no longer active)]

Comment on attachment 664286 [details] [diff] [review]
Part 2: Add private browsing checks to native UI

We do something similar to this in the toolkit favicon service.  I assume that service is not used by Fennec, right?

Comment 56

[(no longer active)]

Comment on attachment 664287 [details] [diff] [review]
Part 3: Split writing of private and non-private data

Review of attachment 664287 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Note that mobile sessionstore implementation is not my strongest suit, so Mark should probably review this as if no-one else has before!

::: mobile/android/chrome/content/browser.js
@@ +7262,5 @@
>        case "PrivateBrowsing:NewTab":
>          BrowserApp.addTab("about:home", { isPrivate: true });
>          break;
> +      case "PrivateBrowsing:Restore":
> +        Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore)

Please use this._sessionStoreService.

Comment 57

[(no longer active)]

Brian, what are your plans on testing this?

Comment 58

[Brian Nicholson (:bnicholson)]

(In reply to Ehsan Akhgari [:ehsan] from comment #55)
> Comment on attachment 664286 [details] [diff] [review]
> Part 2: Add private browsing checks to native UI
> 
> We do something similar to this in the toolkit favicon service.  I assume
> that service is not used by Fennec, right?

Right - we do all handling for favicons in Java.

Comment 59

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Add private browsing checks to native UI, v2

Uses PrivateTab class for the abstraction.

Comment 60

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Add private browsing checks to native UI, v3

Removed extra runnable.

Comment 61

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 664284 [details] [diff] [review]
Part 1: Bare bones PB implementation

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY new_pb_tab "New Private Browsing Tab">

This string seem too long, but I guess we can change it later :)

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+var PrivateBrowsing = {
>+  init: function () {

nit: name this func
function pb_init()

>+    this._sessionStoreService = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);

Not needed?

>+  uninit: function () {

nit: name it

>+  observe: function (aMessage, aTopic, aData) {

nit: name it

Comment 62

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 664300 [details] [diff] [review]
Part 4: Change URL bar color for PB tabs, v2

OK for now. We plan to do some work on this soon.

Comment 63

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 669036 [details] [diff] [review]
Part 2: Add private browsing checks to native UI, v3

"Part 1" patch adds Tab.setIsPrivate and Tab.isPrivate methods; and the Tab.mIsPrivate data member. I assume we don't need those with this patch.

I really like this patch BTW.

Comment 64

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 664287 [details] [diff] [review]
Part 3: Split writing of private and non-private data

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

You are handling all private session and bundle management in GeckoApp. I was initially wondering if this should be split into BrowserApp. Maybe later.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

> var PrivateBrowsing = {
>   init: function () {
>     Services.obs.addObserver(this, "PrivateBrowsing:NewTab", false);
>+    Services.obs.addObserver(this, "PrivateBrowsing:Restore", false);

Handle this in SessionStore directly. It's just a notification and we already have a observer there.

>+    Services.obs.addObserver(this, "sessionstore-state-write-private", false);

Don't bother with this. Send the JSON message directly in SessionStore.

>     this._sessionStoreService = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);

Remove


>diff --git a/mobile/android/components/SessionStore.idl b/mobile/android/components/SessionStore.idl

No changes required

>diff --git a/mobile/android/components/SessionStore.js b/mobile/android/components/SessionStore.js

>   saveState: function ss_saveState() {
>     let data = this._getCurrentState();
>-    this._writeFile(this._sessionFile, JSON.stringify(data));
>+    let nonPrivateData = { windows: [] };
>+    let privateData = { windows: [] };
>+
>+    for (let winIndex = 0; winIndex < data.windows.length; ++winIndex) {
>+      let win = data.windows[winIndex];
>+      let nonPrivateWin = {};
>+      for (let prop in win) {
>+        nonPrivateWin[prop] = data[prop];
>+      }
>+      nonPrivateWin.tabs = [];
>+      nonPrivateData.windows.push(nonPrivateWin);
>+      privateData.windows.push({ tabs: [] });
>+
>+      // Split the session data into private and non-private data objects.
>+      // Non-private session data will be saved to disk, and private session
>+      // data will be sent to Java for Android to hold it in memory.
>+      for (let i = 0; i < win.tabs.length; ++i) {
>+        let tab = win.tabs[i];
>+        let savedWin = tab.isPrivate ? privateData.windows[winIndex] : nonPrivateData.windows[winIndex];
>+        savedWin.tabs.push(tab);
>+        if (win.selected == i + 1) {
>+          savedWin.selected = savedWin.tabs.length;
>+        }
>+      }
>+    }

This might be easier if we store normal tabs in SessionStore._windows[] and private tabs in SessionStore._private[]

>+
>+    // Write only non-private data to disk
>+    this._writeFile(this._sessionFile, JSON.stringify(nonPrivateData));
>+
>+    // If we have private data, send it to Java; otherwise, send null to
>+    // indicate that there is no private data
>+    let privateStr = (privateData.windows[0].tabs.length > 0) ? JSON.stringify(privateData) : null;
>+    Services.obs.notifyObservers(null, "sessionstore-state-write-private", privateStr);

Send to Java directly


>   _collectTabData: function ss__collectTabData(aWindow, aBrowser, aHistory) {

>+    tabData.isPrivate = aBrowser.docShell
>+                                .QueryInterface(Ci.nsILoadContext)
>+                                .usePrivateBrowsing;

No wrapping

>+  _restoreWindow: function ss_restoreWindow(aState, aBringToFront) {
>+    // To do a restore, we must have at least one window with one tab
>+    if (!aState || aState.windows.length == 0 ||
>+        !aState.windows[0].tabs || aState.windows[0].tabs.length == 0) {

No wrap

>+  restorePrivateSession: function ss_restorePrivateSession(aSession) {

You could keep this I suppose. Just call it from in the SessionStore.observe handler for PB:Restore

Comment 65

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Bare bones PB implementation, v2

Rebased on top of bug 722661, and removed the mIsPrivate field on Tab.

Comment 66

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Add private browsing checks to native UI, v4

Rebased for bug 797075.

Comment 67

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Split writing of private and non-private data, v2

Addressed comment 64.

Comment 68

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 669351 [details] [diff] [review]
Part 3: Split writing of private and non-private data, v2

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+        if (mPrivateBrowsingSession != null) {
>+            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent(
>+                    "PrivateBrowsing:Restore", mPrivateBrowsingSession));
>+        }

I suppose this is as good as any place to try to restore the PB session. Let's be on the lookout for edge cases.

>diff --git a/mobile/android/components/SessionStore.js b/mobile/android/components/SessionStore.js

>+      case "PrivateBrowsing:Restore":
>+        let session;
>+        try {
>+          session = JSON.parse(aData);
>+        } catch (e) {
>+          Cu.reportError("SessionStore: Could not parse JSON data");
>+          return;
>+        }
>+
>+        this._restoreWindow(session, session.windows[0].selected != null);

How about we move the "_restoreWindow" call into the try block and remove the "return" from the catch block?

>+      this._sendMessageToJava = aWindow.sendMessageToJava;

This works, but I'd rather see the function implemented locally, like this:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/PromptService.js#752

>   saveState: function ss_saveState() {
>     let data = this._getCurrentState();
>-    this._writeFile(this._sessionFile, JSON.stringify(data));
>+    let nonPrivateData = { windows: [] };
>+    let privateData = { windows: [] };

My OCD is kicking in. Can we use normalWin/normalData and privateWin/privateData ?

r+ with nits fixed

Comment 69

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/093b6818007c
http://hg.mozilla.org/integration/mozilla-inbound/rev/e937f574fb4d
http://hg.mozilla.org/integration/mozilla-inbound/rev/a7b2d57295a0
http://hg.mozilla.org/integration/mozilla-inbound/rev/bb0e7efed1bd

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/093b6818007c
https://hg.mozilla.org/mozilla-central/rev/e937f574fb4d
https://hg.mozilla.org/mozilla-central/rev/a7b2d57295a0
https://hg.mozilla.org/mozilla-central/rev/bb0e7efed1bd

Comment 71

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I don't think this bug can be set to RESOLVED FIXED while important dependencies like 

799945 - Passwords are saved in Private browsing
799936 - GeckoConsole leaks Private Browsing URL's
799985 - Form/Search bar auto-complete history is remembered in private browsing

are still open.

Those bugs should not be fixed after the fact but instead should be part of dropping this feature into nightly.

Comment 72

[(no longer active)]

It's probably worth re-checking the dependencies of this bug now that bug 819398 has been fixed.  :-)

Comment 73

[Andreea Pod]

Test cases added in MozTrap: https://moztrap.mozilla.org/manage/cases/?filter-suite=216

Comment 74

[Brian Nicholson (:bnicholson)]

Private browsing on Fennec has landed and shipped.