1478564 - Optimize TextEditRules::HandleNewLines()

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

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

TextEditRules::HandleNewLines() appears in the profile of attachment 8848015 [details].

It try to do anything even if there is no linebreaker in the given string. So, it should do nothing if there is no linebreaker.

Additionally, it supports \r, however, \r is already removed if it comes from nsTextEditorState::SetValue() and there is only one other caller of the method. So, making it handles \r before calling HandleNewLines(), we can make HandleNewLines() only \n.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f175396421c4f296ed2b5b9af0e69cb75a06e65f

Comment 2

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

Attachment: Bug 1478564 - part 0: Add automated tests for TextEditRules::HandleNewLines()

Review commit: https://reviewboard.mozilla.org/r/259568/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259568/

Comment 3

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

Attachment: Bug 1478564 - part 1: Optimize TextEditRules::HandleNewLines()

TextEditRules::HandleNewLines() is expensive since it may scan all of given
string twice and more.  On the other hand, in most cases, given string does
not contain \n, \r nor \r\n.

First, for avoid using nsTString::FindCharInSet(), HandleNewLine() should
receive string which never contains \r nor \r\n.  Then, it always can use
nsTSubstring::FindChar() instead.

Next, HandleNewLines() should do nothing if given string does not contain \n.

Finally, because of unused, this removes unnecessary HandleNewLines() argument
which can specify the way to handle new lines.

Review commit: https://reviewboard.mozilla.org/r/259860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259860/

Comment 4

[Makoto Kato [:m_kato]]

Comment on attachment 8995405 [details]
Bug 1478564 - part 1: Optimize TextEditRules::HandleNewLines()

https://reviewboard.mozilla.org/r/259860/#review266906

::: editor/libeditor/TextEditRules.cpp:618
(Diff revision 1)
> -  switch(aNewlineHandling) {
> +  switch(TextEditorRef().mNewlineHandling) {
>      case nsIPlaintextEditor::eNewlinesReplaceWithSpaces:
> +      // Default of Firefox:
>        // Strip trailing newlines first so we don't wind up with trailing spaces
> -      aString.Trim(CRLF, false, true);
> -      aString.ReplaceChar(CRLF, ' ');
> +      aString.Trim(LFSTR, false, true);
> +      aString.ReplaceChar(LFSTR, ' ');

RepalceChar(kLF, ' ') is faster than ReplaceChar(const char* aSet, ...) according to implemation of both methods. https://searchfox.org/mozilla-central/source/xpcom/string/nsTStringObsolete.cpp#211

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 8995404 [details]
Bug 1478564 - part 0: Add automated tests for TextEditRules::HandleNewLines()

https://reviewboard.mozilla.org/r/259568/#review266910

Comment 6

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

Comment on attachment 8995405 [details]
Bug 1478564 - part 1: Optimize TextEditRules::HandleNewLines()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259860/diff/1-2/

Comment 7

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

Comment on attachment 8995405 [details]
Bug 1478564 - part 1: Optimize TextEditRules::HandleNewLines()

https://reviewboard.mozilla.org/r/259860/#review266906

> RepalceChar(kLF, ' ') is faster than ReplaceChar(const char* aSet, ...) according to implemation of both methods. https://searchfox.org/mozilla-central/source/xpcom/string/nsTStringObsolete.cpp#211

Good catch!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16b8136f47f1
part 0: Add automated tests for TextEditRules::HandleNewLines() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/16001f32b7f9
part 1: Optimize TextEditRules::HandleNewLines() r=m_kato

Comment 9

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/16b8136f47f1
https://hg.mozilla.org/mozilla-central/rev/16001f32b7f9