Closed Bug 1062264 Opened 5 years ago Closed 4 years ago

Inconsistency between the tab name from Private Window across platforms

Categories

(Firefox :: General, defect)

35 Branch
defect
Not set

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox35 --- affected

People

(Reporter: cbadau, Unassigned, Mentored)

Details

(Whiteboard: [good first bug][lang=html])

Attachments

(3 files, 12 obsolete files)

5.88 KB, patch
jaws
: feedback+
Details | Diff | Splinter Review
3.88 KB, patch
Details | Diff | Splinter Review
17.45 KB, patch
Details | Diff | Splinter Review
This bug was filled based on the discussion from bug 1009370 comment 32.

Reproducible on the latest Nightly 35.0a1(BuildID: 20140902121319)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0

Steps to reproduce: 
1. Launch Firefox. 
2. Open a new Private Window. 

Actual results: 
 - on Windows and Ubuntu, the tab/newtab name from Private Window is "You're browsing privately"
 - on Mac, the tab/newtab name from Private Window is "New Tab"

Expected results: 
"Let's use »Private Browsing« as the tab title (on all platforms)".
Version: 34 Branch → 35 Branch
Mentor: bmcbride
Whiteboard: [good second bug][lang=html]
Some more context for this:

Currently about:privatebrowsing will use one of these strings for the page title, depending on if that window is a private window or not:
http://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd#5-6

We want to change that so it only ever uses "Private Browsing" for the page title. Since that is becoming uniform, the favicon should also be made so it doesn't change also. So that means updating the page:
http://dxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
So the title and favicon are defined in the markup, instead of set via JS.


Note that those strings are also used as headings in the page - we want to keep that as-is, and only change the page title.
Hi, I'm interested in patching this bug. Can you assign this to me? Thanks!!
Attachment #8485438 - Flags: review+
Assignee: nobody → aman_alam
Status: NEW → ASSIGNED
Comment on attachment 8485438 [details] [diff] [review]
Private browsing tab fix for MacOS

Sorry for the wrong flag... #firstBug
Attachment #8485438 - Flags: review+ → review?(bmcbride)
Whiteboard: [good second bug][lang=html] → [good first bug][lang=html]
Comment on attachment 8485438 [details] [diff] [review]
Private browsing tab fix for MacOS

Review of attachment 8485438 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
@@ +32,1 @@
>          document.title = "]]>&aboutPrivateBrowsing.title;<![CDATA[";

We want all the platforms to use the same page title, so instead of setting it in JavaScript, you should remove any code that sets document.title, and instead add it to the HTML markup.

::: browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd
@@ +2,4 @@
>     - 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/. -->
>  
> +<!ENTITY aboutPrivateBrowsing.title            "Private Browsing">

This is used for the main heading in the page - we want to keep that as-is. So you'll need to add a new string, aboutPrivateBrowsing.pageTitle
Attachment #8485438 - Flags: review?(bmcbride) → review-
Any update on this bug? Looking to make my first contribution.
Alam: Do you intend to continue working on this bug?
Flags: needinfo?(aman_alam)
If Alam does not wish to take it I'll still be willing. Just need some mentorship in setting up.
Over to you, Stephen!

I'd recommend a read through https://developer.mozilla.org/en-US/docs/Introduction
Drop into #introduction on IRC (https://wiki.mozilla.org/IRC) if you'd like some guidance setting up.
Assignee: aman_alam → stephen.jd.murphy
Flags: needinfo?(aman_alam)
Thanks Blair!

I've set up mozilla-central and have been over the documentation but could do with some guidance setting up with XCode and understanding the file structure.
Getting back to this tonight, just following the instructions above in Comment 6.

I removed the two document.title calls in aboutPrivateBrowsing.xhtml and it looks like so:

 if (!PrivateBrowsingUtils.isWindowPrivate(window)) {
        setFavIcon("chrome://global/skin/icons/question-16.png"); 
        setFavIcon("chrome://browser/skin/Privacy-16.png");
      }

Next I added the following line to aboutPrivateBrowsing.dtd

<!ENTITY aboutPrivateBrowsing.pageTitle        "Private Browsing">
<!ENTITY aboutPrivateBrowsing.title            "You're browsing privately">
<!ENTITY aboutPrivateBrowsing.title.normal     "Open a private window?">

After which I built/ran and had observed no changes to the private window. I know I am missing logic but this is what I got from the instructions above. Could you tell/show where I am going wrong? Thanks!
Flags: needinfo?(bmcbride)
(Apologies for the lag - been a very rough few weeks)


(In reply to Stephen M [ste] from comment #12)
> After which I built/ran and had observed no changes to the private window. I
> know I am missing logic but this is what I got from the instructions above.
> Could you tell/show where I am going wrong? Thanks!

It's difficult for me to follow with just snippets - could you attach your entire patch?
(See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch)
Flags: needinfo?(bmcbride) → needinfo?(stephen.jd.murphy)
Attached patch aboutPrivateBrowsing.dtd (obsolete) — Splinter Review
Flags: needinfo?(stephen.jd.murphy)
Attachment #8532948 - Flags: review?(bmcbride)
Attached file aboutPrivateBrowsing.xhtml (obsolete) —
I've attached both files. Take a quick look if you can see where I am going wrong. Thanks Blair!
Attachment #8532949 - Flags: review?(bmcbride)
A couple of general things to note first:
* It looks like you're working on old versions of these files - they're a month or more out of date. Could you confirm you're working from a cloned version of the repository and that it's up to date (https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_check_out_Mozilla.3F)
* Attaching just the files also makes it difficult for me, because I can't easily see what's changed. It's best to always attach a patch file (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch)


As for the actual changes related to the page title...
* You'll need to add a <title> element to the <head> in aboutPrivateBrowsing.xhtml, for the page to have a title - since it's no longer being set via JavaScript.
Flags: needinfo?(stephen.jd.murphy)
Attachment #8532948 - Flags: review?(bmcbride)
Attachment #8532949 - Flags: review?(bmcbride)
Attached patch aboutPrivateBrowsing.xhtml (obsolete) — Splinter Review
Attachment #8532949 - Attachment is obsolete: true
Flags: needinfo?(stephen.jd.murphy)
Attachment #8533087 - Flags: feedback?(bmcbride)
Attached patch aboutPrivateBrowsing.dtd (obsolete) — Splinter Review
Sorry about that, I was following this mdn post and thought I had pulled down the latest changes. https://developer.mozilla.org/en/docs/Simple_Firefox_build

I've reattached both files as patches and made the previous obsolete.
Attachment #8532948 - Attachment is obsolete: true
Attachment #8533090 - Flags: feedback?(bmcbride)
Update:

I have added this to the HTML

<title>&aboutPrivateBrowsing.pageTitle;</title>

which shows "Private Browsing" as the title. However I am getting Javascript errors in the terminal. Can you review the HTML file (line 29), I expect this if statement is causing the js errors.

Cheers
Ah, I think you're misunderstanding what I mean by "patch file" :) When reviewing bugs, reviewers don't want the source files you've modified - because easy/simple way to see the differences between what's in the repository and the modified files you've uploaded, or even know what version of the files you started with. Instead, we want a file that describes the changes you've made - a patch file (or diff file). You need to generate such a file, using:

  hg export > bugXXX.patch

That file will describe all changes you've made to the tree (ie, both files).

Also, you'll need to base your changes off a more recent version. If you look at the repository here, note that the JS parts have been moved to their own file:
http://dxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
http://dxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/content/aboutPrivateBrowsing.js
Flags: needinfo?(stephen.jd.murphy)
Attachment #8533087 - Flags: feedback?(bmcbride)
Attachment #8533090 - Flags: feedback?(bmcbride)
Attachment #8485438 - Attachment is obsolete: true
Attached patch Bug1062264.patch (obsolete) — Splinter Review
How is this?

I've tested it and it works on OSX. Not sure it's correct or merely a 'hack' job. Let me know
Flags: needinfo?(stephen.jd.murphy)
Attachment #8536548 - Flags: review?(bmcbride)
Comment on attachment 8536548 [details] [diff] [review]
Bug1062264.patch

Review of attachment 8536548 [details] [diff] [review]:
-----------------------------------------------------------------

(Sorry for not looking at this sooner - I had some time off work last week.)

This is the patch file I was after :)

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js
@@ +12,5 @@
>  if (!PrivateBrowsingUtils.isWindowPrivate(window)) {
>    document.title = stringBundle.GetStringFromName("title.normal");
>    setFavIcon("chrome://global/skin/icons/question-16.png");
>  } else {
> +  document.title = stringBundle.GetStringFromName("pageTitle");

Since you're setting the page title in the .xhtml file, you don't need to set it here. So both cases where document.title is set can be removed.

@@ +17,1 @@
>    setFavIcon("chrome://browser/skin/Privacy-16.png");

And then additionally, we want the page's favicon to always be "chrome://browser/skin/Privacy-16.png" - so that will need to be added to the .xhtml file too (like how it's done in http://dxr.mozilla.org/mozilla-central/source/browser/base/content/aboutaccounts/aboutaccounts.xhtml#22).

Which means you'll be able to remove this entire if/else block, and the setFavIcon function later in the file.
Attachment #8536548 - Flags: review?(bmcbride) → review-
Attached patch Bug1062264-2.patch (obsolete) — Splinter Review
Updated the .js file to remove the if/else and set the favicon in the <head> of xhtml file

Let me know how that looks! :)
Attachment #8533087 - Attachment is obsolete: true
Attachment #8533090 - Attachment is obsolete: true
Attachment #8536548 - Attachment is obsolete: true
Attachment #8541247 - Flags: review?(bmcbride)
Comment on attachment 8541247 [details] [diff] [review]
Bug1062264-2.patch

Review of attachment 8541247 [details] [diff] [review]:
-----------------------------------------------------------------

Er, hmm, that's odd, this patch includes changes to xpcom/tests/unit/data/SmallApp.app/Contents/MacOS/SmallApp - not sure how you managed that!

Anyway... this is a patch based on your previous patch. ie, it's changes you've made since the last patch, but doesn't include your original changes. What I need is a match that includes *all* of your changes. Unfortunately, how to do this depends on what you've done with your repository. I'd recommend that you join #introduction on IRC (http://wiki.mozilla.org/IRC) and ask for some help.
Attachment #8541247 - Flags: review?(bmcbride) → review-
Interesting! I'll take a look and get back to you. Might need to redo my repo, if so I'll do all of my changes at once and give you an updated patch file.
Attached patch bug1062264.patch (obsolete) — Splinter Review
How is this!

/Stephen
Attachment #8541247 - Attachment is obsolete: true
Attachment #8541913 - Flags: review?(bmcbride)
Comment on attachment 8541913 [details] [diff] [review]
bug1062264.patch

Review of attachment 8541913 [details] [diff] [review]:
-----------------------------------------------------------------

Bingo :) This is looking better.

So now, you just have a few things to clean up:
* aboutPrivateBrowsing.js still has the setFavIcon() function, which is no longer used. So that should be removed.
* stringBundle is no longer used in that file either, so can also be removed
* The string bungle that was loaded, aboutPrivateBrowsing.properties, is no longer used - so that file should be removed
* aboutPrivateBrowsing.properties is referenced in a jar.mn manifest file (jar.mn files describe how files get packaged for the application that gets build). So you'll need to remove the line referencing it in browser.locales/jar.mn

And finally, we have a test that checks the page title of the page, browser_privatebrowsing_windowtitle.js - this will need updated. You can test if this is passing or failing by running the following:
  ./mach test browser/components/privatebrowsing/test/browser/browser_privatebrowsing_windowtitle.js
Attachment #8541913 - Flags: review?(bmcbride) → review-
Attached patch bug1062264.patch (obsolete) — Splinter Review
I've gone ahead and made the changes and have updated the patch file. 

I ran the test and can confirm the title works as expected? 

What else! :) Trying to understand the patch file too.
Attachment #8541913 - Attachment is obsolete: true
Attachment #8542126 - Flags: review?(bmcbride)
Comment on attachment 8542126 [details] [diff] [review]
bug1062264.patch

Review of attachment 8542126 [details] [diff] [review]:
-----------------------------------------------------------------

Did you upload the wrong file? This is identical to the previous patch.
Attachment #8542126 - Flags: review?(bmcbride)
Attached patch mypatch.diff (obsolete) — Splinter Review
How is this? 

I'm struggling to read the patch/diff files and the documentation is a little confusing to follow when I just want to 'update' changes to a patch file.
Attachment #8542126 - Attachment is obsolete: true
Attachment #8543647 - Flags: review?(bmcbride)
Comment on attachment 8543647 [details] [diff] [review]
mypatch.diff

Review of attachment 8543647 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, that looks good. Still need to update the test file though.

What has your workflow been for creating these patches?
Attachment #8543647 - Flags: review?(bmcbride) → review-
I've ran the test but am unable to locate a test file after it finishes. Should it be located in a global directory such as the patch/diff files?

My workflow seems okay.

1. Fix issue
2. build/Run to test
3. Create patch
4. Update patch with diff
5. upload

My issue was understanding the MDN pages on mercurial, the content was a little confusing between creating a patch and how to 'update' a patch.
Sorry, I meant: How are you creating/updating your patch? ie, are you commiting each time, or using MQ, using commit --amend, or what?


The test file is at browser/components/privatebrowsing/test/browser/browser_privatebrowsing_windowtitle.js
When you rebuild Firefox it gets copied elsewhere, but you just need to pass the original path to `mach`, and it'll run the test.

So, with your patch now and the test file unmodified, running:

  ./mach test browser/components/privatebrowsing/test/browser/browser_privatebrowsing_windowtitle.js

should result in test failures.
Hey Blair, sorry for the inactivity it's been a busy month! I'm setting up my dev environment again later today so lets get this bug squashed sooner rather than later!
Attached patch bug1062264.patch (obsolete) — Splinter Review
Blair,

Long time no talk. Sorry for the delay in getting back.

I've set back up my developer environment and redid the changes from scratch. Take a look and lets get this finished :)
Attachment #8543647 - Attachment is obsolete: true
Flags: needinfo?(bmcbride)
Attachment #8574413 - Flags: review?(bmcbride)
Comment on attachment 8574413 [details] [diff] [review]
bug1062264.patch

Review of attachment 8574413 [details] [diff] [review]:
-----------------------------------------------------------------

I understand busy... I'm so swamped with stuff at the moment that I'm going to see if Jared has time to look at this for me.
Attachment #8574413 - Flags: review?(bmcbride) → review?(jaws)
(You don't need to needinfo me if you flag me for review - I'll always see review requests.)
Flags: needinfo?(bmcbride)
Comment on attachment 8574413 [details] [diff] [review]
bug1062264.patch

Review of attachment 8574413 [details] [diff] [review]:
-----------------------------------------------------------------

This isn't the right patch. This is a merge changeset.
Attachment #8574413 - Flags: review?(jaws)
Attached patch 1062264.patchSplinter Review
Hey Jared,

Sorry about that! Still wrapping my head around this after being away. How is the patch file I attached now?
Attachment #8574413 - Attachment is obsolete: true
Flags: needinfo?(jaws)
Comment on attachment 8577719 [details] [diff] [review]
1062264.patch

Thanks, this looks good. No problem about the patch/file issues, that happens to most people who are new working with Mercurial & large code projects like mozilla-central.

This is close, just two things are left:
1) You'll need to update /browser/components/privatebrowsing/test/browser_privatebrowsing_windowtitle.js. The test can be made much simpler now that it is the same for all cases.
2) You should run `hg remove /browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.properties` now that the file isn't used anymore.
Flags: needinfo?(jaws)
Attachment #8577719 - Flags: feedback+
Thanks for the feedback! I had forgot to set up mercurial properly, this is the only project in which I use it so I am new to it. Mostly using git day to day.

1) With the _windowtitle.js I take it I should remove the if/else starting on line 24 - 39? I can see what the logic is (I think) but my js skills are not that advanced.

2)Removed!
Woops, forget to needinfo you guys.
Flags: needinfo?(jaws)
Flags: needinfo?(bmcbride)
Yeah, it looks like 24-39 is the only part that needs changing.
Flags: needinfo?(jaws)
Flags: needinfo?(bmcbride)
Cool, how is this?
Flags: needinfo?(jaws)
At a quick glance it looks like that will break the test, since those variables are probably tested later on in the test to see what the value is. Do you know how to run the test? 

Also, can you put those changes in to the larger patch as well as include the removal of aboutPrivateBrowsing.properties?
Flags: needinfo?(jaws)
Back, computer was in the shop!

I'm not sure how to run the test, anything to advice before I go searching around?

I'll attempt to get a larger patch. New dev setup
Flags: needinfo?(jaws)
Seems like this bug has been patched? Private Tab on Aurora reports the intended Title.
Flags: needinfo?(jaws)
@Jared, are you free to help me resolve this bug? Or can you pass it on to someone else? It's been outstanding a bit too long now and my apologies it is not actually resolved. Help a newbie out?
Status: ASSIGNED → NEW
Flags: needinfo?(jaws)
(In reply to Stephen M [ste] from comment #48)
> @Jared, are you free to help me resolve this bug? Or can you pass it on to
> someone else? It's been outstanding a bit too long now and my apologies it
> is not actually resolved. Help a newbie out?

Hey I can help you but I'm a bit busy this week. I will try to come back to this bug sometime this week.
Hi Stephen, what can I help you with? browser_privatebrowsing_windowtitle.js is still expecting and verifying that the window title is different between Windows and OSX.

Would you like more explanation behind comment 45?
Flags: needinfo?(jaws)
Hey,

In short I have the issue fixed but I need help figuring out how to finish this off. I believe testing is next up and then passing the patch over for review and included into the Nightly trunk?

Would an updated patch file be helpful? Lets do this! Thanks
Flags: needinfo?(jaws)
Yeah, can you upload an updated patch file?
Flags: needinfo?(jaws)
Mentor: bmcbride → jaws
Attached patch updated-1062264.patch (obsolete) — Splinter Review
Here's where we are with the patch. Brushing off the combwebs
Flags: needinfo?(jaws)
Attachment #8631527 - Flags: feedback?(jaws)
Comment on attachment 8631527 [details] [diff] [review]
updated-1062264.patch

This patch file is empty. Maybe you need to qrefresh or the wrong file got uploaded?
Flags: needinfo?(jaws)
Attachment #8631527 - Flags: feedback?(jaws)
Strange one! Hows this?
Attachment #8631527 - Attachment is obsolete: true
Flags: needinfo?(jaws)
Hey Jared, any update on this? Cheers!
Flagged
Comment on attachment 8631555 [details] [diff] [review]
updated-1062264.patch

Hey Stephen, sorry for the slow reply. The patch here is touching some files that it shouldn't as well as deleting other files. It also only includes the removal of aboutPrivateBrowsing.properties.
Flags: needinfo?(jaws)
Thanks for the update. I'm not sure why it's seeing other info not related to the bug itself. I'll look into and get back to you
It seems I have hit a platform snag with OS X. Since the 10.10.4 update I am unable to create or run a build. I get thrown the same issue even with a 2nd fresh enviroment set up. 

''mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/Nightly.app/Contents/MacOS/firefox does not exist.''
Assignee: stephen.jd.murphy → nobody
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.