Closed
Bug 974318
Opened 10 years ago
Closed 10 years ago
WidgetTextEvent should store text ranges with a class inherited from nsTArray
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Core
DOM: UI Events & Focus Handling
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.akhgari
:
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.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
By the part.2 patch, we can get rid of nsPrivateTextRange!
Attachment #8382165 -
Flags: review?(bugs)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8382174 -
Flags: review?(bugs)
Assignee | ||
Comment 10•10 years ago
|
||
Finally, remove WidgetTextEvent::rangeCount and WidgetTextEvent::rangeArray.
Attachment #8382177 -
Flags: review?(bugs)
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
FYI: Qt widget hasn't support IME composition yet. Therefore, there is no change in widget/qt.
Comment 14•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8382159 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8382167 -
Flags: review?(bugs) → review+
Comment 15•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8382177 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8382165 -
Flags: review?(bugs) → review+
Comment 16•10 years ago
|
||
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...)
Assignee | ||
Comment 17•10 years ago
|
||
(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!
Assignee | ||
Comment 18•10 years ago
|
||
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)
Assignee | ||
Comment 19•10 years ago
|
||
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)
Assignee | ||
Comment 20•10 years ago
|
||
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)
Assignee | ||
Comment 21•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8382172 -
Attachment is obsolete: true
Comment 22•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8382170 -
Flags: review?(smichaud) → review+
Updated•10 years ago
|
Attachment #8382171 -
Flags: review?(karlt) → review+
Updated•10 years ago
|
Attachment #8382168 -
Flags: review?(jmathies) → review+
Comment 23•10 years ago
|
||
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+
Assignee | ||
Comment 24•10 years ago
|
||
(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)
Assignee | ||
Comment 25•10 years ago
|
||
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)
Assignee | ||
Comment 26•10 years ago
|
||
I filed bug 979112 for wide caret support.
Comment 27•10 years ago
|
||
Can you please upload a diff -w version of the patch? There seems to be some code movement going on...
Assignee | ||
Comment 28•10 years ago
|
||
Sure, but this might not help you...
Comment 29•10 years ago
|
||
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+
Comment 30•10 years ago
|
||
(Ok for not adding assertions then)
Assignee | ||
Comment 31•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea20343a2ce https://hg.mozilla.org/integration/mozilla-inbound/rev/5abcc356d54a https://hg.mozilla.org/integration/mozilla-inbound/rev/3d78ee924b64 https://hg.mozilla.org/integration/mozilla-inbound/rev/5a605f80e8da https://hg.mozilla.org/integration/mozilla-inbound/rev/a56a87f85fbe https://hg.mozilla.org/integration/mozilla-inbound/rev/865a904ad55b https://hg.mozilla.org/integration/mozilla-inbound/rev/0ef0f64644b2 https://hg.mozilla.org/integration/mozilla-inbound/rev/31fd85d6ad5a https://hg.mozilla.org/integration/mozilla-inbound/rev/8ecb31d619af https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5c6f6a8f81 Thank you!
Flags: needinfo?(bugs)
Comment 32•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3ea20343a2ce https://hg.mozilla.org/mozilla-central/rev/5abcc356d54a https://hg.mozilla.org/mozilla-central/rev/3d78ee924b64 https://hg.mozilla.org/mozilla-central/rev/5a605f80e8da https://hg.mozilla.org/mozilla-central/rev/a56a87f85fbe https://hg.mozilla.org/mozilla-central/rev/865a904ad55b https://hg.mozilla.org/mozilla-central/rev/0ef0f64644b2 https://hg.mozilla.org/mozilla-central/rev/31fd85d6ad5a https://hg.mozilla.org/mozilla-central/rev/8ecb31d619af https://hg.mozilla.org/mozilla-central/rev/5d5c6f6a8f81
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment 33•10 years ago
|
||
I landed a trivial GCC (mingw) fixup: https://hg.mozilla.org/integration/mozilla-inbound/rev/cdb9a5b0d4b9
Updated•5 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•