Closed
Bug 1253479
Opened 9 years ago
Closed 9 years ago
[e10s] Make widget/tests/test_assign_event_data.html work under e10s
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: adw, Assigned: adw)
References
(Blocks 1 open bug)
Details
(Whiteboard: btpp-active)
Attachments
(1 file, 1 obsolete file)
6.33 KB,
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
[e10s] Make widget/tests/test_assign_event_data.html work under e10s.
Masayuki, would you mind reviewing? It looks like composition events must happen in the parent, but I don't understand why. Anyway, this patch allows this test to work under e10s for me locally, but I don't know if it's the right way to fix this.
Attachment #8726508 -
Flags: review?(masayuki)
Assignee | ||
Comment 1•9 years ago
|
||
Updated•9 years ago
|
Whiteboard: btpp-active
Comment 2•9 years ago
|
||
(In reply to Drew Willcoxon :adw from comment #0)
> Created attachment 8726508 [details] [diff] [review]
> patch
>
> [e10s] Make widget/tests/test_assign_event_data.html work under e10s.
>
> Masayuki, would you mind reviewing? It looks like composition events must
> happen in the parent, but I don't understand why. Anyway, this patch allows
> this test to work under e10s for me locally, but I don't know if it's the
> right way to fix this.
How do the tests fail? Indeed, mozilla::TextComposition class which manages active composition is created both in the chrome process and the content process. However, for this case, only created in the content process should work. It seems that the failure *might* detect an actual bug.
Assignee | ||
Comment 3•9 years ago
|
||
The test hangs after it starts the "WidgetKeyboardEvent (keyup during composition)" subtest. The subtest is listening for keyup, but onEvent and gCallback are never called. At that point the input has focus and it contains ね. The subtest starts by calling synthesizeCompositionChange with a string of ね, which is correctly shown in the <input>. Then it calls synthesizeComposition({ type: "compositioncommitasis" }), and unfortunately I don't know enough about composition events to say what happens then. The ね is not underlined, which I think means the composition was successfully committed?
Do you have any ideas?
Flags: needinfo?(masayuki)
Comment 4•9 years ago
|
||
(In reply to Drew Willcoxon :adw from comment #3)
> The test hangs after it starts the "WidgetKeyboardEvent (keyup during
> composition)" subtest. The subtest is listening for keyup, but onEvent and
> gCallback are never called. At that point the input has focus and it
> contains ね. The subtest starts by calling synthesizeCompositionChange with
> a string of ね, which is correctly shown in the <input>. Then it calls
> synthesizeComposition({ type: "compositioncommitasis" }), and unfortunately
> I don't know enough about composition events to say what happens then. The
> ね is not underlined, which I think means the composition was successfully
> committed?
Yes. That's the expected result. It means that <input> element receives "compositionstart", "compositionupdate" (, "text") and "compositionend" events correctly. Before "compositionend", "keyup" for the 'A' key should be fired... I.e.,
1. compositionstart
2. keydown
3. compositionupdate
4. text
5. keyup
6. compositionend
These events should be fired in this order...
When you call synthesizeCompositionChangeAndComposition() with a key, following methods are called:
TextInputProcessor::SetPendingCompositionString()
TextEventDispatcher::PendingComposition::SetString()
TextInputProcessor::AppendClauseToPendingComposition()
TextEventDispatcher::PendingComposition::AppendClause()
TextInputProcessor::SetCaretInPendingComposition()
TextEventDispatcher::PendingComposition::SetCaret()
TextInputProcessor::FlushPendingComposition()
TextInputProcessor::MaybeDispatchKeydownForComposition()
TextInputProcessor::KeydownInternal()
TextEventDispatcher::DispatchKeyboardEvent()
TextEventDispatcher::DispatchKeyboardEventInternal()
TextEventDispatcher::PendingComposition::Flush()
TextInputProcessor::MaybeDispatchKeyupForComposition()
TextInputProcessor::KeydownInternal()
TextEventDispatcher::DispatchKeyboardEvent()
TextEventDispatcher::DispatchKeyboardEventInternal()
In TextEventDispatcher::DispatchKeyboardEventInternal(), "dom.keyboardevent.dispatch_during_composition" is checked (the value is stored into sDispatchKeyEventsDuringComposition with AddBoolVarCache()). So, I guess that the pref value isn't modified in e10s mode yet...
Flags: needinfo?(masayuki)
Updated•9 years ago
|
tracking-e10s:
--- → +
Assignee | ||
Comment 5•9 years ago
|
||
Thanks Masayuki. The test is using SpecialPowers.setBoolPref(), which actually works in e10s (by sending a message to chrome). But in e10s only, something is resetting the HasUserValue() flag of dom.keyboardevent.dispatch_during_composition between the time the test sets it to true and the time that TextEventDispatcher gets it. So PREF_GetBoolPref takes the "default" path and gets its default value instead of the user value. That doesn't happen in non-e10s.
I'm still trying to figure out what's causing it, but it's not PREF_ClearUserPref, PREF_ClearAllUserPrefs, or pref_HashPref.
Assignee | ||
Comment 6•9 years ago
|
||
Hmm, it's because there are two different values of gHashTable in prefapi. The one used by the set() in the test is not the one used by the get() in TextEventDispatcher. Is SpecialPowers really setting prefs in the parent?
Assignee | ||
Comment 7•9 years ago
|
||
Oh... the pref is indeed set in the parent, but TextEventDispatcher's get() is called in the child. My patch synthesized the events in the parent, which is why it worked I guess.
Assignee | ||
Comment 8•9 years ago
|
||
But nsPrefBranch::SetBoolPref throws if it's called from the child...
Comment 9•9 years ago
|
||
(In reply to Drew Willcoxon :adw from comment #7)
> Oh... the pref is indeed set in the parent, but TextEventDispatcher's get()
> is called in the child. My patch synthesized the events in the parent,
> which is why it worked I guess.
Impossible to change prefs of child process sounds bad because a lot of modules use mozilla::Preferences even in a child process. So, I think that it's a test framework's bug...
Assignee | ||
Comment 10•9 years ago
|
||
But nsPrefBranch::SetBoolPref and Preferences::SetBool both ENSURE_MAIN_PROCESS() (i.e., return NS_ERROR_NOT_AVAILABLE) when called from the child, so it's not really a test framework bug.
Given that it's apparently impossible to set preferences in the child, it seems like there are only two ways to fix this: either have TextEventDispatcher get the pref from the parent, not the child; or have the preference(s) synced from the parent to the child.
Assignee | ||
Comment 11•9 years ago
|
||
It looks like there is preference syncing from the parent to the child. I don't know how to tap into it to tell when the child's preference store is updated, but when I add some setTimeouts to the test after it sets and clears the pref, it passes. Maybe I can just add a preference observer in the child, in the test.
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#594
http://mxr.mozilla.org/mozilla-central/search?string=PreferenceUpdate
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#3151
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#2345
Assignee | ||
Comment 12•9 years ago
|
||
Yeah, that did it.
Attachment #8726508 -
Attachment is obsolete: true
Attachment #8726508 -
Flags: review?(masayuki)
Attachment #8728736 -
Flags: review?(masayuki)
Assignee | ||
Comment 13•9 years ago
|
||
Comment 14•9 years ago
|
||
Comment on attachment 8728736 [details] [diff] [review]
patch v2
Looks great! Thank you for your polite work!
*I* think that the feature should be in specialpowers API. Could you file a follow up bug? (I don't know who is managing specialpowers API, though)
Attachment #8728736 -
Flags: review?(masayuki) → review+
Assignee | ||
Comment 15•9 years ago
|
||
Assignee | ||
Comment 16•9 years ago
|
||
Thanks Masayuki. I filed bug 1255649 for SpecialPowers.
Comment 17•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•