Last Comment Bug 856322 - Private Browsing changes for browser feed preview
: Private Browsing changes for browser feed preview
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Feed Discovery and Preview (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.20
Assigned To: Philip Chee
:
:
Mentors:
Depends on:
Blocks: 460895
  Show dependency treegraph
 
Reported: 2013-03-30 08:45 PDT by Philip Chee
Modified: 2013-04-17 05:23 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
unaffected
fixed
fixed
fixed


Attachments
Patch v1.0 Add PB awareness to feed preview. (14.10 KB, patch)
2013-03-30 09:02 PDT, Philip Chee
neil: review-
Details | Diff | Splinter Review
Patch v1.1 fix review issues. (10.62 KB, patch)
2013-04-10 08:43 PDT, Philip Chee
neil: review+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Philip Chee 2013-03-30 08:45:23 PDT
Browser Feed Preview needs to be aware of Private Browsing. Patch coming up.
Comment 1 Philip Chee 2013-03-30 09:02:56 PDT
Created attachment 731486 [details] [diff] [review]
Patch v1.0 Add PB awareness to feed preview.

> -function OpenBrowserWindow()
> +function OpenBrowserWindow(aOptions)
In order not to diverge our PB test too much from the Firefox version I've added a private browsing argument to OpenBrowserWindow()

> +    var notificationBox = getNotificationBox(aContentWindow);
> +    var win = notificationBox.ownerDocument.defaultView;
We can get the chrome window from the notificationBox so we don't need to use all those helper functions like Firefox does.

> -    classID: WCCR_CLASSID,
> -    contractID: WCCR_CONTRACTID,
> +      classID: WCCR_CLASSID,
> +      contractID: WCCR_CONTRACTID,
Remove some stray \tabs.

> -#include "prmem.h"
> -
Useless include, see Bug 798595.

> --- /dev/null
> +++ b/suite/browser/test/browser/browser_privatebrowsing_protocolhandler.js

To run this test:

$ TEST_PATH=suite/browser/test/browser_privatebrowsing_protocolhandler.js  pymake -C ../objdir-sm/ mochitest-browser-chrome

INFO TEST-START | Shutdown
Browser Chrome Test Summary
        Passed: 2
        Failed: 0
        Todo: 0
Comment 2 neil@parkwaycc.co.uk 2013-03-30 16:06:30 PDT
Comment on attachment 731486 [details] [diff] [review]
Patch v1.0 Add PB awareness to feed preview.

>+function whenNewWindowLoaded(aOptions, aCallback) {
>+  let win = OpenBrowserWindow(aOptions);
This is teh suck. Just open the browser window directly.
var { private } = aOptions;
var features = private ? "private,chrome,all,dialog=no" : "non-private,chrome,all,dialog=no";
var win = window.openDialog(getBrowserURL(), "_blank", features, "about:blank");

>+    var flags = isPB ? this._faviconService.FAVICON_LOAD_PRIVATE :
>+                       this._faviconService.FAVICON_LOAD_NON_PRIVATE;
[I guess I need to fix up tabbrowser.xml ...]

>+      function (aURI, aDataLen, aData, aMimeType) {
Nit: no space before (?

>+    var isPB = win.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                  .getInterface(Components.interfaces.nsIWebNavigation)
>+                  .QueryInterface(Components.interfaces.nsIDocShell)
>+                  .QueryInterface(Components.interfaces.nsILoadContext)
>+                  .usePrivateBrowsing;
IIRC you can test the private browsing state of the content window to avoid having to do it on the chrome window.
Comment 3 Philip Chee 2013-04-10 08:43:37 PDT
Created attachment 735813 [details] [diff] [review]
Patch v1.1 fix review issues.

> This is teh suck. Just open the browser window directly.
> var { private } = aOptions;
> var features = private ? "private,chrome,all,dialog=no" : "non-private,chrome,all,dialog=no";
> var win = window.openDialog(getBrowserURL(), "_blank", features, "about:blank");
Fixed.

>>+      function (aURI, aDataLen, aData, aMimeType) {
> Nit: no space before (?
Fixed.

>>+    var isPB = win.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>>+                  .getInterface(Components.interfaces.nsIWebNavigation)
>>+                  .QueryInterface(Components.interfaces.nsIDocShell)
>>+                  .QueryInterface(Components.interfaces.nsILoadContext)
>>+                  .usePrivateBrowsing;
> IIRC you can test the private browsing state of the content window to avoid
> having to do it on the chrome window.
Fixed.
Comment 4 Philip Chee 2013-04-10 21:12:08 PDT
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/0d3770a601e5
Comment 5 Philip Chee 2013-04-13 02:53:09 PDT
Comment on attachment 735813 [details] [diff] [review]
Patch v1.1 fix review issues.

[Approval Request Comment]
Regression caused by (bug #): Not a regression. 
User impact if declined: Potential information leak
Testing completed (on m-c, etc.): Baked on comm-central without any problems detected.
Risk to taking this patch (and alternatives if risky): Low to none.
String changes made by this patch: None.

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