Closed Bug 806723 Opened 7 years ago Closed 7 years ago

Port plugin test_privatemode.xul to the new per-tab PB APIs

Categories

(Firefox :: Private Browsing, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 21
Tracking Status
firefox20 --- fixed

People

(Reporter: ehsan, Assigned: raymondlee)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
A patch in bug 722850 is modifying this test and should probably land first.
Depends on: 722850
Removed this test from global PB builds: https://hg.mozilla.org/mozilla-central/rev/917b065521cd
s/global/per-window/
Assignee: nobody → raymond
Attached patch v1 (WIP) (obsolete) — Splinter Review
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?
Flags: needinfo?(josh)
Status: NEW → ASSIGNED
(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!
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.
Flags: needinfo?(josh)
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.
Flags: needinfo?(benjamin)
(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. :-)
Attached patch v2 (obsolete) — Splinter Review
Removed the lastReportedPrivateModeState() check
Attachment #702137 - Attachment is patch: true
Is this ready for review?
Still awaiting Benjamin's input.
I don't know the code in question, so I don't think I can provide useful feedback.
Flags: needinfo?(benjamin)
Comment on attachment 702137 [details] [diff] [review]
v2

Josh, can you answer comment 7?
Attachment #702137 - Flags: feedback?(joshmoz)
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.
Attachment #702137 - Flags: feedback?(joshmoz) → feedback+
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...
Blocks: 817477
Attachment #702137 - Flags: review?(josh)
Attachment #702137 - Flags: review?(josh) → review+
Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=10996563b54f
Attachment #697386 - Attachment is obsolete: true
Attachment #702137 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9441271bdb45
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
You need to log in before you can comment on or make changes to this bug.