1563508 - Rewrite window_composition_text_querycontent.xul with async/await

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, task, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Currently, it's difficult to add new async tests into window_composition_text_querycontent.xul since it was written with yield. It should be rewritten with async/await.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1563508 - part 1: Make window_composition_text_querycontent.xul use `let` in functions instead of `var`

For making the scope of variants clearer, make it use let instead of var.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1563508 - part 2: Make `runIMEContentObserverTest()` async

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1563508 - part 3: Make `runEditorReframeTests()` async

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1563508 - part 4: Make `runAsyncForceCommitTest()` async

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1563508 - part 5: Make `runRemoveContentTest()` async and fix an editor's bug which found by the test order change

Previously, runRemoveContentTest() was the last test which synthesizes
composition in the <textarea>. However, new test order move it to middle
of the other tests. Then, it hits an MOZ_ASSERT() in
TextComposition::EditorWillHandleCompositionChangeEvent() that detects a
bug. When editable element is removed from the DOM tree during a composition,
editor cannot listen to eCompositionEnd event. Therefore, when the editor
gets back, it still has destroyed TextComposition and keeps handling new
composition with the old one. Therefore this patch makes
EditorBase::InstallEventListeners() forget the destroyed composition.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1563508 - part 6: Make `runPanelTest()` async

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1563508 - part 7: Remove unnecessary code from window_composition_text_querycontent.xul

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/012d470fdcd5
part 1: Make window_composition_text_querycontent.xul use `let` in functions instead of `var` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/80398c8d9431
part 2: Make `runIMEContentObserverTest()` async r=m_kato
https://hg.mozilla.org/integration/autoland/rev/d3e054aa705f
part 3: Make `runEditorReframeTests()` async r=m_kato
https://hg.mozilla.org/integration/autoland/rev/0941d29ab5f3
part 4: Make `runAsyncForceCommitTest()` async r=m_kato
https://hg.mozilla.org/integration/autoland/rev/45935cb1d499
part 5: Make `runRemoveContentTest()` async and fix an editor's bug which found by the test order change r=m_kato
https://hg.mozilla.org/integration/autoland/rev/047f80a81b16
part 6: Make `runPanelTest()` async r=m_kato
https://hg.mozilla.org/integration/autoland/rev/006d54dad1f5
part 7: Remove unnecessary code from window_composition_text_querycontent.xul r=m_kato

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/012d470fdcd5
https://hg.mozilla.org/mozilla-central/rev/80398c8d9431
https://hg.mozilla.org/mozilla-central/rev/d3e054aa705f
https://hg.mozilla.org/mozilla-central/rev/0941d29ab5f3
https://hg.mozilla.org/mozilla-central/rev/45935cb1d499
https://hg.mozilla.org/mozilla-central/rev/047f80a81b16
https://hg.mozilla.org/mozilla-central/rev/006d54dad1f5

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Looks like this grafts cleanly to Beta. Worth an approval request to fix bug 1553008?

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)

Looks like this grafts cleanly to Beta. Worth an approval request to fix bug 1553008?

Yeah, the change is really safe and fixes the crash. So, let's take them.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 9076194 [details]
Bug 1563508 - part 1: Make window_composition_text_querycontent.xul use let in functions instead of var

Beta/Release Uplift Approval Request

• User impact if declined: User may meet the crash reported to bug 1553008.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): When I write "part 5", I hit a MOZ_ASSERT() which caused bug 1553008. So, these patches almost changes the automated test, but changes EditorBase a little in "part 5".
• String changes made/needed:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9076194 [details]
Bug 1563508 - part 1: Make window_composition_text_querycontent.xul use let in functions instead of var

Mostly test-only changes, but part 5 fixes a real-world crash. Approved for 69.0b6.

Comment 14

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/63472899c1b1
https://hg.mozilla.org/releases/mozilla-beta/rev/c10e8a001a98
https://hg.mozilla.org/releases/mozilla-beta/rev/93c68c624bd5
https://hg.mozilla.org/releases/mozilla-beta/rev/e8a0379f87fd
https://hg.mozilla.org/releases/mozilla-beta/rev/255e9fcc1f56
https://hg.mozilla.org/releases/mozilla-beta/rev/d6e09426a5f8
https://hg.mozilla.org/releases/mozilla-beta/rev/922cbc9dc3c0