Should not truncate input string between a surrogate pair at handling maxlength

RESOLVED FIXED in mozilla33

Status

()

Core
Editor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks: 1 bug, {intl})

Trunk
mozilla33
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

This is a part of bug 670837. At least, we shouldn't separate a surrogate pair at handling maxlength in editor.

Note that the goal of this bug isn't an ideal goal of this issue. For example, coming patch won't care IVS.
Created attachment 8441466 [details] [diff] [review]
part.1 Add tests for inputting surrogate pair character

Simon, this tests must represent what you mentioned in bug 670837 comment 8.

This drops high surrogate if the max length boundary causes separating a surrogate pair.
Attachment #8441466 - Flags: feedback?(smontagu)
Created attachment 8441469 [details] [diff] [review]
part.2 Shouldn't separate input string between a surrogate pair at handling maxlength

And the actual logic to drop high surrogate which is remaining by truncating input string in editor.

I'll request review to Ehsan if Simon agree with this behavior.
Attachment #8441469 - Flags: review?(smontagu)

Comment 3

4 years ago
I think this behavior is fine.
Comment on attachment 8441466 [details] [diff] [review]
part.1 Add tests for inputting surrogate pair character

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

feedback+ on the behaviour, but I would like the tests better if you used a helper function instead of repeating so much.
Attachment #8441466 - Flags: feedback?(smontagu) → feedback+
Attachment #8441469 - Flags: review?(smontagu) → review+
(In reply to Simon Montagu :smontagu from comment #4)
> Comment on attachment 8441466 [details] [diff] [review]
> part.1 Add tests for inputting surrogate pair character
> 
> Review of attachment 8441466 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> feedback+ on the behaviour, but I would like the tests better if you used a
> helper function instead of repeating so much.

Thanks, I'll recreate it.
Created attachment 8442835 [details] [diff] [review]
part.1 Add tests for inputting surrogate pair character

Updated the tests and add some more tests.

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=b349f0e6abf7
Attachment #8441466 - Attachment is obsolete: true
Attachment #8441469 - Flags: review?(ehsan)
Attachment #8442835 - Flags: review?(ehsan)

Comment 7

4 years ago
Comment on attachment 8441469 [details] [diff] [review]
part.2 Shouldn't separate input string between a surrogate pair at handling maxlength

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

Also, please update the comment earlier in this function.

::: editor/libeditor/text/nsTextEditRules.cpp
@@ +1249,5 @@
>      {
> +      int32_t oldLength = aOutString->Length();
> +      if (oldLength + resultingDocLength > aMaxLength) {
> +        int32_t newLength = aMaxLength - resultingDocLength;
> +        if (newLength > 0) {

Based on the if condition above, this should always be true, so you can just MOZ_ASSERT it.

@@ +1260,5 @@
> +          }
> +          // XXX What should we do if we're removing IVS and its preceding
> +          //     character won't be removed?
> +        }
> +        MOZ_ASSERT(newLength >= 0);

And then you can remove this MOZ_ASSERT.
Attachment #8441469 - Flags: review?(ehsan) → review+

Comment 8

4 years ago
Comment on attachment 8442835 [details] [diff] [review]
part.1 Add tests for inputting surrogate pair character

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

::: editor/libeditor/text/tests/test_bug1026397.html
@@ +18,5 @@
> +<pre id="test">
> +<script type="application/javascript">
> +
> +/** Test for Bug 1026397 **/
> +SimpleTest.expectAssertions(10);

Please annotate this with a bug # about the assertions.
Attachment #8442835 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/be72c3a82c8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd91cb6f8ee3

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #8)
> Comment on attachment 8442835 [details] [diff] [review]
> part.1 Add tests for inputting surrogate pair character
> 
> Review of attachment 8442835 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: editor/libeditor/text/tests/test_bug1026397.html
> @@ +18,5 @@
> > +<pre id="test">
> > +<script type="application/javascript">
> > +
> > +/** Test for Bug 1026397 **/
> > +SimpleTest.expectAssertions(10);
> 
> Please annotate this with a bug # about the assertions.

Filed bug 1028485. Although, I already have a patch for it, I'd like to create automated tests for it.
https://hg.mozilla.org/mozilla-central/rev/be72c3a82c8b
https://hg.mozilla.org/mozilla-central/rev/bd91cb6f8ee3
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.