Closed Bug 1368888 Opened 3 years ago Closed 3 years ago

twice nsTextEditorState::GetValue call per HTMLInputElement::SetValue call

Categories

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

55 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Actually, we use twice nsTextEditorState::GetValue call per HTMLInputElement::SetValue call.  nsTextEditorState::GetValue spends 10% of HTMLInputElement::SetValue, so we should remove twice call.
Priority: -- → P3
Comment on attachment 8877538 [details]
Bug 1368888 - Don't get previous value twice in input.value setter.

https://reviewboard.mozilla.org/r/149000/#review153448

::: commit-message-b266a:3
(Diff revision 1)
> +Bug 1368888 - Don't get previous value twice in input.value setter. r?smaug
> +
> +Actually, we get previous input.value twice in HTMLInputElement::SetValue and nsTextEditorState::SetValue when setting input.value.  Since nsTextEditorState::GetValue uses DocumentEncoder, it is expensive.  So we should use old value as parameter of nsTextEditorState::SetValue if possible.

Drop "Actually,"

::: dom/html/HTMLInputElement.h:946
(Diff revision 1)
>     *
>     * @param aValue      String to set.
>     * @param aFlags      See nsTextEditorState::SetValueFlags.
>     */
> -  nsresult SetValueInternal(const nsAString& aValue, uint32_t aFlags);
> +  nsresult SetValueInternal(const nsAString& aValue,
> +                            const nsAString* aOldValue,

This needs some comment. I thought first this was out-param, but apparently aOldValue is in-param

::: dom/html/nsTextEditorState.cpp:2530
(Diff revision 1)
>        } else {
>          // If setting value won't change current value, we shouldn't commit
>          // composition for compatibility with the other browsers.
>          nsAutoString currentValue;
> +        if (aOldValue) {
> +          currentValue.Assign(*aOldValue);

Please add some #ifdef DEBUG code here to ensure aOldValue equals the value one would get from mBoundFrame->GetText()

::: dom/html/nsTextEditorState.cpp:2605
(Diff revision 1)
>      }
>  #endif
>  
>      nsAutoString currentValue;
> +    if (aOldValue) {
> +      currentValue.Assign(*aOldValue);

Also here. Add #ifdef DEBUG code to ensure aOldValue equals with value from mBoundFrame->GetText
Attachment #8877538 - Flags: review?(bugs) → review+
Comment on attachment 8877538 [details]
Bug 1368888 - Don't get previous value twice in input.value setter.

Could you additional review?

During committing composition, nsTextEditorState::GetValue and mBoundFrame->GetText might not return same value.  So we don't trust old value during it.
Attachment #8877538 - Flags: review+ → review?(bugs)
Comment on attachment 8877538 [details]
Bug 1368888 - Don't get previous value twice in input.value setter.

https://reviewboard.mozilla.org/r/149000/#review154410
Attachment #8877538 - Flags: review?(bugs) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/4b9cc1cd08ea
Don't get previous value twice in input.value setter. r=smaug
https://hg.mozilla.org/mozilla-central/rev/4b9cc1cd08ea
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.