Rename prompt IDs from the form of info.body to infoBody

RESOLVED FIXED in Firefox 59

Status

()

P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jaws, Assigned: gorovoja, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla59
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Multiple places within our prompt code we have IDs for elements that use of the form of `info.body`. When referenced from CSS we have to escape the period and write the selector as `info\.body`. This makes it hard to search for the ID in our codebase as it appears in two different forms.

These are the three that need changing:
https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/toolkit/components/prompts/content/commonDialog.xul#64,72,79

You will need to use searchfox.org to search the codebase for any references to `info.title`, `info\.title`, `info.icon`, `info\.icon`, `info.body`, and `info\.body`.

The same changes should be made to https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/toolkit/components/prompts/content/tabprompts.xml#45-46
(Assignee)

Comment 1

a year ago
Hi I would be happy to take this bug as my first one )
(Assignee)

Comment 2

a year ago
Created attachment 8928143 [details] [diff] [review]
changes

not sure if I had to put any flags or what so ever, first attempt of me to do something in OSS and Firefox in general, hope to hear reviews
Comment on attachment 8928143 [details] [diff] [review]
changes

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

Hello, in the future you should set the review flag. See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_-_Get_your_code_reviewed

I think you forgot to update the CSS references that Jared mentioned though e.g. https://dxr.mozilla.org/mozilla-central/search?q=info%5C.body
Attachment #8928143 - Flags: feedback+
Assignee: nobody → gorovoja
Status: NEW → ASSIGNED
(Assignee)

Comment 4

a year ago
Created attachment 8928448 [details] [diff] [review]
newPatch

Silly mistake by me, did not figure out how mozilla's search engine works properly.. changed the rest of the code and added flag.
Attachment #8928448 - Flags: review+
Comment on attachment 8928448 [details] [diff] [review]
newPatch

Thanks, when you set the review flag, please set it to a '?' and then in the Requestee field put in ':jaws' (without the quotes).
Attachment #8928448 - Flags: review+ → review?(jaws)
Can you fold your patches together? You can use the `hg histedit` command to "roll" your patches together. See https://www.mercurial-scm.org/wiki/HisteditExtension for how to use the command.
Flags: needinfo?(gorovoja)
Comment on attachment 8928448 [details] [diff] [review]
newPatch

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

Thanks for writing these patches! Please see my note below and my previous note about rolling up the patches together.

::: toolkit/themes/osx/global/commonDialog.css
@@ +26,1 @@
>  #info\.header,

Please change this one too.
Attachment #8928448 - Flags: review?(jaws) → review-
(Assignee)

Comment 8

a year ago
Created attachment 8928965 [details]
final?
Flags: needinfo?(gorovoja)
Attachment #8928965 - Flags: review?(jaws)
(Assignee)

Comment 9

a year ago
oh, it ended up being wrong format, I need to re-do it..
(Assignee)

Comment 10

a year ago
Created attachment 8929230 [details] [diff] [review]
final maybe

hopefully this one contains everything needed.
Attachment #8928143 - Attachment is obsolete: true
Attachment #8928448 - Attachment is obsolete: true
Attachment #8928965 - Attachment is obsolete: true
Attachment #8928965 - Flags: review?(jaws)
Attachment #8929230 - Flags: review?(jaws)
Comment on attachment 8929230 [details] [diff] [review]
final maybe

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

Thanks, this looks good. I'll push this patch to tryserver to make sure that all tests pass, then we can get this landed. In the meantime, can you add a commit message to this? You can use "Bug 1416875 - Renamed prompt IDs from the form of info.body to InfoBody so they won't need escaping. r=jaws". Then you can attach the commit with the commit message to this bug.
Attachment #8929230 - Flags: review?(jaws) → review+
(Assignee)

Comment 13

a year ago
I've tryed to do that, but seems like you already did it?
I pushed the patch to our "try" server which will apply your patch and make sure that there are no test failures.

The patch still needs an updated commit message. If you need help setting the commit message you can join #introduction on the Mozilla IRC server at irc.mozilla.org.
Oh, I see what you mean, yes I did put a commit message on it when I pushed it to tryserver. I'll go ahead and land this for you now.

Comment 16

a year ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/491fbd1c506f
Renamed prompt IDs from the form of info.body to InfoBody so they won't need escaping. r=jaws
https://hg.mozilla.org/mozilla-central/rev/491fbd1c506f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Congratulations on landing your first patch! Thank you for your contribution to Firefox and Mozilla!
(Assignee)

Comment 19

a year ago
Thank you so much for help :)
You need to log in before you can comment on or make changes to this bug.