This patch is an automatic replacement of s/NS_NOTREACHED/MOZ_ASSERT_UNREACHABLE/. Reindenting long lines and whitespace fixups follow in patch 6b.
MozReview-Commit-ID: 5UQVHElSpCr
This was done automatically replacing:
s/mozilla::Move/std::move/
s/ Move(/ std::move(/
s/(Move(/(std::move(/
Removing the 'using mozilla::Move;' lines.
And then with a few manual fixups, see the bug for the split series..
MozReview-Commit-ID: Jxze3adipUh
nsContentIterator used to maintain a stack of indices so that when it
finished iterating through a subtree it would know the position of the
next node. Maintaining this stack is expensive and unnecessary since we
have fast getters for next and previous siblings.
(Path is actually r=froydnj.)
Bug 1400459 devirtualized nsIAtom so that it is no longer a subclass of
nsISupports. This means that nsAtom is now a better name for it than nsIAtom.
MozReview-Commit-ID: 91U22X2NydP
This patch adds an overload to nsIContentIterator::Init which accepts
RangeBoundary objects, and modifies the codepath to avoid using the offset of
the start or end nodes to construct the nsIContentIterator.
MozReview-Commit-ID: 5ZqKeiUunoN
nsContentIterator used to maintain a stack of indices so that when it
finished iterating through a subtree it would know the position of the
next node. Maintaining this stack is expensive and unnecessary since we
have fast getters for next and previous siblings.
It a stateless wrapper around static methods in nsHTMLTags and nsHTMLElement,
and hence an unnecessary layer of indirection that just adds complexity and
slowness. This patch removes it, cutting almost 300 lines of code.
This requires making nsElementTable.h an exported header, to expose the
nsHTMLElement methods.
nsIContentIterator::Init() takes nsRange but it's too expensive for some users.
So, there should be another Init() which can be specified a range in DOM tree
with 2 pairs of nsINode* and uint32_t.
MozReview-Commit-ID: 6JXic0KOM2d
When the node borders one of the range bounds, `NodeIsInTraversalRange`
should return different results depending on whether it's in pre mode or
not.
> <div><br></div>
> \__/
In this pre mode example, the node <br> is within the range, and the
node position (which is at the start of the node in pre mode) and the
start bound are both (<div>, 0). Therefore, it shows the start bound
should be inclusive in pre mode.
> <div><br></div>
> \___/
In this pre mode example, the node <br> is outside of the range, yet the
node position and the end bound are both (<div>, 0). Therefore, it shows
the end bound should be exclusive in pre mode.
> <div><br></div>
> \____/
in this post mode example, the node <br> is outside of the range, yet
the node position (which is at the end of the node in post mode) and the
start bound are both (<div>, 1). Therefore, it shows the start bound
should be exclusive in post mode.
> <div><br></div>
> \__/
In this post mode example, the node <br> is within the range, and the
node position and the end bound are both (<div>, 1). Therefore, it shows
the end bound should be inclusive in post mode.
In summary, the correct pre mode bound check is `start <= node < end`,
and the correct post mode bound check is `start < node <= end`. This
patch fixes `NodeIsInTraversalRange` to have the correct bounds check.
MozReview-Commit-ID: IjJN1ua6jQ9
When the start node is a non-container node (i.e. <br>), and the start
offset is 0, we should not include a newline character for the node. For
example, for this range,
> <br/>hello
> \___/
the start node/offset is (<br/>, 0) and end node/offset is ("hello", 1).
The calculated range offset should be 0, and the range length should be
2: 1 for the <br/> newline character plus 1 for "h".
The patch also ensures this behavior for pre-mode nsContentIterator, for
both start and end node adjustments. For start nodes, we include any
non-container nodes with offset 0 in the range. For end node, we exclude
any non-container nodes with offset 0 from the range.
MozReview-Commit-ID: Lt2tCLbapq7
This does NOT change variable names like |endNode| because it's not odd and somebody use it for nsINode and endContent for nsIContent. So, changing them needs more work.
MozReview-Commit-ID: 22imUltlu5R
This does NOT change variable names like |startNode| because it's not odd and somebody use it for nsINode and startContent for nsIContent. So, changing them needs more work.
MozReview-Commit-ID: H19pTDprRuT
Bug 1319660 - 1. Don't take shortcut if old replacement ranges don't match; r=esawin
The block at [1] is a shortcut we take when we reconcile Java text
changes with Gecko text changes. However, we only checked that the new
ranges are the same, i.e. that the new Gecko text is the same as the new
Java text. We should also be checking that the old ranges are the same,
i.e. that the replaced Gecko text is the same as the replaced Java text.
[1] https://dxr.mozilla.org/mozilla-central/rev/bbbd2f7539f224a482cc6d2dd10e6a5f31c8baf3/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditable.java#1233
Bug 1319660 - 2. Use previous node instead of sibling when adjusting last node; r=masayuki r=smaug
nsContentIterator in pre mode adjusts its last node if the node is a
childless node like <br>. However, right now it's using GetPrevSibling,
which can lead to error in some edge cases such as:
<p></p><div><br></div>
In this case, if the last node is <br> with offset 0, GetPrevSibling
will return <p> because <p> is <br>'s parent's previous sibling, and the
last node will be set to <p>. However, the correct last node in this
case is <div>, because <br> with offset 0 refers to the position to the
left of <br>, which is <div> with offset 0. In this case, PrevNode
returns the correct <div> value, so we should set the last node to the
result of PrevNode.
For the first node, for a childless node in pre mode, GetNextSibling and
NextNode are the same, so there is no bug in this case. Nevertheless,
this patch changes the call to NextNode to be consistent with calling
PrevNode for the last node.
Bug 1319660 - 3. Add test for correctly adjusting last node in content iterator; r=masayuki
Add a test for the previous patch that makes sure querying selected text
in an edge case works correctly.
Bug 1319660 - 4. Add test for start node regression; r=me
Add a new test case for the NextNode() regression. r=me for trivial
test-only patch.
Bug 1319660 - 5. Restore GetNextSibling call for first node of pre-content-iterator; r=smaug
The last patch changed the `GetNextSibling()` call to `NextNode()`
because I assumed they're equivalent in this case. That turned out to
not be the case because we can reach this line even if the node has
children -- the index just has to be after the last child. So this patch
restores the `GetNextSibling` call to restore the correct behavior.
I also added some comment to clarify that we can reach this line due to
one of two conditions: 1) the node has no children; 2) the node has
children but the index is after the last child.
This patch also replaces the `HasChildren()` check when setting
`cChild`. If the index is after the last child (i.e. index ==
childCount), `GetChildAt()` fails and we erroneously log an assertion
warning, even though the input was valid. The new check handles all
cases whether start node has children or not.
nsContentIterator in pre mode adjusts its last node if the node is a
childless node like <br>. However, right now it's using GetPrevSibling,
which can lead to error in some edge cases such as:
<p></p><div><br></div>
In this case, if the last node is <br> with offset 0, GetPrevSibling
will return <p> because <p> is <br>'s parent's previous sibling, and the
last node will be set to <p>. However, the correct last node in this
case is <div>, because <br> with offset 0 refers to the position to the
left of <br>, which is <div> with offset 0. In this case, PrevNode
returns the correct <div> value, so we should set the last node to the
result of PrevNode.
For the first node, for a childless node in pre mode, GetNextSibling and
NextNode are the same, so there is no bug in this case. Nevertheless,
this patch changes the call to NextNode to be consistent with calling
PrevNode for the last node.
This change avoids lots of false positives for Coverity's CHECKED_RETURN
warning, caused by NS_WARN_IF's current use in both statement-style and
expression-style.
In the case where the code within the NS_WARN_IF has side-effects, I made the
following change.
> NS_WARN_IF(NS_FAILED(FunctionWithSideEffects()));
> -->
> Unused << NS_WARN_IF(NS_FAILED(FunctionWithSideEffects()));
In the case where the code within the NS_WARN_IF lacks side-effects, I made the
following change.
> NS_WARN_IF(!condWithoutSideEffects);
> -->
> NS_WARNING_ASSERTION(condWithoutSideEffects, "msg");
This has two improvements.
- The condition is not evaluated in non-debug builds.
- The sense of the condition is inverted to the familiar "this condition should
be true" sense used in assertions.
A common variation on the side-effect-free case is the following.
> nsresult rv = Fn();
> NS_WARN_IF_(NS_FAILED(rv));
> -->
> DebugOnly<nsresult rv> = Fn();
> NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Fn failed");
Although, this patch is pretty big and complicated, but I have no idea to separate this since this rewrites all over the OnQueryTextRect() with new utility methods which are implemented for OnQueryTextRectArray().
Currently, OnQueryTextRect() doesn't support line breaks before block level elements. Therefore, in HTML editors, eQueryTextRect may not work fine if the range includes open tags of block level elements. This causes odd positioning of IME's candidate window in non-e10s mode.
This implements ContentEventHandler::GetLastFrameHavingFlatTextInRange() which scans the given range from last to first. However, this hits wrong NS_ASSERTION() in nsContentIterator::Last(). mLast can be nullptr if there is no content but Init() returns NS_OK even in such case. I think that returning NS_OK is fine because it's not illegal case. Therefore this patch fixes the wrong assertion.
MozReview-Commit-ID: E6OnIA1eMou
nsContentIterator isn't designed as working fine with a tree whose some nodes are being removed. In such case, NextNode() and PrevNode() meets orphan node (i.e., a node whose parent is nullptr). Then, nsContentIterator should mark it as "done".
However, it should keep crashing if it's debug build for detecting bugs explicitly.
MozReview-Commit-ID: 81ZQgoHD67T
The bulk of this commit was generated with a script, executed at the top
level of a typical source code checkout. The only non-machine-generated
part was modifying MFBT's moz.build to reflect the new naming.
CLOSED TREE makes big refactorings like this a piece of cake.
# The main substitution.
find . -name '*.cpp' -o -name '*.cc' -o -name '*.h' -o -name '*.mm' -o -name '*.idl'| \
xargs perl -p -i -e '
s/nsRefPtr\.h/RefPtr\.h/g; # handle includes
s/nsRefPtr ?</RefPtr</g; # handle declarations and variables
'
# Handle a special friend declaration in gfx/layers/AtomicRefCountedWithFinalize.h.
perl -p -i -e 's/::nsRefPtr;/::RefPtr;/' gfx/layers/AtomicRefCountedWithFinalize.h
# Handle nsRefPtr.h itself, a couple places that define constructors
# from nsRefPtr, and code generators specially. We do this here, rather
# than indiscriminantly s/nsRefPtr/RefPtr/, because that would rename
# things like nsRefPtrHashtable.
perl -p -i -e 's/nsRefPtr/RefPtr/g' \
mfbt/nsRefPtr.h \
xpcom/glue/nsCOMPtr.h \
xpcom/base/OwningNonNull.h \
ipc/ipdl/ipdl/lower.py \
ipc/ipdl/ipdl/builtin.py \
dom/bindings/Codegen.py \
python/lldbutils/lldbutils/utils.py
# In our indiscriminate substitution above, we renamed
# nsRefPtrGetterAddRefs, the class behind getter_AddRefs. Fix that up.
find . -name '*.cpp' -o -name '*.h' -o -name '*.idl' | \
xargs perl -p -i -e 's/nsRefPtrGetterAddRefs/RefPtrGetterAddRefs/g'
if [ -d .git ]; then
git mv mfbt/nsRefPtr.h mfbt/RefPtr.h
else
hg mv mfbt/nsRefPtr.h mfbt/RefPtr.h
fi
