Closed Bug 1478564 Opened Last year Closed Last year

Optimize TextEditRules::HandleNewLines()

Categories

(Core :: Editor, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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 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
Attachment #8995405 - Flags: review?(m_kato) → review+
Comment on attachment 8995404 [details]
Bug 1478564 - part 0: Add automated tests for TextEditRules::HandleNewLines()

https://reviewboard.mozilla.org/r/259568/#review266910
Attachment #8995404 - Flags: review?(m_kato) → review+
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!
Pushed by masayuki@d-toybox.com:
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
https://hg.mozilla.org/mozilla-central/rev/16b8136f47f1
https://hg.mozilla.org/mozilla-central/rev/16001f32b7f9
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.