Make selection change part of the IME change transaction

RESOLVED FIXED in Firefox 45

Status

()

Firefox for Android
Keyboards and IME
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

unspecified
Firefox 46
All
Android
Points:
---

Firefox Tracking Flags

(firefox45 fixed, firefox46 fixed)

Details

Attachments

(2 attachments)

In bug 1051556 we made IME _text_ change transactional. This bug is about including selection change in the transaction.
Created attachment 8703771 [details] [diff] [review]
Make selection change part of the IME change transaction (v1)

We notify IME text changes in transactions. We should make selection
change notification part of that transaction.
Attachment #8703771 - Flags: review?(snorp)
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 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+
(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 5

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/df4a07a432dc

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/df4a07a432dc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Created attachment 8708004 [details] [diff] [review]
Patch for Aurora

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?
status-firefox45: --- → affected
Comment on attachment 8708004 [details] [diff] [review]
Patch for Aurora

Fix a crash, taking it.
Attachment #8708004 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
The aurora-specific patch should work (attachment 8708004 [details] [diff] [review]). Thanks!
Flags: needinfo?(nchen)

Comment 11

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/088009bfaae4
status-firefox45: affected → fixed
You need to log in before you can comment on or make changes to this bug.