Closed Bug 728508 Opened 10 years ago Closed 9 years ago

Fix check for duplicate keywords on the search engine manager

Categories

(Firefox :: Search, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: reuben, Unassigned)

References

()

Details

Attachments

(1 file, 1 obsolete file)

We're using search.sqlite to store search engine keywords now.
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
Attached patch Patch. (obsolete) — Splinter Review
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)
Attached patch Patch.Splinter Review
Oops, forgot to qref.
Attachment #598490 - Attachment is obsolete: true
Attachment #598491 - Flags: review?(gavin.sharp)
Attachment #598490 - Flags: review?(gavin.sharp)
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
Closed: 10 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
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
Closed: 10 years ago9 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
Closed: 9 years ago9 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.