Closed
Bug 889022
Opened 10 years ago
Closed 10 years ago
try to remove mail/base/content/widgetglue.js
Categories
(Thunderbird :: Mail Window Front End, defect)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 26.0
People
(Reporter: aceman, Assigned: sshagarwal)
References
(Blocks 1 open bug)
Details
(Keywords: addon-compat)
Attachments
(1 file, 2 obsolete files)
58.52 KB,
patch
|
neil
:
review+
mkmelin
:
review+
|
Details | Diff | Splinter Review |
According to comments inside mail/base/content/widgetglue.js the goal is to remove that file. So let's try it here. 1. Replace all calls to GetMsgFolderFromUri() in /mail and /mailnews with MailUtils.getFolderForURI() and include MailUtils if needed (http://mxr.mozilla.org/comm-central/search?string=GetMsgFolderFromUri&find=%2Fmail&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central). 2. Remove that function from the file. 3. Remove includes of widgetglue.js (http://mxr.mozilla.org/comm-central/search?string=widgetglue.js&find=%2Fmail&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central) in /mail. 4. The includes in /mailnews may need more thought. Review if any function called from that file may be in the /suite version of widgetglue.js . In that case the include can't be removed. Depending on the result of step 4: a) if no include of widgetglue.js needed to be preserved, remove mail/base/content/widgetglue.js altogether. b) if some includes were left, this needs decision on how to proceed. Whether we leave an empty file mail/base/content/widgetglue.js or somehow make the include Seamonkey only.
Assignee | ||
Comment 1•10 years ago
|
||
Sir, I have tried to remove the calls and possible includes.
Attachment #770359 -
Flags: feedback?(acelists)
Comment on attachment 770359 [details] [diff] [review] Patch Review of attachment 770359 [details] [diff] [review]: ----------------------------------------------------------------- Looks very good. I tried to run TB and some of the dialogs affected here and I didn't see any problem (and nothing in Error console). Tests do pass for me locally. I would be happy with this but we need confirmation that the /mailnews changes do not break something in Seamonkey. We left SM include its own version of widgetglue.js in /suite files so that should work. But I wonder if some function that is in /suite/.../widgetglue.js is in some other file in TB (that is incidentally included) so till now include of "widgetglue.js + the other file" always worked, but now only include of "the other file" may not be enough. ::: mail/base/content/SearchDialog.js @@ +573,5 @@ > destUri = destFolder.getAttribute('file-uri'); > > + let destMsgFolder = MailUtils.getFolderForURI(destUri) > + .QueryInterface( > + Components.interfaces.nsIMsgFolder); This indenting is strange, please create something according to the rules (either dot under dot or arguments under bracket, or just 2 spaces from the start of line). ::: mail/base/content/mail3PaneWindowCommands.js @@ +926,2 @@ > else > + MsgCopyMessage(MailUtils.getFolderForURI(folderId)); Can you move the MailUtils.getFolderForURI call up into the 'var folderId' line? ::: mail/base/content/messageWindow.js @@ +1097,2 @@ > else > + MsgCopyMessage(MailUtils.getFolderForURI(folderId)); Can you move the MailUtils.getFolderForURI call up into the 'var folderId' line? ::: mail/base/content/msgMail3PaneWindow.js @@ +20,1 @@ > /* This is where functions related to the 3 pane window are kept */ Keep the blank line after imports. ::: mail/base/content/widgetglue.js @@ -21,5 @@ > // folder. Callers who don't want to check those attributes will specify the > // same and then this routine will simply return a msgfolder. This scenario > // applies to a new imap account creation where special folders are created > // on demand and hence needs to prior check of existence. > - You seem to have removed all includes of this file in /mailnews. In that case remove the mail/base/content/widgetglue.js file altogether. Before that, see if anything from this comment can be useful to include into MailUtils.js as the description for the getFolderForURI() function. ::: mailnews/base/prefs/content/amUtils.js @@ +73,1 @@ > !aIsServer).server; Fix the next line indent here. ::: mailnews/base/search/content/searchWidgets.xml @@ +446,5 @@ > { > case "movemessage": > case "copymessage": > let msgFolder = document.getAnonymousNodes(actionTarget)[0].value ? > + MailUtils.getFolderForURI(document.getAnonymousNodes(actionTarget)[0].value) : null; There is still one reference to GetMsgFolderFromUri in in the top of this file, in a comment. I think remove that reference and import MailUtils.js into the validateAction method here, similarly to how iteratorUtils.jsm is imported in another method in this file. Then call as this.MailUtils.getFolderForURI() .
Attachment #770359 -
Flags: feedback?(iann_bugzilla)
Attachment #770359 -
Flags: feedback?(acelists)
Attachment #770359 -
Flags: feedback+
Assignee | ||
Comment 3•10 years ago
|
||
Fixed the nits. Requesting review due to mailnews/ changes.
Attachment #770359 -
Attachment is obsolete: true
Attachment #770359 -
Flags: feedback?(iann_bugzilla)
Attachment #790776 -
Flags: review?(neil)
Attachment #790776 -
Flags: feedback?(acelists)
Comment 4•10 years ago
|
||
(In reply to aceman from comment #2) > I would be happy with this but we need confirmation that the /mailnews > changes do not break something in Seamonkey. We left SM include its own > version of widgetglue.js in /suite files so that should work. But I wonder > if some function that is in /suite/.../widgetglue.js is in some other file > in TB (that is incidentally included) so till now include of "widgetglue.js > + the other file" always worked, but now only include of "the other file" > may not be enough. It looks like the other functions in SeaMonkey's widgetglue.js are all called from the 3-pane window (or possibly the message window) so you won't be accidentally removing the include of widgetglue there.
Comment on attachment 790776 [details] [diff] [review] Patch v2 Review of attachment 790776 [details] [diff] [review]: ----------------------------------------------------------------- Looks almost done to me. Thanks. Please also find a reviewer for the TB part. ::: mail/base/content/SearchDialog.js @@ +573,5 @@ > destUri = destFolder.getAttribute('file-uri'); > > + let destMsgFolder = MailUtils.getFolderForURI(destUri) > + .QueryInterface( > + Components.interfaces.nsIMsgFolder); This indent is still strange. ::: mail/base/content/mail3PaneWindowCommands.js @@ +920,5 @@ > MailOfflineMgr.openOfflineAccountSettings(); > break; > case "cmd_moveToFolderAgain": > + var folder = MailUtils.getFolderForURI( > + Services.prefs.getCharPref("mail.last_msg_movecopy_target_uri")); I think this indent is also wrong. ::: mail/base/content/messageWindow.js @@ +1098,2 @@ > else > + MsgCopyMessage(folder); [Strange that this code is duplicated here (it was in mail3PaneWindowCommands.js too). But not your fault.] ::: mail/base/content/widgetglue.js @@ -20,5 @@ > -// to this routine. Qualifying against those checks would return an existing > -// folder. Callers who don't want to check those attributes will specify the > -// same and then this routine will simply return a msgfolder. This scenario > -// applies to a new imap account creation where special folders are created > -// on demand and hence needs to prior check of existence. So nothing worth moving to MailUtils.jsm?
Attachment #790776 -
Flags: feedback?(acelists) → feedback+
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to :aceman from comment #5) > Comment on attachment 790776 [details] [diff] [review] > Patch v2 > > Review of attachment 790776 [details] [diff] [review]: > > So nothing worth moving to MailUtils.jsm? I din't find anything to move there.
Assignee | ||
Comment 7•10 years ago
|
||
Fixed the nits.
Attachment #790776 -
Attachment is obsolete: true
Attachment #790776 -
Flags: review?(neil)
Attachment #790869 -
Flags: review?(neil)
Attachment #790869 -
Flags: review?(mkmelin+mozilla)
Comment 8•10 years ago
|
||
Comment on attachment 790869 [details] [diff] [review] Patch v2 (revised) Review of attachment 790869 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. r=mkmelin
Attachment #790869 -
Flags: review?(mkmelin+mozilla) → review+
Comment 9•10 years ago
|
||
Comment on attachment 790869 [details] [diff] [review] Patch v2 (revised) I didn't notice any issues with this patch.
Attachment #790869 -
Flags: review?(neil) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 10•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/5db6ab4f7b09
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 26.0
Comment 11•10 years ago
|
||
There is an issue with 2 of my addons, quickFilters and QuickFolders. I have written a workaround to use MailUtils.getFolderForURI and import MailUtils.js directly; I would expect that more addons are using GetMsgFolderFromUri() so it would be good to add this to MDN. The minimal (Addons) patch code (SeaMOnkey + Thunderbird + Postbox) is below. POstbox is a little more involved as it doesn't use MailUtils. if (Components.classes["@mozilla.org/xre/app-info;1"].getService(Components.interfaces.nsIXULAppInfo).ID != "postbox@postbox-inc.com") { Components.utils.import("resource:///modules/MailUtils.js"); } getMsgFolderFromUri: function(uri, checkFolderAttributes) { let msgfolder = null; if (typeof MailUtils != 'undefined') { return MailUtils.getFolderForURI(uri, checkFolderAttributes); } try { // Postbox let resource = GetResourceFromUri(uri); msgfolder = resource.QueryInterface(Components.interfaces.nsIMsgFolder); if (checkFolderAttributes) { if (!(msgfolder && (msgfolder.parent || msgfolder.isServer))) { msgfolder = null; } } } catch (ex) { //dump("failed to get the folder resource\n"); } return msgfolder; }
Keywords: addon-compat
Comment 12•9 years ago
|
||
BTW, this change won't break only addons that call GetMsgFolderFromUri explicitly. It will also break all user scripts created in addons like KeyConfig (http://forums.mozillazine.org/viewtopic.php?t=72994) that call the function.
![]() |
Reporter | |
Comment 13•9 years ago
|
||
That is why we marked this bug "addon-compat" and did it soon in the release cycle. The addons/scripts can use the new function directly and it should also work in older TB versions as it was always there, GetMsgFolderFromUri was just a wrapper.
Comment 14•9 years ago
|
||
The impact of backwards compatibility breaks like this one isn't limited to addon authors who are expected to look for bugs marked addon-compat. They also affect end users who have added user scripts. When measuring whether breaking backwards compatibility in a particular instance is desirable, I hope that you will consider end users who don't monitor bugzilla. My goal isn't to get into a lengthy discussion, so I'll leave it at that.
You need to log in
before you can comment on or make changes to this bug.
Description
•