Use Services.prompt instead of gPromptService in MailNews Core

VERIFIED FIXED in Thunderbird 13.0

Status

MailNews Core
Backend
--
minor
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: sgautherie, Assigned: aceman)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 13.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

11.36 KB, patch
standard8
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
"Found 6 matching lines in 3 files"
Flags: in-testsuite-
(Assignee)

Comment 1

6 years ago
Can I take this as I am already going to convert many other services?
Blocks: 720356
Yep, doesn't look like anyone else has it at the moment.
Assignee: nobody → acelists
(Assignee)

Comment 3

6 years ago
Created attachment 590828 [details] [diff] [review]
patch
Attachment #590828 - Flags: review?(mbanner)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED

Comment 4

6 years ago
May I suggest that you do whitespace cleanup in a separate bug?
(Assignee)

Comment 5

6 years ago
I don't know what the conventions are here.

But if I do not do whitespace cleanup while I do some serious changes, I will probably not bother later. Mixed patches passed so far. I don't know. I didn't do pure whitespace cleanup as the point of some bug so far. But if TB devs would wish it I can think about it. I just think these bugs descending from bug 720356 could be good place to also attach whitespace cleanup as I think they aren't hard to review.

Comment 6

6 years ago
It's just a suggestion. Please feel free to ignore me.
(Assignee)

Comment 7

6 years ago
I understand your suggestion and why it is good to separate the changes. Thank you. I just can't yet manage so many pending patches (I am new to this hg mq stuff and also due to slower reviews) and make each change 100% separated.
Comment on attachment 590828 [details] [diff] [review]
patch

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

Generally I don't mind a little bit of whitespace cleanup, especially at end of lines (which my emacs does anyway). Some of this was bordering on a bit much for one patch, but I think its fine.

r=me with the nits fixed.

::: mailnews/addrbook/content/abMailListDialog.js
@@ +70,5 @@
>  
>    if (addressbook.mailListNameExists(listname))
>    {
> +    Services.prompt.alert(window,
> +                    gAddressBookBundle.getString("mailListNameExistsTitle"),

nit: the following lines should be 2-space indented, or aligned with the first letter after the opening bracket. Giving the length of the parameters in this case, I'd go with 2-space as they were.

@@ +565,5 @@
>  }
>  
>  function DropListAddress(target, address)
>  {
> +  awClickEmptySpace(target, true);    //that will automatically set the focus on a new available row, and make sure is visible

Nit: as you're touching this, please put the comment on the previous line with a space after // and a captial letter and full stop.

::: mailnews/base/search/content/CustomHeaders.js
@@ +83,5 @@
>    if (hdrs)
>    {
>      hdrs = hdrs.replace(/\s+/g,'');  //remove white spaces before splitting
>      gArrayHdrs = hdrs.split(":");
> +    for (var i = 0; i< gArrayHdrs.length; i++)

nit: space in between i and < (various places in this file where you've already changed lines)

@@ +113,4 @@
>    else
>    {
>      // otherwise, the default action for the dialog is the OK button
> +    if (! gOKButton.disabled)

nit: no space after !

::: mailnews/base/search/content/FilterEditor.js
@@ +321,5 @@
>    // we must check that the original is not the current as that is what
>    // the duplicateFilterNameExists function will have picked up.
>    if ((!gFilter || gFilter.filterName != filterName) && duplicateFilterNameExists(filterName))
>    {
> +    Services.prompt.alert(window,gFilterBundle.getString("cannotHaveDuplicateFilterTitle"),

nit: please put the second parameter on the next line.

@@ +374,5 @@
>    }
>  
>    if (!allValid)
>    {
> +    Services.prompt.alert(window, gFilterBundle.getString("searchTermsInvalidTitle"),

nit: put this second parameter on the next line as well, just to sync it up with the one below.

::: mailnews/base/search/content/searchWidgets.xml
@@ +414,1 @@
>                                gFilterBundle.getString(errorString) :

nit: re-align these to be:

errorString = errorString ?
              gFilterBundle.getString(errorString) :
              customError;
Attachment #590828 - Flags: review?(mbanner) → review+
(Assignee)

Comment 9

6 years ago
(In reply to Mark Banner (:standard8) from comment #8)
> Comment on attachment 590828 [details] [diff] [review]
> patch
> 
> Review of attachment 590828 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Generally I don't mind a little bit of whitespace cleanup, especially at end
> of lines (which my emacs does anyway). Some of this was bordering on a bit
> much for one patch, but I think its fine.
> 
Thanks! I have an editor that highlights trailing whitespace so it annoyed me if I could not clean it up :) Ok, I will try to keep myself to only cleanup spaces in and around the lines I would change anyway (the meat of a bug). I tried to operate like that so far.
(Assignee)

Comment 10

6 years ago
(In reply to Mark Banner (:standard8) from comment #8)

> ::: mailnews/addrbook/content/abMailListDialog.js
> @@ +70,5 @@
> >  
> >    if (addressbook.mailListNameExists(listname))
> >    {
> > +    Services.prompt.alert(window,
> > +                    gAddressBookBundle.getString("mailListNameExistsTitle"),
> 
> nit: the following lines should be 2-space indented, or aligned with the
> first letter after the opening bracket. Giving the length of the parameters
> in this case, I'd go with 2-space as they were.

Even with my increased indent, the lines are not longer then other ones in that file and are not longer than 80chars.
(Assignee)

Comment 11

6 years ago
Created attachment 592187 [details] [diff] [review]
patch v2
Attachment #590828 - Attachment is obsolete: true
Attachment #592187 - Flags: review?(mbanner)
(In reply to :aceman from comment #10)
> (In reply to Mark Banner (:standard8) from comment #8)
> 
> > ::: mailnews/addrbook/content/abMailListDialog.js
> > @@ +70,5 @@
> > >  
> > >    if (addressbook.mailListNameExists(listname))
> > >    {
> > > +    Services.prompt.alert(window,
> > > +                    gAddressBookBundle.getString("mailListNameExistsTitle"),
> > 
> > nit: the following lines should be 2-space indented, or aligned with the
> > first letter after the opening bracket. Giving the length of the parameters
> > in this case, I'd go with 2-space as they were.
> 
> Even with my increased indent, the lines are not longer then other ones in
> that file and are not longer than 80chars.

Yes, but your alignment is wrong, it should be either aligned with the bracket:

Services.prompt.alert(window,
                      gAddressBookBundle.getString("mailListNameExistsTitle"),

or 2-space indented:

Services.prompt.alert(window,
  gAddressBookBundle.getString("mailListNameExistsTitle"),

at the moment you're in-between.
(Assignee)

Comment 13

6 years ago
Created attachment 592728 [details] [diff] [review]
patch v3

OK, I understand now. So I put it back as it was before.
Attachment #592187 - Attachment is obsolete: true
Attachment #592187 - Flags: review?(mbanner)
Attachment #592728 - Flags: review?(mbanner)
Comment on attachment 592728 [details] [diff] [review]
patch v3

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

Looks good, thanks.
Attachment #592728 - Flags: review?(mbanner) → review+
(Assignee)

Comment 15

6 years ago
Created attachment 598949 [details] [diff] [review]
patch v4

Great, I already forgot about this bug and have done addressbook again in bug 722187. So the reviewed patch would clash :(((

I dropped the changes to /mailnews/addrbook/*, otherwise unchanged.
Attachment #592728 - Attachment is obsolete: true
Attachment #598949 - Flags: review?(mbanner)
Attachment #598949 - Flags: review?(mbanner) → review+
(Reporter)

Updated

6 years ago
Depends on: 722187
Whiteboard: [good first bug]
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Comment 16

6 years ago
> Keywords: checkin-needed
I think it's time you apply for commit privileges to Mercurial. This way you don't have to wait for someone to check in your patches for you.
(Assignee)

Comment 17

6 years ago
How would you then check I have the proper reviews?
Checked in: http://hg.mozilla.org/comm-central/rev/08ff06f0d7a8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
(In reply to :aceman from comment #17)
> How would you then check I have the proper reviews?

Well it's not because you have commit privileges that you can commit without reviews :-)
(Assignee)

Comment 20

6 years ago
Of course;) I just ask if it isn't good to have a third party to check proper reviews in the bug and then check-in.
So that something isn't checked in by mistake.
(Reporter)

Comment 21

6 years ago
V.Fixed, per MXR. (except the one from comment 15)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.