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

Status: RESOLVED FIXED

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

Description

[Makoto Kato [:m_kato]]

Actually, we use twice nsTextEditorState::GetValue call per HTMLInputElement::SetValue call.  nsTextEditorState::GetValue spends 10% of HTMLInputElement::SetValue, so we should remove twice call.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1368888 - Don't get previous value twice in input.value setter.

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.

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

Comment 2

[Olli Pettay [:smaug]]

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

Comment 3

[Makoto Kato [:m_kato]]

Comment on attachment 8877538 [details]
Bug 1368888 - Don't get previous value twice in input.value setter.

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

Comment 4

[Makoto Kato [:m_kato]]

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.

Comment 5

[Olli Pettay [:smaug]]

Comment on attachment 8877538 [details]
Bug 1368888 - Don't get previous value twice in input.value setter.

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b9cc1cd08ea
Don't get previous value twice in input.value setter. r=smaug

Comment 7

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4b9cc1cd08ea