Closed
Bug 1236640
Opened 7 years ago
Closed 7 years ago
Make selection change part of the IME change transaction
Categories
(Firefox for Android Graveyard :: Keyboards and IME, defect)
Tracking
(firefox45 fixed, firefox46 fixed)
RESOLVED
FIXED
Firefox 46
People
(Reporter: jchen, Assigned: jchen)
References
Details
Attachments
(2 files)
4.30 KB,
patch
|
esawin
:
review+
|
Details | Diff | Splinter Review |
6.05 KB,
patch
|
jchen
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
In bug 1051556 we made IME _text_ change transactional. This bug is about including selection change in the transaction.
Assignee | ||
Comment 1•7 years ago
|
||
We notify IME text changes in transactions. We should make selection change notification part of that transaction.
Attachment #8703771 -
Flags: review?(snorp)
Assignee | ||
Comment 2•7 years ago
|
||
Comment on attachment 8703771 [details] [diff] [review] Make selection change part of the IME change transaction (v1) Review of attachment 8703771 [details] [diff] [review]: ----------------------------------------------------------------- Oops... I'm so used to automatically asking snorp for review these days :p
Attachment #8703771 -
Flags: review?(snorp) → review?(esawin)
Comment 3•7 years ago
|
||
Comment on attachment 8703771 [details] [diff] [review] Make selection change part of the IME change transaction (v1) Review of attachment 8703771 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/nsWindow.cpp @@ +2306,5 @@ > } > > mIMETextChangedDuringFlush = false; > > + auto abortFlushing = [=] () -> bool { abortFlushing sounds like a command rather than a conditional statement. We could also drop the "Flushing" given that it's a local function in this context. How about "shouldAbort" or "aborting" instead or similar? @@ +2307,5 @@ > > mIMETextChangedDuringFlush = false; > > + auto abortFlushing = [=] () -> bool { > + if (mIMETextChangedDuringFlush) { Could change to if (!...) { return false; } to avoid unnecessary nesting. @@ +2316,5 @@ > + PostFlushIMEChanges(FLUSH_FLAG_RETRY); > + } else { > + // Don't retry if already retrying, to avoid infinite loops. > + __android_log_print(ANDROID_LOG_WARN, "GeckoViewSupport", > + "Already retrying IME flush"); Why not use ALOG?
Attachment #8703771 -
Flags: review?(esawin) → review+
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Eugen Sawin [:esawin] from comment #3) > Comment on attachment 8703771 [details] [diff] [review] > Make selection change part of the IME change transaction (v1) > > Review of attachment 8703771 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +2316,5 @@ > > + PostFlushIMEChanges(FLUSH_FLAG_RETRY); > > + } else { > > + // Don't retry if already retrying, to avoid infinite loops. > > + __android_log_print(ANDROID_LOG_WARN, "GeckoViewSupport", > > + "Already retrying IME flush"); > > Why not use ALOG? ALOG is debug-only but I wanted this message for release as well.
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/df4a07a432dc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Assignee | ||
Comment 7•7 years ago
|
||
Approval Request Comment [Feature/regressing bug #]: Bug 1051556 [User impact if declined]: Suspected crashes [Describe test coverage new/current, TreeHerder]: Locally, m-c [Risks and why]: Small risk; patch is tested and there hasn't been any problems on m-c so far. [String/UUID change made/needed]: None
Attachment #8708004 -
Flags: review+
Attachment #8708004 -
Flags: approval-mozilla-aurora?
Updated•7 years ago
|
status-firefox45:
--- → affected
Comment 8•7 years ago
|
||
Comment on attachment 8708004 [details] [diff] [review] Patch for Aurora Fix a crash, taking it.
Attachment #8708004 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 9•7 years ago
|
||
does not apply to aurora cleanly: grafting 322256:df4a07a432dc "Bug 1236640 - Make selection change part of the IME change transaction; r=esawin" merging widget/android/nsWindow.cpp warning: conflicts while merging widget/android/nsWindow.cpp! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use hg resolve and hg graft --continue)
Flags: needinfo?(nchen)
Assignee | ||
Comment 10•7 years ago
|
||
The aurora-specific patch should work (attachment 8708004 [details] [diff] [review]). Thanks!
Flags: needinfo?(nchen)
Comment 11•7 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/088009bfaae4
Updated•2 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•