Closed Bug 1741148 Opened 3 years ago Closed 3 years ago

`nsINode::ComputeIndexOf` should return `uint32_t`

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
97 Branch
Tracking Flags:
Tracking Status
firefox97 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(11 files)

Bug 1741148 - part 1: Rename `nsINode::ComputeIndexOf` to `ComputeIndexOf_Deprecated` r=smaug!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1741148 - part 2: Re-implement `nsINode::ComputeIndexOf` as returning `Maybe<uint32_t>` rather than `int32_t` r=smaug!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1741148 - part 3: Make users of `nsINode::ComputeIndexOf_Deprecated()` use `nsINode::ComputeIndexOf()` if the result is not set to `int32_t` nor return as `int32_t` r=smaug!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1741148 - part 4: Add `nsINode::ComputeIndexInParentNode()` and `nsINode::ComputeIndexInParentContent()` r=smaug!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1741148 - part 5: Make `nsINode::CompareDocumentPosition()` and `nsContentUtils::PositionIsBefore()` treat offset in DOM node with `Maybe<uint32_t>` r=smaug!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1741148 - part 6: Make `nsContentUtils::GetInclusiveAncestorsAndOffsets()` return array of `Maybe<uint32_t>` as offsets in DOM nodes r=smaug!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1741148 - part 7: Make `AdjustRangeForSelection()` in `ContentEventHandler.cpp` use `Maybe<uint32_t>` for treating offset in a DOM node r=smaug!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1741148 - part 8: Make `TextTrackManager` treat offset in DOM node with `Maybe<uint32_t>` r=smaug!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1741148 - part 9: Make `nsLayoutUtils::DoCompareTreePosition()` stop using `nsINode::ComputeIndexOf_Deprecated()` r=smaug!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1741148 - part 10: Make `nsIFrame::ContentIndexInContainer()` return `Maybe<uint32_t>` for index of a DOM node r=smaug!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1741148 - part 11: Make `ContentEventHandler` use `nsINode::ComputeIndexOf()` r=smaug!
48 bytes, text/x-phabricator-request
Details | Review

(Submitted unexpectedly while writing using autocomplete of the cc field... Sorry for the bug spam)

Perhaps, Maybe<uint32_t> or Result<uint32_t, bool>. I think that the former is better because of consistency with the othe APIs.

Severity: -- → S3
Priority: -- → P3
See Also: → 1740853
Type: task → enhancement
Assignee: nobody → masayuki
Status: NEW → ASSIGNED

We need too big patches for completely fixing this bug, but only fixing simple cases is not so hard.

It's hard to fix some callers. Therefore, in this bug, we should fix only
simple cases. Therefore, we should rename existing API first.

Depends on D131111

Offset in a node is declared as "unsigned long" by the standards and we don't
limit node can have less than INT32_MAX. So it should return uint32_t, but
it also needs to represent the case of "not found". For consistency with some
other APIs like nsContentUtils::ComparePoints, using Maybe must be a good
style rather than Result<uint32_t, bool>.

This patch fixes the callers in assertions for example.

Depends on D131334

This patch fixes only the cases if the result of ComputeIndexOf_Deprecated()
is used as unsigned integer with implicit or explicit cast.

Depends on D131335

Some callers of nsINode::ComputeIndexOf() do not use its parent node except
calling it. These tiny methods make such callers simpler.

Depends on D131336

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b644587bf3fd
part 1: Rename `nsINode::ComputeIndexOf` to `ComputeIndexOf_Deprecated` r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/ee5f49423ff5
part 2: Re-implement `nsINode::ComputeIndexOf` as returning `Maybe<uint32_t>` rather than `int32_t` r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/5fce50b4cf88
part 3: Make users of `nsINode::ComputeIndexOf_Deprecated()` use `nsINode::ComputeIndexOf()` if the result is not set to `int32_t` nor return as `int32_t` r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/4aa928849867
part 4: Add `nsINode::ComputeIndexInParentNode()` and `nsINode::ComputeIndexInParentContent()` r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/1eebd7413964
part 5: Make `nsINode::CompareDocumentPosition()` and `nsContentUtils::PositionIsBefore()` treat offset in DOM node with `Maybe<uint32_t>` r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/8353ad6df753
part 6: Make `nsContentUtils::GetInclusiveAncestorsAndOffsets()` return array of `Maybe<uint32_t>` as offsets in DOM nodes r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/c39bc3629e9c
part 7: Make `AdjustRangeForSelection()` in `ContentEventHandler.cpp` use `Maybe<uint32_t>` for treating offset in a DOM node r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3e82a69a2edc
part 8: Make `TextTrackManager` treat offset in DOM node with `Maybe<uint32_t>` r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/4212569de1be
part 9: Make `nsLayoutUtils::DoCompareTreePosition()` stop using `nsINode::ComputeIndexOf_Deprecated()` r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/4a88e5e7aaaa
part 10: Make `nsIFrame::ContentIndexInContainer()` return `Maybe<uint32_t>` for index of a DOM node r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/20af5ad7b7cf
part 11: Make `ContentEventHandler` use `nsINode::ComputeIndexOf()` r=smaug
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: