Closed
Bug 1375502
Opened 7 years ago
Closed 7 years ago
ContentEventHandler shouldn't use nsRange for representing two DOM points
Categories
(Core :: DOM: UI Events & Focus Handling, enhancement)
Core
DOM: UI Events & Focus Handling
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: masayuki, Assigned: masayuki)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
59 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
ContentEventHandler creates nsRange for representing 2 DOM points which are computed from offset and length of flatten text. Then, the range is used for passing other methods to compute something of the range, e.g., rect of the text. ContentEventHandler should have a local class to represent 2 DOM points. Instead of expensive nsRange.
Assignee | ||
Comment 1•7 years ago
|
||
Hmm, I'm writing a patch for this bug as mats suggested. However, ContentEventHandler uses nsIContentIterator::Init(nsRange*) a lot. So, I need to change nsIContentIterator first. Then, the class needs to be a public class. I think that we need a non-local class anyway. So, I'd like to create dom/base/RangeData.(h|cpp) (and existing RangeData should be renamed to RangeAndStyle or something). How about this issue and do you think that it should be created with duplicated code? (I mean, not touching nsRange for now?)
Flags: needinfo?(mats)
Comment 2•7 years ago
|
||
Not touching nsRange now is fine. I wonder if it shouldn't be called RangeData... one possibility is StaticRange, since that is something proposed in the editing task force (or whatever the group is called @w3c). If nsIContentIterator takes a StaticRange as a param, should it be allowed to be used on stack only, since DOM mutations would invalidate the StaticRange? Perhaps add a new class extending nsContentIterator which takes StaticRange in constructor and this new class could be used on stack only?
Comment 3•7 years ago
|
||
> ContentEventHandler uses nsIContentIterator::Init(nsRange*) a lot
Well, seven times to be precise. ;-)
Adding a nsIContentIterator::Init(nsINode* aStartParent, uint32_t aStartOffset,
nsINode* aEndParent, uint32_t aEndOffset)
is an alternative too...
Flags: needinfo?(mats)
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #2) > Not touching nsRange now is fine. > I wonder if it shouldn't be called RangeData... one possibility is > StaticRange, since that is something proposed in the editing task force (or > whatever the group is called @w3c). Oh, thanks. Looks like almost same as what I need. However, it still checks the start position is same as or less than new end position. So, it needs to run nsContentUtils::ComparePoints() but it some times appears in profile but in some cases the validness is guaranteed by the caller. So, I need rawer class of StaticRange too. > If nsIContentIterator takes a StaticRange as a param, should it be allowed > to be used on stack only, since DOM mutations would invalidate the > StaticRange? Ideally, yes. However, nsContentSubtreeIterator has mRange: https://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/dom/base/nsContentIterator.cpp#1185,1227 And it may be used in Prev(): https://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/dom/base/nsContentIterator.cpp#1470,1491,1513,1524,1544 However, I don't think that it needs to listen mutation because using content iterator across some DOM mutation doesn't makes sense and actually instances are used soon. So, there is a dilemma, it *should* listen mutation, but nobody requests it. Perhaps, nsFilteredContentIterator is also similar but the user of it is complicated. So, I'm not sure. > Perhaps add a new class extending nsContentIterator which takes StaticRange > in constructor and this new class could be used on stack only? That may be a good solution. But it needs not small work. So, perhaps, adding new Init() method is simpler here as mats said. (In reply to Mats Palmgren (:mats) from comment #3) > > ContentEventHandler uses nsIContentIterator::Init(nsRange*) a lot > > Well, seven times to be precise. ;-) > > Adding a nsIContentIterator::Init(nsINode* aStartParent, uint32_t > aStartOffset, > nsINode* aEndParent, uint32_t aEndOffset) > is an alternative too... Yeah, it looks ugly but a realistic solution for now... (Although, if we make new class public, we can create new Init() with the new class.)
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a2d638bf09a7f1b7b11428cfb7a4b9010ffc439
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d851b46c70c7d0399267e7cf1d950e7c1f026878
Assignee | ||
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7c4975b279e8df43d5c94d8ba49ad0812083e58
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be572c1d5c39b3bb528a3128a142ed36a77f6855
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=76c0b3e4b589eda5caa9af162e92e4aadd3b4639
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8882517 [details] Bug 1375502 - part1: ContentEventHandler shouldn't use nsRange for temporary use https://reviewboard.mozilla.org/r/152200/#review159200 This looks good overall, but please fix these nits: ::: dom/events/ContentEventHandler.h:46 (Diff revision 1) > { > +private: > + /** > + * RawRange is a helper class of ContentEventHandler class. It just stores > + * two DOM points in a document. This doesn't allow to set reversed range > + * in debug build but not checked in opt build due to performance reason. The second sentence could be clearer. Perhaps something like this instead: The caller is responsible for making sure the start/end nodes are in document order. This is enforced by assertions in DEBUG builds. ::: dom/events/ContentEventHandler.h:54 (Diff revision 1) > + { > + public: > + RawRange() > + : mRoot(nullptr) > + , mStartParent(nullptr) > + , mEndParent(nullptr) These three are unnecessary, please remove. ::: dom/events/ContentEventHandler.h:56 (Diff revision 1) > + RawRange() > + : mRoot(nullptr) > + , mStartParent(nullptr) > + , mEndParent(nullptr) > + , mStartOffset(0) > + , mEndOffset() This should be mEndOffset(0). ::: dom/events/ContentEventHandler.h:101 (Diff revision 1) > + nsCOMPtr<nsINode> mStartParent; > + nsCOMPtr<nsINode> mEndParent; I'd prefer "container" in the names instead of "parent" because that's what the spec is using: https://dom.spec.whatwg.org/#interface-range (I filed bug 1377989 on renaming nsRange::mStartParent/mEndParent) (Fwiw, it's also what StaticRange is using: https://w3c.github.io/staticrange/#interface-staticrange ) ::: dom/events/ContentEventHandler.h:104 (Diff revision 1) > + > + nsCOMPtr<nsINode> mRoot; > + nsCOMPtr<nsINode> mStartParent; > + nsCOMPtr<nsINode> mEndParent; > + int32_t mStartOffset; > + int32_t mEndOffset; Let's use the right type from the start here, which is uint32_t since we never allow negative values. Unfortunately, nsRange is using int32_t, but we should really fix that too so that it follows the spec: https://dom.spec.whatwg.org/#interface-range (I filed bug 1377978 about that) ::: dom/events/ContentEventHandler.h:333 (Diff revision 1) > nsresult QueryContentRect(nsIContent* aContent, > WidgetQueryContentEvent* aEvent); > // Make the DOM range from the offset of FlatText and the text length. > // If aExpandToClusterBoundaries is true, the start offset and the end one are > // expanded to nearest cluster boundaries. > - nsresult SetRangeFromFlatTextOffset(nsRange* aRange, > + nsresult SetRawRangeFromFlatTextOffset(RawRange& aRawRange, I'd prefer the type to be "RawRange*" to make it clear this is an [in]outparam at the call site. ::: dom/events/ContentEventHandler.cpp:56 (Diff revision 1) > + aOffset >= 0 && > + static_cast<size_t>(aOffset) <= aParent->Length(); > +} > + > +nsINode* > +ContentEventHandler::RawRange::IsValidBoundary(nsINode* aNode) const You're also adding an almost identical copy in part 2, so now we have three copies. This is bad for code maintenance. Please add an external function instead: nsINode* IsValidBoundary(nsINode* aNode, bool aMaySpanAnonymousSubtrees); in nsRange.h (in "mozilla" namespace) or nsContentUtils.h for example. Then make nsRange, RawRange and nsContentIterator call that function. ::: dom/events/ContentEventHandler.cpp:158 (Diff revision 1) > + > +nsresult > +ContentEventHandler::RawRange::SetEndAfter(nsINode* aEndParent) > +{ > + int32_t offset = -1; > + nsINode* parent = nsRange::GetParentAndOffsetAfter(aEndParent, &offset); Probably need to check for -1 here after the uint32_t change.
Attachment #8882517 -
Flags: review?(mats) → review-
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8882518 [details] Bug 1375502 - part2: Add nsIContentIterator::Init(nsINode*, uint32_t, nsINode*, uint32_t) https://reviewboard.mozilla.org/r/152202/#review159226 ::: dom/base/nsContentIterator.cpp:113 (Diff revision 1) > > virtual nsresult Init(nsIDOMRange* aRange) override; > > + virtual nsresult Init(nsINode* aStartParent, int32_t aStartOffset, > + nsINode* aEndParent, int32_t aEndOffset, > + bool aAllowToSpanAnonymousSubtrees = false) override; I suggest we use uint32_t instead of int32_t to avoid having to change this later. Please remove the aAllowToSpanAnonymousSubtrees param because we're trying hard to remove that concept and we shouldn't spread it more than absolutely necessary. We can solve this by adding a field instead, nsContentIterator::mMaySpanAnonymousSubtrees, and then initialize that from range->GetMaySpanAnonymousSubtrees() in the two Init methods that takes a range as a param. This should be enough for the needs of nsFind which is the only consumer that sets that flag. Leave nsFind as is as much as possible, i.e. only change nsIDOMNode -> nsINode and nsIDOMRange -> nsRange for now, so that it still uses Init(range) to get the MaySpanAnonymousSubtrees flag across to the iterator. I don't think the ranges in nsFind are particularly important for performance since they don't exist unless the user is using Find. A general comment: please don't do name changes or other casual improvements that aren't related to the functional changes you're doing - it only makes it harder to review the changes since it's not clear which parts are necessary and which are optional. I appreciate additional improvements of course, but those should be in separate patches (or bugs). ::: dom/base/nsContentIterator.cpp:305 (Diff revision 1) > nsContentIterator::Init(nsIDOMRange* aDOMRange) > { > if (NS_WARN_IF(!aDOMRange)) { > return NS_ERROR_INVALID_ARG; > } > nsRange* range = static_cast<nsRange*>(aDOMRange); Here is where I suggest we add: mMaySpanAnonymousSubtrees = range->GetMaySpanAnonymousSubtrees(); ::: dom/base/nsContentIterator.cpp:389 (Diff revision 1) > + mCommonParent = nsContentUtils::GetCommonAncestor(aStartParent, aEndParent); > + if (NS_WARN_IF(!mCommonParent)) { > return NS_ERROR_FAILURE; > } > > - bool startIsData = startNode->IsNodeOfType(nsINode::eDATA_NODE); > + bool startIsData = aStartParent->IsNodeOfType(nsINode::eDATA_NODE); Please don't make these name changes for now. (I also think "container" is a better name than "parent") ::: dom/base/nsContentIterator.cpp:1365 (Diff revision 1) > +nsContentSubtreeIterator::Init(nsINode* aStartParent, int32_t aStartOffset, > + nsINode* aEndParent, int32_t aEndOffset, > + bool aAllowToSpanAnonymousSubtrees) > +{ > + mRange = new nsRange(aStartParent); > + mRange->SetMaySpanAnonymousSubtrees(aAllowToSpanAnonymousSubtrees); This shouldn't be necessary if we keep nsFind as is, i.e. calling Init(range) ::: dom/base/nsIContentIterator.h:37 (Diff revision 1) > virtual nsresult Init(nsIDOMRange* aRange) = 0; > > + /* Initializes an iterator for the subtree between aStartParent - aStartOffset > + and aEndParent - aEndOffset > + Callers should guarantee that the start point is same as or less than > + the end point. It would be good to clarify what "same as or less than" means. I suggest: The start point must be before, or the same as, the end point in document order. ::: dom/base/nsRange.h:105 (Diff revision 1) > > void SetMaySpanAnonymousSubtrees(bool aMaySpanAnonymousSubtrees) > { > mMaySpanAnonymousSubtrees = aMaySpanAnonymousSubtrees; > } > - > + bool GetMaySpanAnonymousSubtrees() const { return mMaySpanAnonymousSubtrees; } It's a bit sad that we must add this to the API. I suggest we make it 'private' and add nsContentIterator as a 'friend', to make sure no new uses are added by mistake. (We want to remove this eventually, once nsFind is fixed to not need this.) ::: editor/txtsvc/nsFilteredContentIterator.cpp:95 (Diff revision 1) > +nsFilteredContentIterator::Init(nsINode* aStartParent, int32_t aStartOffset, > + nsINode* aEndParent, int32_t aEndOffset, > + bool aAllowToSpanAnonymousSubtrees) > +{ > + mRange = new nsRange(aStartParent); > + mRange->SetMaySpanAnonymousSubtrees(aAllowToSpanAnonymousSubtrees); This shouldn't be necesary since nsFind doesn't use this method. ::: toolkit/components/find/nsFind.cpp (Diff revision 1) > - NS_ENSURE_TRUE_VOID(node); > + true /* allow to span anonymouse subtree */); > - > - nsCOMPtr<nsIDOMRange> range = CreateRange(node); > - range->SetStart(mStartNode, mStartOffset); > - range->SetEnd(mEndNode, mEndOffset); > - mOuterIterator->Init(range); This is the call I suggest we keep as is to avoid the aAllowToSpanAnonymousSubtrees param in the new Init methods.
Attachment #8882518 -
Flags: review?(mats) → review-
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8882517 [details] Bug 1375502 - part1: ContentEventHandler shouldn't use nsRange for temporary use https://reviewboard.mozilla.org/r/152200/#review159200 > I'd prefer "container" in the names instead of "parent" because that's what the spec is using: > https://dom.spec.whatwg.org/#interface-range > (I filed bug 1377989 on renaming nsRange::mStartParent/mEndParent) > > (Fwiw, it's also what StaticRange is using: > https://w3c.github.io/staticrange/#interface-staticrange ) Thank you for your review and sorry for delay to reply. Sounds like that for avoiding to make our code more complicated, I should fix bug 1377989 first.
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8882517 [details] Bug 1375502 - part1: ContentEventHandler shouldn't use nsRange for temporary use https://reviewboard.mozilla.org/r/152200/#review159200 > Let's use the right type from the start here, which is uint32_t > since we never allow negative values. > Unfortunately, nsRange is using int32_t, but we should really fix > that too so that it follows the spec: > https://dom.spec.whatwg.org/#interface-range > (I filed bug 1377978 about that) Okay, I'll take this too before this bug.
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8882518 [details] Bug 1375502 - part2: Add nsIContentIterator::Init(nsINode*, uint32_t, nsINode*, uint32_t) https://reviewboard.mozilla.org/r/152202/#review167286 ::: dom/base/nsContentIterator.cpp:111 (Diff revision 1) > > virtual nsresult Init(nsINode* aRoot) override; > > virtual nsresult Init(nsIDOMRange* aRange) override; > > + virtual nsresult Init(nsINode* aStartParent, int32_t aStartOffset, Hello, In bug 651120, I'm trying to drop the use of nsAttrAndChildArray for children storage and use a linked list instead. This change means that IndexOf and GetChildAt will become more expensive. nsContentIterator actually doesn't need to operate on indices and I'm working on another patch to make nsRanges more friendly to node references (1380367). nsContentIterator::Init will do eventually do aParent->GetChildAt(aOffset) so can't we pass the reference node directly here? Anyhow, not adding new code that uses node indices would help me a lot.
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8882517 [details] Bug 1375502 - part1: ContentEventHandler shouldn't use nsRange for temporary use https://reviewboard.mozilla.org/r/152200/#review159200 > You're also adding an almost identical copy in part 2, so now we have three copies. > This is bad for code maintenance. > > Please add an external function instead: > nsINode* IsValidBoundary(nsINode* aNode, bool aMaySpanAnonymousSubtrees); > in nsRange.h (in "mozilla" namespace) or nsContentUtils.h for example. > Then make nsRange, RawRange and nsContentIterator call that function. I'll add nsRange::ComputeRootNode(nsINode*, bool) and nsRange::ComputeRootNode(nsINode*). The former will be shared by nsRange::IsValidBoundary() and the latter.
Assignee | ||
Comment 18•7 years ago
|
||
(In reply to Cătălin Badea (:catalinb) from comment #16) > Comment on attachment 8882518 [details] > Bug 1375502 - part2: Add nsIContentIterator::Init(nsINode*, int32_t, > nsINode*, int32_t) > > https://reviewboard.mozilla.org/r/152202/#review167286 > In bug 651120, I'm trying to drop the use of nsAttrAndChildArray for > children storage and use a linked list instead. This change means that > IndexOf and GetChildAt will become more expensive. nsContentIterator > actually doesn't need to operate on indices and I'm working on another patch > to make nsRanges more friendly to node references (1380367). > > nsContentIterator::Init will do eventually do aParent->GetChildAt(aOffset) > so can't we pass the reference node directly here? > > Anyhow, not adding new code that uses node indices would help me a lot. What the patches try to do here is, replacing Init(nsIDOMRange*) with node and offset in some callers. So, in some places, it could be but it's not a goal of this bug to reduce the runtime cost of nsINode methods. So, I won't add any new code here. (Only nsContentIterator and ContentHandlerHandler part will be treated in this bug.) # Anyway, nsRange's performance is my remaining bugs to improve setting <input>.value performance. So, I cannot avoid to touch around nsINode::IndexOf() users, unfortunately.
Assignee | ||
Comment 19•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=88b29555adb106962e3d29d869af89e38d21316d
Assignee | ||
Comment 20•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=971b7f4153eb3b18ec8e03e537c9a82704dc9f86
Assignee | ||
Comment 21•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ea0da1bb4c88ea559ad56da8b0e4f2915fd1e71
Assignee | ||
Comment 22•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6454cbb55701d7168b3f23af8aa7469a544ce20
Assignee | ||
Comment 23•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8882518 [details] Bug 1375502 - part2: Add nsIContentIterator::Init(nsINode*, uint32_t, nsINode*, uint32_t) https://reviewboard.mozilla.org/r/152202/#review159226 > I suggest we use uint32_t instead of int32_t to avoid having to change > this later. > > Please remove the aAllowToSpanAnonymousSubtrees param because we're trying > hard to remove that concept and we shouldn't spread it more than absolutely > necessary. We can solve this by adding a field instead, > nsContentIterator::mMaySpanAnonymousSubtrees, and then initialize that > from range->GetMaySpanAnonymousSubtrees() in the two Init methods > that takes a range as a param. This should be enough for the needs > of nsFind which is the only consumer that sets that flag. > Leave nsFind as is as much as possible, i.e. only change nsIDOMNode > -> nsINode and nsIDOMRange -> nsRange for now, so that it still > uses Init(range) to get the MaySpanAnonymousSubtrees flag across > to the iterator. > > I don't think the ranges in nsFind are particularly important for > performance since they don't exist unless the user is using Find. > > A general comment: please don't do name changes or other casual > improvements that aren't related to the functional changes you're > doing - it only makes it harder to review the changes since it's > not clear which parts are necessary and which are optional. > > I appreciate additional improvements of course, but those should > be in separate patches (or bugs). Sure, I'll work on nsFind in another bug when I have much time. And new patch completely will get rid of GetMaySpanAnonymousSubtrees(). > Please don't make these name changes for now. > (I also think "container" is a better name than "parent") Okay, I'll remove patches to rename them with "a" prefix. > It's a bit sad that we must add this to the API. > I suggest we make it 'private' and add nsContentIterator as a 'friend', to make sure no new uses are added by mistake. > (We want to remove this eventually, once nsFind is fixed to not need this.) So, this has gone. > This shouldn't be necesary since nsFind doesn't use this method. And this won't appear anymore.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8882517 [details] Bug 1375502 - part1: ContentEventHandler shouldn't use nsRange for temporary use https://reviewboard.mozilla.org/r/152200/#review177632 r=mats ::: dom/base/nsRange.h:321 (Diff revision 2) > nsINode** aFarthestAncestor); > > public: > + /** > + * Compute the root node of aNode for initializing range classes. > + * When aNode is in a subtree, this returns shadow root or binding parent. nits: s/a subtree/an anonymous subtree/ s/this returns shadow/this returns the shadow/ s/Otherwise, returns root/Otherwise, the root/ s/or a document fragment/or document fragment/ s/neither start/neither the start/ s/any ranges/any range/ ::: dom/events/ContentEventHandler.h:24 (Diff revision 2) > +class RawRange; > + Looks like something left over from an earlier version? RawRange is local to ContentEventHandler now, right? ::: dom/events/ContentEventHandler.h:69 (Diff revision 2) > + return IsPositioned() && > + mStartContainer == mEndContainer && > + mStartOffset == mEndOffset; nit: I'd expect IsPositioned() to always be true in practice, so it would slightly faster to test it last. ::: dom/events/ContentEventHandler.h:143 (Diff revision 2) > - // mFirstSelectedRange is the first selected range of mSelection. If > + // mFirstSelectedRawRange is the first selected range of mSelection. If > // mSelection is normal selection, this must not be nullptr if Init() > // succeed. Otherwise, this may be nullptr if there are no selection > // ranges. > - RefPtr<nsRange> mFirstSelectedRange; > + RawRange mFirstSelectedRawRange; The code comment needs to be updated. Something like this? "mFirstSelectedRawRange is initialized from the first range of mSelection, if it exists. Otherwise, it is reset by Clear()." ::: dom/events/ContentEventHandler.h:327 (Diff revision 2) > // Make the DOM range from the offset of FlatText and the text length. > // If aExpandToClusterBoundaries is true, the start offset and the end one are > // expanded to nearest cluster boundaries. > - nsresult SetRangeFromFlatTextOffset(nsRange* aRange, > + nsresult SetRawRangeFromFlatTextOffset(RawRange* aRawRange, Update the code comment: s/Make the DOM range/Initialize aRawRange/ ::: dom/events/ContentEventHandler.cpp:50 (Diff revision 2) > + return aContainer && static_cast<size_t>(aOffset) <= aContainer->Length(); > +} I don't think this static_cast is necessary. ::: dom/events/ContentEventHandler.cpp:66 (Diff revision 2) > + > + if (!IsValidOffset(aStartContainer, aStartOffset)) { > + return NS_ERROR_DOM_INDEX_SIZE_ERR; > + } > + > + // Collapse if not positioned yet, if positioned in another document. nit: s/yet, if/yet, or if/ ::: dom/events/ContentEventHandler.cpp:74 (Diff revision 2) > + MOZ_ASSERT( > + nsContentUtils::ComparePoints(aStartContainer, > + static_cast<int32_t>(aStartOffset), > + mEndContainer, > + static_cast<int32_t>(mEndOffset)) <= 0); > + mStartContainer = aStartContainer; > + mStartOffset = aStartOffset; It seems better to do the assignments first, then the MOZ_ASSERT after it. Ditto in SetEnd and SetStartAndEnd. All three can then be replaced with an AssertStartIsBeforeOrEqualToEnd() convenience method. ::: dom/events/ContentEventHandler.cpp:97 (Diff revision 2) > + > + if (!IsValidOffset(aEndContainer, aEndOffset)) { > + return NS_ERROR_DOM_INDEX_SIZE_ERR; > + } > + > + // Collapse if not positioned yet, if positioned in another document. nit: s/yet, if/yet, or if/ ::: dom/events/ContentEventHandler.cpp:282 (Diff revision 2) > : mPresContext(aPresContext) > , mPresShell(aPresContext->GetPresShell()) > , mSelection(nullptr) > - , mFirstSelectedRange(nullptr) > , mRootContent(nullptr) > { nit: we can remove mSelection(nullptr) and mRootContent(nullptr) in the ctor too, since they are both smart pointers.
Attachment #8882517 -
Flags: review?(mats) → review+
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8882518 [details] Bug 1375502 - part2: Add nsIContentIterator::Init(nsINode*, uint32_t, nsINode*, uint32_t) https://reviewboard.mozilla.org/r/152202/#review177662 ::: dom/base/nsContentIterator.cpp:315 (Diff revision 2) > } > > nsresult > nsContentIterator::Init(nsIDOMRange* aDOMRange) > { > + mIsDone = false; It's a bit weird to leave mIsDone = false for all the error cases IMO, but that's what the current code does for the most part so let's leave it at that. Might be worth filing a follow-up bug to clean this up though. (i.e. make mIsDone = true if the initialization fails in some way so that any attempt to use the iterator after that fails safely) ::: dom/base/nsContentIterator.cpp:1265 (Diff revision 2) > + * Callers must guarantee that mRange isn't nullptr and it's positioned. > + */ nit: s/it's/is/ ::: dom/base/nsContentIterator.cpp:1351 (Diff revision 2) > + mIsDone = false; > + (same comment here about mIsDone) ::: dom/base/nsIContentIterator.h:35 (Diff revision 2) > Subclasses should make sure they implement both of these! > */ > virtual nsresult Init(nsIDOMRange* aRange) = 0; > > + /* Initializes an iterator for the subtree between > + aStartContainer - aStartOffset and aEndContainer - aEndOffset nit: probably slightly easier to read if use / instead of - ::: dom/base/nsRange.h:332 (Diff revision 2) > + * Return true if aStartContainer - aStartOffset and > + * aEndContainer - aEndOffset are valid start and end points for a range. ditto ::: dom/base/nsRange.cpp:1271 (Diff revision 2) > + // Use NS_WARN_IF() only for the cases that the arguments are explicitly odd. > + if (NS_WARN_IF(!aStartContainer) || NS_WARN_IF(!aEndContainer) || nit: s/that the/where the/ s/explicitly odd/unexpected/ ::: dom/base/nsRange.cpp:1282 (Diff revision 2) > + if (ComputeRootNode(aStartContainer) != ComputeRootNode(aEndContainer)) { > + return false; > + } I wonder if we can skip this check if we instead require that 'disconnected' is false after the ComparePoints call? ::: dom/base/nsRange.cpp:1287 (Diff revision 2) > + if (nsContentUtils::ComparePoints(aStartContainer, > + static_cast<int32_t>(aStartOffset), > + aEndContainer, > + static_cast<int32_t>(aEndOffset), > + &disconnected) == 1) { > + return false; > + } > + > + return true; It might be easier to read this if we assign the result to a variable first, e.g. int32_t order = nsContentUtils::ComparePoints(...); return order != 1 && !disconnected; ::: editor/txtsvc/nsFilteredContentIterator.cpp:112 (Diff revision 2) > + if (NS_WARN_IF(range->GetStartContainer() != aStartContainer) || > + NS_WARN_IF(range->GetEndContainer() != aEndContainer) || > + NS_WARN_IF(range->StartOffset() != aStartOffset) || > + NS_WARN_IF(range->EndOffset() != aEndOffset)) { > + return NS_ERROR_UNEXPECTED; > + } I don't see why we need to test this. Can we assert it instead?
Attachment #8882518 -
Flags: review?(mats) → review+
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8897847 [details] Bug 1375502 - part3: Rename startNode of nsContentIterator::InitInternal() to aStartContainer https://reviewboard.mozilla.org/r/169140/#review177664
Attachment #8897847 -
Flags: review?(mats) → review+
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8897848 [details] Bug 1375502 - part4: Rename endNode of nsContentIterator::InitInternal() to aEndContainer https://reviewboard.mozilla.org/r/169142/#review177666
Attachment #8897848 -
Flags: review?(mats) → review+
Comment 35•7 years ago
|
||
mozreview-review |
Comment on attachment 8897849 [details] Bug 1375502 - part5: Rename startIndx of nsContentIterator::InitInternal() to aStartOffset https://reviewboard.mozilla.org/r/169144/#review177668
Attachment #8897849 -
Flags: review?(mats) → review+
Comment 36•7 years ago
|
||
mozreview-review |
Comment on attachment 8897850 [details] Bug 1375502 - part6: Rename endIndx of nsContentIterator::InitInternal() to aEndOffset https://reviewboard.mozilla.org/r/169146/#review177670
Attachment #8897850 -
Flags: review?(mats) → review+
Updated•7 years ago
|
Flags: needinfo?(mats)
Assignee | ||
Comment 37•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8882517 [details] Bug 1375502 - part1: ContentEventHandler shouldn't use nsRange for temporary use https://reviewboard.mozilla.org/r/152200/#review177632 > Looks like something left over from an earlier version? > RawRange is local to ContentEventHandler now, right? Oh, right. > The code comment needs to be updated. Something like this? > "mFirstSelectedRawRange is initialized from the first range of mSelection, if it exists. Otherwise, it is reset by Clear()." Thank you for suggesting the new comment!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 44•7 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/5f774cd37780 part1: ContentEventHandler shouldn't use nsRange for temporary use r=mats https://hg.mozilla.org/integration/autoland/rev/98ea23c3d283 part2: Add nsIContentIterator::Init(nsINode*, uint32_t, nsINode*, uint32_t) r=mats https://hg.mozilla.org/integration/autoland/rev/ad0e99eaa19a part3: Rename startNode of nsContentIterator::InitInternal() to aStartContainer r=mats https://hg.mozilla.org/integration/autoland/rev/0520af4d2204 part4: Rename endNode of nsContentIterator::InitInternal() to aEndContainer r=mats https://hg.mozilla.org/integration/autoland/rev/678408c0a0df part5: Rename startIndx of nsContentIterator::InitInternal() to aStartOffset r=mats https://hg.mozilla.org/integration/autoland/rev/a1c5a363c929 part6: Rename endIndx of nsContentIterator::InitInternal() to aEndOffset r=mats
Comment 45•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5f774cd37780 https://hg.mozilla.org/mozilla-central/rev/98ea23c3d283 https://hg.mozilla.org/mozilla-central/rev/ad0e99eaa19a https://hg.mozilla.org/mozilla-central/rev/0520af4d2204 https://hg.mozilla.org/mozilla-central/rev/678408c0a0df https://hg.mozilla.org/mozilla-central/rev/a1c5a363c929
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•5 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•