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

RESOLVED FIXED in Firefox 58

Status

()

Product:
Core
Component:
Editor
Importance:
P3
normal
Status:
RESOLVED FIXED
Reported:
a year ago
Modified:
a year ago

People

(Reporter: Ehsan, Assigned: masayuki)

Tracking

Version:
unspecified
Target:
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Bug 1408544 - part 1: Reimplement EditorDOMPoint as a subclass of RangeBoundary
59 bytes, text/x-review-board-request
catalinb
: review+
m_kato
: review+
Details
Bug 1408544 - part 2: RangeBoundaryBase shouldn't compute mRef when mOffset is specified
59 bytes, text/x-review-board-request
catalinb
: review+
Details
Comment hidden (empty)
(Reporter)

Comment 1

•
a year 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 year ago
Blocks: 651120
Priority: -- → P3
 (Assignee)

Comment 2

•
a year ago 
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8918448 - Attachment is obsolete: true
 (Assignee)

Comment 22

•
a year ago 
I'd like you to review the patches:

m_kato: editor part
catalinb: RangeBoundary changes

Comment 23

•
a year 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

•
a year 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

•
a year 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

•
a year 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 30

•
a year 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

•
a year 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

Comment 34

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