Closed
Bug 794909
Opened 12 years ago
Closed 11 years ago
Switch to Services.jsm: compose code
Categories
(Thunderbird :: Message Compose Window, defect)
Thunderbird
Message Compose Window
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 20.0
People
(Reporter: aryx, Assigned: aryx)
References
Details
Attachments
(1 file, 4 obsolete files)
55.53 KB,
patch
|
Details | Diff | Splinter Review |
Using Services.jsm will clean up the readability of our code a bit, and should avoid the costs the getService call each time we want a service. https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/Services.jsm
Assignee | ||
Comment 1•12 years ago
|
||
This patch currently causes Mozmill errors, see https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=c04f17949001 I didn't modify anything in mail/test/mozmill/ If I download the build and run mail/test/mozmill/composition/test-save-changes-on-quit.js locally, it shows me: fail :: Component returned failure code: 0x80550013 [nsIMsgFolder.createSubfolder] name: null message: Component returned failure code: 0x80550013 [nsIMsgFolder.createSubfolder] lineNumber: 87 fileName: undefined stack: undefined test: function (module) { let fdh = collector.getModule("folder-display-helpers"); fdh.installInto(module); let composeHelper = collector.getModule("compose-helpers"); composeHelper.installInto(module); let ph = collector.getModule("prompt-helpers"); ph.installInto(module); } if the line is correct, there are two files calling createSubfolder in line 87: http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-tree-modes/test-smart-folders.js#87 (another mozmill test) and http://mxr.mozilla.org/comm-central/source/mailnews/test/resources/messageInjection.js#87 (mailnews)
@@ -2825,67 +2804,56 @@ function CheckValidEmailAddress(to, cc, if (invalidStr) { - if (gPromptService) - gPromptService.alert(window, getComposeBundle().getString("addressInvalidTitle"), - getComposeBundle().getFormattedString("addressInvalid", - [invalidStr], 1)); - return false; + Services.prompt.alert(window, getComposeBundle().getString("addressInvalidTitle"), + getComposeBundle().getFormattedString("addressInvalid", + [invalidStr], 1)); Why is the return removed?
Assignee | ||
Comment 3•12 years ago
|
||
Because of my lack of concentration
Comment 4•12 years ago
|
||
Ok, I figured out what was going on. Services.prompt, by design, caches a copy of the Prompt Service. test-save-changes-on-quit.js is a test that mocks the prompt service so that it doesn't block us, and allows us to read data that would be passed to the prompt. However, switching to using Services.prompt bypasses the mock, because when the test executes, Services.prompt already has the real service cached, and this is what is used. The solution is to overwrite Services.prompt every time the mock prompt service is registered and unregistered, effectively flushing the cache. I've got a patch coming up that will fix the issue.
Comment 5•12 years ago
|
||
Comment 6•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #5) > Created attachment 665483 [details] [diff] [review] > Fix for test-save-changes-on-quit.js tests no review flags ?
Comment 7•12 years ago
|
||
(In reply to Ludovic Hirlimann [:Usul] from comment #6) > (In reply to Mike Conley (:mconley) from comment #5) > > Created attachment 665483 [details] [diff] [review] > > Fix for test-save-changes-on-quit.js tests > > no review flags ? I figured Archaeopteryx was still interested in driving this, and might fold my patch into his, and r? when we was ready.
Assignee | ||
Comment 8•12 years ago
|
||
Thank you, I folded the patches and the new patch passed on Thunderbird-Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=88d6f34dd1cc
Attachment #665429 -
Attachment is obsolete: true
Attachment #665483 -
Attachment is obsolete: true
Attachment #667508 -
Flags: review?(mconley)
Comment 9•12 years ago
|
||
Comment on attachment 667508 [details] [diff] [review] Siwth to Services.jsm: Compose code, patch, v3 Review of attachment 667508 [details] [diff] [review]: ----------------------------------------------------------------- Just a few issues here, but nothing major. ::: mail/components/compose/content/MsgComposeCommands.js @@ +120,5 @@ > defaultSaveOperation = "draft"; > gSendOrSaveOperationInProgress = false; > gAutoSaving = false; > gCloseWindowAfterSave = false; > + gIsOffline = Services.io.offline; Do we even need this global anymore? If not, just replace it entirely. @@ +1558,5 @@ > if (LDAPSession) { > let url = getPref(autocompleteDirectory + ".uri", true); > > + LDAPSession.serverURL = Services.io.newURI(url, null, null) > + .QueryInterface(Components.interfaces.nsILDAPURL); Please line up the . in .QueryInterface vertically with the . in .io @@ +1881,5 @@ > // see if the string is a mailto url....do this by checking the first 7 characters of the string > if (/^mailto:/i.test(mailtoUrl)) > { > // if it is a mailto url, turn the mailto url into a MsgComposeParams object.... > + var uri = Services.io.newURI(mailtoUrl, null, null); While you're here, replace the var with let @@ +2651,5 @@ > // and the message (still) contains attachment keywords. > if ((gRemindLater || (getPref("mail.compose.attachment_reminder_aggressive") && > document.getElementById("attachmentNotificationBox").currentNotification)) && > ShouldShowAttachmentNotification(false)) { > + var flags = Services.prompt.BUTTON_POS_0 * Services.prompt.BUTTON_TITLE_IS_STRING + While you're here, please replace the vars with lets @@ +3335,5 @@ > if (gSendOrSaveOperationInProgress) > { > var result; > > + var brandBundle = document.getElementById("brandBundle"); While you're here, please replace these vars with lets. @@ +3362,5 @@ > { > // call window.focus, since we need to pop up a dialog > // and therefore need to be visible (to prevent user confusion) > window.focus(); > + result = Services.prompt.confirmEx(window, Line up .confirmEx on the next line like this: result = Services.prompt .confirmEx(window, @@ +3472,5 @@ > { > var lastDirectory; > > try { > + lastDirectory = Services.prefs.getComplexValue(kComposeAttachDirPrefName, Components.interfaces.nsILocalFile); > 80 chars - let's break this up over two lines. @@ +3490,5 @@ > try { > var file = attachedLocalFile.QueryInterface(Components.interfaces.nsIFile); > var parent = file.parent.QueryInterface(Components.interfaces.nsILocalFile); > > + Services.prefs.setComplexValue(kComposeAttachDirPrefName, Components.interfaces.nsILocalFile, parent); This is probably > 80 chars too. @@ +3665,5 @@ > } > > function AttachPage() > { > + var result = {value:"http://"}; Replace the vars with lets @@ +3666,5 @@ > > function AttachPage() > { > + var result = {value:"http://"}; > + if (Services.prompt.prompt(window, Please align like: Services.prompt .prompt(window @@ +3800,5 @@ > return; // not one attachment selected > > var item = bucket.getSelectedItem(0); > var attachmentName = {value: item.attachment.name}; > + if (Services.prompt.prompt( Services.prompt .prompt @@ +3849,5 @@ > else > { > // turn the url into a nsIURL object then open it > > + var url = Services.io.newURI(attachmentUrl, null, null); let not var @@ +3854,5 @@ > url = url.QueryInterface( Components.interfaces.nsIURL ); > > if (url) > { > + var channel = Services.io.newChannelFromURI(url); let not var @@ +4247,5 @@ > // > // also skip mailto:, since it doesn't make sense > // to attach and send mailto urls > try { > + var scheme = Services.io.extractScheme(rawData); let not var @@ +4342,5 @@ > [msgfolder.name, > msgfolder.server.prettyName]); > > var CheckMsg = bundle.getString("CheckMsg"); > + Services.prompt.alertCheck(window, SaveDlgTitle, dlgMsg, Services.prompt .alertCheck(window @@ +4695,2 @@ > if (aIsComplex) { > return prefB.getComplexValue(aPrefName, Ci.nsISupportsString).data; Need to take care of this prefB too. ::: mailnews/compose/content/mailComposeEditorOverlay.xul @@ +87,5 @@ > //is it a Windows remote file? > if (/^\s*file:\/\/\/\/\//i.test(gMsgCompInputElement.value)) > { > try { > + if (Services.prefs.getBoolPref("mail.compose.dont_attach_source_of_local_network_links")) Let's break this up like: Services.prefs .getBoolPref("mail.compose.dont_attach_source_of_local_network_links") Even better - assign the pref key to a const, and use that instead. ::: mailnews/compose/test/unit/test_smtpPassword2.js @@ +33,5 @@ > var i; > var count = {}; > > // Test - Check there are two logins to begin with. > + var logins = Services.logins.findLogins(count, kServerUrl, null, kServerUrl); let not var ::: mailnews/compose/test/unit/test_smtpPasswordFailure1.js @@ +113,2 @@ > let count = {}; > + let logins = Services.logins.findLogins(count, "smtp://localhost", null, Services.logins .findLogins(count ::: mailnews/compose/test/unit/test_smtpPasswordFailure2.js @@ +138,2 @@ > let count = {}; > + let logins = Services.logins.findLogins(count, "smtp://localhost", null, "smtp://localhost"); Services.logins .findLogins ::: mailnews/compose/test/unit/test_smtpPasswordFailure3.js @@ +122,2 @@ > let count = {}; > + let logins = Services.logins.findLogins(count, "smtp://localhost", null, Services.logins .findLogins
Attachment #667508 -
Flags: review?(mconley) → review-
Comment 10•12 years ago
|
||
May have been bitrotted in MsgComposeCommands.js by bug 271730.
Comment 11•11 years ago
|
||
:Aryx, can you finish this?
Assignee: nobody → archaeopteryx
Status: NEW → ASSIGNED
Flags: needinfo?(archaeopteryx)
Assignee | ||
Comment 12•11 years ago
|
||
Comments have been addressed. Try run: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=c05548089b51
Attachment #667508 -
Attachment is obsolete: true
Attachment #690325 -
Flags: review?(mconley)
Flags: needinfo?(archaeopteryx)
Comment 13•11 years ago
|
||
Comment on attachment 690325 [details] [diff] [review] Switch to Services.jsm: Compose code, patch, v4 Review of attachment 690325 [details] [diff] [review]: ----------------------------------------------------------------- Just some style nits, but this looks good. Thanks! ::: mail/components/compose/content/MsgComposeCommands.js @@ +1610,5 @@ > // > if (LDAPSession) { > let url = getPref(autocompleteDirectory + ".uri", true); > > + LDAPSession.serverURL = Services.io.newURI(url, null, null) Nit - I'd prefer you break it up like this: LDAPSession.serverURL = Services.io .newURI(url, null, null) .QueryInterface(Components.interfaces.nsILDAPURL); @@ +2744,5 @@ > var dontAskAgain = getPref(kDontAskAgainPref); > if (!dontAskAgain) > { > var checkbox = {value:false}; > + var okToProceed = Services.prompt.confirmCheck( Switch var to let @@ +2938,5 @@ > var warn = getPref("mail.warn_on_send_accel_key"); > > if (warn) { > var checkValue = {value:false}; > + var buttonPressed = Services.prompt.confirmEx(window, Switch var to let @@ +3382,5 @@ > + let brandBundle = document.getElementById("brandBundle"); > + let brandShortName = brandBundle.getString("brandShortName"); > + let promptTitle = getComposeBundle().getString("quitComposeWindowTitle"); > + let promptMsg = getComposeBundle().getFormattedString("quitComposeWindowMessage2", > + [brandShortName], 1); Alignment on lines 3385-3386 looks wrong. I think it should look like: let promptMsg = getComposeBundle().getFormattedString("quitComposeWindowMessage2", [brandShortName], 1); @@ +3534,5 @@ > function SetLastAttachDirectory(attachedLocalFile) > { > try { > var file = attachedLocalFile.QueryInterface(Components.interfaces.nsIFile); > + var parent = file.parent.QueryInterface(Components.interfaces.nsIFile); Switch var to let @@ +3655,5 @@ > // them. This fixes issues where an icon wasn't showing up if you dragged a > // web url that had a query or reference string after the file name and for > // mailnews urls where the filename is hidden in the url as a &filename= > // part. > + var url = Services.io.newURI(attachment.url, null, null); Switch var to let
Attachment #690325 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 14•11 years ago
|
||
Successful run on try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=24ccf058a3d1 - the oranges are the same like for other try runs.
Attachment #690325 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 15•11 years ago
|
||
https://hg.mozilla.org/comm-central/rev/99481e63615e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
You need to log in
before you can comment on or make changes to this bug.
Description
•