Last Comment Bug 728508 - Fix check for duplicate keywords on the search engine manager
: Fix check for duplicate keywords on the search engine manager
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
https://hg.mozilla.org/mozilla-centra...
: 461042 (view as bug list)
Depends on: 709589
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-17 18:46 PST by Reuben Morais [:reuben]
Modified: 2013-03-13 23:20 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch. (3.36 KB, patch)
2012-02-17 20:51 PST, Reuben Morais [:reuben]
no flags Details | Diff | Splinter Review
Patch. (2.82 KB, patch)
2012-02-17 20:52 PST, Reuben Morais [:reuben]
no flags Details | Diff | Splinter Review

Description Reuben Morais [:reuben] 2012-02-17 18:46:11 PST
We're using search.sqlite to store search engine keywords now.
Comment 1 Reuben Morais [:reuben] 2012-02-17 19:04:06 PST
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] 2012-02-17 20:51:23 PST
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.
Comment 3 Reuben Morais [:reuben] 2012-02-17 20:52:14 PST
Created attachment 598491 [details] [diff] [review]
Patch.

Oops, forgot to qref.
Comment 4 Reuben Morais [:reuben] 2012-02-17 21:32:19 PST
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] 2012-02-18 04:45:54 PST
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] 2012-02-18 04:47:35 PST
(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] 2012-02-18 06:50:38 PST
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: gavin@gavinsharp.com] 2012-02-18 13:49:39 PST
(this wasn't "FIXED", since nothing landed on trunk yet)
Comment 9 Matthew N. [:MattN] (In Taipei until Sep. 23) 2012-03-21 17:31:31 PDT
Bug 709589 landed now.
Comment 10 Matthew N. [:MattN] (In Taipei until Sep. 23) 2012-03-21 23:37:03 PDT
Sorry, only 3 of the many patches landed there.
Comment 11 Matthew N. [:MattN] (In Taipei until Sep. 23) 2012-03-21 23:38:43 PDT
Apologizes again, I got confused with bug 643172 since the summary is basically the same.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-13 23:20:43 PDT
*** Bug 461042 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.