1942028 - folder.getChildNamed() should not throw when child not found

Status: RESOLVED FIXED

(MailNews Core :: Backend, task)

Description

[Magnus Melin [:mkmelin]]

folder.getChildNamed() should not throw when not found. This is poor ergonomics for callers, and most callers are not prepared for it anyway.

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: Bug 1942028 - folder.getChildNamed() should not throw when child not found. r=darktrojan

Comment 2

[Ben Campbell]

(sorry, a bit bikeshed-y here, but it illustrates a couple of interface design issues that I think are worthwhile)

As a general principle, I think exceptions should be used for, well, exceptional things :-)
So it comes down to how the function is intended to be used.

If it's speculative, designed to be used to test if a child exists then yeah, sure, return a null.
And .findChildNamed() would be a better name I think.

If it's not speculative (GetThisChildDammit()!), then returning a null just places extra burden on the caller, and leaves lots of scope for screwing up (i.e. not checking for null!)

Bigger picture:
This method is used almost exclusively by test code, and mostly in the speculative sense. Give this:
a) "find" would be a better name better than "get".
b) The function should probably be removed entirely! If it's only really used by tests, there's an argument it should be moved out of the public interface and into test support code. It's not like it's hard to implement from the public interface. And reducing the surface of public interfaces is a good thing - most of ours have got pretty bloated and unwieldly.

Comment 3

[Magnus Melin [:mkmelin]]

(In reply to Ben Campbell from comment #2)

As a general principle, I think exceptions should be used for, well, exceptional things :-)

Agreed.

Anyway, I'm just fixing the one thing for now (triggered by not having to go on with this pattern for one of Geoff's adapter). It is used for some non-test code as well, and while there may be something that could replace it, I think that's out of scope for now. Plus, the tests do need something to replace it with.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ac75a2c1806e
folder.getChildNamed() should not throw when child not found. r=darktrojan

Comment 5

[John Bieling (:TbSync)]

This change is going to cause regressions, because the lookup name is not localized, but the folder name is:
https://hg.mozilla.org/comm-central/diff/ac75a2c1806ea04fcae17bc76da484956afc4a8a/mail/modules/SmartMailboxUtils.sys.mjs

So it will work in US locale, but not in German for example.
I am working on a fix in the tags support and my patch will revert this:
https://phabricator.services.mozilla.com/D234679

Comment 6

[Magnus Melin [:mkmelin]]

(Not a regression, it's always been like this.)
Re comment 5, I'm not sure where that happens? The input param name was previously used in the URI...

Comment 7

[John Bieling (:TbSync)]

(Not a regression, it's always been like this.)

That is really not true. Name and URI can be totally unrelated, and lookup via name will fail on non-US systems. This will be a major regression once it hits Beta and is used by non english users

Select the unified inbox folder and then execute this on the console:

var inbox = gTabmail.currentAbout3Pane.gFolder;
console.log("URI", inbox.URI)
console.log("Name", inbox.name)
console.log("~~~ getChildWithURI")
console.log(inbox.parent.getChildWithURI(`${inbox.parent.URI}/Inbox`, false, true ));
console.log(inbox.parent.getChildWithURI(`${inbox.parent.URI}/inbox`, false, true ));
console.log("~~~ getChildNamed")
console.log(inbox.parent.getChildNamed("Inbox"));
console.log(inbox.parent.getChildNamed("inbox"));

I get

URI mailbox://nobody@smart%20mailboxes/Inbox
Name Posteingang

~~~ getChildWithURI

XPCWrappedNative_NoHelper { QueryInterface: QueryInterface(), URI: Getter, name: Getter & Setter, prettyName: Getter & Setter, abbreviatedName: Getter, prettyPath: Getter, setPrettyNameFromOriginal: setPrettyNameFromOriginal(), parent: Getter & Setter, messages: Getter, 

XPCWrappedNative_NoHelper { QueryInterface: QueryInterface(), URI: Getter, name: Getter & Setter, prettyName: Getter & Setter, abbreviatedName: Getter, prettyPath: Getter, setPrettyNameFromOriginal: setPrettyNameFromOriginal(), parent: Getter & Setter, messages: Getter, 

~~~ getChildNamed
Uncaught NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgFolder.getChildNamed]
    <anonymous> debugger eval code:8
    getEvalResult resource://devtools/server/actors/webconsole/eval-with-debugger.js:306
    evalWithDebugger resource://devtools/server/actors/webconsole/eval-with-debugger.js:218
    evaluateJS resource://devtools/server/actors/webconsole.js:953
    evaluateJSAsync resource://devtools/server/actors/webconsole.js:846
    makeInfallible resource://devtools/shared/ThreadSafeDevToolsUtils.js:103