Closed Bug 1163416 Opened 5 years ago Closed 5 years ago

"New Non-e10s Window" is not localizable


(Firefox :: General, defect)

Not set



Firefox 41
Tracking Status
e10s - ---
firefox41 --- fixed


(Reporter: flod, Assigned: dvedvick, Mentored)



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


(2 files)

Bug 1163062 made strings localizable, but not this one. Not sure if there are others that I never noticed.
There is also "After restart, a tab will open to where you can provide us feedback about your e10s experience."
Mentor: felipc
Whiteboard: [good first bug][lang=js]
I wouldn't mind taking a look at this if that's okay. It would be my first bug :)

It also looks like  "New Non-e10s Window" needs to be localized in CustomizableWidgets.jsm.
Yep, your help is very welcome! I have assigned the bug to you. Do you know how to edit the source code and compile it? Let me know if you have any questions
Assignee: nobody → dvedvick
Yes I do! Thanks!
Felipe, I finally got through making the changes that I could find.
Comment on attachment 8613311 [details] [diff] [review]
Make various e10s related string literals localizable

Review of attachment 8613311 [details] [diff] [review]:

That's great, David! Very close to final. Comments below.

BTW, when you post a patch, you generally should already request feedback or review on that patch, as it is easier for someone to notice. Since I think there will be no more changes needed on the next patch, you can already request "review?" from me.

::: browser/components/preferences/in-content/main.js
@@ -366,5 @@
>                    .getAttribute("windowtype") == "navigator:browser") {
>        // We should only include visible & non-pinned tabs
>        tabs = win.gBrowser.visibleTabs.slice(win.gBrowser._numPinnedTabs);
> -      

your text editor is probably fixing some leftover whitespace on this file. That is generally a good thing to do, but since this is touching a part of the file that is unrelated to your patch, we generally avoid doing this kind of changes.

::: browser/locales/en-US/chrome/browser/preferences/
@@ +144,5 @@
>  featureEnableRequiresRestart=%S must restart to enable this feature.
>  featureDisableRequiresRestart=%S must restart to disable this feature.
>  shouldRestartTitle=Restart %S
> +
> +#### e10S

Please use the proper comment style as above. A Localization Note is very helpful for people who will do these translations, so you can write a very brief explanation about this string. I think the main important to mention is that this string will appear when the user unchecks the "enable multi-process" checkbox.
Attachment #8613311 - Flags: feedback+
Hi Felipe, I think I implemented the changes. I was experimenting with the Atom editor for this, and it worked fine, but yes those whitespaces seem to be removed irregardless of what I did. So gedit fixed that!
Attachment #8613847 - Flags: review?(felipc)
Felipe - oops, it looks like Atom still attempted to fix some whitespaces in Until I can configure Atom to *not* do that (or if it's a bug, when it is fixed) I will need to use an older, saner editor! Is it possible for me to edit existing patches, and re-upload them? What's the proper process for re-work?
Comment on attachment 8613847 [details] [diff] [review]
Implement code review changes

Hi David, thanks for the update! About the whitespace changes done by Atom, that's no problem for your first patch! I'll adjust that when generating the commit for your patch.
Attachment #8613847 - Flags: review?(felipc) → review+
David, I see that your author name in the patch is:

david <dvedvick@...>

do you want to use only "david" like that or do you want to change that for the proper commit?
Flags: needinfo?(dvedvick)
Felipe, I see your point. Use "vedvick", that's a fairly unique name. Thanks.
Flags: needinfo?(dvedvick)
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.