1503438 - replace some findSubFolder uses by getChildNamed in NNTP and tests

Status: RESOLVED FIXED

(MailNews Core :: Networking: NNTP, enhancement)

Description

[:aceman]

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.

Comment 1

[:aceman]

Attachment: 1503438.patch

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

Comment 2

[Jorg K (CEST = GMT+2)]

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.

Comment 3

[:aceman]

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

Comment 4

[Jorg K (CEST = GMT+2)]

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

Comment 5

[Ben Campbell]

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

Comment 6

[:aceman]

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.

Comment 7

[Jorg K (CEST = GMT+2)]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8b4648e6ba48
replace some findSubFolder() uses by getChildNamed() in NNTP and tests. r=jorgk