Closed Bug 722986 Opened 12 years ago Closed 12 years ago

WebContentConverter uses global Private Browsing state to make decisions

Categories

(Firefox Graveyard :: RSS Discovery and Preview, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 14

People

(Reporter: jdm, Assigned: hessamms)

References

Details

(Whiteboard: [mentor=ehsan][lang=js])

Attachments

(1 file, 1 obsolete file)

The global state is going away. This should query the window docshell instead.
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.
The test added in bug 539296 can be used to ensure the correctness of the patch here.
Whiteboard: [mentor=ehsan][lang=js]
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.
Assignee: nobody → hessamms
Attached patch v1 (obsolete) — Splinter Review
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 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.  :-)
Attachment #615415 - Flags: review+
Attached patch v2Splinter Review
Attachment #615415 - Attachment is obsolete: true
Attachment #615443 - Flags: checkin?
Attachment #615443 - Flags: checkin?
Attachment #615443 - Flags: review+
Whiteboard: [mentor=ehsan][lang=js] → [mentor=ehsan][lang=js][autoland:-b do -p all -u mochitests -t none]
Whiteboard: [mentor=ehsan][lang=js][autoland:-b do -p all -u mochitests -t none] → [mentor=ehsan][lang=js][autoland-try:-b do -p all -u mochitests -t none]
Whiteboard: [mentor=ehsan][lang=js][autoland-try:-b do -p all -u mochitests -t none] → [mentor=ehsan][lang=js][autoland-in-queue]
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0988a66f47a

Thanks for your patch, Hessam!
Target Milestone: --- → Firefox 14
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
Whiteboard: [mentor=ehsan][lang=js][autoland-in-queue] → [mentor=ehsan][lang=js]
https://hg.mozilla.org/mozilla-central/rev/d0988a66f47a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: