1104139 - [Contacts] Refactor highlight so we can avoid using regexp

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect)

Description

[Adrián de la Rosa]

To have a more simple code and improve performance we should refactor highlighting in contact search to avoid using regular expressions.

Comment 2

[Adrián de la Rosa]

Attachment: Pull request

Do you think this is the correct approach? Highlighting tests are already passing. I haven't changed the highlighting tag because it changed styles even using the corresponding class, so maybe we should try to do the change in a future bug.

If you are ok with this, I will ask to know where inside shared this function should be.

Thanks!

Comment 3

[Jose Manuel Cantera]

Comment on attachment 8529195 [details] [review]
Pull request

we need more work on this

Comment 4

[Adrián de la Rosa]

Comment on attachment 8529195 [details] [review]
Pull request

I would like to ask someone from shared if they are ok with using that file for the highlighting function, because the file seems to be unused and deprecated. Apart from that, tests are passing and highlighting works fine for me.

Comment 5

[Jose Manuel Cantera]

(In reply to Adrián de la Rosa from comment #4)
> Comment on attachment 8529195 [details] [review]
> Pull request
> 
> I would like to ask someone from shared if they are ok with using that file
> for the highlighting function, because the file seems to be unused and
> deprecated. Apart from that, tests are passing and highlighting works fine
> for me.

We can ask Julien if the shared/js/utils.js is going to be deprecated or not. Nonetheless I still find it useful to share the highlight function in gaia/shared. thanks

Comment 6

[Jose Manuel Cantera]

Comment on attachment 8529195 [details] [review]
Pull request

we are on the right track, please address the comments on GH, and then we would need to add to the patch additional unit tests (if needed) and a brand new integration test suite for contacts search. 

thanks!

Comment 7

[Julien Wajsberg [:julienw]]

I'm not a "shared" peer, redirecting the NI to Kevin.

Clearly createHighlightHTML is not used anywhere currently. In the file it's said it comes from the SMS app, but the blame shows that this part actually came from the Browser app, and was likely moved to shared afterwards.

In the SMS app we currently have a different implementation using Template.

Comment 8

[Kevin Grandon :kgrandon]

I'm fine using this file. This utility should arguably be its own file, though you don't have to do that in this pull request.

I think I prefer the regular expression approach personally. Have we checked if there's a performance difference between using a word list and a regular expression?

Comment 9

[Adrián de la Rosa]

Comment on attachment 8529195 [details] [review]
Pull request

Before asking for another review, I would like your opinion about some issues. First, I wasn't able to find where search.js was loaded from contacts so I lazyLoaded a related file here: https://github.com/mozilla-b2g/gaia/pull/26492/files?diff=unified#diff-139a10c960db3187930ef89ff828a686R183 . Jose Manuel noted that it should be lazyloaded with the rest of search dependencies.

Second, Jose Manuel left this comment: https://github.com/mozilla-b2g/gaia/pull/26492/files?diff=unified#r21031394 but I'm not sure about what does he means, and he's on pto right now.

And last, this suggestion from Kevin that I think you should know how to answer better than me: https://github.com/mozilla-b2g/gaia/pull/26492/files?diff=unified#r21061559

Thanks!

Comment 10

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Adrián de la Rosa from comment #9)
> Comment on attachment 8529195 [details] [review]
> Pull request
> 
> Before asking for another review, I would like your opinion about some
> issues. First, I wasn't able to find where search.js was loaded from
> contacts so I lazyLoaded a related file here:
> https://github.com/mozilla-b2g/gaia/pull/26492/files?diff=unified#diff-
> 139a10c960db3187930ef89ff828a686R183 . Jose Manuel noted that it should be
> lazyloaded with the rest of search dependencies.

Right now the search dependency is loaded using the views loading mechanism, take a look to the |loadView| method (and you'll be redirected to |load|) inside |contacts.js|

> 
> Second, Jose Manuel left this comment:
> https://github.com/mozilla-b2g/gaia/pull/26492/files?diff=unified#﷒0﷓ but
> I'm not sure about what does he means, and he's on pto right now.

I guess that Jose is talking to store in two arrays the positions that you have to highlight or keep as they are.

> 
> And last, this suggestion from Kevin that I think you should know how to
> answer better than me:
> https://github.com/mozilla-b2g/gaia/pull/26492/files?diff=unified#﷒1﷓

I already replied in github.

Also I agree with Kevin, I prefer to remove stuff from the |shared/js/utilities.js| seems we are loading a lot of stuff that we won't need.


> 
> Thanks!

More extra feedback, remember that search is shared cause it's also used in FTU, which means that we will need to add any extra dependency (if needed) to the importers (in contacts and ftu).

As well, don't know if more people is using the old highlight method, in that case will be nice to leave the previous one, as the regexp will be quite faster. I understant that the highlight requirements in contacts are different (for example one string ñ, should highlight if we search by n), so if this highligth method is used in other place, would keep it and add a new one, if not, I'm ok with modifying it.

Comment 11

[Adrián de la Rosa]

Comment on attachment 8529195 [details] [review]
Pull request

The PR has now two commits, one being from bug 1100930. We can mark the other as duplicate of this, or I can rebase into master once the other bug is accepted. I need the function I created for the other bug because I can't use textContent since I need to know where the <strong> tag is, but if I use innerHTML I will get the content encoded.

Comment 12

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8529195 [details] [review]
Pull request

Left some comments, good job. Just a suggestion, I think that we should re-implement integration tests already existing properly and add a suite to test this new behavior thought

Comment 13

[Adrián de la Rosa]

Comments solved, thanks for your suggestions!

Comment 15

[Adrián de la Rosa]

Removed commit from bug 1100930. Marking this bug as depending on 1100930 to make clear there's missing code here. I'll rebase this PR when that one lands.

Comment 16

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8529195 [details] [review]
Pull request

Left a couple of comments in the PR, pretty tiny, could you address them and rebase?

And once we have a clean build we can land this.

Thanks!

Comment 17

[Adrián de la Rosa]

Comment on attachment 8529195 [details] [review]
Pull request

Comments addressed!

Comment 18

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8529195 [details] [review]
Pull request

Codewise looking ok, but unit tests and integration tests are failing for contact search:

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=9c82d9bc1dab

Comment 19

[Adrián de la Rosa]

Oh, I forgot this patch needs bug 1100930 to land first. I'll ask for review again once that happens.

Comment 20

[Adrián de la Rosa]

Comment on attachment 8529195 [details] [review]
Pull request

Asking for review now that the bug that was blocking this one is merged in master.

Comment 21

[Jose Manuel Cantera]

Comment on attachment 8529195 [details] [review]
Pull request

please address the latest comments on GH and land once you get a green tree

thanks!

Comment 22

[Adrián de la Rosa]

Every test passed! Asking for check in.

Comment 23

[Ryan VanderMeulen (PTO, back July 27)]

Master: https://github.com/mozilla-b2g/gaia/commit/c901afbedd226890ca88a57d57f4e25c3e36a9d5