464736 - Disable menu File > Import when in Private Browsing mode, because it doesn't work for cookies and history

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Marcia Knous [:marcia]]

Seen while testing  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081113 Minefield/3.1b2pre.

STR
1. Enter PB mode.
2. File | Import
3. Import from Safari - check all radio buttons

Expected: I would be able to import all data even in PB mode
Actual: Nothing imports

When I tested this the other day the import did work, so perhaps something changed between now and then.  Also, when performing the same STR on Windows Vista, FF takes an awful long time to complete the import process. I will file a separate bug on that issue.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

I just tried this (with today's Minefield nightly) and had no problems.

Do you still see this, Marcia?

I tested on OS X 10.5.5.

Comment 2

[Marcia Knous [:marcia]]

A cursory test using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081120 Minefield/3.1b2pre reveals the following:

*Bookmarks are imported
*Saved form history is not (Bug 363488)
*Browsing History is not
*Cookies are not
*Preferences - I am uncertain as to what preferences would come over so that is an open issue.

Updating bug summary to reflect the latest issues I see.

Comment 3

[(no longer active)]

I think the reason is that the usual Places API for adding history entries is disabled in PB mode, and the cookies DB is also using an in-memory DB which will expire at the end of the private session.

Before we jump at a fix, do we really want to allow imports from within the PB mode at all?  I think we should disable the File -> Import menu entry.  Another place which can invoke this UI is when importing bookmarks from the Library.  I guess we can leave that enabled since adding bookmarks using the Places API does work in PB mode.

mconnor/Alex/Beltzner, what do you think?

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

I'm fine with disabling the menu items to import bookmarks.  In general this seems like an odd thing to want to be able to do while in private browsing mode.

Comment 5

[(no longer active)]

Attachment: Patch (v1)

Simple patch, with a unit test.

Comment 6

[(no longer active)]

Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.

Comment 7

[(no longer active)]

Attachment: Patch (v1.1)

Unbitrotted patch.

Comment 8

[(no longer active)]

Comment on attachment 357556 [details] [diff] [review]
Patch (v1.1)

mconnor: this patch should not be too difficult to review, could you please take a look at it?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 357556 [details] [diff] [review]
Patch (v1.1)

Sigh... I'm not sure how I ended up missing this review request...

Comment 10

[:Gavin Sharp [email: [e-mail]]]

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js#365 also triggers the import window (although possibly only for bookmarks). It might need similar treatment?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Ideally, I guess we would have a consolidated way to opening migration.xul that fails if private browsing is enabled.

Comment 12

[(no longer active)]

(In reply to comment #10)
> http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js#365
> also triggers the import window (although possibly only for bookmarks). It
> might need similar treatment?

I don't think so, because bookmarks can actually be stored inside the private browsing mode.

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/59e2945cce3c

Comment 14

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/36dad7d32058

Comment 15

[Boris Zbarsky [:bzbarsky]]

I backed this out on suspicion of causing semi-random mochichrome orange (which did indeed clear up).  Please reland on its own at some point, or at least not with the other two patches it landed with.

Comment 16

[(no longer active)]

Relanded on trunk: <http://hg.mozilla.org/mozilla-central/rev/44e28804fd03>

Comment 17

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090220 Shiretoko/3.1b3pre. The "Import" option is greyed out in the File menu and in the Bookmarks organizer during PB.

Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090220 Minefield/3.2a1pre.

Comment 18

[nemo]

Based on discussion w/ Marcia on #testday reopening since the Import HTML in Organise Bookmarks was not, in fact, disabled along with the File Import and leads to the same dialog.

Comment 19

[Mike Connor [:mconnor]]

First, really should have filed a new bug instead of reopening an old bug.  Second, this is fine, and was brought up in comment 10 by gavin.  Since we actually save bookmarks in Private Browsing mode, this is fine and there's no need to disable this case.

Comment 20

[nemo]

Sorry, missed comment #10 - this came up in discussion of a testcase referencing that menu item and after reading comment #17.

Bookmarks organiser option doesn't matter then. Fine.  Although is certainly not that useful and possibly a bit confusing.

Anyway, since I do try to rarely get in trouble with bug statuses, and did discuss it with others, for future reference, if it seems the patch did not actually disable the item it was supposed to disable, why wouldn't the bug be reopened?

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #20)
> Anyway, since I do try to rarely get in trouble with bug statuses, and did
> discuss it with others, for future reference, if it seems the patch did not
> actually disable the item it was supposed to disable, why wouldn't the bug be
> reopened?

It did disable the item it meant to disable, as far as I can tell. The File->Import menu item is disabled when private browsing mode is enabled (testing the latest Windows branch nightly). Are you saying you're not seeing that?

Comment 22

[(no longer active)]

(In reply to comment #21)
> (In reply to comment #20)
> > Anyway, since I do try to rarely get in trouble with bug statuses, and did
> > discuss it with others, for future reference, if it seems the patch did not
> > actually disable the item it was supposed to disable, why wouldn't the bug be
> > reopened?
> 
> It did disable the item it meant to disable, as far as I can tell. The
> File->Import menu item is disabled when private browsing mode is enabled
> (testing the latest Windows branch nightly).

And in fact we have an automated test which makes sure that's the case all the time.

Comment 23

[Steffen Wilberg]

Tweaking summary to match the outcome.