it's an addon, so how about moving it there ?

It doesn't matter if it is an add-on or not. What does matter is if the user experience is improved, if search engines can benefit from being moved into the EM, etc. I have no idea if any of this is true in relation to this and the change in the EM side would entail adding a new em:type which is simple enough. The work on the search engine side would entail packaging them appropriate which is probably not simple. Also, the decision on whether to do this or not would not be made from the EM side so I'm changing the component to search.

As an average user I would think: how many clicks does it need to get there? But there are many different average users :) What people really find convenient you can only know when you make a test browser with the new feature, let a thousand users use it for some weeks and then ask the right questions.

(In reply to comment #1) > It doesn't matter if it is an add-on or not. What does matter is if the user > experience is improved, if search engines can benefit from being moved into the > EM, etc. It's the place where all the other "extras" (exts/skins) go so that would make it a logical place (from my point of view) It's a separate manager (/window) less, at least that's what the user sees. It can be made in the same style as the EM pane Concerning Ria's click concern... no extra clicks required. Manage Search Engines in the Search Box would simply open the addons manager with the search manager pane focussed

Some thoughts: * Are the packaging requirements really a problem here? Can the EM not just host the exact same UI bits as Gavin has put into the search manager dialog, calling the same methods? Or is this a toolkit/core vs. firefox UI problem? * Note that Gavin designed the search management dialog as modal (at least on OSX it is); I'm unsure if that's a requirement or not, but it differs from the non-modal EM. * The number of clicks to get to the current search manager and the search tab of the EM could easily be made to be the same; just retarget the "Manage Search Engines ..." button to open the EM window with the Search Pane pre-selected. * I agree with Peter that, like themes or plugins, the user probably won't draw a distinction between types of artifacts that can extend their web browsing experience. As long as the UI mechanisms can be made to fit in with the other aspects of the EM, I think we should try to do this.

(In reply to comment #4) > * Are the packaging requirements really a problem here? Can the EM not just > host the exact same UI bits as Gavin has put into the search manager dialog, > calling the same methods? Or is this a toolkit/core vs. firefox UI problem? It will be a problem but it can be fixed. We would have to add something similar to what the pref panels provides and load an overlay (possibly add the ability for x number of overlays) specified by the app.

I think this is something we want to investigate, but not in a way that blocks any critical path work on Firefox 2. Targetting to Fx3.

(In reply to comment #5) > Created an attachment (id=220129) [edit] > screenshot of integrated searchEM > I think that the UI of the Search tab should be consistent with the rest of Add-ons i.e. there shouldn't be any bar on the right and Move Up/Down buttons should be in the richlistbox. They seem a little redundant anyway as this could be done by dragging. I am not sure whether richlistbox supports drag&drop reordering, though.

I think dictionaries should also be in the add-on manager.

(In reply to comment #9) > I think dictionaries should also be in the add-on manager. > I filed Bug 338074 for that

*** Bug 338416 has been marked as a duplicate of this bug. ***

*** Bug 340974 has been marked as a duplicate of this bug. ***

If it is not added in the add-ons window, it should be at least added in the Tools menu beside the Add-ons and Downloads buttons, because when people are looking for adjusting some options in their browser, they expect that it would be in Tools menu. I've experienced this. I tried to find where I could manage search engines and I had sought through all the menus but I couldn't realize at any price that the managing window could only be entered from under the search engine icon.

Just installed RC1 and my "Search web for" context menu now appears hard-coded to Google instead of my-favourite-web-search-engine. After digging through Bugzilla I learned that there is something called a Search Manager that I cannot find in any place. The reason appears to be that I hid the search tool bar (because I use quick searches). So: whatever we agree upon: there must be some way to access the Search Manager while the search tool bar is hidden. Personally I like the add-ons manager.

(In reply to comment #14) > Just installed RC1 and my "Search web for" context menu now appears hard-coded > to Google instead of my-favourite-web-search-engine. After digging through > Bugzilla I learned that there is something called a Search Manager that I > cannot find in any place. The reason appears to be that I hid the search tool > bar (because I use quick searches). > > So: whatever we agree upon: there must be some way to access the Search Manager > while the search tool bar is hidden. Personally I like the add-ons manager. > donno why you choose this bug or any bug for that matter to ask this question (next time use mozillazine.org) but select the the little drop down icon next to the site icon in the search bar and then select 'Manage Search Engines...'

(In reply to comment #15) > (In reply to comment #14) > > So: whatever we agree upon: there must be some way to access the Search Manager > > while the search tool bar is hidden. Personally I like the add-ons manager. > > > donno why you choose this bug or any bug for that matter to ask this question > (next time use mozillazine.org) but select the the little drop down icon next > to the site icon in the search bar and then select 'Manage Search Engines...' I guess you misread my comment, because it is not a question. The point I wanted to make is that some people (I am one of them) have the search bar hidden and therefore they have no means to access the search engine manager. Worse still: they won't even know that there is a search engine manager at all. That is design error and should be fixed. The solution I propose is to add a tab to the add-ons manager.

(In reply to comment #16) > The point I wanted to make is that some people (I am one of them) have the > search bar hidden and therefore they have no means to access the search engine > manager. Worse still: they won't even know that there is a search engine > manager at all. That is design error and should be fixed. Well why would you need to manage the search engines if the search bar was hidden anyways? Apparently you aren't using the search bar then. > > The solution I propose is to add a tab to the add-ons manager. > Thats what this bug is for but not for the reason that you are commenting.

(In reply to comment #17) > Well why would you need to manage the search engines if the search bar was > hidden anyways? Apparently you aren't using the search bar then. Because I need the search manager to set the "Search web for" engine when right-clicking a selected word on a page. I am sorry if my first post (comment #14) was cryptic and caused confusion. Let me try to explain it with more detail: When you highlight a word or phrase on a page, you can right-click it and a pop-up menu appears with the option "Search web for <highlighted phrase>...". In Firefox 1.5 there is a setting in about:config where you can set the URL of the search engine that is used for those higlight searches (browser.search.defaulturl), quite similar to the quick searches. In Firefox 2.0 that setting is ignored and the last-selected search engine is used instead. Having the search box hidden from the toolbar, there is no way to access and change that last-selected search engine anymore, because the search manager can currently only be accessed from that hidden search box. Furthermore FF2.0 copies the toolbar settings from FF1.5, thus the user simply cannot find how to change that search engine anywhere: it probably would not even know there is such a thing as a search manager, because the search box is hidden. Browsing through the about:config list is also fruitless. > Thats what this bug is for but not for the reason that you are commenting. I know that this reason was not mentioned before, but that is exactly the point: to give another reason as why the search manager should be moved (or copied) to the add-ons manager. IMHO it is a very good reason. Should we change this bug's target to FF2.0? Should we make it blocking?

(In reply to comment #18) > Should we change this bug's target to FF2.0? Should we make it blocking? No, there is no way this will be done for 2.0. There is much more to it than just copying the search engine mgr into the add-ons mgr. For starters, the add-ons mgr is in toolkit which is shared by all applications and search engines are in Firefox... this means it can't just be copied or whatever since it would show up in all applications including the ones that don't have a search engine mgr.

Not a blocker, and Rob seems to think it's not at all possible for Firefox 2.0, but it would be nice. Michael Wu did some backend support in bug 382367 to do this for plugins.

Er, that was Firefox 2, this is Firefox 3, oops. :) Adding wanted.

Adding the search engines manager to Add-ons is an excellent way to show users what add-ons are, and where to configure them.

It would be great if this could happen to concentrate all 'add-ons' in the Add-ons Manager. A Search Engine could also then have an Options button, much like the extensions, to control its name, associated keywords, etc. If this was done, the 'Add a keyword for this search' feature (right-clicking in a text box on a website) could also create a search engine here, rather than a bookmark as it currently does. This would be more logical, and could allow for extra customization in the feature.

Note that Internet Explorer has a new Add-ons managment window, which shows search providers along other add-ons. From: http://blogs.msdn.com/ie/archive/2008/03/20/add-on-management-improvements-in-internet-explorer-8.aspx Fixing this would allow an easier transition from future Internet Explorer 8 users.

With the advent of search keywords, this is more important. It's not nec. now to have a separate search bar shown. BUT without it--you can't configure the keywords ("Manage Search Engines"). Whether in Add-Ons or not, the configuration dialog should be reachable without having to customize your toolbar, go to Manage, then customize to remove it again. Should this be a separate PR?

I agree to manage search plugins in add-ons manager. First because it's better to have less special behaviour and exceptions as possible (see also bug 370482). Second because normally an user expects to manage _all_ add-ons in add-ons manager.

Nominating for blocking‑firefox3.1 now that we have some time.

user-doc is certainly not -needed for something which does not yet exist.

Definitely not blocking.

Moving mockup from duped bug 455951.

As said, besides providing consistency for all kinds of Firefox enhancements it will also provide a logical way to manage search engines to users who opt to hide the search bar. The Omnibar extension adds an explicit web search option to the location bar ( and I guess it's won't be the last to do so), making the search bar not that necessary except for managing search engines.

I think a larger discussion - perhaps in dev-apps-firefox - needs to be had to think about how we want to resolve search here. I'm not convinced that this is a thing that needs to go in the add-ons manager as opposed to some other web services panel, but I am convinced that right now it's haphazard and sloppy the way these customizations are spewed across our product. Not a super-high priority for 3.6, though.

Please consider this thing again, now for FF 4.0: is it possible to move a manager of search engines to the main Add-ons manager, a common place where user is expecting to have an access to all "external" browser components? Also an option to include the search engine add-ons in the common checked list of updates (I mean the auto- and manual check which is implemented in 4.0 beta 3) would be another advantage of such decision.

(In reply to comment #37) > Please consider this thing again, now for FF 4.0: is it possible to move a > manager of search engines to the main Add-ons manager, a common place where > user is expecting to have an access to all "external" browser components? Also > an option to include the search engine add-ons in the common checked list of > updates (I mean the auto- and manual check which is implemented in 4.0 beta 3) > would be another advantage of such decision. There isn't really anything open for consideration here. We want to do this, I even have the beginnings of a patch to do it. If someone wants to volunteer time to take it and finish it up in time for the feature freeze of Firefox 4 then I would be very happy to accept it. None of the existing developers have time to work on this before then though.

I'm willing to help out with this bug. I see that Dave mentioned (back in 2010) that he had started a patch on this bug. What is that status of that patch? I could work on this, but I would of course need some help on getting started, as I have yet to touch the add-ons section of the browser code. Anything I could help with?

I'm guessing this would end up going in Toolkit, right? That would be really nice for Thunderbird, since it uses nsSearchService as of bug 677421.

I have been coming up with a plan on how to fix this bug. First, I'll create the actual XUL interface for the new addon. After that, I will implement all of the logic code behind it, and then attach its functionality to the browser. So my first patch will demonstrate the actual look of the addon. Also, where in the tree should the addon's code reside? Do I need to create a new directory for it? If so, what should its name be? Thanks

(In reply to Quentin Headen from comment #44) > I have been coming up with a plan on how to fix this bug. First, I'll create > the actual XUL interface for the new addon. Actually, you don't need to worry about proving an interface - the Add-ons Manager does all that automatically. Once you register the new addon type with the Addons Manager (via AddonManagerPrivate.registerProvider), the frontend (about:addons) will automatically add a new category item, and re-use the existing list-style interface. For an example, see: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/PluginProvider.jsm#360 For the interface, there's only a couple of things that need to be added: * Supporting drag&drop so search engines can be re-ordered * A text field in the details view, to edit the search engine's keyword * A checkbox somewhere to disable/enable search suggestions * A button somewhere I think you should work on those last, as they depend on everything else working. > Also, where in the tree should the addon's code reside? Do I need to create > a new directory for it? If so, what should its name be? Other than unit tests, you won't need to add any new files. The main part of the patch will be implementing an addon provider, which should be part of /toolkit/components/search/nsSearchService.js Dave's blog post on doing this will help, although it's a little outdated now: http://www.oxymoronical.com/blog/2010/07/How-to-extend-the-new-Add-ons-Manager I finally found Dave's old unfinished patch that did some of this - have a look at the patch in bug 552747. It needs a lot of updating: addon providers have changed a bit since then, the search component has probably changed, and it doesn't remove the old UI. But much of that patch is still relevant, so I think you should use that as a starting point. So I think the big steps here are: 1. Modify nsSearchService.js so that it implements the API expected by the Add-ons Manager, and registers as an addon provider. 2. Add features to the UI, such as changing the order of search engines 3. Remove the old "Manage Search Engines" UI, and make the menu of Firefox's search box open the Add-ons Manager (I think those steps should be separate patches.)

I found the extension created by Erik Vold. It adds to the "about: addon" new category "Search Engines" and in it are all the search engines. It is not comprehensive and contains errors, but I think that might be useful. https://addons.mozilla.org/pl/firefox/addon/search-engine-manager/?src=userprofile https://static-ssl-cdn.addons.mozilla.net/img/uploads/previews/full/65/65993.png?modified=1324621536

(In reply to fx4waldi from comment #46) > Created attachment 586649 [details] > Search Engine Manager 0.1.0 by Erik Vold Note that unless the author has released it under the terms of the MPL, or the tri-license, or is willing to do so, we cannot use this as a direct basis for our work, even though the implementation is likely similar you are advised to start from scratch.

This patch is no where near complete, but it demonstrates adding the "Search" category to the addon manager and populating it with installed search engines. I just want these early changes reviewed to make sure I am moving in the correct direction.

Comment on attachment 586651 [details] [diff] [review] Alpha Stage Patch v0.1 Review of attachment 586651 [details] [diff] [review]: ----------------------------------------------------------------- This is a great start :) I don't see any major issues so far. ::: toolkit/components/search/nsSearchService.js @@ +3723,5 @@ > + searchService: null, // This is to be filled by the SearchService object > + > + getAddonByID: function (aId, aCallback) { > + > + var engine = this.searchService.getEngineByName(aId); In SearchEngineAddon you make the ID be |engine.name + "@search.mozilla.org"| - so you'll need to remove that added part before passing it to getEngineByName(). @@ +3733,5 @@ > + aCallback([]); > + return; > + } > + > + var engines = this.searchService.getEngines(30); The count parameter for getEngines() is what's called an out-param, where the function uses the parameter to output a value. In JavaScript, it's done by passing in an object, and the .value property is set. In this case, it's set to the length of the array - but that's really only useful for C++ code, since JavaScript arrays have a .length property. So here you can ignore it - but the function still expects you to pass in *something*. So you can give it an empty object that isn't assigned to a variable. ie: var engines = this.searchService.getEngines({}); @@ +3796,5 @@ > + return true; > + }, > + > + get scope() { > + return AddonManager.SCOPE_ALL; The Add-ons Manager doesn't expect to see SCOPE_ALL as a possible value here, so it'd be safest to return SCOPE_PROFILE until you add code to return the correct scope (SCOPE_PROFILE or SCOPE_APPLICATION). ::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties @@ +109,5 @@ > type.theme.name=Appearance > type.locale.name=Languages > type.plugin.name=Plugins > type.dictionary.name=Dictionaries > +type.searchengine.name=Search Call it "Search Engines", to avoid confusion with the Add-ons Manager search.

@Blair Thanks for your feedback. I've implemented your suggested fixes, and I also enabled icons for each search engine using the iconUri parameter of the addons in the next version of the patch I am currently working on. @Anyone I have two questions about the addon sorting. First of all, it seems that the addons are automatically sorted alphabetically. This wouldn't be desired for the search engine addons because they must be sorted by the priority the user gives to each. Where does the code reside for the sorting of addons? Is there any existing way I can specify to the AddonManager how they should be sorted? Also, where would I need to go to add drag/drop or move up/move down functionality to the search addons? Thanks

(In reply to Quentin Headen from comment #50) > I have two questions about the addon sorting. First of all, it seems that > the addons are automatically sorted alphabetically. This wouldn't be desired > for the search engine addons because they must be sorted by the priority the > user gives to each. Where does the code reside for the sorting of addons? Is > there any existing way I can specify to the AddonManager how they should be > sorted? The sorting code is here: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#1316 Called from here: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#2431 At the moment, the UI only supports that one method of sorting. To support ordering: * Add an additional property to SearchEngineAddon - "orderValue", that specifies where in the list the addon is * Add a new flag for AddonType, AddonManager.TYPE_UI_ORDERED - and register the search-engines type with that (the last optional parameter of registerProvider() is for specifying flags) * In the UI (extensions.js) have gListView.show() check to see whether the addon type has that flag (you can get the type via AddonManager.addonTypes[typeName]) * Then sort based on addon.orderValue > Also, where would I need to go to add drag/drop or move up/move down > functionality to the search addons? There's two ways of doing this: - Via the addon-generic XBL binding: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.xml#792 - Or via adding a new binding to the addon-list <richlistbox>, that's used when the addon type specifies TYPE_UI_ORDERED. I'm not sure if the built-in Drag&Drop API would work for this or not. In bug 625465 I'm making it so that a single click enters the details view (instead of double click like it is now) - which means that when you start dragging an addon, there needs to be a threshold (say, 5px or 10px) before the drag actually starts.

Just FYI, if you're using any code from the existing engine manager at all, please make sure to integrate the work that landed (and is still being done for one item) in bug 709589, just so that we don't lose improvements. If you're not taking code from there, we should be good anyhow (unless you build in suboptimal constructs as well, but I hope your reviewers catch those). :)

Here is another incremental patch. I'm releasing this patch a bit early because I am a little lost on how to implement the drag/drop system. As you can see, I've added a new TYPE_UI_DRAGGABLE flag that is used along with the TYPE_UI_ORDERED flag this fix introduced. In extensions.js, I've added a sample "dragstart" event listener for each addon element in the addon category list. By adding this event listener, dragging is enabled, but I am trying to figure out how to order the engines based on that. Am I moving in the right direction here?

Comment on attachment 590566 [details] [diff] [review] Alpha Stage Patch v0.2 Review of attachment 590566 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Quentin Headen from comment #53) > As you > can see, I've added a new TYPE_UI_DRAGGABLE flag that is used along with the > TYPE_UI_ORDERED flag this fix introduced. TYPE_UI_DRAGGABLE doesn't make any sense without TYPE_UI_ORDERED, and TYPE_UI_ORDERED will always need a way for the UI to change the order, so let's just merge the two so we only have TYPE_UI_ORDERED. > In extensions.js, I've added a sample "dragstart" event listener for each > addon element in the addon category list. By adding this event listener, > dragging is enabled, but I am trying to figure out how to order the engines > based on that. We only want to be able to move items to different positions in this list, so the Drag&Drop API (dragstart, dragend, etc) may not be the best fit. If you listen for mousedown and mousemove events, you can get the position of the mouse cursor after it's moved to figure out whether the selected item should be moved up or down. > Am I moving in the right direction here? Yep, it's shaping up good :)

Note that there should be some kind of keyboard bindings or buttons or such for moving as well, as we need to care about accessibility of reordering as well.

OK, here is my new patch. I finally was able to implement the drag/drop system, and the drag/drop now also changes the engine order in the browser's preferences. I still haven't added any visual feedback to the drag, but the items are reordered after the drop. Please let me know what you think so far.

Comment on attachment 593049 [details] [diff] [review] Alpha Stage Patch v0.3 Review of attachment 593049 [details] [diff] [review]: ----------------------------------------------------------------- Great to see re-ordering working :) I do have some issues with the approach, however. ::: toolkit/components/search/nsSearchService.js @@ +3729,5 @@ > + // extracting the clean search engine name. > + aId = aId.substring(0, aId.length - 19); > + > + var engine = this.searchService.getEngineByName(aId); > + aCallback(engine); Need to wrap engine in a SearchEngineAddon (this is the reason why the details view isn't working). @@ +3765,5 @@ > + this.__defineGetter__("description", function() engine.description); > + this.__defineGetter__("iconURL", function() engine.iconURI.spec); > + > + if(orderPriority) > + this.uiPriority = orderPriority; uiPriority should be a getter and a setter. Setting it should call moveEngine, like onAddonReordered does currently (see below for the reasoning for doing this). Also, I think we need to rename uiPriority, since it isn't only used by the Add-ons Manager UI (it changes how search engines are used in the browser). Other addon types may want to use it too, for different reasons (this would fix bug 595847). How about renaming it to "position"? When moveEngine() is called, it sends a notification that the position of the engine has changed. Because any code can call that (not just the Add-ons Manager), when that happens you need to send a "onPropertyChanged" event by calling AddonManagerPrivate.callAddonListeners (like we discussed on IRC awhile ago). And the UI will need to listen for that event, and update the position of the list item for that engine. ::: toolkit/mozapps/extensions/content/extensions.js @@ +2449,5 @@ > + el.addEventListener("mousedown", function (event) { > + gListView._draggedItem = event.target; > + }); > + > + el.addEventListener("mouseup", function (event) { Will need to make sure that this doesn't leak memory, since you're never removing the event listeners. @@ +2459,5 @@ > + // Insert after on drag down, insert before on drag up > + if(newIndex > oldIndex) > + gListView.reorderItem(gListView._draggedItem, newIndex + 1); > + else if(newIndex < oldIndex) > + gListView.reorderItem(gListView._draggedItem, newIndex); This should only happen when it's been notified that the change has occurred (when a onPropertyChanged event is received) - since it could fail for some reason, or the order could be changed by something other than the Add-ons Manager. @@ +2465,5 @@ > + // Notify the provider of the change > + let provider = AddonManagerPrivate.getProviderByType(gListView._type); > + > + if(provider.onAddonReordered) // Make sure the function is defined > + provider.onAddonReordered(gListView._draggedItem.mAddon, newIndex); AddonManager.jsm is designed so you never directly use a provider, and we need to keep it that way. Whenever doing something with an addon, interact with the addon object. In other words, make it so that the following works: var addon = gListView._draggedItem.mAddon; addon.uiPriority = newIndex; Also, the UI should never touch AddonManagerPrivate, as that's meant for only providers to use. Everything else (such as the UI) should only use AddonManager. @@ +2584,5 @@ > + reorderItem: function(aItem, aNewIndex) { > + let referenceItem = null; > + > + if(this._listBox.childNodes.length > aNewIndex) > + referenceItem = this._listBox.getItemAtIndex(aNewIndex); You don't need to check the value of aNewIndex - if it's greater than length -1, then getItemAtIndex() will return null.

CC'ing fryn, who may be able to provide pointers on how to provide visual feedback when dragging items in a richlistbox.

Here is the updated patch, using the existing functions and objects to communicate with the addon manager in a more standard way. Thanks for the suggestions in the previous review. :)

Personally, I would manage it from the search bar. And make it work in the same way that the user can select and change search engines in the Firefox Search Bar. I think that makes the most sense.

(In reply to Josh Feingold from comment #60) > Personally, I would manage it from the search bar. And make it work in the > same way that the user can select and change search engines in the Firefox > Search Bar. I think that makes the most sense. I'm assuming you're coming from the Thunderbird bug (bug 724937 for everyone else). Note that this is a Toolkit bug, and so would affect Firefox as well.

Comment on attachment 594068 [details] [diff] [review] Alpha Stage Patch v0.4 Review of attachment 594068 [details] [diff] [review]: ----------------------------------------------------------------- Looking good - the code feels much cleaner in this version of the patch. ::: toolkit/components/search/nsSearchService.js @@ +2485,5 @@ > + // Register it with the addon manager > + AddonManagerPrivate.registerProvider(this.searchProvider, [ > + new AddonManagerPrivate.AddonType("searchengine", URI_EXTENSION_STRINGS, > + STRING_TYPE_NAME, > + AddonManager.VIEW_TYPE_LIST, 7000, AddonManager.TYPE_UI_ORDERED) Style nit: Indent the contents of the array, and wrap all lines to 80 characters. @@ +3713,5 @@ > } > }; > > +// This provies the list of search engines for the addons manager > +// "Search" category General style nit: Comments should end in full-stops. @@ +3726,5 @@ > + getAddonByID: function (aId, aCallback) { > + > + // Strip the 19 character @search.mozilla.org part from the ID > + // extracting the clean search engine name. > + aId = aId.substring(0, aId.length - 19); Check that aID does indeed end in "@search.mozilla.org" - if it doesn't, then it can't be a search engine and you can return early. @@ +3728,5 @@ > + // Strip the 19 character @search.mozilla.org part from the ID > + // extracting the clean search engine name. > + aId = aId.substring(0, aId.length - 19); > + > + var addon = new SearchEngineAddon(this.searchService.getEngineByName(aId), 0); Check that the result of getEngineByName() isn't null before constructing a new SearchEngineAddon. Also need to pass in the real position here. @@ +3757,5 @@ > + this.__defineGetter__("id", function() engine.name + "@search.mozilla.org"); > + this.__defineGetter__("name", function() engine.name); > + this.__defineGetter__("description", function() engine.description); > + this.__defineGetter__("iconURL", function() engine.iconURI.spec); > + this.__defineGetter__("position", function() this._position); You forgot to set this._position. The engines are showing in the UI in the right order because that's the order of the array being given to the UI (via getAddonsByTypes). @@ +3764,5 @@ > + // If we change the position of the addon, we also > + // need to move the engine associated with it > + this._position = aPosition; > + Services.search.moveEngine(this._engine, this._position); > + AddonManagerPrivate.callAddonListeners("onPropertyChanged", this, ["position"]); Still need to make this event be sent any time moveEngine is called, not just when SearchEngineAddon.position is set. ::: toolkit/mozapps/extensions/content/extensions.js @@ +2408,5 @@ > item.showInDetailView(); > } > }, false); > + > + AddonManager.addAddonListener(this); initialize() is too early to add event listeners - initialize() is called when the page is first loaded, so you'll end up getting events during times when they aren't relevant. See where addInstallListener() is called in show() and removeInstallListener() is called in hide() for where to add/remove listeners. Should also add the listener only when receiving onPropertyChanged is useful (ie, when the requested type has TYPE_UI_ORDERED). @@ +2448,5 @@ > + sortMethods = ["position"]; > + > + for (let i = 0; i < elements.length; i++) { > + elements[i].addEventListener("mousedown", gListView.onAddonDragBegin); > + elements[i].addEventListener("mouseup", gListView.onAddonDragEnd); Nit: Use "self" instead of "gListView". @@ +2452,5 @@ > + elements[i].addEventListener("mouseup", gListView.onAddonDragEnd); > + } > + } > + else > + sortMethods = ["uiState", "name"]; Style nit: When the if statement uses {}, the else should too.

I looked through the chain of comments here, and as an "average user" here is what I really think should be happening from a GUI perspective. Instead of calling it the "add-on manager", I would call it the Option Manager, which would be called from the Tools menu. I would then put the Options GUI as one of the tabs above Extensions. I would also move Preferences from Tools to a setting under Options. From there, I would put such things as Dictionaries and Search Engines under Options. The basic point is that I think as a user, we expect to go to one place in tools to change options. And all configuration can be done via that one place. Things that are part of the Core Software belong in Options, with the exception of Add-Ons and Appearance, which are different. On that note, I would say that Plugins are really Extensions from a user perspective and that they should all just be called Extensions. Having Add-Ons, Extensions, and Plugins just confuses things.

(In reply to Josh Feingold from comment #63) Some comments I will make: Please fill a separate bug. I agree that users aspect it to be all on one place, but I fear they will not do this. On a side note, Firefox Sync (formerly Weave) had an pref panel in a browser tab. That would be the right place for placing all the preferences together. But this looked to be not right, the user interface can easily be forgotten, and the user cannot easily find the option he is searching for. The pref panel of Weave was not consistent with the others (it was experimental). The designers of Weave recognized this problem, and stopped experimenting with own preference panels. If all options are ever added together, there has to be a strong control on HOW they are presented to the user. > (…) On that note, > I would say that Plugins are really Extensions from a user perspective and > that they should all just be called Extensions. Having Add-Ons, Extensions, > and Plugins just confuses things. These are surely separate things. Extensions are for customizing Firefox, plugins are cross-browser, and they customize the content of the web. Add-ons are for customizing the browser with extra stuff. The options are for customizing the default browser.

@Blair Thanks for the feedback, I'll certainly implement the changes. Where do I need to start writing tests at? Are there any tests I can hook into, or do I need to write some new ones from scratch? Thanks

(In reply to Josh Feingold from comment #63) > I looked through the chain of comments here, and as an "average user" here > is what I really think should be happening from a GUI perspective. ... (In reply to Maaren from comment #64) > Some comments I will make: These aren't directly related to this specific bug. Could you post your thoughts to the dev-apps-firefox mailing list, where discussion can happen? https://groups.google.com/forum/#!forum/mozilla.dev.apps.firefox Or: https://lists.mozilla.org/listinfo/dev-apps-firefox

(In reply to Quentin Headen from comment #65) > Where do I > need to start writing tests at? Are there any tests I can hook into, or do I > need to write some new ones from scratch? nsSearchService seems to be woefully under-tested (although bug 699856 is adding more). Any existing test is here: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/ (Yes, there's only one.) So you'll need to write new tests for nsSearchService. The Add-ons Manager is well tested. You might find some inspiration for testing nsSearchService by looking at the tests for the other providers here: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/ For example, this tests the plugins provider: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_plugins.js These tests are xpcshell tests: https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests For the changes to the UI (extensions.js), look here: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/ For example, this tests the list view, which is what you'll need to test: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_list.js These tests are browser-chrome tests: https://developer.mozilla.org/en/Browser_chrome_tests Other than tests, here's a (very) rough list of what still needs to be done: * Enabling/disabling * Uninstalling * Providing the right value for SearchEngineAddon.scope (search engines can come from the application directory) * Editing keywords * Visual feedback when dragging a search engine * A way to re-order search engines using the keyboard (Ctrl+Up, Ctrl+Down) For editing keywords, you can make all SearchEngineAddons use an inline preferences file that contains the UI to change the keyword (by doing it this way, the UI doesn't need any special-case code just for search engines - which I really want to avoid). See https://developer.mozilla.org/en/Extensions/Inline_Options You'll need a inline preferences XUL file that adds a textbox, with type="control" (so you can run custom code when the value is changed). And add two properties to SearchEngineAddon: optionsType (set to 2), and optionsURL (set to the chrome URL of the inline preferences file). Then in nsSearchService.js, observe the notification that's sent when the inline options display, as described here: https://developer.mozilla.org/en/Extensions/Inline_Options#Display_notification From there you can detect when the value is changed in the UI, and set the engine keyword.

(In reply to Blair McBride (:Unfocused) from comment #67) > nsSearchService seems to be woefully under-tested (although bug 699856 is > adding more). (see also bug 458810)

I'm working on the inline options, and things are coming along, but I'm having some trouble figuring out where I should call Services.obs.addObserver, and Services.obs.removeObserver. Can anyone give me a hint on this one? Thanks

(In reply to Quentin Headen from comment #69) > I'm working on the inline options, and things are coming along Great :) > figuring out where I should call > Services.obs.addObserver, and Services.obs.removeObserver. Call addObserver() in SearchEngineProvider.startup() - you want to observe all cases where inline options are shown, because you won't know if it's for a search engine until the notification is sent. The data argument for your observe() function will contain the addon ID. And call removeObserver() in SearchEngineProvider.shutdown(). (When you add the startup and shutdown methods, AddonManager will automatically call them at the right time.)

Here is an updated patch that contains a couple of new features. This patch adds the Ctrl-Up/Ctrl-Down keyboard reordering of the search addons. It also adds the ability to set each engine's keywords in the addon preferences. I also implemented most of the suggested fixes in comment #62 by Blair.

Comment on attachment 600846 [details] [diff] [review] Patch v0.5 Review of attachment 600846 [details] [diff] [review]: ----------------------------------------------------------------- Looking good - making some good progress here :) General comments: * Be careful about trailing whitespace and lines with only whitespace - there's a few of them in here. * The code style here is to not add newlines before '{', and not add newlines before else statements. * There's also a bunch of functions added here where the first line is empty (it shouldn't be). * Comments are sentences, end them with a full-stop. ::: toolkit/components/search/jar.mn @@ +1,2 @@ > +toolkit.jar: > +* content/components/search/searchPrefs.xul (searchPrefs.xul) Nit: Indent the last token, like other jar.mn files do (makes it easier to read). ::: toolkit/components/search/nsSearchService.js @@ +2487,5 @@ > + new AddonManagerPrivate.AddonType("searchengine", URI_EXTENSION_STRINGS, > + STRING_TYPE_NAME, > + AddonManager.VIEW_TYPE_LIST, 7000, > + AddonManager.TYPE_UI_ORDERED) > + ]); Style nit: These lines should be lined up with the first character of the first argument on the first line. @@ +3743,5 @@ > + }, > + > + getAddonByID: function(aId, aCallback) { > + > + if (aId.indexOf("@search.mozilla.org") < 0) This string should be in a constant at the top of the file (and use that for the length at well, to avoid hard-coding a magic number). @@ +3744,5 @@ > + > + getAddonByID: function(aId, aCallback) { > + > + if (aId.indexOf("@search.mozilla.org") < 0) > + return; You can't just return - the callback *has* to be called. If no addon matches, pass null when calling the callback. @@ +3750,5 @@ > + // Strip @search.mozilla.org from the id > + var engineName = aId.substring(0, aId.length - 19); > + > + var engine = this.searchService.getEngineByName(engineName); > + var position = this.searchService.getEnginePosition(engine); Should be calling getEnginePosition() only after null-checking engine. @@ +3754,5 @@ > + var position = this.searchService.getEnginePosition(engine); > + > + if (engine) { > + var addon = new SearchEngineAddon(engine, position); > + aCallback(addon); If engine is null, then the callback will never be called. If that case, just pass it null. @@ +3791,5 @@ > + > + keywordOption.addEventListener("change", function onKeywordOptionChanged(aEvent) { > + var newKeyword = keywordOption.value; > + engine.alias = newKeyword; > + keywordOption.removeEventListener("change", onKeywordOptionChanged); Should be removing this event listener when the settings stop being displayed, but there's no way to do that yet. I've filed bug 733677. @@ +3808,5 @@ > + this.__defineGetter__("position", function() this._position); > + > + this.__defineGetter__("description", function() { > + if (engine.alias) > + return "Keyword: " + engine.alias; Not sure if this makes sense or not. It'd be good to display it in the list view, but showing it in the detail view seems redundant. Could you ping Boriss on IRC (in the #ux channel) about this? If we do want to keep it, it needs to be localised. @@ +3818,5 @@ > + // If we change the position of the addon, we also > + // need to move the engine associated with it > + Services.search.moveEngine(this._engine, this._position); > + this._position = aPosition; > + AddonManagerPrivate.callAddonListeners("onPropertyChanged", this, ["position"]); Still need to make it so this event is triggered by moveEngine. A way to show why that matters: Open the add-ons manager, and open the old search engine manager. in the old search engine manager, move an engine. It should update the position in the add-ons manager (currently, it doesn't). ::: toolkit/components/search/searchPrefs.xul @@ +1,4 @@ > +<?xml version="1.0"?> > + > +<vbox xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> > + <setting pref="searchaddons.option.keyword" type="control"> Remove the pref attribute - it's not used here, and it's not required. @@ +1,5 @@ > +<?xml version="1.0"?> > + > +<vbox xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> > + <setting pref="searchaddons.option.keyword" type="control"> > + <textbox id="searchaddons-option-keyword" label="Search Engine Keyword"/> Rather than using a custom setting (type=control), use type="string". That means you can get rid of this textbox (it will be generated for you). You will need to add an "id" attribute to the <setting> though. Also, instead of the "label" attribute here, add a "title" attribute to the <setting>. That string should be localized, too (DTD files work here just like in any other XUL file). @@ +2,5 @@ > + > +<vbox xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> > + <setting pref="searchaddons.option.keyword" type="control"> > + <textbox id="searchaddons-option-keyword" label="Search Engine Keyword"/> > + Search Engine Keyword Text like this in inline settings is used for the description - it's not needed if it's just the same as the setting's title. ::: toolkit/mozapps/extensions/content/extensions.js @@ +1718,5 @@ > if (String.fromCharCode(aEvent.charCode) == "/") { > this.focusSearchBox(); > } > + // Capture Ctrl-Up and Ctrl-Down to reorder addons > + else if(aEvent.ctrlKey && (aEvent.keyCode == 38 || aEvent.keyCode == 40)) Shouldn't be handling this in gHeader - add event listeners in gListView.show(), if it's showing an addon type that uses ordering. Rather than hard-code the numeric values (38 and 40), use the constants from KeyEvent. In this case, KeyEvent.DOM_VK_UP and KeyEvent.DOM_VK_DOWN. @@ +2378,5 @@ > item.showInDetailView(); > } > }, false); > + > + AddonManager.addAddonListener(this); Still need to move this into show() - see comment 62. @@ +2382,5 @@ > + AddonManager.addAddonListener(this); > + }, > + > + shutdown: function() { > + AddonManager.removeAddonListener(this); And this into hide() @@ +2492,5 @@ > }, > + > + onPropertyChanged: function(aAddon, aProperties) { > + if (aProperties.indexOf("position") != -1) { > + var addonID = aAddon.id; addonID isn't used anywhere. @@ +2516,5 @@ > + > + var selectedItem = this.getSelectedItem(); > + var oldIndex = this._listBox.getIndexOfItem(selectedItem); > + > + if(event.keyCode == 38) // Move Up KeyEvent.DOM_VK_UP @@ +2523,5 @@ > + > + if (newIndex >= 0) > + selectedItem.mAddon.position = newIndex; > + } > + else if (event.keyCode == 40) // Move Down KeyEvent.DOM_VK_DOWN

Here is another version of my patch. I made the suggested changes, and I also fixed a major bug I found in the last patch. Let me know what you think.

Oops, I made a mistake. I forgot to "hg add" one of the new files I created. Here is the updated patch. Sorry about that.

Here is another version of the patch. This version adds simple drag/drop feedback, and it uses the dragstart, dragover, and drop events to implement the reordering system.

Since this new addons manager is replacing the old one, should I include code that causes the "Manage Search Engines" button on the toolbar search box? This would make sense because the old search engine manager will be removed.

(In reply to Quentin Headen from comment #76) > Since this new addons manager is replacing the old one, should I include > code that causes the "Manage Search Engines" button on the toolbar search > box? This would make sense because the old search engine manager will be > removed. You should file a separate bug for that - don't want this bug to get out of scope.

Comment on attachment 606837 [details] [diff] [review] Patch v0.7 Review of attachment 606837 [details] [diff] [review]: ----------------------------------------------------------------- Looking good :) Some general notes: * extensions.js and nsSearchService.js both have numerous lines that have trailing whitespace ::: toolkit/components/search/jar.mn @@ +1,2 @@ > +toolkit.jar: > +* content/components/search/searchPrefs.xul (searchPrefs.xul) This doesn't need to be run through the text preprocessor (that's what the * does) - remove the *. ::: toolkit/components/search/nsSearchService.js @@ +2562,5 @@ > gEnginesLoaded = true; > this._addObservers(); > + > + // Create the search engine provider, and give it a reference > + // of the search service Nit: Full-stop at end. @@ +2566,5 @@ > + // of the search service > + this.searchProvider = new SearchEngineProvider(); > + this.searchProvider.searchService = this; > + > + // Register it with the addon manager Nit: Full-stop at end. @@ +3472,5 @@ > // Since we moved an engine, we need to update the preferences. > this._needToSetOrderPrefs = true; > + > + // We also must reorder any addon associated with the engine, if > + // one was passed Nit: Full-stop at end. @@ +3933,5 @@ > + return; > + } > + > + // Strip @search.mozilla.org suffix from the id. > + var engineName = aId.substring(0, aId.length - 19); Use SEARCH_ADDON_ID_SUFFIX.length here, instead of a hard-coded magic number. @@ +3964,5 @@ > + aCallback(engineAddons); > + }, > + > + observe: function(aSubject, aTopic, aData) { > + //Ensure that the addon is a search engine, and that the options are being opened. Nit: Missing space at the start of the comment. Also, wrap it to 80 characters. @@ +3965,5 @@ > + }, > + > + observe: function(aSubject, aTopic, aData) { > + //Ensure that the addon is a search engine, and that the options are being opened. > + if (aData.indexOf(SEARCH_ADDON_ID_SUFFIX) < 0 || aTopic != "addon-options-displayed") Wrap this to 80 characters also. @@ +3969,5 @@ > + if (aData.indexOf(SEARCH_ADDON_ID_SUFFIX) < 0 || aTopic != "addon-options-displayed") > + return; > + > + // Strip @search.mozilla.org from the id. > + var engineName = aData.substring(0, aData.length - 19); Use SEARCH_ADDON_ID_SUFFIX.length here too. @@ +3980,5 @@ > + > + keywordOption.addEventListener("change", function onKeywordOptionChanged(aEvent) { > + var newKeyword = keywordOption.value; > + engine.alias = newKeyword; > + keywordOption.removeEventListener("change", onKeywordOptionChanged); Bug 733677 recently made it so you get a "addon-options-hidden" notification when the pref stops being shown - that's when the event listener should be removed. @@ +4016,5 @@ > +SearchEngineAddon.prototype = { > + version: "", > + creator: "", > + type: "searchengine", > + optionsType: 2, Use AddonManager.OPTIONS_TYPE_INLINE instead of a hardcoded magic number. ::: toolkit/components/search/searchPrefs.xul @@ +1,1 @@ > +<?xml version="1.0"?> New files should have the MPL2 license header added - see https://www.mozilla.org/MPL/headers/ for exact text to use. ::: toolkit/locales/en-US/chrome/search/search.dtd @@ +1,1 @@ > +<!ENTITY searchKeywordPref.label "Search Engine Keyword"> We only capitalize words like that when they're menu items, or they're proper nouns. So: "Search engine keyword" (I ping someone on the UX team about specific wording, but it seems good to me.) ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +1863,5 @@ > // 1-15 are different built-in views for the add-on type > VIEW_TYPE_LIST: "list", > > TYPE_UI_HIDE_EMPTY: 16, > + // Allow custom ordering to addons Nit: Full-stop at end. ::: toolkit/mozapps/extensions/content/extensions.js @@ +1324,5 @@ > return (UISTATE_ORDER.indexOf(a) - UISTATE_ORDER.indexOf(b)); > } > + > + function positionCompare(a, b) { > + // Sort the elements by the value stored in their position attribute Nit: Full-stop at end. @@ +2397,2 @@ > elements.push(createItem(aAddonsList[i])); > + } No need for this change. @@ +2401,2 @@ > elements.push(createItem(aInstallsList[i], true)); > + } Ditto. @@ +2407,5 @@ > + // Check if we want custom, priority-ordered lists lists. > + if (addonType.flags & AddonManager.TYPE_UI_ORDERED) { > + sortMethods = ["position"]; > + > + for (let i = 0; i < elements.length; i++) { Convert this to a for...of loop. See: https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...of (And see bug 740237 for converting the rest of the code to use those.) @@ +2413,5 @@ > + elements[i].addEventListener("dragover", self.onAddonDraggedOver); > + elements[i].addEventListener("drop", self.onAddonDragEnd); > + > + // Allow order to be changed by Ctrl-Up and Ctrl-Down. > + self._listBox.addEventListener("keydown", self.onAddonKeyboardMove); This should be outside the loop. @@ +2437,5 @@ > }, > > hide: function() { > + > + for (let i = 0; i < this._listBox.children.length; i++) { Convert this to a for...of loop too. You also won't need to use getItemAtIndex(i) then. Also, this should only be run if the addon type that was shown has TYPE_UI_ORDERED. Also, remove the above blank line. @@ +2440,5 @@ > + > + for (let i = 0; i < this._listBox.children.length; i++) { > + this._listBox.getItemAtIndex(i).removeEventListener("dragstart", self.onAddonDragBegin); > + this._listBox.getItemAtIndex(i).removeEventListener("dragover", self.onAddonDraggedOver); > + this._listBox.getItemAtIndex(i).removeEventListener("drop", self.onAddonDragEnd); "self" isn't defined in this function, you want "this". @@ +2443,5 @@ > + this._listBox.getItemAtIndex(i).removeEventListener("dragover", self.onAddonDraggedOver); > + this._listBox.getItemAtIndex(i).removeEventListener("drop", self.onAddonDragEnd); > + } > + > + this._listBox.removeEventListener("keydown", self.onAddonKeyboardMove); Ditto. @@ +2449,3 @@ > gEventManager.unregisterInstallListener(this); > doPendingUninstalls(this._listBox); > + AddonManager.removeAddonListener(this); Nit: Move this up one line, so it's directly by the call to unregisterInstallListener() (since they're doing related things). @@ +2496,5 @@ > + this.reorderItem(listItem, aAddon.position); > + } > + }, > + > + onAddonDragBegin: function(aEvent) { The "Addon" in these function names feels redunadant - lets just name them onDragStart, etc. @@ +2503,5 @@ > + "class", "icon"); > + > + var dt = aEvent.dataTransfer; > + dt.setData("application/x-moz-node", aEvent.target); > + dt.setDragImage(icon, 10, 10); We should have this be the full list item (ie, aEvent.target). If you pass a non-image element to setDragImage, it'll automatically create an image for you, which is handy. Also, the coordinates passed to setDragImage should be the mouse position on that list item, so that the drag image is at the right position to the mouse cursor. Should be able to calculate that based on the coordinates in aEvent (pageX/Y maybe?), and maybe aEvent.target.boxObject.x/y @@ +2512,5 @@ > + var validType = dt.types.contains("application/x-moz-node"); > + > + if (validType) { > + aEvent.preventDefault(); > + } Nit: No need for { and } curly braces here (the code style here is to not use them when the contents are only one line). @@ +2525,5 @@ > + draggedItem.mAddon.position = newIndex; > + } > + }, > + > + onAddonKeyboardMove: function(aEvent) { "onKeyDown" @@ +2528,5 @@ > + > + onAddonKeyboardMove: function(aEvent) { > + if (!aEvent.ctrlKey) { > + return; > + } Nit: No curly braces. @@ +2538,5 @@ > + var newIndex = oldIndex - 1; > + > + if (newIndex >= 0) { > + this.selectedItem.mAddon.position = newIndex; > + } Nit: No curly braces. @@ +2544,5 @@ > + var newIndex = oldIndex + 1; > + > + if (newIndex < this.childNodes.length) { > + this.selectedItem.mAddon.position = newIndex; > + } Nit: No curly braces. @@ +2604,5 @@ > + reorderItem: function(aItem, aNewIndex) { > + var oldIndex = this._listBox.getIndexOfItem(aItem); > + var referenceItem = this._listBox.getItemAtIndex(aNewIndex); > + > + if (aNewIndex > oldIndex)// Insert after on drag down Nit: space before the //, full-stop at end. @@ +2606,5 @@ > + var referenceItem = this._listBox.getItemAtIndex(aNewIndex); > + > + if (aNewIndex > oldIndex)// Insert after on drag down > + this._listBox.insertBefore(aItem, referenceItem.nextSibling); > + else if (aNewIndex < oldIndex) // Insert before on drag up Nit: Full-stop at end.

Here is a new patch. This patch adds a few things. First, it adds the ability to uninstall search engines. Second, it adds the ability to enable and disable search engines. Finally, I added some code to correctly set the scope of the search engine addons. I wasn't too sure about the scope code, so please let me know if I am going in the right direction with that.

Comment on attachment 615164 [details] [diff] [review] Patch v0.8 I didn't look through this in detail, I just did a quick skim and noticed a few things: >diff -r bd0775d544ba netwerk/base/public/nsIBrowserSearchService.idl > interface nsIBrowserSearchService : nsISupports >+ void hideEngine(in nsISearchEngine engine); >+ void showEngine(in nsISearchEngine engine); These seem unnecessary - why not just set engine.hidden? >diff -r bd0775d544ba toolkit/components/search/nsSearchService.js >+function SearchEngineAddon(aEngine, aPosition) { Is there a reason why this uses __defineGetter__ a lot, rather than just defining getters on the prototype? >+SearchEngineAddon.prototype = { >+ uninstall: function() { >+ Services.search.removeEngine(this._engine); This seems like it might be problematic for app-installed engines, for which removeEngine just toggles their hidden status. You may need to expose a way to determine whether the engines can actually be removed (e.g. a "removable" attribute on nsISearchEngine?). >+ var defaultEngine = Services.search.defaultEngine; >+ if (defaultEngine) { >+ Services.search.currentEngine = defaultEngine; >+ } This isn't necessary, removeEngine handles this for you.

@Gavin I agree with those suggestions, especially the one about __defineGetter__. I was wondering about one more thing. I notice that both the PluginProvider and the LightweightThemeManager are both defined in their own jsm files. Would it be better if I define the SearchEngineProvider and the SearchEngineAddon in its own jsm file as well?

Depends on how much it needs to access nsSearchService internals, I guess. It would be nice to isolate it in its own module if that's feasible.

Yes, it might be better to isolate SearchEngineProvider and SearchEngineAddon. Because from the way they are designed now, they make use of the external interface to the nsSearchService (i.e. Services.search).

Here is yet another version of the patch. This version moves the SearchEngineProvider and SearchEngineAddon objects into a JavaScript module. It looks much cleaner this way, and it allows other areas of Mozilla's code to access both objects. In this patch, I also moved all get/set accessors into the prototype of SearchEngineAddon, which also looks more consistent. As for functionality, this patch is the same.

Sorry guys. I made a mistake and removed an essential change from the patch, resulting in a breakage of the reordering functionality for the search engines. This new patch fixes it.

Comment on attachment 616237 [details] [diff] [review] Patch v0.9.1 Review of attachment 616237 [details] [diff] [review]: ----------------------------------------------------------------- (Sorry for the delay - I've been off work due to illness. Then Bugzilla ate my review the first time I wrote it.) General notes: * Splitting this into it's own module seems to have worked nicely * You'll have the same trouble with .userDisabled as you did with .position - the state of the search engine can be changed without ever touching the code in SearchEngineAddon. * Need to fire the appropriate events for uninstalling (onUninstalling, onUninstalled), installing (onExternalInstall, onInstalling, onInstalled), and updating (same events as installing) of search engines. See https://developer.mozilla.org/en/Addons/Add-on_Manager/InstallListener for details on how onExternalInstall works (LightweightThemeManager uses this event). * For updates to a search engine, I think all you need to do is fire the right events (onExternalInstall, onInstalling, onInstalled), and everything else will be handled automatically ::: netwerk/base/public/nsIBrowserSearchService.idl @@ +257,5 @@ > * exist. > */ > nsISearchEngine getEngineByName(in AString aEngineName); > > + /** Returns the ordered position of a given engine in the service. Nit: This should be on the second line of the comment, like the other comments in this file. ::: toolkit/components/search/SearchEngineProvider.jsm @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +var EXPORTED_SYMBOLS = ["SearchEngineProvider", "SearchEngineAddon"]; SearchEngineAddon shouldn't be exported - it can't be used outside of this module. In fact, XPIProvider.jsm and PluginProvider.jsm don't even export their provider objects, since the only way to safely use them is via the AddonManager APIs. @@ +13,5 @@ > +const STRING_TYPE_NAME = "type.%ID%.name"; > +const SEARCH_ADDON_ID_SUFFIX = "@search.mozilla.org"; > + > +// The SearchEngineProvider object is used to provide search engine addons > +// for the AddonManager to display under the Search category. Nit: "Search engines category" @@ +26,5 @@ > + }, > + > + getAddonByID: function(aId, aCallback) { > + > + if (aId.indexOf(SEARCH_ADDON_ID_SUFFIX) < 0) { Nit: Unnecessary blank line above. @@ +27,5 @@ > + > + getAddonByID: function(aId, aCallback) { > + > + if (aId.indexOf(SEARCH_ADDON_ID_SUFFIX) < 0) { > + aCallback([]); The callback for getAddonByID expects an object, not an array - so you should be passing in null here. @@ +91,5 @@ > + var newKeyword = keywordOption.value; > + engine.alias = newKeyword; > + }); > + > + keywordOption.addEventListener("addon-options-hidden", function(aEvent) { addon-options-hidden isn't an event, it's an observer notification like addon-options-displayed is. Also, it's possible that the document (aSubject) you get for addon-options-hidden might NOT be the same document as you got for the last addon-options-displayed notification. So you need to make sure you're removing the right event listener (which I think means that you need to define onKeywordOptionChanged as a property of SearchEngineProvider). @@ +117,5 @@ > + _engine: null, > + > + // Addon methods > + uninstall: function() { > + Services.search.removeEngine(this._engine); As Gavin said in comment 80, this will cause problems for search engines that are in the application directory (also for those installed as part of an extension). In those cases, search engines can't truly be uninstalled - they're just hidden. But we're now using that for disabling engines. This is a generic problem with normal addons too, which just don't allow uninstall in these cases. That should change in the future (see bug 640775, bug 732005). But it's a tricky UX problem: Such addons are generally installed as part of other software (or as part of Firefox) - if you uninstall it's addon, how do you get it back? Is it ok to tell the user an addon was uninstalled, when it was really only disabled and hidden? Until we have a generic solution to that issue, I think we should just do what normal addons do: If a search engine isn't installed in the user profile (ie, _engine._isInProfile == false), then don't return PERM_CAN_UNINSTALL in the permissions property (and make uninstall() throw an exception). Additionally, when an engine is successfully uninstalled, you'll need to fire the "onUninstalling" and "onUninstalled" events via the Addons Manager. @@ +130,5 @@ > + return this._engine.name; > + }, > + > + get iconURL() { > + return this._engine.iconURI.spec; It's possible for _engine.iconURI to be null, in which case this will throw. It should return null if there's no icon. @@ +145,5 @@ > + return ""; > + }, > + > + get userDisabled() { > + return this._disabled; I don't think you need this._disabled - this getter can just return this._engine.hidden (and it will always be in sync that way) @@ +157,5 @@ > + return Ci.nsIBlocklistService.STATE_NOT_BLOCKED; > + }, > + > + get isActive() { > + return true; isActive indicates whether an addon is enabled or not. For normal addons, its based on appDisabled, softDisabled, and userDisabled - though we only care about userDisabled for search engines. @@ +187,5 @@ > + > + get scope() { > + var scope = 0; > + > + if (this._engine._isDefault()) this._engine._isDefault is a property, not a method. Although, I think you should be using _engine._isInProfile here, since that's what you're really checking. There isn't a direct 1:1 match with how search engines are installed, and the different scopes the Addons Manager knows about. I think just splitting it into SCOPE_PROFILE and SCOPE_APPLICATION like you have is the best way of handling that. @@ +190,5 @@ > + > + if (this._engine._isDefault()) > + scope = AddonManager.SCOPE_APPLICATION; > + else > + scope = AddonManager.SCOPE_PROFILE; Nit: This is very verbose - just return the constant directly, no need to use a temporary variable. @@ +196,5 @@ > + return scope; > + }, > + > + get providesUpdatesSecurely() { > + return true; This won't always be true - need to check whether updateURL is a https:// URL. @@ +200,5 @@ > + return true; > + }, > + > + // Set Accessors > + set position(aVal) { Nit: Move these setters so they're right by their respective getter - makes the code easier to read, as you only need to look at once place for a given property. @@ +203,5 @@ > + // Set Accessors > + set position(aVal) { > + // If we change the position of the addon, we also > + // need to move the engine associated with it. > + this._position = aVal; Need to make sure aVal is in fact an integer, and throw an exception if it's not. It'll break in sorts of weird ways it you set this to a string :) @@ +219,5 @@ > + else > + this._engine.hidden = false; > + > + AddonManagerPrivate.callAddonListeners(aVal ? "onDisabling" : "onEnabling", this, false); > + AddonManagerPrivate.callAddonListeners(aVal ? "onDisabled" : "onEnabled", this); Similar to position, it's not only this setter that can change engine.hidden. You need to fire onDisabling/onDisabled *any time* engine.hidden changes - ie, triggered via the hidden setter itself. One way to do that would be to listen for the SEARCH_ENGINE_CHANGED notification, and figure out which property changed (eg, hidden, position, etc), and fire the appropriate Addon Manager event. And you really don't need this._disabled (see my comment on the disabled getter), so this property should only be a wrapper for this._engine.hidden (ie, it should *only* set or get this._engine.hidden). ::: toolkit/components/search/nsSearchService.js @@ +3461,5 @@ > + // associated with this search engine. > + SearchEngineProvider.getAddonByID(aEngine.name + SEARCH_ADDON_ID_SUFFIX, > + function(aAddon) { > + if (aAddon) { > + AddonManagerPrivate.callAddonListeners("onPropertyChanged", aAddon, Since you're refactored SearchEngineProvider into it's own module, this doesn't really fit here anymore. Ideally, nsSearchService shouldn't need to touch AddonManager/AddonManagerPrivate, or SearchEngineProvider. As a possible alternative, there's a notification sent above (notifyAction) - should be able to just listen for that in SearchEngineProvider, and call AddonManagerPrivate.callAddonListeners() from there. @@ +3467,5 @@ > + } > + }); > + }, > + > + hideEngine: function SRCH_SVC_hideEngine(aEngine) { Forgot to remove these functions? hideEngine and showEngine ::: toolkit/mozapps/extensions/content/extensions.js @@ +1325,5 @@ > } > > + function positionCompare(a, b) { > + // Sort the elements by the value stored in their position attribute. > + return a.position - b.position; This should ideally take into account the possibility of .position being undefined. It shouldn't be possible for that to happen with search engines, but it could be possible if another addon type starts using TYPE_UI_ORDERED. @@ +2414,5 @@ > + > + // Allow order to be changed by Ctrl-Up and Ctrl-Down. > + self._listBox.addEventListener("keydown", self.onKeyDown); > + } else { > + sortMethods = ["uiState", "name"]; Just set this as the initial value of sortMethods, rather than an empty array that's never used. Makes it more obvious when reading the code too, IMO. @@ +2498,5 @@ > + } > + }, > + > + onDragBegin: function(aEvent) { > + gListView._draggedItem = aEvent.target; Seems you don't need gListView._draggedItem any more, since it's set as the dataTransfer. Should be getting the node from that in onDragEnd. @@ +2501,5 @@ > + onDragBegin: function(aEvent) { > + gListView._draggedItem = aEvent.target; > + > + var positionX = aEvent.target.pageX; > + var positionY = aEvent.target.pageY; This isn't doing anything - pageX is a property of aEvent, not aEvent.target. But what you really want is: aEvent.pageX - aEvent.target.boxObject.x That will position the image such that the cursor is in the same position on the image, as it was on the actual list item. (Ignoring the small amount of scaling that happens to the image. Not sure what we can do about that, but it's a small detail.)

Quentin: Any update on this?

Sorry I've been slow on this bug. I've been caught up on bug 731667. I'll slow down on that one just a bit so that I can dedicate more time to this one as well. Both are very important to the browser. I should be releasing a patch for this bug in a week or so.

Here is the new patch I had promised. I'm sorry it took so long, I've been wrangling with other bugs. In this patch, I've implemented the suggestions that were given on comment 86. It seems to be working well.

Comment on attachment 650437 [details] [diff] [review] Patch 11 Review of attachment 650437 [details] [diff] [review]: ----------------------------------------------------------------- Good to see this progressing again :) Bug 767236 has been working towards naming all the anonymous functions in the Add-ons Manager code, so any functions added here should be named too. The changes for extensions.js have landed, so there's some examples there. But the general convention we've been using for properties on objects is ObjectName_propertyName ::: netwerk/base/public/nsIBrowserSearchService.idl @@ +260,5 @@ > * exist. > */ > nsISearchEngine getEngineByName(in AString aEngineName); > > + /** Nit: Trailing whitespace here (the review tool makes these stick out like a neon sign, now I can't help but comment on them...) @@ +265,5 @@ > + * Returns the ordered position of a given engine in the service. > + * > + * @param aEngine The engine to get the position of. > + * > + * @returns The array position index of the engine. Should mention it can also return -1 ::: toolkit/components/search/SearchEngineProvider.jsm @@ +4,5 @@ > + > +Components.utils.import("resource://gre/modules/Services.jsm"); > +Components.utils.import("resource://gre/modules/AddonManager.jsm"); > + > +var EXPORTED_SYMBOLS = []; Nit: Its customary to put this line above other code such as imports. @@ +22,5 @@ > +// The SearchEngineProvider object is used to provide search engine addons > +// for the AddonManager to display under the Search engines category. > +var SearchEngineProvider = { > + // Keeps a persistent reference to the keyword option DOM textbox > + _keywordOption: null, We need to support the possibility of about:addons being loaded into multiple tabs, which means there may be more than one instance of the inline prefs open at once. Should be able to handle those events without needing to store this anyway. @@ +28,5 @@ > + startup: function() { > + Services.obs.addObserver(this, ADDON_OPTIONS_DISPLAYED, false); > + Services.obs.addObserver(this, ADDON_OPTIONS_HIDDEN, false); > + Services.obs.addObserver(this, SEARCH_ENGINE_TOPIC, false); > + AddonManager.addAddonListener(this); Why do you need to listen for AddonListener events? Just for testing? @@ +39,5 @@ > + AddonManager.removeAddonListener(this); > + }, > + > + getAddonByID: function(aId, aCallback) { > + if (aId.indexOf(SEARCH_ADDON_ID_SUFFIX) < 0) { Hmm, this worries be a little - can you make it a little safer by ensuring SEARCH_ADDON_ID_SUFFIX is at the end of the ID? @@ +56,5 @@ > + var position = Services.search.getEnginePosition(engine); > + > + // A position of -1 indicates the engine wasn't found > + if (position >= 0) > + addon = new SearchEngineAddon(engine, position); Should cache these (XPIProvider does this), to avoid always constructing new objects every time. That should also help solve the issue with keeping _position updated (see below). @@ +79,5 @@ > + > + aCallback(engineAddons); > + }, > + > + observe: function(aSubject, aTopic, aData) { Hm, this is a rather long method - it'd probably benefit from being split up. @@ +89,5 @@ > + this.getAddonByID(engine.name + SEARCH_ADDON_ID_SUFFIX, > + function(aAddon) { > + if (aAddon) { > + // Notify the addon manager of a possible position change. > + AddonManagerPrivate.callAddonListeners("onPropertyChanged", Nit: Trailing whitespace. @@ +111,5 @@ > + case ADDON_OPTIONS_DISPLAYED: > + // Ensure that the addon is a search engine, and that the options are > + // being opened. > + if (aData.indexOf(SEARCH_ADDON_ID_SUFFIX) < 0 || > + aTopic != "addon-options-displayed") Ensure SEARCH_ADDON_ID_SUFFIX is at the end of the ID? Also, checking aTopic here again doesn't seem necessary. @@ +116,5 @@ > + return; > + > + // Strip @search.mozilla.org from the id. > + var engineName = aData.substring(0, aData.length - > + SEARCH_ADDON_ID_SUFFIX.length); You do this conversion (ID -> name) a lot, might be worth splitting it out to it's own function. And maybe the same with checking the ID ends in SEARCH_ADDON_ID_SUFFIX too. @@ +119,5 @@ > + var engineName = aData.substring(0, aData.length - > + SEARCH_ADDON_ID_SUFFIX.length); > + var engine = Services.search.getEngineByName(engineName); > + > + if (engine) { Can just return early here. @@ +183,5 @@ > +} > + > +SearchEngineAddon.prototype = { > + version: "", > + creator: "", creator is expected to be an object (AddonManagerPrivate.AddonAuthor), so if its empty it should be null. @@ +186,5 @@ > + version: "", > + creator: "", > + type: "searchengine", > + optionsType: AddonManager.OPTIONS_TYPE_INLINE, > + optionsURL: "chrome://toolkit/content/searchPrefs.xul", Should probably convert these to getters, to ensure they're read-only. Could have just used Object.freeze(), but _position needs to be able to be updated :\ (This is why XPIProvider's AddonWrapper uses __defineGetter__) @@ +204,5 @@ > + get name() { > + return this._engine.name; > + }, > + > + get iconURL() { Bug 760892 recently added a generic "icon" property to Addon objects. It holds an object whose property names are the size in pixels of an icon, and the value is the URL to that size icon. Still need to support iconURL though. @@ +219,5 @@ > + }, > + > + set position(aVal) { > + // Ensure that aVal is a number > + if (isNaN(aVal)) This will guard against non-numbers, but will let through floats. @@ +224,5 @@ > + return; > + > + // If we change the position of the addon, we also > + // need to move the engine associated with it. > + this._position = aVal; Setting this._position here means it will get out of sync when something else changes the position of the engine. @@ +271,5 @@ > + > + get permissions() { > + // Only allow search addons installed in a user's profile to be uninstalled. > + if (this.scope == AddonManager.SCOPE_PROFILE) > + var permissions = AddonManager.PERM_CAN_UNINSTALL; I like to avoid declaring variables in conditions, and using them outside it - IMO it makes it easy to mis-read the code. Declare/set permissions=0 above? ::: toolkit/components/search/nsSearchService.js @@ +8,5 @@ > > Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > Components.utils.import("resource://gre/modules/Services.jsm"); > +Components.utils.import("resource://gre/modules/AddonManager.jsm"); > +Components.utils.import("resource://gre/modules/SearchEngineProvider.jsm"); Looks like AddonManager.jsm doesn't need imported here anymore. And SearchEngineProvider isn't used here, but it needs loaded somehow. See how the plugin provider does it: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/extensions.manifest#11 @@ +65,5 @@ > const SEARCH_DATA_XML = Ci.nsISearchEngine.DATA_XML; > const SEARCH_DATA_TEXT = Ci.nsISearchEngine.DATA_TEXT; > > +// The ID suffix used when searching for an addon by its ID > +const SEARCH_ADDON_ID_SUFFIX = "@search.mozilla.org"; This is unused now too. @@ +3338,5 @@ > } > return null; > }, > > + getEnginePosition: function SRCH_SVC_getEnginePosition(aEngine) { Nit: Trailing whitespace. ::: toolkit/mozapps/extensions/content/extensions.js @@ +1293,5 @@ > + function positionCompare(a, b) { > + // Sort the elements by the value stored in their position attribute. > + // We must also anticipate one or two undefined positions. > + if ((a == undefined && b != undefined) || > + (b == undefined && a != undefined)) Nit: Trailing whitespace on these 2 lines. @@ +1295,5 @@ > + // We must also anticipate one or two undefined positions. > + if ((a == undefined && b != undefined) || > + (b == undefined && a != undefined)) > + return -1; > + else if (a == undefined && b == undefined) These if-statements can be simplified a bit if this one (both undefined) is first. @@ +2406,5 @@ > gEventManager.unregisterInstallListener(this); > doPendingUninstalls(this._listBox); > + > + let addonType = AddonManager.addonTypes[this._type]; > + if(addonType.flags & AddonManager.TYPE_UI_ORDERED) { Nit: Missing space after 'if' @@ +2415,5 @@ > + } > + this._listBox.removeEventListener("keydown", this.onKeyDown); > + } > + gEventManager.unregisterInstallListener(this); > + AddonManager.removeAddonListener(this); Calling unregisterInstallListener() twice in this method. And removeAddonListener() should be called before doPendingUnininstalls(), as doPendingUnininstalls() will trigger events that this view no longer cares about. @@ +2498,5 @@ > + if (!aEvent.ctrlKey) > + return; > + > + // "this" should refer to the listbox object. > + var oldIndex = this.getIndexOfItem(this.selectedItem); Nit: Move this so its only executed when needed.

Have you been able to make any more progress here, Quentin?

Hey Blair. I've been progressing slowly on it because I was wrapping up bug 731667. The final code for that one is now in review, so I can give all of my development time to this one. I'm going to start working full fledged on it tomorrow.

Great - thanks for the update :)

Here is an updated version of the patch. From what I see, it implements all of the suggestions Blair made in the last two feedback comments he posted. I also implemented the install/uninstall code for the search engines addons. I tested the installation and uninstallation of search addons manually, and all seems to work well. I also removed all of the trailing whitespace my patch had added before. This patch has no trailing whitespace in it.

Oops, I forgot to re-remove the exportation of the SearchEngineProvider in SearchEngineProvider.js. I had exported it again so I could manually work with the object in Scratchpad for debugging. Please ignore that line. It will be removed in the next patch.

Comment on attachment 682704 [details] [diff] [review] Patch 12 Review of attachment 682704 [details] [diff] [review]: ----------------------------------------------------------------- This is feeling a lot more polished - good work :) ::: toolkit/components/search/SearchEngineProvider.jsm @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public You're using engine.wrappedJSObject.whatever a lot in this file - in most cases, you can avoid wrappedJSObject, as the property is defined in nsISearchEngine. If you're unfamiliar with wrappedJSObject, it lets you access parts of the JS object that aren't exposed via the XPCOM interface for that JS XPCOM object. In general, it's nice to avoid using it for objects you haven't created - ie, use their public API, rather than their internal state (it won't always be possible here though). @@ +6,5 @@ > + > +Components.utils.import("resource://gre/modules/Services.jsm"); > +Components.utils.import("resource://gre/modules/AddonManager.jsm"); > + > +const Ci = Components.interfaces; Nit: Its customary to put this line above other code such as imports. @@ +50,5 @@ > + > + if (engine) { > + var position = Services.search.getEnginePosition(engine); > + > + if (position >= 0) { Add a comment here explaining when this can be false, as it's not obvious. @@ +75,5 @@ > + var engines = Services.search.getEngines({}); > + var engineAddons = []; > + > + // Build the array of engine addons. > + for (var i = 0; i < engines.length; i++) { Try to use for-of loops where you can, they make for cleaner and simpler code: https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for...of @@ +86,5 @@ > + if (this._addonCache.has(engineID)) { > + addon = this._addonCache.get(engineID); > + } else { > + addon = new SearchEngineAddon(engines[i]); > + this._addonCache.set(engineID, addon); Can simplify this a bit via: if (!this._addonCache.has(engineID)) this._addonCache.set(engineID, new SearchEngineAddon(engine)); engineAddons.push(this._addonCache.get(engineID)); @@ +129,5 @@ > + }, > + > + onSearchEngineAdded: function(aEngine) { > + if (!aEngine) > + return; Is this ever possible? Ditto for the same check in onSearchEngineRemoved. @@ +141,5 @@ > + AddonManagerPrivate.callInstallListeners("onExternalInstall", null, > + addon, null, false); > + AddonManagerPrivate.callAddonListeners("onInstalling", addon, false); > + AddonManagerPrivate.callAddonListeners("onInstalled", addon, false); > + AddonManagerPrivate.callAddonListeners("onPropertyChanged", addon, I don't think onPropertyChanged should be needed here. If the UI isn't updating correctly without it, that sounds like a bug in the UI. @@ +159,5 @@ > + }); > + }, > + > + onSearchEngineMoved: function(aEngine) { > + this.getAddonByID(aEngine.wrappedJSObject.name + SEARCH_ADDON_ID_SUFFIX, Split this ID into a separate variable like do you in onSearchEngineRemoved? Ditto in onSearchEngineHiddenChanged. @@ +175,5 @@ > + function(aAddon) { > + if (aAddon) { > + AddonManagerPrivate.callAddonListeners( > + aEngine.wrappedJSObject.hidden ? "onDisabling" : "onEnabling", aAddon, false); > + AddonManagerPrivate.callAddonListeners( This is a bit hard to read - just use a normal if-else? @@ +217,5 @@ > + engine.wrappedJSObject.alias = keywordOption.value; > + }, > + > + // Utility methods. > + isValidSearchAddonID: function(aId) { We have a *very* new shiny API to do this with one line of code now :) someString.endsWith(otherString) will return true if |otherString| is at the end of |someString|. ie: aId.endsWith(...); @@ +259,5 @@ > + this.__defineGetter__("optionsURL", > + function() "chrome://toolkit/content/searchPrefs.xul"); > +} > + > +SearchEngineAddon.prototype = { Missing some required methods/properties from https://developer.mozilla.org/en-US/docs/Addons/Add-on_Manager/Addon That includes findUpdates(), which needs to send some events even if it doesn't do anything - see toolkit/mozapps/extensions/LightweightThemeManager.jsm for the minimum you need to do for that method. @@ +278,5 @@ > + }, > + > + get icons() { > + var url = this.iconURL; > + return {"32" : url}; Should return an empty object here if iconURL is null. @@ +296,5 @@ > + }, > + > + set position(aVal) { > + // Ensure that aVal is an integer. > + if (isNaN(aVal) || (parseFloat(aVal) != parseInt(aVal))) Thanks to the wonders of type coercion, |"555" == 555| is true. So this can be simplified to: if (aVal != parseInt(aVal)) @@ +304,5 @@ > + }, > + > + get description() { > + if (this._engine.alias) > + return "Keyword: " + this._engine.alias; Need to put this into a .properties file somewhere, for localization. @@ +349,5 @@ > + // Only allow search addons installed in a user's profile to be uninstalled. > + if (this.scope != AddonManager.SCOPE_APPLICATION) > + permissions |= AddonManager.PERM_CAN_UNINSTALL; > + > + if (!this.appDisabled) { This doesn't seem necessary, given appDisabled is always false. ::: toolkit/components/search/nsSearchService.js @@ +7,5 @@ > const Cr = Components.results; > > Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > Components.utils.import("resource://gre/modules/Services.jsm"); > +Components.utils.import("resource://gre/modules/AddonManager.jsm"); AddonsManager.jsm doesn't needed to be imported here, does it? @@ +3284,5 @@ > } > return null; > }, > > + getEnginePosition: function SRCH_SVC_getEnginePosition(aEngine) { Isn't this function equivalent to the following? this._sortedEngines.indexOf(aEngine.wrappedJSObject); ::: toolkit/mozapps/extensions/content/extensions.js @@ +2463,5 @@ > if (aInstall.existingAddon) > this.removeItem(aInstall, true); > }, > > + onPropertyChanged: function gListView_onPropertyChanged(aAddon, aProperties) { General nit: These functions added here, and reorderItem, use 'var' - new code should generally use 'let', unless 'var' is explicitly needed (pretty rare).

Quentin, will you have time to address Blair's feedback sometime soon?

Actually, I don't think I will be able to. I swamped with school work right now. Did you or anyone else want to take over this bug for now? I want to work on it, but I see that I really don't have the time.

Unassigning, as per Quentin's request.

I've unbitrotted this and tried to address the feedback from comment 99. I've left the patch username as you, Quentin, because I've barely had to do anything, so unless large parts need revisiting I think you should get the credits here. This isn't quite there yet. A quick thing I noticed was that now, if I install a search engine with the add-on manager open in another tab, after switching to the add-on manager it shows up at the top with no icon and "undefined" as the name... Some comments back to Blair about things I wasn't sure about: (In reply to Blair McBride [:Unfocused] from comment #96) > You're using engine.wrappedJSObject.whatever a lot in this file - in most > cases, you can avoid wrappedJSObject ... (it won't always be possible here though). I've replaced it everywhere in the file. As far as I could tell it was only used for .name, .alias and .hidden - all of which seem to be exposed on nsISearchEngine? But maybe I missed something. In particular, maybe this is why I'm now seeing 'undefined' as the name of the search engine. However, I don't have time to debug this right now, and I guess it might help the next person if I still put this up... :-) > @@ +50,5 @@ > > + > > + if (engine) { > > + var position = Services.search.getEnginePosition(engine); > > + > > + if (position >= 0) { > > Add a comment here explaining when this can be false, as it's not obvious. In fact, it's so non-obvious that I looked at this for 15 minutes, and gave up. I've left it in there, but I don't understand how this could be the case. Maybe it's because I'm sleepy... > @@ +129,5 @@ > > + }, > > + > > + onSearchEngineAdded: function(aEngine) { > > + if (!aEngine) > > + return; > > Is this ever possible? Ditto for the same check in onSearchEngineRemoved. I do not believe so, and have removed them. > @@ +141,5 @@ > > + AddonManagerPrivate.callInstallListeners("onExternalInstall", null, > > + addon, null, false); > > + AddonManagerPrivate.callAddonListeners("onInstalling", addon, false); > > + AddonManagerPrivate.callAddonListeners("onInstalled", addon, false); > > + AddonManagerPrivate.callAddonListeners("onPropertyChanged", addon, > > I don't think onPropertyChanged should be needed here. If the UI isn't > updating correctly without it, that sounds like a bug in the UI. Removed, but see the top of this comment... sounds like I need to doublecheck what the UI is doing in this case. > @@ +259,5 @@ > > + this.__defineGetter__("optionsURL", > > + function() "chrome://toolkit/content/searchPrefs.xul"); > > +} > > + > > +SearchEngineAddon.prototype = { > > Missing some required methods/properties from > https://developer.mozilla.org/en-US/docs/Addons/Add-on_Manager/Addon > That includes findUpdates(), which needs to send some events even if it > doesn't do anything - see > toolkit/mozapps/extensions/LightweightThemeManager.jsm for the minimum you > need to do for that method. I believe I've added all of these now. > @@ +304,5 @@ > > + }, > > + > > + get description() { > > + if (this._engine.alias) > > + return "Keyword: " + this._engine.alias; > > Need to put this into a .properties file somewhere, for localization. Done. > @@ +3284,5 @@ > > } > > return null; > > }, > > > > + getEnginePosition: function SRCH_SVC_getEnginePosition(aEngine) { > > Isn't this function equivalent to the following? > this._sortedEngines.indexOf(aEngine.wrappedJSObject); I think so; changed. > General nit: These functions added here, and reorderItem, use 'var' - new > code should generally use 'let', unless 'var' is explicitly needed (pretty > rare). All of it should now be using 'let' unless I missed something.

Wow! This bug is still open? What else needs to be done on it?

Yea, everyone has been focused on getting Australis finished :) If you have a bit of time for this now, that's be awesome :) (Apologies in advance, my review queue is massive at the moment) I don't have time to go through this in detail to see what's missing, but IIRC there's not much. Gijs mentioned a bug at the beginning of comment 100 around issues of having two about:addons tabs open at the same time. If everything is working correctly, they should always stay in sync. I suspect there may be a couple of bugs left around there - so I'd recommend just playing around with having two tabs open.

Comment on attachment 771717 [details] [diff] [review] Patch 12.1 (unbitrotted) Review of attachment 771717 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/search/SearchEngineProvider.jsm @@ +245,5 @@ > + uninstall: function() { > + Services.search.removeEngine(this._engine); > + }, > + > + findUpdates: function(listener, reason, appVersion, platformVersion) { This can now just call AddonManagerPrivate.callNoUpdateListeners(...), added today in bug 975000: http://hg.mozilla.org/mozilla-central/file/ac486fb972ca/toolkit/mozapps/extensions/AddonManager.jsm#l2328 @@ +356,5 @@ > + > + get providesUpdatesSecurely() { > + // An addon with a secure update system will have an https URL. > + if (this.updateURL) > + return (this.updateURL.indexOf("https://") >= 0); This needs to check https:// is at the *start*, not just anywhere in the string. .startsWith() ftw

Sorry for the noise but I just added myself to the CC list and it appears I somehow removed a mentor(?). Feel free to revert that unintentional change.

+1

This, combined with an AwesomeBar context menu entry to open it directly, would solve the issue I feel still isn't resolved in bug 522040. (If the user has removed the search box for a Chrome-like address bar, there's no quick, intuitive way to configure AwesomeBar search keywords. By comparison the Chrome Omnibar has an "Edit search engines..." entry in the context menu.)

JFYI I have a prototype of this here: https://github.com/erikvold/searchengine-manager-jp https://addons.mozilla.org/firefox/addon/search-engine-manager and if bug 1071311 is completed then this should be pretty easy.

I am just an invested user but I think we can close this bug off now. The new search system has rolled out to the channels with the old manager scrapped an a new manager written into the search settings.

WONTFIX in favor of bug 1106205.