Last Comment Bug 722986 - WebContentConverter uses global Private Browsing state to make decisions
: WebContentConverter uses global Private Browsing state to make decisions
Status: RESOLVED FIXED
[mentor=ehsan][lang=js]
:
Product: Firefox
Classification: Client Software
Component: RSS Discovery and Preview (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: Firefox 14
Assigned To: Hessam Salehi
:
: Marco Bonardo [::mak]
Mentors:
Depends on: 722840
Blocks: PBnGen
  Show dependency treegraph
 
Reported: 2012-01-31 22:26 PST by Josh Matthews [:jdm] (on vacation until Dec 5)
Modified: 2012-04-17 07:48 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (2.60 KB, patch)
2012-04-16 12:22 PDT, Hessam Salehi
ehsan: review+
Details | Diff | Splinter Review
v2 (2.09 KB, patch)
2012-04-16 13:28 PDT, Hessam Salehi
josh: review+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (on vacation until Dec 5) 2012-01-31 22:26:30 PST
The global state is going away. This should query the window docshell instead.
Comment 1 :Ehsan Akhgari 2012-04-04 20:28:31 PDT
The _getBrowserWindowForContentWindow function can be used to get the chrome window object, and then the API added in bug 723353 can be used to retrieve the PB status of the respective browser window.
Comment 2 :Ehsan Akhgari 2012-04-04 20:29:04 PDT
The test added in bug 539296 can be used to ensure the correctness of the patch here.
Comment 3 :Ehsan Akhgari 2012-04-09 13:49:43 PDT
The code in question is here: <http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/src/WebContentConverter.js#400>

At the beginning of that function, we determine whether we're currently in private browsing mode using nsIPrivateBrowsingService.  That is the global private browsing API, and the goal of this bug is to change it to use the per-window private browsing API.

The aContentWindow parameter passed to this function is the content's window object.  You can call _getBrowserWindowForContentWindow to get the browser window object from it (in fact, this function already does that here <http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/src/WebContentConverter.js#491> so you can just move that line to the beginning of the function.  Once you have access to the browser window object, you can use the API added in bug 723353 in order to see whether that window is currently in private mode, in which case you need to log a message to the console and return the same way that we currently do.
Comment 4 Hessam Salehi 2012-04-16 12:22:39 PDT
Created attachment 615415 [details] [diff] [review]
v1
Comment 5 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-04-16 12:36:44 PDT
Comment on attachment 615415 [details] [diff] [review]
v1

Great, Hassem! It looks like we have tests that cover this behaviour, so we can just run this patch through the tryserver and make sure nothing is broken.
Comment 6 :Ehsan Akhgari 2012-04-16 12:46:44 PDT
Comment on attachment 615415 [details] [diff] [review]
v1

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

This looks great, I have requested a couple of changes below.  Please submit a new version of the patch which fixes them, and set the checkin-needed flag when the try server push results are known to be green.

Thanks!

::: browser/components/feeds/src/WebContentConverter.js
@@ +401,5 @@
>    function WCCR_registerProtocolHandler(aProtocol, aURIString, aTitle, aContentWindow) {
>      LOG("registerProtocolHandler(" + aProtocol + "," + aURIString + "," + aTitle + ")");
>  
> +    var browserWindow = this._getBrowserWindowForContentWindow(aContentWindow);    
> +        if (browserWindow.gPrivateBrowsingUI.privateWindow) {

Please fix the indentation of the if statement here.

@@ +409,5 @@
>        Cc["@mozilla.org/consoleservice;1"].
>        getService(Ci.nsIConsoleService).
>        logStringMessage("Web page denied access to register a protocol handler inside private browsing mode");
>        return;
> +    }    

Please remove the trailing spaces at the end of the line.  :-)
Comment 7 Hessam Salehi 2012-04-16 13:28:33 PDT
Created attachment 615443 [details] [diff] [review]
v2
Comment 8 Mozilla RelEng Bot 2012-04-16 14:36:36 PDT
Autoland Patchset:
	Patches: 615443
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=0c0cbbef3e1a
Try run started, revision 0c0cbbef3e1a. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=0c0cbbef3e1a
Comment 9 :Ehsan Akhgari 2012-04-16 20:55:34 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0988a66f47a

Thanks for your patch, Hessam!
Comment 10 Mozilla RelEng Bot 2012-04-16 21:15:21 PDT
Try run for 0c0cbbef3e1a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0c0cbbef3e1a
Results (out of 124 total builds):
    success: 112
    warnings: 9
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-0c0cbbef3e1a
Comment 11 Marco Bonardo [::mak] 2012-04-17 07:48:30 PDT
https://hg.mozilla.org/mozilla-central/rev/d0988a66f47a

Note You need to log in before you can comment on or make changes to this bug.