1236640 - Make selection change part of the IME change transaction

Status: RESOLVED FIXED

(Firefox for Android :: Keyboards and IME, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

In bug 1051556 we made IME _text_ change transactional. This bug is about including selection change in the transaction.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 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.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 3

[Eugen Sawin [: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?

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

(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

[Pulsebot]

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

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df4a07a432dc

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 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

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8708004 [details] [diff] [review]
Patch for Aurora

Fix a crash, taking it.

Comment 9

[Carsten Book [:Tomcat]]

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)

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

The aurora-specific patch should work (attachment 8708004 [details] [diff] [review]). Thanks!

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/088009bfaae4