Closed Bug 1623210 (unnecessary-nullcheck-in-editor) Opened 4 years ago Closed 6 months 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)

enhancement

Tracking

()

RESOLVED INVALID

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

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?

Flags: needinfo?(masayuki)

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.

Status: NEW → RESOLVED
Closed: 6 months ago
Flags: needinfo?(masayuki)
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.