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
Type:
defect
Importance:
P3
normal
Status:
RESOLVED FIXED
Opened:
2 years ago
Updated:
2 years 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 
No description provided.
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...
Blocks: 651120
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
I'd like you to review the patches:

m_kato: editor part
catalinb: RangeBoundary changes
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 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 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+
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 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]
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
https://hg.mozilla.org/mozilla-central/rev/f03a38c18b12
https://hg.mozilla.org/mozilla-central/rev/2a2bb9c3b9a8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1415226
You need to log in before you can comment on or make changes to this bug.