Make selection change part of the IME change transaction

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

unspecified
Firefox 46
All
Android
Points:
---
Dependency tree / graph

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.
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 6

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/df4a07a432dc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
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?
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)
You need to log in before you can comment on or make changes to this bug.