728508 - Fix check for duplicate keywords on the search engine manager

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Reuben Morais [:reuben]]

We're using search.sqlite to store search engine keywords now.

Comment 1

[Reuben Morais [:reuben]]

Punching this bug into its real direction. The search engine manager needs to properly check for duplicate keywords instead of silently failing at editKeyword: https://hg.mozilla.org/mozilla-central/file/tip/browser/components/search/content/engineManager.js#l143

Comment 2

[Reuben Morais [:reuben]]

Attachment: Patch.

Is it better to import PlacesUtils if all I want is a single method from nsINavBookmarksService? In this patch I'm getting the service directly.

Comment 3

[Reuben Morais [:reuben]]

Attachment: Patch.

Oops, forgot to qref.

Comment 4

[Reuben Morais [:reuben]]

As per IRC conversation, the duplicate check on the search engine service is not needed, and the only bug stopping this validation to work is engine being undefined when showing the error message. Attachment 580762 [details] [diff] fixes this.

Comment 5

[Marco Bonardo [:mak]]

as far as I know bookmarks keywords and search engine keywords are pretty much different things, so I don't understand this bug, and I don't understand why it has a pending review patch and is marked as fixed.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Reuben Morais [:reuben] from comment #2)
> Created attachment 598490 [details] [diff] [review]
> Patch.
> 
> Is it better to import PlacesUtils if all I want is a single method from
> nsINavBookmarksService? In this patch I'm getting the service directly.

it's always better to use PlacesUtils, since it already has the cached service for you, you can have a lazyModuleGetter

Comment 7

[Reuben Morais [:reuben]]

Comment on attachment 598491 [details] [diff] [review]
Patch.

(In reply to Marco Bonardo [:mak] from comment #5)
> as far as I know bookmarks keywords and search engine keywords are pretty
> much different things, so I don't understand this bug, and I don't
> understand why it has a pending review patch and is marked as fixed.

Yes, I'm sorry about that, this bug involved a lot of IRC conversation so it's definitely not clear. The only bug here is that engine.name is undefined at https://hg.mozilla.org/mozilla-central/file/tip/browser/components/search/content/engineManager.js#l181 but that's already fixed by attachment 580762 [details] [diff] [review] on bug 709589. Gavin already r+'d that one now so this bug is no longer needed.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(this wasn't "FIXED", since nothing landed on trunk yet)

Comment 9

[Matthew N. [:MattN]]

Bug 709589 landed now.

Comment 10

[Matthew N. [:MattN]]

Sorry, only 3 of the many patches landed there.

Comment 11

[Matthew N. [:MattN]]

Apologizes again, I got confused with bug 643172 since the summary is basically the same.