930118 - Ensure feed folder autocreation does not throw, sanitize derived folder name

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Comment 1

[alta88]

Attachment: feedFolderName.patch

Comment 2

[alta88]

requires bug 476641 for non bitrotteness.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 821146 [details] [diff] [review]
feedFolderName.patch

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

Looks reasonable, with some comments/nitx, r=mkmelin

::: mailnews/extensions/newsblog/content/Feed.js
@@ +76,5 @@
>    },
>  
>    get name()
>    {
> +    // Used only for the feed's title in Subcribe dialog and opml export.

the "only" is a bit strong. you can't know what it will be used for (outside core or in the future)

@@ +89,2 @@
>  
> +    // Get a unique sanitized name. Use title and/or description as a base;

you use it as or only

@@ +443,5 @@
>  
>      try {
>        this.folder = this.server.rootMsgFolder.
>                                  QueryInterface(Ci.nsIMsgLocalMailFolder).
> +                                createLocalSubfolder(this.folderName);

it's like that from the start, but let's correct it. dots aligned, on new line

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +1221,4 @@
>      if (feed.title != editNameValue)
>      {
> +      if (!editNameValue)
> +        document.getElementById("nameValue").value = feed.title;

braces for all if-else, if one of the clauses need it

@@ +2278,5 @@
>  
>            feed.createFolder();
> +          if (!feed.folder)
> +          {
> +            // Non success.  Remove intermediate traces from the feeds database.

no double spacing please

@@ +2302,5 @@
>            lastFolder = feed.folder;
>          }
>          else
>          {
> +          // A folder outline.  If a folder exists in the account structure at

no double spacing

::: mailnews/extensions/newsblog/content/utils.js
@@ +432,5 @@
> + * @param  string       aProposedName - proposed name.
> + * @param  string       aDefaultName  - default name if proposed sanitizes to
> + *                                      blank, caller ensures sane value.
> + * @param  bool         aUnique       - if true, return a unique indexed name.
> + * @return string                     - sanitized unique name.

usually no dot after explanation i think

@@ +441,5 @@
> +    // 2) Remove nonprintable ascii.
> +    // 3) Remove invalid win chars '* | \ / : < > ? "'.
> +    // 4) Remove all '.' as starting/ending with one is trouble on osx/win.
> +    let folderName = aProposedName.replace(/[\n\r\t]+/g, " ").
> +                                   replace(/[\x00-\x1F]+/g, "").

put the dots on the new line (and line them up)

@@ +446,5 @@
> +                                   replace(/[*|\\\/:<>?"]+/g, "").
> +                                   replace(/[\.]+/g, "");
> +
> +    // Prefix with # if name is:
> +    // 1) a reserved win filename.

I'm not sure # is a good choice. Should we use something else, like ___foo___ ? Think i saw a bug flying by recently regarding # . Probably bc # is special in urls...

@@ +463,5 @@
> +    // Now ensure the folder name is not a dupe; if so append index.
> +    let folderNameBase = folderName;
> +    let i = 2;
> +    while (aParentFolder.containsChildNamed(folderName))
> +      folderName = folderNameBase + "-" + i++;

please add braces

Comment 4

[alta88]

Attachment: feedFolderName.patch

address all comments; using __foo is probably enough..

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/efb6b395ade2