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)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

Bug 1375502 - part1: ContentEventHandler shouldn't use nsRange for temporary use
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1375502 - part2: Add nsIContentIterator::Init(nsINode*, uint32_t, nsINode*, uint32_t)
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1375502 - part3: Rename startNode of nsContentIterator::InitInternal() to aStartContainer
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1375502 - part4: Rename endNode of nsContentIterator::InitInternal() to aEndContainer
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1375502 - part5: Rename startIndx of nsContentIterator::InitInternal() to aStartOffset
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1375502 - part6: Rename endIndx of nsContentIterator::InitInternal() to aEndOffset
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.
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)
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?
> 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)
(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.)
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 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-
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.
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 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.
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.
(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.
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.
mats: Could you review them?
Flags: needinfo?(mats)
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 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 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 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 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 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+
Flags: needinfo?(mats)
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!
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
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: