Closed
Bug 1236640
Opened 9 years ago
Closed 9 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•9 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•9 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•9 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•9 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•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Assignee | ||
Comment 7•9 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•9 years ago
|
status-firefox45:
--- → affected
Comment 8•9 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•9 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•9 years ago
|
||
The aurora-specific patch should work (attachment 8708004 [details] [diff] [review]). Thanks!
Flags: needinfo?(nchen)
Comment 11•9 years ago
|
||
bugherder uplift |
Updated•4 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
•