replace some findSubFolder uses by getChildNamed in NNTP and tests

RESOLVED FIXED in Thunderbird 65.0

Status

enhancement
--
minor
RESOLVED FIXED
9 months ago
6 months ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 65.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

nsIMsgFolder::findSubFolder() creates the folder if it doesn't exist yet.
We do not need this semantics at some places so those can use nsIMsgFolder::getChildNamed().

This should also help bug 453908 if we want to kill findSubFolder() once.
Posted patch 1503438.patchSplinter Review
I looked at what the tests do and also I only convert places where after findSubFolder() we still check if a folder was returned and thus do not expect on it being automatically created (which findSubFolder does based on the comment before it).

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=208744682&revision=3b09be7b0466be6fbd49452c9b02d6574cf0d0a2
Attachment #9021375 - Flags: review?(jorgk)
Comment on attachment 9021375 [details] [diff] [review]
1503438.patch

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

::: mailnews/news/src/nsNntpIncomingServer.cpp
@@ +1169,5 @@
>    if (!serverFolder)
>      return NS_ERROR_FAILURE;
>  
> +  nsCOMPtr<nsIMsgFolder> newsgroupFolder;
> +  rv = serverFolder->GetChildNamed(nsDependentString(aUnicharName),

Have you tested that? So one API takes the unicode name, the other the escaped version.

Looks like FindSubFolder() in Ben's new world will call GetExistingFolder() and that still goes via the escaped URL. Killing FindSubFolder() doesn't appear realistic since some call sites only have the URL.
Attachment #9021375 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+2) from comment #2)
> > +  nsCOMPtr<nsIMsgFolder> newsgroupFolder;
> > +  rv = serverFolder->GetChildNamed(nsDependentString(aUnicharName),
> 
> Have you tested that? So one API takes the unicode name, the other the
> escaped version.

I tested Unsubscribe on some servers only using plain 7bit ASCII chars. Do we have a sample server with unicode accented newgsroup names?
 
> Looks like FindSubFolder() in Ben's new world will call GetExistingFolder()
> and that still goes via the escaped URL.

And that is suspicious. FindSubFolder() is described in the .idl as creating the folder so it is equivalent to GetOrCreateFolder(). If he changes it, he must convert all callers.

> Killing FindSubFolder() doesn't
> appear realistic since some call sites only have the URL.
GetExistingFolder() also takes the URL. The difference is in the escaping and whether it should create the folder or not.
(In reply to :aceman from comment #3)
> Do we have a sample server with unicode accented newgsroup names?
Not accented but unicode Asian, news.newsfan.net, see bug 1389762 comment #0.
(In reply to :aceman from comment #3)
> (In reply to Jorg K (GMT+2) from comment #2)
> > Looks like FindSubFolder() in Ben's new world will call GetExistingFolder()
> > and that still goes via the escaped URL.
> 
> And that is suspicious. FindSubFolder() is described in the .idl as creating
> the folder so it is equivalent to GetOrCreateFolder(). If he changes it, he
> must convert all callers.

Sorry about that - I'm just finalising a revision to that patch and have changed it back to GetOrCreateFolder().
(it was a leftover from the more gung-ho total-rdf-removal stuff where I got rid of implicit folder creation completely)
Thanks, yes unsubscribing groups with Chinese characters works on that server.
I have specifically dumped that rv = serverFolder->GetChildNamed() on those groups returns NS_OK.
Attachment #9021375 - Flags: feedback?(Pidgeot18)
(In reply to Jorg K (GMT+2) from comment #4)
> Not accented but unicode Asian, news.newsfan.net, see bug 1389762 comment #0.
For the record, to see the Chinese groups on the server we need to set "Chinese Simplified (GBK)" as the default text encoding for that server. Internally we use UTF-8 in nsCStrings of course.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8b4648e6ba48
replace some findSubFolder() uses by getChildNamed() in NNTP and tests. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
Attachment #9021375 - Flags: feedback?(Pidgeot18)
You need to log in before you can comment on or make changes to this bug.