Closed Bug 1740853 Opened 9 months ago Closed 8 months ago

nsContentUtils::ComparePoints should use `uint32_t` for offset in DOM node

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
97 Branch
Tracking Status
firefox97 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Depends on 2 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

nsContentUtils::ComparePoints should take uint32_t for DOM node offset to handle all possible values.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED

They are defined as "unsigned long" by the standards. So we should use
uint32_t rather than int32_t with the methods. However, layout code
uses int32_t a lot for representing the offset. Therefore, this patch
adds *_FixOffset1 etc for the cases which cannot fix easily in this patch.

Depends on D131034

It takes text node's offset that is never larger than INT32_T because
nsTextFragment limits max text length of a text node is
<= NS_MAX_TEXT_FRAGMENT_LENGTH, that means equals or less than 0x1FFFFFFF.
So that it must be safe to convert the offset to uint32_t from int32_t.

See Also: → 1741148

Masayuki: thanks for working on this.

Are the patches belonging to this ticket intended to land before the ones in bug 1735446? Knowing that would allow to optimize my reviewing efforts. If so, it'd be helpful mark this ticket as blocking the other.

(In reply to Mirko Brodesser (:mbrodesser) from comment #3)

Masayuki: thanks for working on this.

Are the patches belonging to this ticket intended to land before the ones in bug 1735446? Knowing that would allow to optimize my reviewing efforts. If so, it'd be helpful mark this ticket as blocking the other.

They should be landed in same Nightly build if possible, but for mozregressions, we should land each one separately in autoland.

Depends on: 1735446
See Also: 1735446

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #4)

(In reply to Mirko Brodesser (:mbrodesser) from comment #3)

Masayuki: thanks for working on this.

Are the patches belonging to this ticket intended to land before the ones in bug 1735446? Knowing that would allow to optimize my reviewing efforts. If so, it'd be helpful mark this ticket as blocking the other.

They should be landed in same Nightly build if possible, but for mozregressions, we should land each one separately in autoland.

Thanks for the response, but it didn't answer the question fully. Since the patches of bug 1735446 depend on this bug's patches, it seems reasonable to land the patches of this ticket first. Please let me know if you intend doing that.

(In reply to Mirko Brodesser (:mbrodesser) from comment #5)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #4)

(In reply to Mirko Brodesser (:mbrodesser) from comment #3)

Masayuki: thanks for working on this.

Are the patches belonging to this ticket intended to land before the ones in bug 1735446? Knowing that would allow to optimize my reviewing efforts. If so, it'd be helpful mark this ticket as blocking the other.

They should be landed in same Nightly build if possible, but for mozregressions, we should land each one separately in autoland.

Thanks for the response, but it didn't answer the question fully. Since the patches of bug 1735446 depend on this bug's patches, it seems reasonable to land the patches of this ticket first. Please let me know if you intend doing that.

I think they (and bug 1741148) depend on each other. It does not make sense to reshuffle the order of patches since it may require a lot of both machinery and manual merge which may cause unexpected failures and these patches should be landed at same Nightly build as far as possible.

Blocks: 1741148
See Also: 1741148

This patch changes the signed offset of nsContentUtils::ComparePoints_Fix*()s
to Maybe<uint32_t>, and adds a helper method to getting the "maybe 0 or
positive offset value.

Note that I found a bug in AccessibleCaretManager, that is it calls
nsContentUtils_FixOffset2() in 2 places, but aOffset1 is not guaranteed that
its value is 0 or positive. Therefore, this patch changes them to
nsContentUtils_FixBothOffsets().

Depends on D131470

Attachment #9252824 - Attachment is obsolete: true

For reducing the legacy behavior emulator of nsContentUtils::ComparePoints
and make it simpler, this patch makes it take int64_t as the offset.
Additionally, it's named "ComparePoints_AllowNegativeOffsets`.

Depends on D131470

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/385a7ae7ebb4
part 1: Make `nsContentUtils::ComparePoints` take `uint32_t` for offset in DOM nodes r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/33469f58d744
part 2: Make `HyperTextAccessible::GetSpellTextAttr` take `uint32_t` for offset in a text node r=Jamie
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d6422d3053bf
part 3: Redesign `nsContentUtils::ComparePoints_Fix*()` as taking `int64_t` for the offset r=smaug
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch
You need to log in before you can comment on or make changes to this bug.