(no longer active) Reporter
	Description • 12 years ago

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_privatemode.xul

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.

	Josh Matthews [:jdm]
	Comment 1 • 12 years ago

A patch in bug 722850 is modifying this test and should probably land first.

	(no longer active) Reporter
	Comment 2 • 12 years ago

Removed this test from global PB builds: https://hg.mozilla.org/mozilla-central/rev/917b065521cd

	(no longer active) Reporter
	Comment 3 • 12 years ago

s/global/per-window/

	Raymond Lee [:raymondlee] Assignee
	Comment 4 • 12 years ago

In a private window, lastReportedPrivateModeState() always returns false regarding whether it's called before or after queryPrivateModeState() so I guess there is a bug.  Furthermore, I don't think we need to check lastReportedPrivateModeState() as the private mode state wouldn't be changed for per-window PB window.

@ehsan / josh: what is your opinion?

	Raymond Lee [:raymondlee] Assignee
	Comment 5 • 12 years ago

(In reply to Raymond Lee [:raymondlee] from comment #4)
> Created attachment 697386 [details] [diff] [review]
> v1 (WIP)
> 
> In a private window, lastReportedPrivateModeState() always returns false
> regarding whether it's called before or after queryPrivateModeState() so I
> guess there is a bug.  Furthermore, I don't think we need to check
> lastReportedPrivateModeState() as the private mode state wouldn't be changed
> for per-window PB window.
> 

@josh: any info you can provide?  thanks!

	Josh Matthews [:jdm]
	Comment 6 • 12 years ago

Hmm. I ran the mochitest-chrome tests for dom/plugins/test and it passed just fine. I can't see any reason why lastReportedPrivateModeState() should be failing from reading the relevant code.

	Josh Matthews [:jdm]
	Comment 7 • 12 years ago

Aha, I see now. I was confused by the fact that you made the test pass in its current state. Stepping through the code, it's functioning as specified: when the window is put into private mode at creation time, there's no content to notify (in this case, the plugin via NPP_SetValue). I think this is a check that you can remove from the test, since the querying check ensures that the plugin can see the correct value if it wants it, but I'll get Benjamin to confirm.

	Raymond Lee [:raymondlee] Assignee
	Comment 8 • 12 years ago

(In reply to Josh Matthews [:jdm] from comment #7)
> Aha, I see now. I was confused by the fact that you made the test pass in
> its current state. Stepping through the code, it's functioning as specified:
> when the window is put into private mode at creation time, there's no
> content to notify (in this case, the plugin via NPP_SetValue). I think this
> is a check that you can remove from the test, since the querying check
> ensures that the plugin can see the correct value if it wants it, but I'll
> get Benjamin to confirm.

Thanks Josh!  Sorry about the confusion. :-)

	Raymond Lee [:raymondlee] Assignee
	Comment 9 • 12 years ago

Removed the lastReportedPrivateModeState() check

	(no longer active) Reporter
	Comment 10 • 12 years ago

Is this ready for review?

	Josh Matthews [:jdm]
	Comment 11 • 12 years ago

Still awaiting Benjamin's input.

	Benjamin Smedberg
	Comment 12 • 12 years ago

I don't know the code in question, so I don't think I can provide useful feedback.

	Josh Matthews [:jdm]
	Comment 13 • 12 years ago

Comment on attachment 702137 [details] [diff] [review]
v2

Josh, can you answer comment 7?

	Josh Aas
	Comment 14 • 12 years ago

Comment on attachment 702137 [details] [diff] [review]
v2

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

I may have written this code but I have no more recollection of it than anyone else at this point, sorry. The plugin private mode spec is here:

https://wiki.mozilla.org/Plugins:PrivateMode

This generally looks fine to me though.

	(no longer active) Reporter
	Comment 15 • 12 years ago

Over in bug 817477, I'm preparing to remove support for global private browsing altogether.  This is one of the last remaining pieces that I would like us to fix before that bug lands...

	Raymond Lee [:raymondlee] Assignee
	Comment 16 • 12 years ago

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=10996563b54f

	(no longer active) Reporter
	Comment 17 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9441271bdb45

	(no longer active) Reporter
	Comment 18 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/55237706be0b

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/9441271bdb45