714604 - Use Services.prompt instead of gPromptService in MailNews Core

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Serge Gautherie (:sgautherie)]

"Found 6 matching lines in 3 files"

Comment 1

[:aceman]

Can I take this as I am already going to convert many other services?

Comment 2

[Mark Banner (:standard8)]

Yep, doesn't look like anyone else has it at the moment.

Comment 3

[:aceman]

Attachment: patch

Comment 4

[Philip Chee]

May I suggest that you do whitespace cleanup in a separate bug?

Comment 5

[:aceman]

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

[Philip Chee]

It's just a suggestion. Please feel free to ignore me.

Comment 7

[:aceman]

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 8

[Mark Banner (:standard8)]

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;

Comment 9

[:aceman]

(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.

Comment 10

[:aceman]

(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.

Comment 11

[:aceman]

Attachment: patch v2

Comment 12

[Mark Banner (:standard8)]

(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.

Comment 13

[:aceman]

Attachment: patch v3

OK, I understand now. So I put it back as it was before.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 592728 [details] [diff] [review]
patch v3

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

Looks good, thanks.

Comment 15

[:aceman]

Attachment: 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.

Comment 16

[Philip Chee]

> 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.

Comment 17

[:aceman]

How would you then check I have the proper reviews?

Comment 18

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/08ff06f0d7a8

Comment 19

[Ludovic Hirlimann [:Usul]]

(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 :-)

Comment 20

[:aceman]

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.

Comment 21

[Serge Gautherie (:sgautherie)]

V.Fixed, per MXR. (except the one from comment 15)