1623210 - (unnecessary-nullcheck-in-editor) [meta] Make all non-nullable arguments of editor methods changed to reference unless it's out param nor XPCOM methods

Status: RESOLVED INVALID

(Core :: DOM: Editor, enhancement, P5)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

if (NS_WARN_IF(!aContent)) {
  return NS_ERROR_INVALID_ARG;
}

Like this check really annoying. And each caller should check such illegal case before trying to do something.

Comment 1

[Masatoshi Kimura [:emk]]

We have several ways to assert non-nullness; OwningNonNull, mozilla::NotNull, MOZ_NONNULL, MOZ_NONNULL_RETURN... (Don't ask me why there are so many ways.)

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Masatoshi Kimura [:emk] from comment #1)

We have several ways to assert non-nullness; OwningNonNull, mozilla::NotNull, MOZ_NONNULL, MOZ_NONNULL_RETURN... (Don't ask me why there are so many ways.)

Although, it's possible of a nullptr-referece, using reference of C++ is reasonable because:

1. Using OwningNonNull as an argument is not a standard manner because of the refcount cost in the hot paths
2. NotNull was designed for multiple compilers and avoiding some complicated special cases. So we don't have any reasons to use it in editor because using it may increase the binary size
3. I don't believe 100% of static analysis due to edge cases, so I don't believe MOZ_NONNULL etc in unrealistic scenarios which should be banned by the review process

Currently, we use clang in any platforms and crashes near null are not treated as non-security issues. So just using C++'s feature must be fine in the editor module.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The meta keyword is there, the bug doesn't depend on other bugs and there is no activity for 12 months.
:masayuki, maybe it's time to close this bug?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, we still need to work on this, but it should be done when touching each method a lot. So, I think that this meta bug does not make sense.