Fix check for duplicate keywords on the search engine manager

RESOLVED FIXED

Status

()

Firefox
Search
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: reuben, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
We're using search.sqlite to store search engine keywords now.
(Reporter)

Comment 1

6 years ago
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
Summary: Remove the moz_keywords table from places.sqlite → Fix check for duplicate keywords on the search engine manager
(Reporter)

Comment 2

6 years ago
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.
Assignee: nobody → reuben.morais
Attachment #598490 - Flags: review?(gavin.sharp)
(Reporter)

Comment 3

6 years ago
Created attachment 598491 [details] [diff] [review]
Patch.

Oops, forgot to qref.
Attachment #598490 - Attachment is obsolete: true
Attachment #598491 - Flags: review?(gavin.sharp)
Attachment #598490 - Flags: review?(gavin.sharp)
(Reporter)

Comment 4

6 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Depends on: 709589
Resolution: --- → FIXED
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.
(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
(Reporter)

Comment 7

6 years ago
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.
Attachment #598491 - Flags: review?(gavin.sharp)
(this wasn't "FIXED", since nothing landed on trunk yet)
Assignee: reuben.morais → nobody
Status: RESOLVED → REOPENED
Component: Bookmarks & History → Search
QA Contact: bookmarks → search
Resolution: FIXED → ---
Bug 709589 landed now.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → WORKSFORME
Sorry, only 3 of the many patches landed there.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Apologizes again, I got confused with bug 643172 since the summary is basically the same.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 461042
You need to log in before you can comment on or make changes to this bug.