Closed Bug 1563508 Opened 5 months ago Closed 5 months ago

Rewrite window_composition_text_querycontent.xul with async/await

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox69 --- fixed
firefox70 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(7 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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.

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

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.

Pushed by masayuki@d-toybox.com:
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
Blocks: 1553008

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

Flags: needinfo?(masayuki)

(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.

Flags: needinfo?(masayuki)

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:
Attachment #9076194 - Flags: approval-mozilla-beta?
Attachment #9076195 - Flags: approval-mozilla-beta?
Attachment #9076196 - Flags: approval-mozilla-beta?
Attachment #9076197 - Flags: approval-mozilla-beta?
Attachment #9076198 - Flags: approval-mozilla-beta?
Attachment #9076199 - Flags: approval-mozilla-beta?
Attachment #9076200 - Flags: approval-mozilla-beta?

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.

Attachment #9076194 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9076195 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9076196 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9076197 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9076198 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9076199 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9076200 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.