Closed Bug 1472529 Opened Last year Closed Last year

A bit more nsFind cleanup.

Categories

(Core :: Find Backend, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(3 files)

Still trying to make the Shadow DOM changes not gross, so here goes another round of cleanup.
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)
Attachment #8989040 - Flags: review?(mats)
Flags: needinfo?(emilio)
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.
Attachment #8989040 - Flags: review-
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 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 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)
Attachment #8989040 - Flags: review?(mats) → review+
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?
Attachment #8989142 - Flags: review?(mats) → review+
Sigh, MozReview... by "this assertion" I mean the MOZ_ASSERT(nonAnonNode).
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)
Attachment #8989141 - Flags: review?(bugs) → review+
Pushed by emilio@crisal.io:
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
Flags: needinfo?(emilio)
https://hg.mozilla.org/mozilla-central/rev/83489aa3a90d
https://hg.mozilla.org/mozilla-central/rev/b886d32b447e
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1473694
You need to log in before you can comment on or make changes to this bug.