Last Comment Bug 714604 - Use Services.prompt instead of gPromptService in MailNews Core
: Use Services.prompt instead of gPromptService in MailNews Core
Status: VERIFIED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 13.0
Assigned To: :aceman
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 722187
Blocks: 720356
  Show dependency treegraph
 
Reported: 2012-01-02 04:00 PST by Serge Gautherie (:sgautherie)
Modified: 2012-02-23 01:03 PST (History)
5 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (17.22 KB, patch)
2012-01-23 12:42 PST, :aceman
standard8: review+
Details | Diff | Splinter Review
patch v2 (17.73 KB, patch)
2012-01-27 11:15 PST, :aceman
no flags Details | Diff | Splinter Review
patch v3 (17.51 KB, patch)
2012-01-30 08:25 PST, :aceman
standard8: review+
Details | Diff | Splinter Review
patch v4 (11.36 KB, patch)
2012-02-20 13:25 PST, :aceman
standard8: review+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2012-01-02 04:00:43 PST
"Found 6 matching lines in 3 files"
Comment 1 :aceman 2012-01-23 07:09:45 PST
Can I take this as I am already going to convert many other services?
Comment 2 Mark Banner (:standard8) 2012-01-23 07:12:36 PST
Yep, doesn't look like anyone else has it at the moment.
Comment 3 :aceman 2012-01-23 12:42:51 PST
Created attachment 590828 [details] [diff] [review]
patch
Comment 4 Philip Chee 2012-01-25 11:31:41 PST
May I suggest that you do whitespace cleanup in a separate bug?
Comment 5 :aceman 2012-01-25 11:46:15 PST
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 2012-01-26 11:10:08 PST
It's just a suggestion. Please feel free to ignore me.
Comment 7 :aceman 2012-01-26 12:16:59 PST
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) 2012-01-27 03:59:16 PST
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 2012-01-27 04:07:17 PST
(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 2012-01-27 11:05:39 PST
(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 2012-01-27 11:15:46 PST
Created attachment 592187 [details] [diff] [review]
patch v2
Comment 12 Mark Banner (:standard8) 2012-01-30 07:19:50 PST
(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 2012-01-30 08:25:36 PST
Created attachment 592728 [details] [diff] [review]
patch v3

OK, I understand now. So I put it back as it was before.
Comment 14 Mark Banner (:standard8) 2012-02-20 13:06:12 PST
Comment on attachment 592728 [details] [diff] [review]
patch v3

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

Looks good, thanks.
Comment 15 :aceman 2012-02-20 13:25:00 PST
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.
Comment 16 Philip Chee 2012-02-20 19:45:20 PST
> 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 2012-02-21 00:30:30 PST
How would you then check I have the proper reviews?
Comment 18 Mark Banner (:standard8) 2012-02-21 05:33:04 PST
Checked in: http://hg.mozilla.org/comm-central/rev/08ff06f0d7a8
Comment 19 Ludovic Hirlimann [:Usul] 2012-02-21 06:20:30 PST
(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 2012-02-21 06:26:46 PST
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) 2012-02-23 01:03:04 PST
V.Fixed, per MXR. (except the one from comment 15)

Note You need to log in before you can comment on or make changes to this bug.