1741148 - `nsINode::ComputeIndexOf` should return `uint32_t`

Status: RESOLVED FIXED

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

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(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.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

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

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1741148 - part 1: Rename `nsINode::ComputeIndexOf` to `ComputeIndexOf_Deprecated` r=smaug!

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

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1741148 - part 2: Re-implement `nsINode::ComputeIndexOf` as returning `Maybe<uint32_t>` rather than `int32_t` r=smaug!

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

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: 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!

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

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1741148 - part 4: Add `nsINode::ComputeIndexInParentNode()` and `nsINode::ComputeIndexInParentContent()` r=smaug!

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

Depends on D131336

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1741148 - part 5: Make `nsINode::CompareDocumentPosition()` and `nsContentUtils::PositionIsBefore()` treat offset in DOM node with `Maybe<uint32_t>` r=smaug!

Depends on D131337

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1741148 - part 6: Make `nsContentUtils::GetInclusiveAncestorsAndOffsets()` return array of `Maybe<uint32_t>` as offsets in DOM nodes r=smaug!

Depends on D131338

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1741148 - part 7: Make `AdjustRangeForSelection()` in `ContentEventHandler.cpp` use `Maybe<uint32_t>` for treating offset in a DOM node r=smaug!

Depends on D131339

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1741148 - part 8: Make `TextTrackManager` treat offset in DOM node with `Maybe<uint32_t>` r=smaug!

Depends on D131340

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1741148 - part 9: Make `nsLayoutUtils::DoCompareTreePosition()` stop using `nsINode::ComputeIndexOf_Deprecated()` r=smaug!

Depends on D131341

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1741148 - part 10: Make `nsIFrame::ContentIndexInContainer()` return `Maybe<uint32_t>` for index of a DOM node r=smaug!

Depends on D131342

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1741148 - part 11: Make `ContentEventHandler` use `nsINode::ComputeIndexOf()` r=smaug!

Depends on D131344

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b644587bf3fd
part 1: Rename `nsINode::ComputeIndexOf` to `ComputeIndexOf_Deprecated` r=smaug

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4aa928849867
part 4: Add `nsINode::ComputeIndexInParentNode()` and `nsINode::ComputeIndexInParentContent()` r=smaug

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e82a69a2edc
part 8: Make `TextTrackManager` treat offset in DOM node with `Maybe<uint32_t>` r=smaug

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4212569de1be
part 9: Make `nsLayoutUtils::DoCompareTreePosition()` stop using `nsINode::ComputeIndexOf_Deprecated()` r=smaug

Comment 23

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/b644587bf3fd
https://hg.mozilla.org/mozilla-central/rev/ee5f49423ff5
https://hg.mozilla.org/mozilla-central/rev/5fce50b4cf88
https://hg.mozilla.org/mozilla-central/rev/4aa928849867
https://hg.mozilla.org/mozilla-central/rev/1eebd7413964

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20af5ad7b7cf
part 11: Make `ContentEventHandler` use `nsINode::ComputeIndexOf()` r=smaug

Comment 26

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8353ad6df753
https://hg.mozilla.org/mozilla-central/rev/c39bc3629e9c
https://hg.mozilla.org/mozilla-central/rev/3e82a69a2edc
https://hg.mozilla.org/mozilla-central/rev/4212569de1be
https://hg.mozilla.org/mozilla-central/rev/4a88e5e7aaaa
https://hg.mozilla.org/mozilla-central/rev/20af5ad7b7cf