Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

Trunk
Firefox 35
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

Right now whenever we change text or apply composition styling, we always start a new composition. This can result in side effects such as bug 892945. We should reuse compositions when we can.
Simple refactoring to prepare for the later patches. Also gets rid of a case of double-checked locking, which is a bad idiom.
Attachment #8479222 - Flags: review?(cpeterson)
Fix crash when logging is turned on.
Attachment #8479225 - Flags: review?(cpeterson)
Small optimizations. Skip notification if the text change span is empty and skip sending the "qury text content" event if the query range is empty.
Attachment #8479230 - Flags: review?(cpeterson)
This patch adds a COMPOSE_TEXT action to complement the existing REPLACE_TEXT action. The difference between the two is COMPOSE_TEXT will leave a composition open on the Gecko side, so that when we apply styling later (underline, etc.), we don't have to open a new composition.
Attachment #8479303 - Flags: review?(cpeterson)
Comment on attachment 8479222 [details] [diff] [review]
Refactor GeckoEditable (v1)

Review of attachment 8479222 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: mobile/android/base/GeckoEditable.java
@@ +252,4 @@
>              case Action.TYPE_ACKNOWLEDGE_FOCUS:
>                  GeckoAppShell.sendEventToGecko(GeckoEvent.createIMEEvent(
>                          GeckoEvent.ImeAction.IME_ACKNOWLEDGE_FOCUS));
> +                return;

nit: you don't really need to change this switch statements' breaks to returns. Because the change is unnecessary and breaks are the typical switch pattern, I would recommend keeping the breaks and the implicit return at the end of the method. But you can use whatever pattern you prefer for this code, since you read it more than me. :)
Attachment #8479222 - Flags: review?(cpeterson) → review+
Comment on attachment 8479225 [details] [diff] [review]
Fix debug logging crash (v1)

Review of attachment 8479225 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8479225 - Flags: review?(cpeterson) → review+
This patch implements the Gecko side of COMPOSE_TEXT handling. COMPOSE_TEXT and REPLACE_TEXT share most of the code path, except the COMPOSE_TEXT leaves the composition open at the end.
Attachment #8479306 - Flags: review?(cpeterson)
Comment on attachment 8479230 [details] [diff] [review]
Bug 1058136 - Don't notify null text changes;

Review of attachment 8479230 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: widget/android/nsWindow.cpp
@@ +2136,5 @@
>  
> +        if (change.mStart == change.mOldEnd &&
> +                change.mStart == change.mNewEnd) {
> +            continue;
> +        }

Can you avoid inserting these empty spans into mIMETextChanges in the first place? Or do these spans become empty when NotifyIMEOfTextChange() merges spans?
Attachment #8479230 - Flags: review?(cpeterson) → review+
Comment on attachment 8479303 [details] [diff] [review]
Send separate compose event for composing text (v1)

Review of attachment 8479303 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8479303 - Flags: review?(cpeterson) → review+
Comment on attachment 8479306 [details] [diff] [review]
Handle compose event to optimize composition usage (v1)

Review of attachment 8479306 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Android's composition span code is pretty crazy! Since, AFAIK, we never use more than one composition, I've long wondered whether our implementation could be simplified compared to Android's API.

::: widget/android/nsWindow.cpp
@@ +1788,5 @@
> +                    mIMEKeyEvents.Clear();
> +                    FlushIMEChanges();
> +                    mozilla::widget::android::GeckoAppShell::NotifyIME(
> +                        AndroidBridge::NOTIFY_IME_REPLY_EVENT);
> +                    break;

Maybe add a short comment that this break statement is breaking out of `switch (ae->Action())`. This was pretty confusing.
Attachment #8479306 - Flags: review?(cpeterson) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f97b62e3c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/37addbdb5cd1
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e5502347731
https://hg.mozilla.org/integration/mozilla-inbound/rev/032246f68eaa
https://hg.mozilla.org/integration/mozilla-inbound/rev/713e1d6f09c4


(In reply to Chris Peterson (:cpeterson) from comment #8)
> 
> ::: widget/android/nsWindow.cpp
> @@ +2136,5 @@
> >  
> > +        if (change.mStart == change.mOldEnd &&
> > +                change.mStart == change.mNewEnd) {
> > +            continue;
> > +        }
> 
> Can you avoid inserting these empty spans into mIMETextChanges in the first
> place? Or do these spans become empty when NotifyIMEOfTextChange() merges
> spans?

I think it's possible for merges to become empty, so we should check it here to be safe.

> ::: widget/android/nsWindow.cpp
> @@ +1788,5 @@
> > +                    mIMEKeyEvents.Clear();
> > +                    FlushIMEChanges();
> > +                    mozilla::widget::android::GeckoAppShell::NotifyIME(
> > +                        AndroidBridge::NOTIFY_IME_REPLY_EVENT);
> > +                    break;
> 
> Maybe add a short comment that this break statement is breaking out of
> `switch (ae->Action())`. This was pretty confusing.

Added.
Turns out we do get text change notifications, we just have to process them right.
Attachment #8483860 - Flags: review?(cpeterson)
Comment on attachment 8483860 [details] [diff] [review]
Properly send text change notifications for composition text (v1)

Review of attachment 8483860 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8483860 - Flags: review?(cpeterson) → review+
Flags: needinfo?(nchen)
You need to log in before you can comment on or make changes to this bug.