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)
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)
2.09 KB,
patch
|
jdm
:
review+
|
Details | Diff | Splinter Review |
The global state is going away. This should query the window docshell instead.
Comment 1•12 years ago
|
||
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•12 years ago
|
||
The test added in bug 539296 can be used to ensure the correctness of the patch here.
Reporter | ||
Updated•12 years ago
|
Whiteboard: [mentor=ehsan][lang=js]
Comment 3•12 years ago
|
||
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•12 years ago
|
||
Reporter | ||
Comment 5•12 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 6•12 years ago
|
||
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•12 years ago
|
||
Attachment #615415 -
Attachment is obsolete: true
Attachment #615443 -
Flags: checkin?
Assignee | ||
Updated•12 years ago
|
Attachment #615443 -
Flags: checkin?
Reporter | ||
Updated•12 years ago
|
Attachment #615443 -
Flags: review+
Reporter | ||
Updated•12 years ago
|
Whiteboard: [mentor=ehsan][lang=js] → [mentor=ehsan][lang=js][autoland:-b do -p all -u mochitests -t none]
Reporter | ||
Updated•12 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•12 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•12 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
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0988a66f47a Thanks for your patch, Hessam!
Target Milestone: --- → Firefox 14
Comment 10•12 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•12 years ago
|
Whiteboard: [mentor=ehsan][lang=js][autoland-in-queue] → [mentor=ehsan][lang=js]
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d0988a66f47a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•