Closed
Bug 714604
Opened 13 years ago
Closed 12 years ago
Use Services.prompt instead of gPromptService in MailNews Core
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
VERIFIED
FIXED
Thunderbird 13.0
People
(Reporter: sgautherie, Assigned: aceman)
References
()
Details
Attachments
(1 file, 3 obsolete files)
11.36 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
"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
Comment 2•12 years ago
|
||
Yep, doesn't look like anyone else has it at the moment.
Assignee: nobody → acelists
Attachment #590828 -
Flags: review?(mbanner)
Comment 4•12 years ago
|
||
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.
Comment 6•12 years ago
|
||
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 8•12 years ago
|
||
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.
Assignee | ||
Comment 10•12 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•12 years ago
|
||
Attachment #590828 -
Attachment is obsolete: true
Attachment #592187 -
Flags: review?(mbanner)
Comment 12•12 years ago
|
||
(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•12 years ago
|
||
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 14•12 years ago
|
||
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•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #598949 -
Flags: review?(mbanner) → review+
Keywords: checkin-needed
Comment 16•12 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•12 years ago
|
||
How would you then check I have the proper reviews?
Comment 18•12 years ago
|
||
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
Comment 19•12 years ago
|
||
(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•12 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•12 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.
Description
•