From 3f7057ece8533965ffcfedb47c06084a47611706 Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Mon, 25 Nov 2019 06:35:17 +0000 Subject: [PATCH] Bug 1597679 - part 3: Create methods to compare given string with values of `TextControlState`, `nsTextControlFrame`, `HTMLInputElement` and `HTMLTextAreaElement` r=smaug For avoiding unnecessary copy of string buffer only for comparing setting value and current value, especially with `nsAutoString`, this patch creates `*Equals()` methods for every class. And also this avoids to call `nsContentUtils::PlatformToDOMLineBreaks()` in most paths. Differential Revision: https://phabricator.services.mozilla.com/D54331 --HG-- extra : moz-landing-system : lando --- dom/html/HTMLInputElement.cpp | 5 +++ dom/html/HTMLInputElement.h | 7 ++++ dom/html/HTMLTextAreaElement.cpp | 14 +++---- dom/html/HTMLTextAreaElement.h | 6 +++ dom/html/TextControlState.cpp | 62 ++++++++++++++++++----------- dom/html/TextControlState.h | 19 +++++++++ layout/forms/nsTextControlFrame.cpp | 45 ++++++++++++++++----- layout/forms/nsTextControlFrame.h | 7 ++++ 8 files changed, 125 insertions(+), 40 deletions(-) diff --git a/dom/html/HTMLInputElement.cpp b/dom/html/HTMLInputElement.cpp index edaa265a39fe..c0483590b22a 100644 --- a/dom/html/HTMLInputElement.cpp +++ b/dom/html/HTMLInputElement.cpp @@ -6756,6 +6756,11 @@ void HTMLInputElement::GetTextEditorValue(nsAString& aValue, } } +bool HTMLInputElement::TextEditorValueEquals(const nsAString& aValue) const { + TextControlState* state = GetEditorState(); + return state ? state->ValueEquals(aValue) : aValue.IsEmpty(); +} + void HTMLInputElement::InitializeKeyboardEventListeners() { TextControlState* state = GetEditorState(); if (state) { diff --git a/dom/html/HTMLInputElement.h b/dom/html/HTMLInputElement.h index d1cf3a41ee6b..fb39b7c27c26 100644 --- a/dom/html/HTMLInputElement.h +++ b/dom/html/HTMLInputElement.h @@ -250,6 +250,13 @@ class HTMLInputElement final : public TextControlElement, const nsAString& aValue) override; virtual bool HasCachedSelection() override; + /** + * TextEditorValueEquals() is designed for internal use so that aValue + * shouldn't include \r character. It should be handled before calling this + * with nsContentUtils::PlatformToDOMLineBreaks(). + */ + bool TextEditorValueEquals(const nsAString& aValue) const; + // Methods for nsFormFillController so it can do selection operations on input // types the HTML spec doesn't support them on, like "email". uint32_t GetSelectionStartIgnoringType(ErrorResult& aRv); diff --git a/dom/html/HTMLTextAreaElement.cpp b/dom/html/HTMLTextAreaElement.cpp index 7571c36ca97c..dce90e1255d9 100644 --- a/dom/html/HTMLTextAreaElement.cpp +++ b/dom/html/HTMLTextAreaElement.cpp @@ -201,13 +201,8 @@ void HTMLTextAreaElement::GetType(nsAString& aType) { } void HTMLTextAreaElement::GetValue(nsAString& aValue) { - nsAutoString value; - GetValueInternal(value, true); - - // Normalize CRLF and CR to LF - nsContentUtils::PlatformToDOMLineBreaks(value); - - aValue = value; + GetValueInternal(aValue, true); + MOZ_ASSERT(aValue.FindChar(static_cast('\r')) == -1); } void HTMLTextAreaElement::GetValueInternal(nsAString& aValue, @@ -216,6 +211,11 @@ void HTMLTextAreaElement::GetValueInternal(nsAString& aValue, mState->GetValue(aValue, aIgnoreWrap); } +bool HTMLTextAreaElement::ValueEquals(const nsAString& aValue) const { + MOZ_ASSERT(mState); + return mState->ValueEquals(aValue); +} + TextEditor* HTMLTextAreaElement::GetTextEditor() { MOZ_ASSERT(mState); return mState->GetTextEditor(); diff --git a/dom/html/HTMLTextAreaElement.h b/dom/html/HTMLTextAreaElement.h index 4bae95caa9d3..c2ae620beba9 100644 --- a/dom/html/HTMLTextAreaElement.h +++ b/dom/html/HTMLTextAreaElement.h @@ -237,6 +237,12 @@ class HTMLTextAreaElement final : public TextControlElement, void GetDefaultValue(nsAString& aDefaultValue, ErrorResult& aError); void SetDefaultValue(const nsAString& aDefaultValue, ErrorResult& aError); void GetValue(nsAString& aValue); + /** + * ValueEquals() is designed for internal use so that aValue shouldn't + * include \r character. It should be handled before calling this with + * nsContentUtils::PlatformToDOMLineBreaks(). + */ + bool ValueEquals(const nsAString& aValue) const; MOZ_CAN_RUN_SCRIPT_BOUNDARY void SetValue(const nsAString& aValue, ErrorResult& aError); diff --git a/dom/html/TextControlState.cpp b/dom/html/TextControlState.cpp index af6abb2a7253..7bf9f9ab1a97 100644 --- a/dom/html/TextControlState.cpp +++ b/dom/html/TextControlState.cpp @@ -2491,6 +2491,7 @@ void TextControlState::GetValue(nsAString& aValue, bool aIgnoreWrap) const { if (mHandlingState && mHandlingState->IsHandling(TextControlAction::CommitComposition)) { aValue = mHandlingState->GetSettingValue(); + MOZ_ASSERT(aValue.FindChar(static_cast('\r')) == -1); return; } @@ -2498,6 +2499,7 @@ void TextControlState::GetValue(nsAString& aValue, bool aIgnoreWrap) const { (mEditorInitialized || !IsSingleLineTextControl())) { if (aIgnoreWrap && !mBoundFrame->CachedValue().IsVoid()) { aValue = mBoundFrame->CachedValue(); + MOZ_ASSERT(aValue.FindChar(static_cast('\r')) == -1); return; } @@ -2532,6 +2534,7 @@ void TextControlState::GetValue(nsAString& aValue, bool aIgnoreWrap) const { AutoNoJSAPI nojsapi; DebugOnly rv = mTextEditor->ComputeTextValue(flags, aValue); + MOZ_ASSERT(aValue.FindChar(static_cast('\r')) == -1); NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Failed to get value"); } // Only when the result doesn't include line breaks caused by hard-wrap, @@ -2543,13 +2546,27 @@ void TextControlState::GetValue(nsAString& aValue, bool aIgnoreWrap) const { } } else { if (!mTextCtrlElement->ValueChanged() || !mValue) { - mTextCtrlElement->GetDefaultValueFromContent(aValue); + // Use nsString to avoid copying string buffer at setting aValue. + nsString value; + mTextCtrlElement->GetDefaultValueFromContent(value); + // TODO: We should make default value not include \r. + nsContentUtils::PlatformToDOMLineBreaks(value); + aValue = value; } else { aValue = *mValue; + MOZ_ASSERT(aValue.FindChar(static_cast('\r')) == -1); } } } +bool TextControlState::ValueEquals(const nsAString& aValue) const { + // We can avoid copying string buffer in many cases. Therefore, we should + // use nsString rather than nsAutoString here. + nsString value; + GetValue(value, true); + return aValue.Equals(value); +} + #ifdef DEBUG // @param aFlags TextControlState::SetValueFlags bool AreFlagsNotDemandingContradictingMovements(uint32_t aFlags) { @@ -2596,17 +2613,12 @@ bool TextControlState::SetValue(const nsAString& aValue, } else { // If setting value won't change current value, we shouldn't commit // composition for compatibility with the other browsers. - nsAutoString currentValue; - if (aOldValue) { -#ifdef DEBUG - mBoundFrame->GetText(currentValue); - MOZ_ASSERT(currentValue.Equals(*aOldValue)); -#endif - currentValue.Assign(*aOldValue); - } else { - mBoundFrame->GetText(currentValue); - } - if (handlingSetValue.GetSettingValue() == currentValue) { + MOZ_ASSERT(!aOldValue || mBoundFrame->TextEquals(*aOldValue)); + bool isSameAsCurrentValue = + aOldValue + ? aOldValue->Equals(handlingSetValue.GetSettingValue()) + : mBoundFrame->TextEquals(handlingSetValue.GetSettingValue()); + if (isSameAsCurrentValue) { // Note that in this case, we shouldn't fire any events with setting // value because event handlers may try to set value recursively but // we cannot commit composition at that time due to unsafe to run @@ -2691,19 +2703,16 @@ bool TextControlState::SetValueWithTextEditor( } #endif - nsAutoString currentValue; - if (aHandlingSetValue.GetOldValue()) { -#ifdef DEBUG - mBoundFrame->GetText(currentValue); - MOZ_ASSERT(currentValue.Equals(*aHandlingSetValue.GetOldValue())); -#endif - currentValue.Assign(*aHandlingSetValue.GetOldValue()); - } else { - mBoundFrame->GetText(currentValue); - } + MOZ_ASSERT(!aHandlingSetValue.GetOldValue() || + mBoundFrame->TextEquals(*aHandlingSetValue.GetOldValue())); + bool isSameAsCurrentValue = + aHandlingSetValue.GetOldValue() + ? aHandlingSetValue.GetOldValue()->Equals( + aHandlingSetValue.GetSettingValue()) + : mBoundFrame->TextEquals(aHandlingSetValue.GetSettingValue()); // this is necessary to avoid infinite recursion - if (currentValue == aHandlingSetValue.GetSettingValue()) { + if (isSameAsCurrentValue) { return true; } @@ -2763,6 +2772,13 @@ bool TextControlState::SetValueWithTextEditor( // } // However, this path won't be used in web content anymore. nsCOMPtr kungFuDeathGrip = mSelCon.get(); + // Use nsString to avoid copying string buffer in most cases. + nsString currentValue; + if (aHandlingSetValue.GetOldValue()) { + currentValue.Assign(*aHandlingSetValue.GetOldValue()); + } else { + mBoundFrame->GetText(currentValue); + } uint32_t currentLength = currentValue.Length(); uint32_t newlength = aHandlingSetValue.GetSettingValue().Length(); if (!currentLength || diff --git a/dom/html/TextControlState.h b/dom/html/TextControlState.h index b3f1f32a78ce..310be87b9928 100644 --- a/dom/html/TextControlState.h +++ b/dom/html/TextControlState.h @@ -208,6 +208,15 @@ class TextControlState final : public SupportsWeakPtr { // it. eSetValue_MoveCursorToBeginSetSelectionDirectionForward = 1 << 6, }; + /** + * SetValue() sets the value to aValue with replacing \r\n and \r with \n. + * + * @param aValue The new value. Can contain \r. + * @param aOldValue Optional. If you have already know current value, + * set this to it. However, this must not contain \r + * for the performance. + * @param aFlags See SetValueFlags. + */ MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE bool SetValue(const nsAString& aValue, const nsAString* aOldValue, uint32_t aFlags); @@ -215,7 +224,17 @@ class TextControlState final : public SupportsWeakPtr { uint32_t aFlags) { return SetValue(aValue, nullptr, aFlags); } + /** + * GetValue() returns current value either with or without TextEditor. + * The result never includes \r. + */ void GetValue(nsAString& aValue, bool aIgnoreWrap) const; + /** + * ValueEquals() is designed for internal use so that aValue shouldn't + * include \r character. It should be handled before calling this with + * nsContentUtils::PlatformToDOMLineBreaks(). + */ + bool ValueEquals(const nsAString& aValue) const; bool HasNonEmptyValue(); // The following methods are for textarea element to use whether default // value or not. diff --git a/layout/forms/nsTextControlFrame.cpp b/layout/forms/nsTextControlFrame.cpp index 9e1eb0cb8301..b8accb357d8a 100644 --- a/layout/forms/nsTextControlFrame.cpp +++ b/layout/forms/nsTextControlFrame.cpp @@ -1124,18 +1124,43 @@ nsresult nsTextControlFrame::AttributeChanged(int32_t aNameSpaceID, } void nsTextControlFrame::GetText(nsString& aText) { - TextControlElement* textControlElement = - TextControlElement::FromNode(GetContent()); - MOZ_ASSERT(textControlElement); - if (IsSingleLineTextControl()) { - // There will be no line breaks so we can ignore the wrap property. - textControlElement->GetTextEditorValue(aText, true); - } else { - HTMLTextAreaElement* textArea = HTMLTextAreaElement::FromNode(mContent); - if (textArea) { - textArea->GetValue(aText); + if (HTMLInputElement* inputElement = HTMLInputElement::FromNode(mContent)) { + if (IsSingleLineTextControl()) { + // There will be no line breaks so we can ignore the wrap property. + inputElement->GetTextEditorValue(aText, true); + return; } + aText.Truncate(); + return; } + + MOZ_ASSERT(!IsSingleLineTextControl()); + if (HTMLTextAreaElement* textAreaElement = + HTMLTextAreaElement::FromNode(mContent)) { + textAreaElement->GetValue(aText); + return; + } + + MOZ_ASSERT(aText.IsEmpty()); + aText.Truncate(); +} + +bool nsTextControlFrame::TextEquals(const nsAString& aText) const { + if (HTMLInputElement* inputElement = HTMLInputElement::FromNode(mContent)) { + if (IsSingleLineTextControl()) { + // There will be no line breaks so we can ignore the wrap property. + return inputElement->TextEditorValueEquals(aText); + } + return aText.IsEmpty(); + } + + MOZ_ASSERT(!IsSingleLineTextControl()); + if (HTMLTextAreaElement* textAreaElement = + HTMLTextAreaElement::FromNode(mContent)) { + return textAreaElement->ValueEquals(aText); + } + + return aText.IsEmpty(); } /// END NSIFRAME OVERLOADS diff --git a/layout/forms/nsTextControlFrame.h b/layout/forms/nsTextControlFrame.h index 4565b5bb9ae2..45c1ed574136 100644 --- a/layout/forms/nsTextControlFrame.h +++ b/layout/forms/nsTextControlFrame.h @@ -163,6 +163,13 @@ class nsTextControlFrame final : public nsContainerFrame, void GetText(nsString& aText); + /** + * TextEquals() is designed for internal use so that aValue shouldn't + * include \r character. It should be handled before calling this with + * nsContentUtils::PlatformToDOMLineBreaks(). + */ + bool TextEquals(const nsAString& aText) const; + virtual nsresult PeekOffset(nsPeekOffsetStruct* aPos) override; NS_DECL_QUERYFRAME