Refactors private browsing logic #2481

Merged
merged 1 commit into from Dec 21, 2012

Conversation

Projects
None yet
4 participants
Contributor

yurydelendik commented Dec 18, 2012

Followup on #2471

Contributor

yurydelendik commented Dec 18, 2012

/botio-windows lint
/botio-linux preview

Collaborator

pdfjsbot commented Dec 18, 2012

From: Bot.io (Windows)


Received

Command cmd_lint from @yurydelendik received. Current queue size: 0

Live output at: http://107.22.172.223:8877/0225919d4587bdd/output.txt

Collaborator

pdfjsbot commented Dec 18, 2012

From: Bot.io (Linux)


Received

Command cmd_preview from @yurydelendik received. Current queue size: 0

Live output at: http://107.21.233.14:8877/61d9c60b45ce27c/output.txt

Collaborator

pdfjsbot commented Dec 18, 2012

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/0225919d4587bdd/output.txt

Total script time: 1.26 mins

  • Lint: Passed
extensions/firefox/components/PdfStreamConverter.js
+ } catch (x) {
+ // unable to get nsILoadContext
+ }
+ if (typeof docIsPrivate === 'undefined') {
@ehsan

ehsan Dec 19, 2012

Member

The usePrivateBrowsing attribute has been supported for quite a while, and it's currently supported in Firefox 17 (release), Aurora, Beta and Nightly (it was introduced in Firefox 13). In builds without per-window private browsing support (anything less than Firefox 20) this value is kept in sync with nsIPrivateBrowsingService.privateBrowsingEnabled, which means that you can just drop this whole if block and everything will work correctly in both types of builds.

extensions/firefox/components/PdfStreamConverter.js
+ .QueryInterface(Ci.nsIInterfaceRequestor)
+ .getInterface(Ci.nsIWebNavigation)
+ .QueryInterface(Ci.nsILoadContext)
+ .usePrivateBrowsing;
@ehsan

ehsan Dec 19, 2012

Member

Please use PrivateBrowsingUtils.isWindowPrivate instead of using the usePrivateBrowsing attribute directly. See http://mxr.mozilla.org/mozilla-central/source/toolkit/content/PrivateBrowsingUtils.jsm. Basically this needs to look like:

let docIsPrivate = PrivateBrowsingUtils.isWindowPrivate(this.domWindow);

Also, no need for the try/catch block as nothing here can throw as long as this.domWindow is really a DOM window object.

@yurydelendik

yurydelendik Dec 21, 2012

Contributor

This code is also running in the pdf.js add-on and we are trying to support Firefox 10ESR and other things that are using Gecko (e.g. Seamonkey) for now. We will be keeping the try/catch-es -- I added related comments in case if somebody decide to cleanup later.

Member

ehsan commented Dec 19, 2012

I commented on the patch, but this is basically on the right track. Thanks!

Contributor

yurydelendik commented Dec 21, 2012

/botio-linux preview
/botio-windows lint

Collaborator

pdfjsbot commented Dec 21, 2012

From: Bot.io (Windows)


Received

Command cmd_lint from @yurydelendik received. Current queue size: 0

Live output at: http://107.22.172.223:8877/4f81aa84ae51e9a/output.txt

Collaborator

pdfjsbot commented Dec 21, 2012

From: Bot.io (Linux)


Received

Command cmd_preview from @yurydelendik received. Current queue size: 0

Live output at: http://107.21.233.14:8877/d5fe5372dd0df74/output.txt

Collaborator

pdfjsbot commented Dec 21, 2012

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/4f81aa84ae51e9a/output.txt

Total script time: 1.22 mins

  • Lint: Passed

brendandahl added a commit that referenced this pull request Dec 21, 2012

Merge pull request #2481 from yurydelendik/rm-global-pb
Refactors private browsing logic

@brendandahl brendandahl merged commit 4b4601d into mozilla:master Dec 21, 2012

Member

ehsan commented Dec 21, 2012

How can we get this landed on m-c? Is it OK if I just port this patch on m-c while we're waiting for the next pdf.js merge?

Member

ehsan commented Dec 21, 2012

(I submitted #2498 to address my comments here).

Contributor

brendandahl commented Dec 21, 2012

@ehsan I pinged Ryan VanderMeulen and he plans to update pdf.js before the next uplift date.

@yurydelendik yurydelendik deleted the yurydelendik:rm-global-pb branch Feb 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment