Closed Bug 714604 Opened 12 years ago Closed 12 years ago

Use Services.prompt instead of gPromptService in MailNews Core

Categories

(MailNews Core :: Backend, defect)

defect
Not set
minor

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird 13.0

People

(Reporter: sgautherie, Assigned: aceman)

References

()

Details

Attachments

(1 file, 3 obsolete files)

"Found 6 matching lines in 3 files"
Flags: in-testsuite-
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
Attached patch patch (obsolete) — Splinter Review
Attachment #590828 - Flags: review?(mbanner)
Status: NEW → ASSIGNED
May I suggest that you do whitespace cleanup in a separate bug?
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.
It's just a suggestion. Please feel free to ignore me.
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+
(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.
(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.
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attached patch patch v3 (obsolete) — Splinter Review
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+
Attached patch patch v4Splinter Review
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+
Depends on: 722187
Whiteboard: [good first bug]
Keywords: checkin-needed
> 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.
How would you then check I have the proper reviews?
Checked in: http://hg.mozilla.org/comm-central/rev/08ff06f0d7a8
Status: ASSIGNED → RESOLVED
Closed: 12 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 :-)
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.
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.

Attachment

General

Creator:
Created:
Updated:
Size: