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

RESOLVED FIXED in Thunderbird 28.0

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: alta88, Assigned: alta88)

Tracking

unspecified
Thunderbird 28.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

24.45 KB, patch
alta88
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 821146 [details] [diff] [review]
feedFolderName.patch
Assignee: nobody → alta88
Attachment #821146 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

5 years ago
Blocks: 258957
(Assignee)

Updated

5 years ago
Blocks: 274139
(Assignee)

Comment 2

5 years ago
requires bug 476641 for non bitrotteness.
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
Attachment #821146 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 4

5 years ago
Created attachment 823574 [details] [diff] [review]
feedFolderName.patch


address all comments; using __foo is probably enough..
Attachment #821146 - Attachment is obsolete: true
Attachment #823574 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/efb6b395ade2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 28.0
You need to log in before you can comment on or make changes to this bug.