WebContentConverter uses global Private Browsing state to make decisions

RESOLVED FIXED in Firefox 14

Status

()

Firefox
RSS Discovery and Preview
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jdm, Assigned: Hessam Salehi)

Tracking

unspecified
Firefox 14
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

v2
2.09 KB, patch
jdm
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

5 years ago
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
(Assignee)

Comment 4

5 years ago
Created attachment 615415 [details] [diff] [review]
v1
(Reporter)

Comment 5

5 years ago
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+
(Assignee)

Comment 7

5 years ago
Created attachment 615443 [details] [diff] [review]
v2
Attachment #615415 - Attachment is obsolete: true
Attachment #615443 - Flags: checkin?
(Assignee)

Updated

5 years ago
Attachment #615443 - Flags: checkin?
(Reporter)

Updated

5 years ago
Attachment #615443 - Flags: review+
(Reporter)

Updated

5 years ago
Whiteboard: [mentor=ehsan][lang=js] → [mentor=ehsan][lang=js][autoland:-b do -p all -u mochitests -t none]
(Reporter)

Updated

5 years ago
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]

Updated

5 years ago
Whiteboard: [mentor=ehsan][lang=js][autoland-try:-b do -p all -u mochitests -t none] → [mentor=ehsan][lang=js][autoland-in-queue]

Comment 8

5 years ago
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

Comment 10

5 years ago
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

Updated

5 years ago
Whiteboard: [mentor=ehsan][lang=js][autoland-in-queue] → [mentor=ehsan][lang=js]
https://hg.mozilla.org/mozilla-central/rev/d0988a66f47a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.