722996 - cookieAcceptDialog.js uses global Private Browsing state to make decisions

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Josh Matthews [:jdm]]

The global state is going away. We should obtain the window's docshell and query the PB state that way instead.

Comment 1

[Josh Matthews [:jdm]]

This looks like we can use the same strategy as bug 722978.

Comment 2

[Saurabh Anand [:sawrubh]]

Attachment: Patch v1

The concerned test (http://dxr.lanedo.com/mozilla-central/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_cookieacceptdialog.js.html) seem to be passing locally.

Comment 3

[(no longer active)]

Comment on attachment 637261 [details] [diff] [review]
Patch v1

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

Looks good to me!

Comment 4

[(no longer active)]

Josh, Saurabh mentioned that you told him there's a missing piece here.  Would you mind elaborating on that?

Comment 5

[Josh Matthews [:jdm]]

This is toolkit code, yet its relying on gPrivateBrowsingUI which is a browser/ property. We should be explicitly obtaining the load context instead.

Comment 6

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #5)
> This is toolkit code, yet its relying on gPrivateBrowsingUI which is a
> browser/ property. We should be explicitly obtaining the load context
> instead.

That is a very good point.  Josh is right on, as usual.  :-)

Comment 7

[Saurabh Anand [:sawrubh]]

Dao told me once that any files included in html or xul files usually have a window attached to them, since here cookieAcceptDialog.xul exists, hence I thought that we will have access to the browser window, which will have gPrivateBrowsingUI property. 
I am not sure whether this accessible browser window will allow us to QI it (something like this |window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebNavigation).QueryInterface(Ci.nsILoadContext).usePrivateBrowsing| ) like we do when going by the nsILoadContext path ?

@Josh what do you think about this ?

Comment 8

[Josh Matthews [:jdm]]

I can't think of any reason the QI chain in comment 7 wouldn't work, except that I'm not certain that the usePrivateBrowsing flag would propagate to the dialog window from the parent. You might need to use window.opener for that as well.

Comment 9

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #8)
> I can't think of any reason the QI chain in comment 7 wouldn't work, except
> that I'm not certain that the usePrivateBrowsing flag would propagate to the
> dialog window from the parent.

It doesn't.

> You might need to use window.opener for that as well.

Yes, you want to use the opener property.  Also, you can use the API added in bug 769467 (which is currently on the inbound branch.)

Comment 10

[Saurabh Anand [:sawrubh]]

Attachment: Patch v2

What this patch does :
Uses the API introduced in Bug 769467.

This patch passes the entire "privatebrowsing/test/browser" test suite.

Comment 11

[Saurabh Anand [:sawrubh]]

Attachment: Patch v3

Sorry, I attached the not-so complete patch in Patch v2.

This patch is basically (a couple of whitespace cleanup + Patch v2).

Comment 12

[Saurabh Anand [:sawrubh]]

Here is the try run for Patch v3 :

https://tbpl.mozilla.org/?tree=Try&rev=7a7186ba3398

Comment 13

[(no longer active)]

Comment on attachment 643286 [details] [diff] [review]
Patch v3

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

r=me with the changes below.

::: toolkit/components/cookie/content/cookieAcceptDialog.js
@@ +131,4 @@
>  
>    // The Private Browsing service might not be available
>    try {
> +    let opener = window.opener;

No point in declaring this local opener variable.  window is the global object, and you can access its properties by just referencing the name.  In other words, if you drop this line, everything should continue to work properly!  :-)

@@ +132,5 @@
>    // The Private Browsing service might not be available
>    try {
> +    let opener = window.opener;
> +    if (opener) {
> +      if (PrivateBrowsingUtils.isWindowPrivate(opener)) {

Nit: drop the nested ifs, and just && the conditions please.

Comment 14

[Saurabh Anand [:sawrubh]]

Attachment: Patch v4

Addressed Eshan's comments. Carries r+ from him. Passes "privatebrowsing/test" suite and had a good enough try run.

Comment 15

[(no longer active)]

Comment on attachment 643573 [details] [diff] [review]
Patch v4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/82b78defe027

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/82b78defe027