With the current plan to harden the security of Firefox, we want to disallow internal privileged pages to use inline JavaScript. Since their amount is fairly limited, we start this by rewriting them bit by bit.

I am not sure as to whether I got the URLs right and cannot test since I haven't configured the build system yet. As commented in the markup, I have found platform-specific styles for about:privatebrowsing so I decided to create a new file with the inline style.

Comment on attachment 8361861 [details] [diff] [review] aboutPrivateBrowsingCSSJS.patch Thanks for the patch, Daniel. I asked Freddy for feedback on it.

Comment on attachment 8361861 [details] [diff] [review] aboutPrivateBrowsingCSSJS.patch Review of attachment 8361861 [details] [diff] [review]: ----------------------------------------------------------------- This looks good in general. There are some minor questions I have, though: ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js @@ +3,5 @@ > + > +Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm"); > + > +if (!PrivateBrowsingUtils.isWindowPrivate(window)) { > + document.title = "]]>&privatebrowsingpage.title.normal;<![CDATA["; It think we had problems with these custom entities in similar bugs. Have you confirmed this works? ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml @@ +29,3 @@ > > + <!-- Style shared between platforms --> > + <link rel="stylesheet" href="chrome://content/browser/aboutPrivateBrowsing.css" type="text/css" media="all"/> There's already another aboutPrivateBrowsing.css referenced above. What's the reason behind having two separate files? ::: browser/components/privatebrowsing/jar.mn @@ +4,3 @@ > > browser.jar: > * content/browser/aboutPrivateBrowsing.xhtml (content/aboutPrivateBrowsing.xhtml) While you're at it, please remove the trailing whitespace (this is what we call un-needed space characters after an ending line).

Snippet 1: Are referring to the string assigned to document.title? I will build on my system after reformatting HDD and fix. Snippet 2: I believe we need that, since there seems to be different styles for Linux, OS X and Windows as commented. Would you confirm on this please? Snippet 3: Will do. Thanks for the feedback! :)

Here are some other fixes I believe you missed. However, those are coming as I am having trouble with "./mach build". Shall file a build system bug on that. If you would like to/can help me with it, I will send you the output I get.

Sorry for not reaching out again, Daniel. Do you still have build system issues? Feel free to try MDN and Google since some of these problems are common things that other people have dealt with already. If this doesnt help you can file a bug and CC me. I will be happy to help you moving things forward. Can you also please refresh the patch to make sure it still applies? :)

Hello again Fredric, I've been passing through tough personal problems and have had some serious coursework to finish up. That's why it took me another 3 months to come back. I have recently started following CodeFirefox, which has cleared much of my confusion, but after a full build, I am getting a message that I need to clobber which I now did and had it result in a fail since "mozilla-central\obj-i686-pc-mingw32\" is not empty. Googling for solutions did not really help. Is it possible for you to help me over this thread, Twitter or IRC please? Or is filing a Build bug more appropriate? Thanks.

You can try asking in #developers (irc.mozilla.org) before you file a bug for the build system. But I think you can safely delete all "obj-*" dirs when you do a clobbered build.

On running a successful build, I realized that my patch breaks the page, which mach reports 2 warnings for aboutPrivateBrowsing.xhtml and aboutPrivateBrowsing.css: no useful preprocessor directives found Will be hopefully solving this by sometime this week. Thanks! :)

Here's the patch but I had to hack around &privatebrowsingpage.title because it does not work right. Suggestions?

Comment on attachment 8473151 [details] [diff] [review] patch 3 Review of attachment 8473151 [details] [diff] [review]: ----------------------------------------------------------------- This is close, just a couple things to address. ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js @@ +7,5 @@ > + > +Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm"); > + > +if (!PrivateBrowsingUtils.isWindowPrivate(window)) { > + document.title = "&privatebrowsingpage.title.normal;"; This isn't going to work. You'll need to move these strings to a .properties file and use a StringBundle to load the string from the string bundle to assign it to document.title. @@ +11,5 @@ > + document.title = "&privatebrowsingpage.title.normal;"; > + setFavIcon("chrome://global/skin/icons/question-16.png"); > +} else { > +#ifndef XP_MACOSX > + //document.title = "&privatebrowsingpage.title;"; Why is this line commented out? I see that this is redundant, since the title is already set to be this value within the XHTML file, but we shouldn't show a title that might get changed in the next instant. So let's go with only setting the title from the JS file. As a result, you can remove the `*` from the jar.mn for the XHTML file since we won't need preprocessing there anymore. Also, you can remove the `#` characters from the license header there since we won't be preprocessing that file anymore and the `#` was only used to comment out the license header from the build output. ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml @@ +23,3 @@ > <link rel="stylesheet" href="chrome://global/skin/netError.css" type="text/css" media="all"/> > <link rel="stylesheet" href="chrome://browser/skin/aboutPrivateBrowsing.css" type="text/css" media="all"/> > + <link rel="stylesheet" href="chrome://browser/content/aboutPrivateBrowsing.css" type="text/css"></link> Please move the content stylesheet to be included before the skin stylesheet, so that themes can override the content styling if necessary.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11) > Comment on attachment 8473151 [details] [diff] [review] > patch 3 > > Review of attachment 8473151 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is close, just a couple things to address. > > ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js > @@ +7,5 @@ > > + > > +Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm"); > > + > > +if (!PrivateBrowsingUtils.isWindowPrivate(window)) { > > + document.title = "&privatebrowsingpage.title.normal;"; > > This isn't going to work. You'll need to move these strings to a .properties > file and use a StringBundle to load the string from the string bundle to > assign it to document.title. > > @@ +11,5 @@ > > + document.title = "&privatebrowsingpage.title.normal;"; > > + setFavIcon("chrome://global/skin/icons/question-16.png"); > > +} else { > > +#ifndef XP_MACOSX > > + //document.title = "&privatebrowsingpage.title;"; > > Why is this line commented out? I see that this is redundant, since the > title is already set to be this value within the XHTML file, but we > shouldn't show a title that might get changed in the next instant. So let's > go with only setting the title from the JS file. > > As a result, you can remove the `*` from the jar.mn for the XHTML file since > we won't need preprocessing there anymore. Also, you can remove the `#` > characters from the license header there since we won't be preprocessing > that file anymore and the `#` was only used to comment out the license > header from the build output. > Are you sure about that? There are still some XHTML entities remaining once &privatebrowsingpage.title is removed so I don't know know how to go about it. :) > ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml > @@ +23,3 @@ > > <link rel="stylesheet" href="chrome://global/skin/netError.css" type="text/css" media="all"/> > > <link rel="stylesheet" href="chrome://browser/skin/aboutPrivateBrowsing.css" type="text/css" media="all"/> > > + <link rel="stylesheet" href="chrome://browser/content/aboutPrivateBrowsing.css" type="text/css"></link> > > Please move the content stylesheet to be included before the skin > stylesheet, so that themes can override the content styling if necessary. Done!

The XHTML entities will still be inserted appropriately even when preprocessing is disabled.

The preprocessing was only needed for the `#ifndef` and `#endif` tokens.

So I ended up using the string bundle service since <stringbundle> is not available in XHTML. Spent some time Googling and asking on IRC and following https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Miscellaneous#Using_string_bundles_from_JavaScript Only one thing is unclear: How should I construct .properties file URL relative to directory.

Quick update: Almost done. Just some merge conflicts to overcome due to recent changes.

This should do it. Proven to work for the new about:privatebrowsing! :D

Comment on attachment 8485860 [details] [diff] [review] aboutPrivateBrowsing Review of attachment 8485860 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for not getting to this until now. ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml @@ -30,5 @@ > - setFavIcon("chrome://global/skin/icons/question-16.png"); > - } else { > -#ifndef XP_MACOSX > - document.title = "]]>&aboutPrivateBrowsing.title;<![CDATA["; > -#endif Weird, so previously new tabs on Mac in Private Browsing mode would show "New Tab" as the page title, but on Windows and Linux we showed "You're browsing privately". "You're browsing privately" probably doesn't make much sense for the title of new tabs in private browsing mode, with maybe the exception of the first tab. Let's stick with what you've got for this patch and file a new bug to figure out what we want the tab titles to be for new tabs in Private Browsing mode.

I had to update your patch since http://hg.mozilla.org/mozilla-central/rev/380f8554aa17 just landed and added some more classes to elements within the page. I pushed the updated patch to the tryserver so we can run it through all of the automated tests. After the tests finish, we'll know if anything was broken from these changes. If everything looks good, we can add the checkin-needed keyword to this bug. The patch will then be checked-in and should appear in Firefox Nightly within a couple days. You can view the progress of your build at the following URL: https://tbpl.mozilla.org/?tree=Try&rev=0b63634d0440

Comment on attachment 8485860 [details] [diff] [review] aboutPrivateBrowsing Review of attachment 8485860 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch, nicely done! I've landed the patch for you: https://hg.mozilla.org/integration/fx-team/rev/4b5f794b80a8 Below are the changes I made: > Bug 956442 - Rewrite inline script in /browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml (about:privatebrowsing); r=reviewers I fixed the bug number in your commit message (should be 956482). ::: browser/components/privatebrowsing/jar.mn @@ -2,5 @@ > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > browser.jar: > -* content/browser/aboutPrivateBrowsing.css (content/aboutPrivateBrowsing.css) This file still needs to be pre-processed due to the license header.

Backed out for debug OSX perma-fail. https://hg.mozilla.org/integration/fx-team/rev/a6e449e2c13e https://tbpl.mozilla.org/php/getParsedLog.php?id=47913954&tree=Fx-Team

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #21) > Backed out for debug OSX perma-fail. > https://hg.mozilla.org/integration/fx-team/rev/a6e449e2c13e > > https://tbpl.mozilla.org/php/getParsedLog.php?id=47913954&tree=Fx-Team Any insight to what we could do about that?

(In reply to desiradaniel2007 from comment #22) > (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #21) > > Backed out for debug OSX perma-fail. > > https://hg.mozilla.org/integration/fx-team/rev/a6e449e2c13e > > > > https://tbpl.mozilla.org/php/getParsedLog.php?id=47913954&tree=Fx-Team > > Any insight to what we could do about that? It looks like browser_privatebrowsing_windowtitle.js needs to be updated now that OSX uses the same title for new private windows as Windows/Linux.

We also need to clarifiy whether the names I used in the properties file are compliant with the coding conventions. (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23) > It looks like browser_privatebrowsing_windowtitle.js needs to be updated now > that OSX uses the same title for new private windows as Windows/Linux. Hmm, that's likely to be better to be tracked within another bug as it should have been introduced by a change other than mine. Although I don't own a mac, I am willing to give it a shot.

Hi guys, sorry for taking so long on this one. I have just checked the diff for my patch and realized I mistakenly used the older version of the page so perhaps simply rewriting my patch over the last code-base could do it. Expect a working patch any time soon. :)

Sounds good, thanks for the update.

Please build and test on OS X.

Any update regarding review please?

Hey, what a coincidence. I'm actually doing the review as I write this. I had to get my OSX build up to date. Sorry, I should have commented in here earlier.

Thanks! :)

Comment on attachment 8515327 [details] [diff] [review] aboutPrivateBrowsing2 Review of attachment 8515327 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js @@ +13,5 @@ > + document.title = stringBundle.GetStringFromName("title.normal"); > + setFavIcon("chrome://global/skin/icons/question-16.png"); > +} else { > +#ifndef XP_MACOSX > + document.title = stringBundle.GetStringFromName("title"); So there is still an issue here where the XP_MACOSX symbol is not defined in this file. I'm not sure why. We can drop this special casing though, and just make all new tabs in private windows have this string. It doesn't make sense that OSX gets a different new tab title for private windows. Note that you'll still have to update the test as mentioned in comment #23.

I'm sorry, I missed that! Will give it a try later on this week. Should reply by Sunday. Cheers!

Hi, I have modified the test and am not sure whether I have done everything needed but please let me know :)

Has anyone running OS X re-run the test?

The test still hangs for me. I'm looking a bit deeper. We should get you level-1 try access so you can push to the tryserver and have it run the OSX tests for you :) You can follow the instructions here, https://www.mozilla.org/en-US/about/governance/policies/commit/

Ok, I fixed the test on Mac. Daniel, can you run the test on your machine and see if it passes for you in your environment still? To run it, you should run `mach mochitest-browser browser/components/privatebrowsing/test/browser/browser_privatebrowsing_windowtitle.js`

It's a shame for me that I did not do it properly. That was too easy! Will run the test as soon as I'm on that machine. Thanks :)

I tested out that patch on Windows and noticed that it now failed on Windows. With a minor tweak it should now work everywhere. I pushed it to the tryserver to make sure that it will work everywhere now. The rest of the patch looks good. Once the tryserver results come back and show that nothing has failed, we can get this checked in. Nice work and congratulations on getting your first patch r+'d \o/ You can view the progress of your build at the following URL: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b2e5664d809e Alternatively, view them on TBPL (soon to be deprecated): https://tbpl.mozilla.org/?tree=Try&rev=b2e5664d809e

Hey, thanks! I have just run it on my system (Win7) and it seems to pass. (No fail message.) Actually, this is my second patch. Will be keeping tabs on the try build's progress to see if there's anything I can do. Thanks again! :)

Ah, you're multiple bugzilla accounts (that only differ by the domain) confused me. Nonetheless, the congrats stays ;)

I just noticed that the commit message had the wrong bug number listed.

Again? :/ Thanks for the coordination.

You're also missing a license header in the .properties file.

(In reply to Francesco Lodolo [:flod] from comment #45) > You're also missing a license header in the .properties file. Also, file is using Windows line ending, which is definitely unusual (not sure if there's a policy for that).

Added the license header and fixed the line endings, https://hg.mozilla.org/integration/fx-team/rev/6b7bd35aca3d

Cheers guys. Notes taken for next patch!

As written in comment 18, reproduced with Nightly from 2014-09-21 under Mac OS X 10.8.5 the fact that 'New tab' is displayed as title when opening a PB window. With Fx 36 beta 8 (Build ID: 20150209164123), 'You're browsing privately' is displayed on Windows 7 64-bit, Mac OS X 10.8.5 and Ubuntu 14.04 32-bit. Please let me know at what else should I look in order to properly verify this bug. Thanks in advance!

That should be enough to verify this bug. Thanks!