Closed Bug 806729 Opened 7 years ago Closed 7 years ago

Port test_bug536567.html to the new per-tab PB APIs

Categories

(Firefox :: Private Browsing, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: ehsan, Assigned: andreshm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

http://mxr.mozilla.org/mozilla-central/source/layout/forms/test/test_bug536567.html

In order to port this test, the file needs to be copied to the same directory (perhaps with "_perwindowpb" appended to its file name), and then instead of setting privateBrowsingEnabled, we need to open a new private browsing window and then run the test on that window.  Note that the original test should only be added to the list of test files in Makefile.in ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING, and the new test file should be added to the list with the reverse condition.
Removed this test from per-window PB builds: https://hg.mozilla.org/mozilla-central/rev/40d32a8843c0
    1.11  MOCHITEST_CHROME_FILES = \
    1.12 -		test_bug536567.html \
    1.13  		     bug536567_subframe.html \

Why leave the subframe installed?
(In reply to comment #2)
> 
>     1.11  MOCHITEST_CHROME_FILES = \
>     1.12 -        test_bug536567.html \
>     1.13               bug536567_subframe.html \
> 
> Why leave the subframe installed?

It will be used later on.
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
The test is ready, however the last tests are failing, but they should not. 
To explain further, I have found a bug, because when the observer to purge history is notified, the content pref for 'browser.update.lastDir' is not being clear on PB window. 

Using the observer to purge history:
> Services.obs.notifyObservers(null, "browser:purge-session-history", "");

The 'browser.download.lastDir' content pref is being clear fine for both PB and not PB. 
See: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadLastDir.jsm#55

But, the 'browser.update.lastDir' content pref is not being clear for both and it should. 
See: http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp#538

With that fixed, the test should pass fine. 

Should I file a new bug for this? Or fix it in the same patch?
Attachment #694590 - Flags: review?(josh)
Attachment #694590 - Flags: review?(ehsan)
Comment on attachment 694590 [details] [diff] [review]
Patch v1

I'll be away starting tomorrow.  Josh can review this.
Attachment #694590 - Flags: review?(ehsan)
Eep. That's a good problem to notice, and not simple to fix :(
Ehsan, how do you feel about not clearing the browser.update.lastDir content pref for private windows when purging session history? My thinking is that I don't have a good way to solve the issue right now without further API changes, and it will get cleared with the closing of the private session.
Flags: needinfo?(ehsan)
(In reply to Josh Matthews [:jdm] from comment #7)
> Ehsan, how do you feel about not clearing the browser.update.lastDir content
> pref for private windows when purging session history? My thinking is that I
> don't have a good way to solve the issue right now without further API
> changes, and it will get cleared with the closing of the private session.

That's fine by me.
Flags: needinfo?(ehsan)
Andres, the best course of action here is to fix the test to not check that the browser.download.lastDir content pref is cleared for the private window after the browser:purge-session-history notification is sent.
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #694590 - Attachment is obsolete: true
Attachment #694590 - Flags: review?(josh)
Attachment #697040 - Flags: review?(josh)
Attachment #697040 - Flags: review?(josh) → review+
Keywords: checkin-needed
Attached patch Patch v3Splinter Review
Fixed the issue. Should be working fine now.
Attachment #697040 - Attachment is obsolete: true
Keywords: checkin-needed
Assignee: raymond → andres
https://hg.mozilla.org/mozilla-central/rev/358c8fc9b59b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
You need to log in before you can comment on or make changes to this bug.