Open Bug 1623210 (unnecessary-nullcheck-in-editor) Opened 5 years ago Updated 2 years ago

[meta] Make all non-nullable arguments of editor methods changed to reference unless it's out param nor XPCOM methods

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
enhancement

Tracking

()

People

(Reporter: masayuki, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: meta) 

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.

Priority: -- → P5

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

Alias: stop-editor-unnecessary-nullcheck
Keywords: meta
Alias: stop-editor-unnecessary-nullcheck → unnecessary-nullcheck-in-editor
Summary: Make all non-nullable arguments of editor methods changed to reference unless it's out param nor XPCOM methods → [meta] Make all non-nullable arguments of editor methods changed to reference unless it's out param nor XPCOM methods

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

Summary: [meta] Make all non-nullable arguments of editor methods changed to reference unless it's out param nor XPCOM methods → Make all non-nullable arguments of editor methods changed to reference unless it's out param nor XPCOM methods
Summary: Make all non-nullable arguments of editor methods changed to reference unless it's out param nor XPCOM methods → [meta] Make all non-nullable arguments of editor methods changed to reference unless it's out param nor XPCOM methods
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.