Closed Bug 1470861 Opened 6 years ago Closed 6 years ago

More nsFind cleanup in preparation for Shadow DOM support.

Categories

(Toolkit :: Find Toolbar, enhancement)

Product:
Toolkit
Component:
Find Toolbar
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(7 files)

Bug 1470861: Trivially simplify some code in nsWebBrowserFind.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1470861: Cleanup GetBlockParent.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1470861: Make nsFind::ResetAll reset mIterNode / mIterOffset too.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1470861: Make state-passing explicit in nsFind.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1470861: Remove useless code.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1470861: Make GetBlockParent static.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1470861: Mark some methods const now that they don't mutate nsFind itself.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details 
Just trying to make a bit more sane this code before tweaking the iterators more heavily for Shadow DOM.
Comment on attachment 8987470 [details]
Bug 1470861: Trivially simplify some code in nsWebBrowserFind.

https://reviewboard.mozilla.org/r/252708/#review259254
Attachment #8987470 - Flags: review?(mats) → review+
Comment on attachment 8987471 [details]
Bug 1470861: Cleanup GetBlockParent.

https://reviewboard.mozilla.org/r/252710/#review259258
Attachment #8987471 - Flags: review?(mats) → review+
Comment on attachment 8987472 [details]
Bug 1470861: Make nsFind::ResetAll reset mIterNode / mIterOffset too.

https://reviewboard.mozilla.org/r/252712/#review259284

::: commit-message-050ee:3
(Diff revision 1)
> +We do reset them implicitly next time we call Find(..), since we call
> +ResetAll() at the beginning of it, then NextNode(..), which unconditionally
> +overrides them, but this is clearer for the next thing I want to do.

Do you claim that this patch is idempotent?
If so, then please add "(idempotent patch)" in the commit message for future reference.
After reading the code it looks like it is to me, but this is non-obvious to someone doing code-archeology in the future.

Ditto for the other patches.
Attachment #8987472 - Flags: review?(mats) → review+
Comment on attachment 8987473 [details]
Bug 1470861: Make state-passing explicit in nsFind.

https://reviewboard.mozilla.org/r/252714/#review259272

It's assumed that we don't modify the passed in Ranges:
nsRange* aSearchRange,
nsRange* aStartPoint,
nsRange* aEndPoint

Perhaps we should make those 'const nsRange*' while we're here?
(as a patch at the end if you prefer)

::: toolkit/components/find/nsFind.cpp:500
(Diff revision 1)
> +  nsCOMPtr<nsINode> mIterNode;
> +  nsCOMPtr<nsINode> mCurrNode;
> +  nsCOMPtr<nsIContent> mLastBlockParent;

Worth repeating the comment about using weak pointers here too?

::: toolkit/components/find/nsFind.cpp:1012
(Diff revision 1)
>      if (!frag) {
>  
>        tc = nullptr;

nit: can we remove this too, like you did above?
Attachment #8987473 - Flags: review?(mats) → review+
Comment on attachment 8987474 [details]
Bug 1470861: Remove useless code.

https://reviewboard.mozilla.org/r/252716/#review259462
Attachment #8987474 - Flags: review?(mats) → review+
Comment on attachment 8987475 [details]
Bug 1470861: Make GetBlockParent static.

https://reviewboard.mozilla.org/r/252718/#review259464
Attachment #8987475 - Flags: review?(mats) → review+
Comment on attachment 8987476 [details]
Bug 1470861: Mark some methods const now that they don't mutate nsFind itself.

https://reviewboard.mozilla.org/r/252720/#review259466
Attachment #8987476 - Flags: review?(mats) → review+
For the record, some history for this code is in bug 123087 and bug 126651.
In the former bug, the 2nd NextNode() call could affect mIterNode and
thus whether mIterator was nulled out before returning:
https://searchfox.org/mozilla-central/diff/522c2d7fe39e784ab661674a469049a9261cf641/embedding/components/find/src/nsFind.cpp#744
The latter bug then nulled it out unconditionally:
https://searchfox.org/mozilla-central/diff/68d64a1ce7398500d20bae95bf05984e687e7577/embedding/components/find/src/nsFind.cpp#636
and I suspect it's been a "NOP" ever since.

The bugs have some interesting discussion, but I still don't
understand what that second call was for, although it seems
clear that it was doing something at some point...
(In reply to Mats Palmgren (:mats) from comment #11)
> Comment on attachment 8987472 [details]
> Bug 1470861: Make nsFind::ResetAll reset mIterNode / mIterOffset too.
> 
> https://reviewboard.mozilla.org/r/252712/#review259284
> 
> ::: commit-message-050ee:3
> (Diff revision 1)
> > +We do reset them implicitly next time we call Find(..), since we call
> > +ResetAll() at the beginning of it, then NextNode(..), which unconditionally
> > +overrides them, but this is clearer for the next thing I want to do.
> 
> Do you claim that this patch is idempotent?
> If so, then please add "(idempotent patch)" in the commit message for future
> reference.
> After reading the code it looks like it is to me, but this is non-obvious to
> someone doing code-archeology in the future.
> 
> Ditto for the other patches.

Yes, that's my claim. Will mention it in the commit description.
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/019672e7422e
Trivially simplify some code in nsWebBrowserFind. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/233f590e0fdf
Cleanup GetBlockParent. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee42968fdf9c
Make nsFind::ResetAll reset mIterNode / mIterOffset too. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ffabc61f94f
Make state-passing explicit in nsFind. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/191d3b6e08b6
Remove useless code. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/62310bcf732e
Make GetBlockParent static. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/0653080ae094
Mark some methods const now that they don't mutate nsFind itself. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/7adbfdf31164
Mark some nsFind arguments const. rs=mats
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: