1415409 - Needs to update == operator of RangeBoundaryBase

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

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

In bug 1408544, I made RangeBoundaryBase can store only either mOffset or mRef. So, when comparing two instances, one may have only mOffset and the other may have only mRef. In this case, currently, == operator returns false.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dccff0aebd2b00c31d2ef4e0337aedafac182fb5

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c873cb28d82a40376fcf450e63efbd7dc5090dfb

Comment 3

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

Attachment: Bug 1415409 - Make == operator of RangeBoundaryBase compare mRef and mOffset more carefully

Currently, RangeBoundaryBase can store either only mRef or mOffset.  However,
== operator of RangeBoudaryBase still compares mRef simply.  However, if one
has only mRef and the other has only mOffset, it returns false.

This patch makes == operator checks if both mOffset have been set.  If so,
this checks both mOffset.value() and mRef are matched.  However, if mRef of
only one of them is nullptr, this doesn't make it check mRef because computing
mRef needs some cost and initializing mRef from the other fixes the referring
child stronger.  If the user of the operator sets only mOffset and wait DOM
tree changes, computing mRef may break such callers.

If one has only mRef and the other has only mOffset, this patch makes it
compute mRef.  This is not the best behavior, perhaps.  However, there is no
way to compare these instances.  If this becomes a problem, we should make it
create temporary instance, but it'll waste runtime cost.  So, let's avoid using
this approach for now.

Finally, making it check both mRef simply.

Review commit: https://reviewboard.mozilla.org/r/197560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197560/

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8926287 [details]
Bug 1415409 - Make == operator of RangeBoundaryBase compare mRef and mOffset more carefully

https://reviewboard.mozilla.org/r/197560/#review202816


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

Comment 5

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

catalinb:

This fix is really necessary for fixing remaining bugs of bug 1414710. I really want to take them in 58 because they fixes some  new regressions of the release. So, could you check this patch as soon as possible?

Comment 6

[Cătălin Badea (:catalinb)]

Comment on attachment 8926287 [details]
Bug 1415409 - Make == operator of RangeBoundaryBase compare mRef and mOffset more carefully

https://reviewboard.mozilla.org/r/197560/#review202834

Comment 7

[Cătălin Badea (:catalinb)]

Comment on attachment 8926287 [details]
Bug 1415409 - Make == operator of RangeBoundaryBase compare mRef and mOffset more carefully

https://reviewboard.mozilla.org/r/197560/#review203670

lgtm. sorry for the delay

Comment 8

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

Thank you very much!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e6d62124ec0
Make == operator of RangeBoundaryBase compare mRef and mOffset more carefully r=catalinb

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4e6d62124ec0