1472529 - A bit more nsFind cleanup.

Status: RESOLVED FIXED

(Core :: Find Backend, enhancement)

Description

[Emilio Cobos Álvarez [:emilio]]

Still trying to make the Shadow DOM changes not gross, so here goes another round of cleanup.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1472529: More nsFind cleanup.

The general setup is that the State struct is used to iterate over text nodes
explicitly, and it starts positioned at the first node, which moves all the
mIterOffset weirdness to the constructor instead of passing all the ranges
around everywhere.

Sorry this isn't as well split as the previous bugs...

Review commit: https://reviewboard.mozilla.org/r/254124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254124/

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8989040 [details]
Bug 1472529: More nsFind cleanup.

I wrote a couple tests... I expect the fix to be trivial, but we'll see.

(https://treeherder.mozilla.org/#/jobs?repo=try&revision=af059265f407925f2ca26277aa3b22bdbc784a4f&selectedJob=185838585, for reference)

Comment 3

[Mats Palmgren (inactive)]

Comment on attachment 8989040 [details]
Bug 1472529: More nsFind cleanup.

https://reviewboard.mozilla.org/r/254124/#review260850

::: toolkit/components/find/nsFind.cpp:604
(Diff revision 1)
> -                     nsINode* aStartNode,
> -                     int32_t aStartOffset,
> +  // Disallow copying because copying the iterator would be a lie.
> +  State(const State&) = delete;

nit: please add an empty line after this ctor to decouple its comment from the next ctor

::: toolkit/components/find/nsFind.cpp:606
(Diff revision 1)
>  
> -nsresult
> -nsFind::InitIterator(State& aState,
> -                     nsINode* aStartNode,
> -                     int32_t aStartOffset,
> -                     nsINode* aEndNode,
> +struct nsFind::State final
> +{
> +  // Disallow copying because copying the iterator would be a lie.
> +  State(const State&) = delete;
> +  explicit State(bool aFindBackward,

nit: "explicit" isn't needed

::: toolkit/components/find/nsFind.cpp:715
(Diff revision 1)
> +  nsINode* beginning = mFindBackward
> +    ? aStartPoint.GetEndContainer()
> +    : aStartPoint.GetStartContainer();

nit: I'd probably format this as:
  nsINode* beginning = mFindBackward ? aStartPoint.GetEndContainer()
                                     : aStartPoint.GetStartContainer();

::: toolkit/components/find/nsFind.cpp:722
(Diff revision 1)
> +  mIterOffset = mFindBackward
> +    ? aStartPoint.EndOffset()
> +    : aStartPoint.StartOffset();

ditto

::: toolkit/components/find/nsFind.cpp:736
(Diff revision 1)
> +    // Get the block parent.
> +    nsIContent* blockParent = GetBlockParent(current);

nit: the comment is kind of useless.  Perhaps change it to comment the intent of the whole "if" instead?  (i.e. "return null if we're not in the same block" or some such.)  Otherwise, remove it.

::: toolkit/components/find/nsFind.cpp:945
(Diff revision 1)
> +  Text* current = state.GetCurrentNode();
> +  if (!current) {
> +    return NS_OK;
> +  }
> +  frag = current->GetText();
> +  fragLen = frag->GetLength();
> +
> +  if (frag->Is2b()) {
> +    t2b = frag->Get2b();
> +  } else {
> +    t1b = frag->Get1b();
> +  }
> +
> +  // state.mIterOffset, if set, is the range's idea of an offset, and points
> +  // between characters. But when translated to a string index, it points to a
> +  // character. If we're going backward, this is one character too late and
> +  // we'll match part of our previous pattern.
> +  if (state.mIterOffset >= 0) {
> +    findex = state.mIterOffset - (mFindBackward ? 1 : 0);
> +  } else {
> +    // Otherwise, just start at the appropriate end of the fragment.
> +    findex = mFindBackward ? (fragLen - 1) : 0;
> +  }

Hmm, this is a decent bit of added code duplication... I don't see the point of pulling this out from the loop plus adding a 'firstIteration' bool, instead of just falling into the !frag block on the first iteration.

Please revert this change in this patch.  Feel free to rework it and submit it as a separate patch in this bug or in a follow-up bug.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8989040 [details]
Bug 1472529: More nsFind cleanup.

https://reviewboard.mozilla.org/r/254124/#review260852

::: toolkit/components/find/nsFind.cpp:945
(Diff revision 1)
> +  Text* current = state.GetCurrentNode();
> +  if (!current) {
> +    return NS_OK;
> +  }
> +  frag = current->GetText();
> +  fragLen = frag->GetLength();
> +
> +  if (frag->Is2b()) {
> +    t2b = frag->Get2b();
> +  } else {
> +    t1b = frag->Get1b();
> +  }
> +
> +  // state.mIterOffset, if set, is the range's idea of an offset, and points
> +  // between characters. But when translated to a string index, it points to a
> +  // character. If we're going backward, this is one character too late and
> +  // we'll match part of our previous pattern.
> +  if (state.mIterOffset >= 0) {
> +    findex = state.mIterOffset - (mFindBackward ? 1 : 0);
> +  } else {
> +    // Otherwise, just start at the appropriate end of the fragment.
> +    findex = mFindBackward ? (fragLen - 1) : 0;
> +  }

Yeah, so the annoying part is that I'd need to rework the loop to look at the current node and not the next one, but that was a way bigger change I'd be more comfortable doing in a followup. But we'll see, I have some tests to fix.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8989040 [details]
Bug 1472529: More nsFind cleanup.

https://reviewboard.mozilla.org/r/254124/#review260852

> Yeah, so the annoying part is that I'd need to rework the loop to look at the current node and not the next one, but that was a way bigger change I'd be more comfortable doing in a followup. But we'll see, I have some tests to fix.

I think I'll avoid both issues for now making the initialization lazy again. Though the loop ideally would be simpler...

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1472529: Cleanup CharacterData, and add a non-virtual function to get the text fragment.

I'm going to use that from nsFind.

Review commit: https://reviewboard.mozilla.org/r/254206/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254206/

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1472529: Teach nsFindContentIterator to rewind into NAC.

This gets rid of the remaining tests failures that I was seeing, and should
probably be squashed into the previous patch to make it idempotent before
landing, but thought it probably required more careful review.

Before it kinda worked because it was the only code path where we had an
mIterNode out of sync with the iterator.

Review commit: https://reviewboard.mozilla.org/r/254208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254208/

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8989040 [details]
Bug 1472529: More nsFind cleanup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254124/diff/1-2/

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8989040 [details]
Bug 1472529: More nsFind cleanup.

https://reviewboard.mozilla.org/r/254124/#review261050

::: toolkit/components/find/nsFind.cpp:637
(Diff revision 2)
> +  const bool mFindBackward;
> +  // An offset into the text of the first node we're starting to search at.
> +  int32_t mIterOffset = -1;

nit: now that we have a non-trivial ctor I'd prefer to move the initialization of all members there; it makes the code slightly more readable to have them all in one place IMO.

::: toolkit/components/find/nsFind.cpp:755
(Diff revision 2)
> +    // Get the block parent.
> +    const nsIContent* blockParent = GetBlockParent(current);

nit: the comment is kind of useless.  Perhaps change it to comment the intent of the whole "if" instead?  (i.e. "return null if we're not in the same block" or some such.)  Otherwise, remove it.

::: toolkit/components/find/nsFind.cpp:1014
(Diff revision 2)
> +      DEBUG_FIND_PRINTF("Starting from offset %d of %d\n", findex, fragLen);
> +

(I'm assuming you moved this line intentionally)

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8989142 [details]
Bug 1472529: Teach nsFindContentIterator to rewind into NAC.

https://reviewboard.mozilla.org/r/254208/#review261054

::: toolkit/components/find/nsFind.cpp:258
(Diff revision 1)
>    }
>    return mOuterIterator->IsDone();
>  }
>  
> +static nsIContent*
> +GetAnonymousSubtreeRootParent(nsINode& aNode)

nit: I think we should drop "Get" from the name since this function is expected to always return a non-null value

::: toolkit/components/find/nsFind.cpp:277
(Diff revision 1)
> +  // If this failed, it means that aCurNode is necessarily anonymous.
> +  MOZ_ASSERT(aCurNode->IsInNativeAnonymousSubtree());
> +  nsIContent* nonAnonNode = GetAnonymousSubtreeRootParent(*aCurNode);

nit: I think we should do this assertion on aNode in GetAnonymousSubtreeRootParent itself instead.  I don't think we ever want to call that function on a non-NAC node.

::: toolkit/components/find/nsFind.cpp:280
(Diff revision 1)
> -    mOuterIterator->PositionAt(oldNode);
> -    if (mInnerIterator) {
> +  }
> +
> +  // If this failed, it means that aCurNode is necessarily anonymous.
> +  MOZ_ASSERT(aCurNode->IsInNativeAnonymousSubtree());
> +  nsIContent* nonAnonNode = GetAnonymousSubtreeRootParent(*aCurNode);
> +  MOZ_ASSERT(nonAnonNode);

nit: this assertion is redundant if we make AnonymousSubtreeRootParent guarantee a non-null return value (by dropping "Get" from the name - I think it's already guaranteed by the code)

::: toolkit/components/find/nsFind.cpp:1278
(Diff revision 1)
> -        NS_ASSERTION(NS_SUCCEEDED(rv),
> +        MOZ_ASSERT(NS_SUCCEEDED(rv),
> -                     "nsFindContentIterator failed to rewind "
> +                   "nsFindContentIterator failed to rewind "
> -                     "because it doesn't handle NAC properly...");
> +                   "because it doesn't handle NAC properly...");

Should we drop the last line in this assertion message now we're teaching it to do that?

Comment 11

[Mats Palmgren (inactive)]

Sigh, MozReview... by "this assertion" I mean the MOZ_ASSERT(nonAnonNode).

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8989141 [details]
Bug 1472529: Cleanup CharacterData, and add a non-virtual function to get the text fragment.

https://reviewboard.mozilla.org/r/254206/#review261114

r+

::: commit-message-18aac:2
(Diff revision 1)
> +Bug 1472529: Cleanup CharacterData, and add a non-virtual function to get the text fragment. r?smaug
> +

(quite unrelated changes here)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/83489aa3a90d
Cleanup CharacterData, and add a non-virtual function to get the text fragment. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b886d32b447e
More nsFind cleanup. r=mats

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/83489aa3a90d
https://hg.mozilla.org/mozilla-central/rev/b886d32b447e