Closed
Bug 1408544
Opened 5 years ago
Closed 5 years ago
Use RawRangeBoundary in EditorDOMPoint in order to reduce calls to nsINode::GetChildAt() in EditorBase::GetNodeAtRangeOffsetPoint()
Categories
(Core :: DOM: Editor, defect, P3)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: masayuki)
References
Details
Attachments
(2 files, 1 obsolete file)
No description provided.
Reporter | ||
Comment 1•5 years ago
|
||
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...
Updated•5 years ago
|
Priority: -- → P3
Assignee | ||
Comment 2•5 years 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
Assignee | ||
Comment 3•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4c81c437d15c05d15fcadb3d75bc1bbc13e1514
Assignee | ||
Comment 4•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e262ae038a32b16a4773ee72366902089efed8c
Assignee | ||
Comment 5•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acbd7cbd9c3ed15e21fa7958e192a823b5e45649
Assignee | ||
Comment 6•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecb659f16ee4ae617d10959b7c25ff856489106c
Assignee | ||
Comment 7•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c67e3e1daf9c626a9f19c305de9616b2b028b57
Assignee | ||
Comment 8•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=32bd88be6db5346053cef285146e800705a8a1cf
Assignee | ||
Comment 9•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=590986adf090c074d336d90e019e074bbe898733
Assignee | ||
Comment 10•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=828bbef98832a7cda1ffa0089e6205d7d2cce2ee
Assignee | ||
Comment 11•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5cdc9df4ca1fb8dcc0953da66042f59d9c8a5f2
Assignee | ||
Comment 12•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9113309ad1482ecfce03cd04574e43d638fe03b5
Assignee | ||
Comment 13•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d35142b94449be68fb60bb231108682691cdb14
Assignee | ||
Comment 14•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a2287c63a830ba2cc4d1a83d7bb57abf8025908
Assignee | ||
Comment 15•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecf48f1ec198d370cb247d9442b8750d123bce82
Assignee | ||
Comment 16•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=317ab799abbbfea73b0ac54cc92fb39745e695a7
Assignee | ||
Comment 17•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4565d4717890aef28406c6c38cc61ce04e6e90b
Assignee | ||
Comment 18•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29c26744b0b3917f325eac2c1404d084cb2afe6c
Assignee | ||
Comment 19•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d6beffeeea46945eeedf3663c64d80ebbeba92a
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•5 years ago
|
Attachment #8918448 -
Attachment is obsolete: true
Assignee | ||
Comment 22•5 years ago
|
||
I'd like you to review the patches: m_kato: editor part catalinb: RangeBoundary changes
Comment 23•5 years 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•5 years 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•5 years 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•5 years 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.
Assignee | ||
Comment 27•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a22f3c96b7f3a46f5f104d73c6801fd0169ec3e
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 30•5 years 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•5 years 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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f03a38c18b12 https://hg.mozilla.org/mozilla-central/rev/2a2bb9c3b9a8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•