Use RawRangeBoundary in EditorDOMPoint in order to reduce calls to nsINode::GetChildAt() in EditorBase::GetNodeAtRangeOffsetPoint()

RESOLVED FIXED in Firefox 58

Status

()

Core
Editor
P3
normal
RESOLVED FIXED
a month ago
3 days ago

People

(Reporter: Away for a while, Assigned: masayuki)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Comment 1

a month ago
Created attachment 8918448 [details] [diff] [review]
Use RawRangeBoundary in EditorDOMPoint in order to reduce calls to nsINode::GetChildAt() in EditorBase::GetNodeAtRangeOffsetPoint()

Unfortunately I didn't have enough time to finish up this patch, it currently has test failures which I didn't have time to debug.  I suspect that the changes to WSRunObject are incorrect and should be reverted since all that code deals with are text nodes where the offset is an offset into the text node not an offset into the child array...
(Reporter)

Updated

a month ago
Blocks: 651120

Updated

a month ago
Priority: -- → P3
For avoiding calling nsINode::IndexOf(), a lot of methods of editor now have an out param for returning a child node.  However, these code are really error prone and actually we have a lot of regressions. So, this bug must be P1.

I'll work on this ASAP.

However, currently, my idea is, EditorDOMPoint should be a subclass of RangeBoundaryBase.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4c81c437d15c05d15fcadb3d75bc1bbc13e1514
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e262ae038a32b16a4773ee72366902089efed8c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acbd7cbd9c3ed15e21fa7958e192a823b5e45649
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecb659f16ee4ae617d10959b7c25ff856489106c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c67e3e1daf9c626a9f19c305de9616b2b028b57
https://treeherder.mozilla.org/#/jobs?repo=try&revision=32bd88be6db5346053cef285146e800705a8a1cf
https://treeherder.mozilla.org/#/jobs?repo=try&revision=590986adf090c074d336d90e019e074bbe898733
https://treeherder.mozilla.org/#/jobs?repo=try&revision=828bbef98832a7cda1ffa0089e6205d7d2cce2ee
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5cdc9df4ca1fb8dcc0953da66042f59d9c8a5f2
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9113309ad1482ecfce03cd04574e43d638fe03b5
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d35142b94449be68fb60bb231108682691cdb14
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a2287c63a830ba2cc4d1a83d7bb57abf8025908
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecf48f1ec198d370cb247d9442b8750d123bce82
https://treeherder.mozilla.org/#/jobs?repo=try&revision=317ab799abbbfea73b0ac54cc92fb39745e695a7
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4565d4717890aef28406c6c38cc61ce04e6e90b
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29c26744b0b3917f325eac2c1404d084cb2afe6c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d6beffeeea46945eeedf3663c64d80ebbeba92a
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8918448 - Attachment is obsolete: true
I'd like you to review the patches:

m_kato: editor part
catalinb: RangeBoundary changes
Blocks: 1414710

Comment 23

19 days ago
mozreview-review
Comment on attachment 8925409 [details]
Bug 1408544 - part 1: Reimplement EditorDOMPoint as a subclass of RangeBoundary

https://reviewboard.mozilla.org/r/195356/#review201778
Attachment #8925409 - Flags: review?(m_kato) → review+

Comment 24

18 days ago
mozreview-review
Comment on attachment 8925409 [details]
Bug 1408544 - part 1: Reimplement EditorDOMPoint as a subclass of RangeBoundary

https://reviewboard.mozilla.org/r/195356/#review201930

This is awesome. Thanks!
Attachment #8925409 - Flags: review?(catalin.badea392) → review+

Comment 25

18 days ago
mozreview-review
Comment on attachment 8925410 [details]
Bug 1408544 - part 2: RangeBoundaryBase shouldn't compute mRef when mOffset is specified

https://reviewboard.mozilla.org/r/195844/#review201932

lgtm. Thanks!

::: dom/base/RangeBoundary.h:379
(Diff revision 1)
> +               "Range is positioned on a text node!");
> +    MOZ_ASSERT(mRef || (mOffset.isSome() && mOffset.value() == 0),
> +               "mRef should be computed before a call of InvalidateOffset()");
> +
> +    if (!mRef) {
> +      MOZ_ASSERT(mOffset.isSome() && mOffset.value() == 0,

This assert and the one above at line 375 are equivalent.

::: dom/base/RangeBoundary.h:387
(Diff revision 1)
> +    }
> +    mOffset.reset();
> +  }
> +
> +  void
> +  EnsureOffset() const

Nit: This is used only once in Offset(). There's no need having a separate method.

::: dom/base/RangeBoundary.h:393
(Diff revision 1)
> +  {
> +    if (mOffset.isSome()) {
> +      return;
> +    }
> +    if (!mParent) {
> +      MOZ_ASSERT(!mOffset.isSome());

This assert can never fail.

::: dom/base/RangeBoundary.h:398
(Diff revision 1)
> +      MOZ_ASSERT(!mOffset.isSome());
> +      MOZ_ASSERT(!mRef);
> +      return;
> +    }
> +    if (!mParent->IsContainerNode()) {
> +      MOZ_ASSERT(mOffset.isSome());

nit: mOffset.isSome() is always false at this point (otherwise the code would have returned on line 390). Maybe change this to a MOZ_ASSERT_UNREACHABLE so that it's obvious this should never happen.

::: dom/base/RangeBoundary.h:411
(Diff revision 1)
> +    }
> +    if (!mRef->GetNextSibling()) {
> +      mOffset = mozilla::Some(mParent->GetChildCount());
> +      return;
> +    }
> +    // Use nsINode::IndexOf() as the last resort due to expensive.

nit: ..due to being expensive.

::: dom/base/RangeBoundary.h:423
(Diff revision 1)
> +    if (mRef) {
> +      return;
> +    }
> +    if (!mParent) {
> +      MOZ_ASSERT(!mOffset.isSome());
> +      MOZ_ASSERT(!mRef);

This assert can never fail.

::: dom/base/RangeBoundary.h:438
(Diff revision 1)
> +    MOZ_ASSERT(mRef);
> +  }
> +
>  private:
>    ParentType mParent;
> -  RefType mRef;
> +  mutable RefType mRef;

This kind of breaks the concept of const RangeBoundary. Before, (mParent, mRef) was the cannonical state of the object, which couldn't be modified from a const reference. Now you can potentially take a const reference and modify its state since that's represented by either (mParent, mRef) or (mParent, mOffset) and both mRef and mOffset are mutable.

I guess this is fine since these are private members and the public API carefully controls how we mutate these members.
Attachment #8925410 - Flags: review?(catalin.badea392) → review+
(Assignee)

Comment 26

18 days ago
mozreview-review-reply
Comment on attachment 8925410 [details]
Bug 1408544 - part 2: RangeBoundaryBase shouldn't compute mRef when mOffset is specified

https://reviewboard.mozilla.org/r/195844/#review201932

> Nit: This is used only once in Offset(). There's no need having a separate method.

Hmm, okay, I move this method code into Offset().

> nit: mOffset.isSome() is always false at this point (otherwise the code would have returned on line 390). Maybe change this to a MOZ_ASSERT_UNREACHABLE so that it's obvious this should never happen.

Ah, right.

Okay, I changed this to:
>     if (!mParent) {
>       MOZ_ASSERT(!mRef);
>       return 0;
>     }
>     MOZ_ASSERT(mParent->IsContainerNode(),
>       "If the container cannot have children, mOffset.isSome() should be true");

It's enough to test only on debug build.

> This kind of breaks the concept of const RangeBoundary. Before, (mParent, mRef) was the cannonical state of the object, which couldn't be modified from a const reference. Now you can potentially take a const reference and modify its state since that's represented by either (mParent, mRef) or (mParent, mOffset) and both mRef and mOffset are mutable.
> 
> I guess this is fine since these are private members and the public API carefully controls how we mutate these members.

Yeah, I tried to use const_cast<RangeBoundaryBase<ParentType, RefType>*>(this)->EnsureRef(). But the line becomes too long and makes the code not easier to read. Therefore, I used mutable.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a22f3c96b7f3a46f5f104d73c6801fd0169ec3e
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 30

18 days ago
mozreview-review
Comment on attachment 8925409 [details]
Bug 1408544 - part 1: Reimplement EditorDOMPoint as a subclass of RangeBoundary

https://reviewboard.mozilla.org/r/195356/#review202086


C/C++ static analysis found 1 defect in this patch.

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

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: editor/libeditor/HTMLEditRules.cpp:6428
(Diff revision 2)
>  {
> -  NS_ENSURE_STATE(aPoint.node);
> -  RefPtr<nsRange> range = new nsRange(aPoint.node);
> -  nsresult rv = range->SetStart(aPoint.node, aPoint.offset);
> +  if (NS_WARN_IF(!aPoint.IsSet())) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +  RefPtr<nsRange> range = new nsRange(aPoint.Container());
> +  nsresult rv = range->SetStart(aPoint.Container(), aPoint.Offset());

Warning: Value stored to 'rv' during its initialization is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 33

18 days ago
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/f03a38c18b12
part 1: Reimplement EditorDOMPoint as a subclass of RangeBoundary r=catalinb,m_kato
https://hg.mozilla.org/integration/autoland/rev/2a2bb9c3b9a8
part 2: RangeBoundaryBase shouldn't compute mRef when mOffset is specified r=catalinb
Blocks: 1415062

Comment 34

18 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f03a38c18b12
https://hg.mozilla.org/mozilla-central/rev/2a2bb9c3b9a8
Status: ASSIGNED → RESOLVED
Last Resolved: 18 days ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1415226
Depends on: 1415409
No longer depends on: 1415226
Depends on: 1419745
You need to log in before you can comment on or make changes to this bug.