Closed Bug 1250120 Opened 4 years ago Closed 4 years ago

Change name in Tools -> Web Developer menu

Categories

(DevTools :: Responsive Design Mode, defect, P1)

defect

Tracking

(firefox48 verified)

VERIFIED FIXED
Firefox 48
Iteration:
48.2 - Apr 4
Tracking Status
firefox48 --- verified

People

(Reporter: jryans, Assigned: clarkbw, Mentored)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [multiviewport] [mvp-rdm] [good first bug][difficult=easy])

Attachments

(1 file, 4 obsolete files)

Priority: -- → P3
Whiteboard: [multiviewport][triage] → [multiviewport]
This is a simple string change. Feel free to grab it and ask me or jryans help on what needs to be changed.
Whiteboard: [multiviewport] → [multiviewport][good first bug][difficult=easy]
i would like to take this bug
(In reply to Harshal Lele from comment #2)
> i would like to take this bug
Thanks for your interest. The rationale for changing the name is in bug 1248979.
We want the term "Responsive Design Mode" to be the one consistently displayed everywhere, including in the Tools menu, sub-menu Web Developer.
I believe the string that should be changed is this one:
https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#276

But I found another occurrence of this string here:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/locales/en-US/responsiveUI.properties#47

So we should probably fix both.

Make sure you read through our hacking guide: https://wiki.mozilla.org/DevTools/Hacking
Install the dev env, make your changes, build and test locally.
You can then upload a patch and ask for review here.

Assigning the bug to you now.
Assignee: nobody → frost17121997
Status: NEW → ASSIGNED
There is also a Git focused (as opposed to Mercurial) guide available:

https://developer.mozilla.org/en-US/docs/Tools/Contributing

Use whichever tool you prefer.
Attached patch bug1250120-changedName.patch (obsolete) — Splinter Review
Changed values in browser.dtd and in responsiveUI.properties to "Responsive Design Mode"
Attachment #8722465 - Flags: review?(jryans)
Comment on attachment 8722465 [details] [diff] [review]
bug1250120-changedName.patch

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

Thanks for working on this!

I think there are a few more places to clean up.  Please update all the results in this search:

https://dxr.mozilla.org/mozilla-central/search?q=%22Responsive%20Design%20View%22&case=true

Additionally, when changing string entries in .dtd or .properties files, we have to update the ID as well, so that translators notice the new ID.  As example, "responsiveDesignTool.label" could change to "responsiveDesignTool.label2" or "responsiveDesignMode.label" (though if you use this one, you should probably change the other IDs next to it as well).  You'll also need to search to where the old ID was used and update the usages with the new ID.
Attachment #8722465 - Flags: review?(jryans) → feedback+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Comment on attachment 8722465 [details] [diff] [review]
> bug1250120-changedName.patch
> 
> Review of attachment 8722465 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for working on this!
> 
> I think there are a few more places to clean up.  Please update all the
> results in this search:
> 
> https://dxr.mozilla.org/mozilla-central/
> search?q=%22Responsive%20Design%20View%22&case=true
> 
> Additionally, when changing string entries in .dtd or .properties files, we
> have to update the ID as well, so that translators notice the new ID.  As
> example, "responsiveDesignTool.label" could change to
> "responsiveDesignTool.label2" or "responsiveDesignMode.label" (though if you
> use this one, you should probably change the other IDs next to it as well). 
> You'll also need to search to where the old ID was used and update the
> usages with the new ID.

Do i also have to change "responsiveUI.customNamePromptTitle" to "responsiveUI.customNamePromptTitle2" in responsiveUI.properties or does that remain the same?
(In reply to Harshal Lele from comment #8)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> > Comment on attachment 8722465 [details] [diff] [review]
> > bug1250120-changedName.patch
> > 
> > Review of attachment 8722465 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks for working on this!
> > 
> > I think there are a few more places to clean up.  Please update all the
> > results in this search:
> > 
> > https://dxr.mozilla.org/mozilla-central/
> > search?q=%22Responsive%20Design%20View%22&case=true
> > 
> > Additionally, when changing string entries in .dtd or .properties files, we
> > have to update the ID as well, so that translators notice the new ID.  As
> > example, "responsiveDesignTool.label" could change to
> > "responsiveDesignTool.label2" or "responsiveDesignMode.label" (though if you
> > use this one, you should probably change the other IDs next to it as well). 
> > You'll also need to search to where the old ID was used and update the
> > usages with the new ID.
> 
> Do i also have to change "responsiveUI.customNamePromptTitle" to
> "responsiveUI.customNamePromptTitle2" in responsiveUI.properties or does
> that remain the same?

Yes, any string that already exists in *.dtd or *.properties needs its ID modified when changing the English text.

You can read more about this on the l10n guide[1].

Also, for future questions, please use the needinfo? flag at the bottom of the bug set to mentor, so we can be sure your question in not lost in the deluge of bug mail.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Updating_Entity_Names
Priority: P3 → --
Flags: qe-verify?
Whiteboard: [multiviewport][good first bug][difficult=easy] → [multiviewport] [mvp-rdm] [good first bug][difficult=easy]
Flags: qe-verify? → qe-verify+
Hello Harshal, checking in if you are still working on this bug?  Thanks.
Flags: needinfo?(frost17121997)
(In reply to Marco Mucci [:MarcoM] from comment #10)
> Hello Harshal, checking in if you are still working on this bug?  Thanks.

Due to various reasons, i can't work on this bug right now.Please transfer it to somebody else, and i apologize for the lack of communication.
Flags: needinfo?(frost17121997)
(In reply to Harshal Lele from comment #11)
> (In reply to Marco Mucci [:MarcoM] from comment #10)
> > Hello Harshal, checking in if you are still working on this bug?  Thanks.
> 
> Due to various reasons, i can't work on this bug right now.Please transfer
> it to somebody else, and i apologize for the lack of communication.

No problem, thanks very much for the update.
Assignee: frost17121997 → nobody
Status: ASSIGNED → NEW
Attached patch 1250120.patch (obsolete) — Splinter Review
Updated patch
Assignee: nobody → clarkbw
Status: NEW → ASSIGNED
Comment on attachment 8734366 [details] [diff] [review]
1250120.patch

Can you take a look at this?
Attachment #8734366 - Flags: review?(jryans)
Comment on attachment 8734366 [details] [diff] [review]
1250120.patch

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

String changes look good to me, thanks!

For completeness, it might be nice to also update the code comment here:

https://dxr.mozilla.org/mozilla-central/rev/63be002b4a803df1122823841ef7633b7561d873/docshell/base/nsIDocShell.idl#1035
Attachment #8734366 - Flags: review?(jryans) → review+
Attached patch 1250120.patch (obsolete) — Splinter Review
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #15)
> Comment on attachment 8734366 [details] [diff] [review]
> 1250120.patch
> 
> Review of attachment 8734366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> String changes look good to me, thanks!
> 
> For completeness, it might be nice to also update the code comment here:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 63be002b4a803df1122823841ef7633b7561d873/docshell/base/nsIDocShell.idl#1035

Done.  I should have made a comment about that.  I wasn't sure that we needed to update the comments of the idl file but its probably best to keep everything consistent.

I'll re-request review for the addition of the IDL file.  Assuming an r+ I think we're all done.
Attachment #8722465 - Attachment is obsolete: true
Attachment #8734366 - Attachment is obsolete: true
Attachment #8735462 - Flags: review?(jryans)
Comment on attachment 8735462 [details] [diff] [review]
1250120.patch

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

Looks good, thanks!
Attachment #8735462 - Flags: review?(jryans) → review+
@jryans can you push a try build for me?  I don't think I have hg commit access anymore.
Flags: needinfo?(jryans)
(In reply to Bryan Clark (DevTools PM) [@clarkbw] from comment #18)
> @jryans can you push a try build for me?  I don't think I have hg commit
> access anymore.

I can do so, but I just realized the patch is not quite in the right format.  It's a straight diff, instead of an exported commit with commit message.

If you're using Git, try these steps: https://developer.mozilla.org/en-US/docs/Tools/Contributing#Creating_a_patch_to_check_in

If you're using Mercurial, try these steps: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Please update the patch and I'll try again.  Thanks!
Flags: needinfo?(jryans) → needinfo?(clarkbw)
Attached patch 1250120.patch (obsolete) — Splinter Review
mmm, queues
Attachment #8735462 - Attachment is obsolete: true
Flags: needinfo?(clarkbw)
That's a little closer!  The commit message is still missing though, so it won't apply correctly.
Flags: needinfo?(clarkbw)
Attached patch 1250120.patchSplinter Review
sheesh
Attachment #8735694 - Attachment is obsolete: true
Flags: needinfo?(clarkbw)
Thanks for sticking with it! :)  I went ahead and landed it on fx-team.
I learned a lot more about our contributor flow :)
https://hg.mozilla.org/mozilla-central/rev/1e908cdc6c42
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Iteration: --- → 48.2 - Apr 4
Priority: P2 → P1
QA Contact: mihai.boldan
The Responsive Design Mode name is correctly displayed.
The tests were performed on FF 48.0a1 (2016-04-13) and on Windows 10 x64, Mac OS X 10.11 and on Ubuntu 14.04 x86.
I am marking this issues Verified-Fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.