889022 - try to remove mail/base/content/widgetglue.js

Status: RESOLVED FIXED

Description

[:aceman]

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.

Comment 1

[Suyash Agarwal (:sshagarwal)]

Created attachment 770359 [details] [diff] [review]
Patch

Sir,
I have tried to remove the calls and possible includes.

Comment 2

[:aceman]

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() .

Comment 3

[Suyash Agarwal (:sshagarwal)]

Created attachment 790776 [details] [diff] [review]
Patch v2

Fixed the nits.
Requesting review due to mailnews/ changes.

Comment 4

[[e-mail]]

(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 5

[:aceman]

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?

Comment 6

[Suyash Agarwal (:sshagarwal)]

(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.

Comment 7

[Suyash Agarwal (:sshagarwal)]

Created attachment 790869 [details] [diff] [review]
Patch v2 (revised)

Fixed the nits.

Comment 8

[Magnus Melin]

Comment on attachment 790869 [details] [diff] [review]
Patch v2 (revised)

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

Looks good to me. r=mkmelin

Comment 9

[[e-mail]]

Comment on attachment 790869 [details] [diff] [review]
Patch v2 (revised)

I didn't notice any issues with this patch.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/5db6ab4f7b09

Comment 11

[Axel Grude [:realRaven]]

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 != "[e-mail]")
{
  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;
  }

Comment 12

[WalterGR]

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.

Comment 13

[:aceman]

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

[WalterGR]

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.