Closed Bug 974318 Opened 6 years ago Closed 6 years ago

WidgetTextEvent should store text ranges with a class inherited from nsTArray

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(12 files, 1 obsolete file)

14.80 KB, patch
smaug
: review+
Details | Diff | Splinter Review
34.29 KB, patch
smaug
: review+
ehsan
: review+
Details | Diff | Splinter Review
12.44 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.72 KB, patch
smaug
: review+
Details | Diff | Splinter Review
17.50 KB, patch
jimm
: review+
Details | Diff | Splinter Review
6.66 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
6.54 KB, patch
karlt
: review+
Details | Diff | Splinter Review
2.60 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.92 KB, patch
smaug
: review+
Details | Diff | Splinter Review
100.37 KB, patch
Details | Diff | Splinter Review
5.02 KB, patch
jchen
: review+
Details | Diff | Splinter Review
33.36 KB, patch
Details | Diff | Splinter Review
Currently, WidgetTextEvent stores text ranges with raw C array of TextRange and counter for storing length of the array.

At creating nsDOMTextEvent, we create nsPrivateTextRangeList which is refcountable with copying the raw C array.

So, if WidgetTextEvent stores text ranges with a class inherited from nsTArray which is refcountable, we can get rid of nsPrivateTextRangeList and its related code.
First, create refcountable class which is derived from nsTArray of TextRange.

By this change, native composition event handlers can never use stack for allocating the array. However, current code allocates the copy of it in the heap (nsPrivateTextRange). Therefore, it's not matter.

Additionally, this approach avoids making its copy even if somebody wants to store it.
Attachment #8382159 - Flags: review?(bugs)
This patch makes TextComposition stores the TextRange array.

Then, nsEditor can refer it directly. This patch makes text event handling simpler with following patches.
Attachment #8382161 - Flags: review?(ehsan)
Attachment #8382161 - Flags: review?(bugs)
By the part.2 patch, we can get rid of nsPrivateTextRange!
Attachment #8382165 - Flags: review?(bugs)
By removing the nsPrivateTextRange, now, nsDOMTextEvent doesn't have specific members. So, we can remove nsDOMTextEvent and use nsDOMUIEvent (for .isChar).
Attachment #8382167 - Flags: review?(bugs)
Finally, remove WidgetTextEvent::rangeCount and WidgetTextEvent::rangeArray.
Attachment #8382177 - Flags: review?(bugs)
Comment on attachment 8382161 [details] [diff] [review]
part.2 TextComposition should store TextRangeArray at editor handling text event and nsEditor should refer it

Sorry, Ehsan, for the spam. I'll request this review after smaug's review.
Attachment #8382161 - Flags: review?(ehsan)
FYI: Qt widget hasn't support IME composition yet. Therefore, there is no change in widget/qt.
(In reply to comment #12)
> Comment on attachment 8382161 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=8382161
> part.2 TextComposition should store TextRangeArray at editor handling text
> event and nsEditor should refer it
> 
> Sorry, Ehsan, for the spam. I'll request this review after smaug's review.

No worries! Sounds good.
Attachment #8382159 - Flags: review?(bugs) → review+
Attachment #8382167 - Flags: review?(bugs) → review+
Comment on attachment 8382174 [details] [diff] [review]
part.9 Use WidgetTextEvent::mRanges on CompositionStringSynthesizer

>   var compositionString = utils.createCompositionStringSynthesizer();
>   compositionString.setString(aEvent.composition.string);
>   if (aEvent.composition.clauses[0].length) {
>     for (var i = 0; i < aEvent.composition.clauses.length; i++) {
>-      compositionString.appendClause(aEvent.composition.clauses[i].length,
>-                                     aEvent.composition.clauses[i].attr);
>+      switch (aEvent.composition.clauses[i].attr) {
>+        case compositionString.ATTR_RAWINPUT:
>+        case compositionString.ATTR_SELECTEDRAWTEXT:
>+        case compositionString.ATTR_CONVERTEDTEXT:
>+        case compositionString.ATTR_SELECTEDCONVERTEDTEXT:
>+          compositionString.appendClause(aEvent.composition.clauses[i].length,
>+                                         aEvent.composition.clauses[i].attr);
>+          break;
>+        case 0:
>+          // Ignore dummy clasue for the argument.
clause
Attachment #8382174 - Flags: review?(bugs) → review+
Attachment #8382177 - Flags: review?(bugs) → review+
Attachment #8382165 - Flags: review?(bugs) → review+
Comment on attachment 8382161 [details] [diff] [review]
part.2 TextComposition should store TextRangeArray at editor handling text event and nsEditor should refer it

>+  // Retruns the clauses and/or caret range of the composition string.
Returns

> /* ============ protected methods ================== */
>-static SelectionType TextRangeToSelection(int aTextRangeType)
>+static SelectionType
>+ToSelectionType(uint32_t aTextRangeType)

will continue reviewing here...)
(In reply to Olli Pettay [:smaug] from comment #16)
> Comment on attachment 8382161 [details] [diff] [review]
> part.2 TextComposition should store TextRangeArray at editor handling text
> event and nsEditor should refer it
> 
> >+  // Retruns the clauses and/or caret range of the composition string.
> Returns
> 
> > /* ============ protected methods ================== */
> >-static SelectionType TextRangeToSelection(int aTextRangeType)
> >+static SelectionType
> >+ToSelectionType(uint32_t aTextRangeType)
> 
> will continue reviewing here...)

I'll request review of editor part to Ehsan. So, you can review the part roughly ;-)

Thanks for a lot of review!
Comment on attachment 8382168 [details] [diff] [review]
part.5 Use WidgetTextEvent::mRanges on Windows

WidgetTextEvent::mRanges is nsRefPtr<TextRangeArray>, TextRangeArray is derived from nsAutoTArray<TextRange, 10>.

When there is composition string, this stores the clause ranges and/or caret position.
Attachment #8382168 - Flags: review?(jmathies)
Comment on attachment 8382170 [details] [diff] [review]
part.6 Use WidgetTextEvent::mRanges on Mac

WidgetTextEvent::mRanges is nsRefPtr<TextRangeArray>, TextRangeArray is derived from nsAutoTArray<TextRange, 10>.

When there is composition string, this stores the clause ranges and/or caret position.

FYI: TextRangeArray is conflict with Mac's native type name. Therefore, this patch specifies mozilla:: explicitly.
Attachment #8382170 - Flags: review?(smichaud)
Comment on attachment 8382171 [details] [diff] [review]
part.7 Use WidgetTextEvent::mRanges on GTK

WidgetTextEvent::mRanges is nsRefPtr<TextRangeArray>, TextRangeArray is derived from nsAutoTArray<TextRange, 10>.

When there is composition string, this stores the clause ranges and/or caret position.
Attachment #8382171 - Flags: review?(karlt)
WidgetTextEvent::mRanges is nsRefPtr<TextRangeArray>, TextRangeArray is derived from nsAutoTArray<TextRange, 10>.

When there is composition string, this stores the clause ranges and/or caret position.
Attachment #8384528 - Flags: review?(nchen)
Attachment #8382172 - Attachment is obsolete: true
Comment on attachment 8384528 [details] [diff] [review]
part.8 Use WidgetTextEvent::mRanges on Android

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

Thanks! Looks great!

::: widget/android/nsWindow.cpp
@@ +178,5 @@
>      mIMEMaskEventsCount(1), // Mask IME events since there's no focus yet
>      mIMEUpdatingContext(false),
>      mIMESelectionChanged(false)
>  {
> +    mIMERanges = new TextRangeArray();

Add it to the initializer list above? i.e. "mIMERanges(new TextRangeArray()),"
Attachment #8384528 - Flags: review?(nchen) → review+
Attachment #8382170 - Flags: review?(smichaud) → review+
Attachment #8382171 - Flags: review?(karlt) → review+
Attachment #8382168 - Flags: review?(jmathies) → review+
Comment on attachment 8382161 [details] [diff] [review]
part.2 TextComposition should store TextRangeArray at editor handling text event and nsEditor should refer it


>+    // Caret needs special handling since its length may be 0 and if it's not
>+    // specified explicitly, we need to handle ourselves later.
handle it

>+    if (textRange.mRangeType == NS_TEXTRANGE_CARETPOSITION) {
>+      NS_ASSERTION(!setCaret, "The ranges already has caret position");
>+      NS_ASSERTION(!textRange.Length(), "nsEditor doesn't support wide caret");
Should you use MOZ_ASSERT here

>+    // If the clause length is 0, it's should be a bug.
>+    if (!textRange.Length()) {
>+      NS_WARNING("Any clauses must not be empty");
>+      continue;
>+    }
So should we just warn or could we assert?



A bit tricky to review since the patch changes coding style and makes us use TextRangeArray.
Attachment #8382161 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #23)
> >+    if (textRange.mRangeType == NS_TEXTRANGE_CARETPOSITION) {
> >+      NS_ASSERTION(!setCaret, "The ranges already has caret position");
> >+      NS_ASSERTION(!textRange.Length(), "nsEditor doesn't support wide caret");
> Should you use MOZ_ASSERT here

No, we hit the assertions in the patch :-(

We need to fix the bugs in other bugs... And the second assertion is really layout's fault. We need to support wide caret, see bug 226784 and bug 38415 comment 41.

So, I won't change the assertion in this bug, is it OK?

> >+    // If the clause length is 0, it's should be a bug.
> >+    if (!textRange.Length()) {
> >+      NS_WARNING("Any clauses must not be empty");
> >+      continue;
> >+    }
> So should we just warn or could we assert?

So, it's risky too... I think that we should try it on next cycle because even if it causes regression, I don't have much time for fixing them.
Flags: needinfo?(bugs)
Comment on attachment 8382161 [details] [diff] [review]
part.2 TextComposition should store TextRangeArray at editor handling text event and nsEditor should refer it

Ehsan, could you review the editor part?

We don't need to use nsIPrivateTextRange(List) anymore because WidgetTextEvent has refcountable array of TextRange. This patch makes TextComposition store it at handling the event in focused editor. Then, nsEditor doesn't need to manage the list itself.
Attachment #8382161 - Flags: review?(ehsan)
I filed bug 979112 for wide caret support.
Can you please upload a diff -w version of the patch?  There seems to be some code movement going on...
Attached patch part.2 (-w)Splinter Review
Sure, but this might not help you...
Comment on attachment 8382161 [details] [diff] [review]
part.2 TextComposition should store TextRangeArray at editor handling text event and nsEditor should refer it

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

r=me with the below fixed.

::: editor/libeditor/base/IMETextTxn.cpp
@@ +233,5 @@
> +      NS_ASSERTION(!textRange.Length(), "nsEditor doesn't support wide caret");
> +      // NOTE: If the caret position is larger than max length of the editor
> +      //       content, this may fail.
> +      rv = selection->Collapse(mElement, mOffset + textRange.mStartOffset);
> +      setCaret = NS_SUCCEEDED(rv);

Here you're changing the meaning of the code if setCaret is set to true in the first iteration but to false in the second one.
Attachment #8382161 - Flags: review?(ehsan) → review+
(Ok for not adding assertions then)
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.