Closed Bug 959576 Opened 10 years ago Closed 10 years ago

Create a component to get the list of priority domains

Categories

(Firefox :: Search, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31

People

(Reporter: phlsa, Assigned: mak)

References

Details

(Whiteboard: [search] p=5 s=it-31c-30a-29b.1 [qa-])

Attachments

(1 file, 3 obsolete files)

Whiteboard: p=2
Whiteboard: p=2 → [feature] p=2
QA Contact: petruta.rasa
Whiteboard: [feature] p=2 → [search] p=2
Hey philipp, do we have some definitions here of what a priority domain should be?
- is it an hardcoded list we ship with each release of the product?
- if the user removed a specific engine from the search manager, should we still suggest its domain?
- may the list include engines that are not among search engines we ship with the release?
- does the fact the user visited a given domain or not influence the list?
- may we ever need to update this list remotely like we do with blocklists? (I honestly think it's very unlikely we may ever have such an urgency)
Flags: needinfo?(philipp)
(In reply to Marco Bonardo [:mak] from comment #1)
> Hey philipp, do we have some definitions here of what a priority domain
> should be?
Yes, it is basically the list of search providers we ship.

> - is it an hardcoded list we ship with each release of the product?
I think the contents and ordering of that list might differ by locale, but otherwise: yes.

> - if the user removed a specific engine from the search manager, should we
> still suggest its domain?
Since few users actually do that, I'm OK with not suggesting domains for deleted search engines.

> - may the list include engines that are not among search engines we ship
> with the release?
I don't think so.

> - does the fact the user visited a given domain or not influence the list?
I think it should generally follow our frecency sorting. Perhaps the search providers will get a bump on their score.

> - may we ever need to update this list remotely like we do with blocklists?
> (I honestly think it's very unlikely we may ever have such an urgency)
I think so too.

Brian, you might want to jump in here if you think that any of the above is objectionable :)
Flags: needinfo?(philipp) → needinfo?(clarkbw)
Status: NEW → ASSIGNED
Whiteboard: [search] p=2 → [search] p=5 s=it-30c-29a-28b.3
Assignee: nobody → mak77
Overall what we're trying to do is seed domains from the search engine providers.

(In reply to Philipp Sackl [:phlsa] from comment #2)
> (In reply to Marco Bonardo [:mak] from comment #1)
> > Hey philipp, do we have some definitions here of what a priority domain
> > should be?
> Yes, it is basically the list of search providers we ship.

The domains might slightly differ from what's available in the search engine plugin.  It might work to add the domain to our search plugins as a moz: namespace; which would also allow other plugins similar access.
 
> > - is it an hardcoded list we ship with each release of the product?
> I think the contents and ordering of that list might differ by locale, but
> otherwise: yes.

Each locale has it's own list of search engines so we'd expect the list to be generated according to that.

> > - if the user removed a specific engine from the search manager, should we
> > still suggest its domain?
> Since few users actually do that, I'm OK with not suggesting domains for
> deleted search engines.

If a user wasn't using it then I'd say ok.  If we seeded a search engine domain with a frecency value of 1000 and on removal of that search engine found the frecency higher than 1000 I'd say we should leave it as the user seems to be using it.  If a user didn't use the suggestio (frecency remained 1000) I'd say we could remove it.

> > - may the list include engines that are not among search engines we ship
> > with the release?
> I don't think so.

We already support moz: namespace items in open search like the searchForm:
  <moz:SearchForm>https://developer.mozilla.org/en-US/search</moz:SearchForm>

I don't have a problem if we added something like a root domain:
  <moz:SearchRoot>https://developer.mozilla.org/</moz:SearchRoot>

Ebay and other sites would have a problem if we just grabbed their domain from the search URL since Ebay uses rover.ebay.com for redirecting searches to the proper place.

> > - does the fact the user visited a given domain or not influence the list?
> I think it should generally follow our frecency sorting. Perhaps the search
> providers will get a bump on their score.

I'm not sure I understand.  I'm assuming that this suggestion is part of the Places db and if the user made use of it then the frecency sorting system would work as intended.  If that's not the case then my above answer with frecency value of 1000 probably doesn't make as much sense. 

> > - may we ever need to update this list remotely like we do with blocklists?
> > (I honestly think it's very unlikely we may ever have such an urgency)
> I think so too.

Agreed, if our search engine plugin needed to update it makes sense to me that we'd update this list.

> Brian, you might want to jump in here if you think that any of the above is
> objectionable :)
Flags: needinfo?(clarkbw)
Whiteboard: [search] p=5 s=it-30c-29a-28b.3 → [search] p=5 s=it-30c-29a-28b.3 [qa-]
Attached patch wip (obsolete) — Splinter Review
Gavin, would you have any time to take a brief look at the approach, you are the best person with both autocomplete and search service knowledge afaik. This is clearly not for an actual review (someone else can do that when the approach is defined).

This implements a PriorityUrlProvider module in Places that fetches search engines from the search service, provides a token for the search out of a new MozParam with purpose "autocomplete". In future it may get more providers than just search.

For now the patch just appends the result to the autocomplete popup if there's no other result, for testing purposes.

For reference this is the UX mockup: http://phlsa.github.io/ff-search-suggestion/

there's still some details to figure:

1. UX: From the mockup I see there's this "Search the web" title for the entry, though many engines search a subset of the web, like products on amazon, articles on Wikipedia... What are we actually supposed to do, provide a generic string like "Search with <enginename>" or add a title for each engine (clearly would work only for for default engines)?

2. UX: from the mockup looks like we want to do this kind of completion only if what the user typed was partial, once the user has typed all of the matching token it changes to a "Visit <enginename>", though it would be much simpler to just not suggest anything, since this requirement seems to overlap with the "always show performed action in autocomplete" feature and that may require deeper investigation.

3. UX and techinical: from the mockup we should load the searchForm but with affiliate codes, we don't have such a thing for now, we have the plain searchForm URI, or the submission URI with affiliate codes.  Is this a strong requirement, or something we should followup with marketing?

4. technical: how do we usually notify localizers that they should update their search plugins definition and how?

5. technical: merging this with frecency results is not trivial, I'm likely going to give these results a fixed frecency value (like we are doing with newtab tiles) and try to merge it on-the-fly
Attachment #8388058 - Flags: feedback?(gavin.sharp)
for the UX questions
Flags: needinfo?(philipp)
there's another technical issue, that is more blocking. Due to our inline autocomplete implementation, the inline and popup queries run parallel, and they don't have to return matching results, indeed they also use different frecency values.
So it can happen that inline decides to complete to godaddy.com while the first popup entry is "Search with <google>" (unselected), in this case things are clearly not going to work as expected in the mock-up...

It was made this way to avoid breaking users muscle memory, since when we introduced inline completion we already had adaptive completion, so you could just type a couple letters and key down+enter. Inserting anything as first entry in the popup would broke this behavior. Moreover inserting something that is not predictable (the user can't tell if there will be an inline/search result or not). Even if it's true that somehow frecency is not totally predictable.

Though, it's very likely users habits changed with the introduction of inline completion, I suspect most users now use the inline completion result rather than the adaptive one... This is something we could test with a telemetry probe probably, but that should be done pretty soon (now), and we should wait for results.

So, I think we may re-evaluate the inline implementation decisions and make it just return a first entry in the popup that matches what is inline completed. The search result introduced here would just enter the popup flow as well , the same way. Though, doing this will require a deeper refactoring of the code and tests, likely taking more than a week.
It may also come a little bit unexpected  by users, some vocal minority will definitely dislike the result coming in the middle of their flow.
I think that i will finish this component with some tests (if the component approach is fine and search data issues are figured) and move the autocomplete discussion to bug 959573, where it pertains
(In reply to Marco Bonardo [:mak] from comment #4)
> 1. UX: From the mockup I see there's this "Search the web" title for the
> entry, though many engines search a subset of the web, like products on
> amazon, articles on Wikipedia... What are we actually supposed to do,
> provide a generic string like "Search with <enginename>" or add a title for
> each engine (clearly would work only for for default engines)?
Good observation! Let's just use the name of the engine without any extras. So typing »goo« would just show »Google«. After all, the user isn't yet performing a search.
I updated the mockup to reflect this.

> 2. UX: from the mockup looks like we want to do this kind of completion only
> if what the user typed was partial, once the user has typed all of the
> matching token it changes to a "Visit <enginename>", though it would be much
> simpler to just not suggest anything, since this requirement seems to
> overlap with the "always show performed action in autocomplete" feature and
> that may require deeper investigation.
If it allows us to move faster, I'm fine with leaving this out for now (especially since there is another bug in the backlog that should deal with this too).

> 3. UX and techinical: from the mockup we should load the searchForm but with
> affiliate codes, we don't have such a thing for now, we have the plain
> searchForm URI, or the submission URI with affiliate codes.  Is this a
> strong requirement, or something we should followup with marketing?
It is highly desirable to hide the full URL. Showing the entire thing causes significant noise and would probably inspire mistrust because it looks unfamiliar. So yes, I'd say it's a strong requirement.
Flags: needinfo?(philipp)
Comment on attachment 8388058 [details] [diff] [review]
wip

The use of a MozParam for this seems like too much of a hack - why not just add an attribute to nsISearchEngine called something like "resultDomain", and have it be populated by a <ResultDomain> element's value in the MozSearch document. Similar to how e.g. "Alias" works. It would be optional, with fallback to the primary <Url>'s domain (as in your patch).

I had a bunch of other nitpicks/review comments/things to followup on about the rest of the patch, but nothing major (I assume you plan to adjust some things anyhow).

Re: localizers, we can do outreach with the l10n folks to have them edit their plugins. Thankfully many locales use en-US engines anyways, so even without any l10n-specific changes this will have some benefit.

Re: searchForm, there's a related discussion in bug 940685. I think an "empty" search is fine for now, but we might want to make searchForm more useful in the future.
Attachment #8388058 - Flags: feedback?(gavin.sharp) → feedback+
(In reply to Philipp Sackl [:phlsa] from comment #8)
> > 3. UX and techinical: from the mockup we should load the searchForm but with
> > affiliate codes, we don't have such a thing for now, we have the plain
> > searchForm URI, or the submission URI with affiliate codes.  Is this a
> > strong requirement, or something we should followup with marketing?
> It is highly desirable to hide the full URL. Showing the entire thing causes
> significant noise and would probably inspire mistrust because it looks
> unfamiliar. So yes, I'd say it's a strong requirement.

I think Marco's question is just about what we load, not about what we show. That being said, I don't see searchForm mentioned anywhere in the mockup.
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #10)
> (In reply to Philipp Sackl [:phlsa] from comment #8)
> > > 3. UX and techinical: from the mockup we should load the searchForm but with
> > > affiliate codes, we don't have such a thing for now, we have the plain
> > > searchForm URI, or the submission URI with affiliate codes.  Is this a
> > > strong requirement, or something we should followup with marketing?
> > It is highly desirable to hide the full URL. Showing the entire thing causes
> > significant noise and would probably inspire mistrust because it looks
> > unfamiliar. So yes, I'd say it's a strong requirement.
> 
> I think Marco's question is just about what we load, not about what we show.
> That being said, I don't see searchForm mentioned anywhere in the mockup.

I'm not sure what searchForm exactly is.
So just to make sure we are all on the same page:

- This interaction is exclusively about the navigation bar
- Autocompleting shows a pretty and clean URL (like google.com or bing.com)
- When pressing enter however, we exchange that short URL with the longer one that also includes our affiliate code
- The only exception to this rule is when the user actually types out the entire URL

If anything is unclear here, feel free to ping me!
(In reply to Philipp Sackl [:phlsa] from comment #11)
> - When pressing enter however, we exchange that short URL with the longer
> one that also includes our affiliate code

We don't have support for hiding the url from the user, I may try using actions (same as switch-to-tab), but that may end up in a follow-up. Also because I think we don't have an affiliate code for the search engine url (the .searchForm property), we only have one for direct searches, and I can't tell if that's going to work regardless, since that should be asked upstream.

Actually, I think most of the autocomplete code will slip into the next iteration, since I really don't think we can use our current architecture for it (comment 6), so I'd like to try a small refactoring of that code, make it possible to enable it through a pref and try it in Nightly, before adding stuff to it.

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #9)
> Re: searchForm, there's a related discussion in bug 940685. I think an
> "empty" search is fine for now, but we might want to make searchForm more
> useful in the future.

As pointed out in bug 940685, many of the engines we ship don't support forward-empty-search-to-search-form. While that is ignorable for the search case (we use it only in a couple less discoverable features), it wouldn't be ignorable when we put that directly into autocomplete. Moreover, here sounds like I need a searchForm+affiliate_code url, that doesn't exist (the empty search is that in some case, not all) so searchForm could become that.
(In reply to Marco Bonardo [:mak] from comment #12)
> I think we don't have an affiliate code for the search engine url
> (the .searchForm property), we only have one for direct searches, and I
> can't tell if that's going to work regardless, since that should be asked
> upstream.

According to Bryan we have that code.
Flags: needinfo?(clarkbw)
(In reply to Philipp Sackl [:phlsa] from comment #13)
> According to Bryan we have that code.

well, yes, for the submission url. Not for the search form page, afaik.
(In reply to Marco Bonardo [:mak] from comment #14)
> (In reply to Philipp Sackl [:phlsa] from comment #13)
> > According to Bryan we have that code.
> 
> well, yes, for the submission url. Not for the search form page, afaik.

Quoting an email from Bryan:
> This URL will take you to the google homepage, but it’s also an empty search:
> https://www.google.com/search?q=&ie=utf-8&oe=utf-8&rls=org.mozilla:en-US:official&client=firefox-a&channel=sb&gfe_rd=ctrl&ei=scILU4r9BsqV8Qe40YD4BQ&gws_rd=cr
(In reply to Philipp Sackl [:phlsa] from comment #15)
> Quoting an email from Bryan:
> > This URL will take you to the google homepage, but it’s also an empty search:
> > https://www.google.com/search?q=&ie=utf-8&oe=utf-8&rls=org.mozilla:en-US:official&client=firefox-a&channel=sb&gfe_rd=ctrl&ei=scILU4r9BsqV8Qe40YD4BQ&gws_rd=cr

Yes, but that works just for some of our engines, see https://bugzilla.mozilla.org/show_bug.cgi?id=940685#c13
(In reply to Marco Bonardo [:mak] from comment #16)
> (In reply to Philipp Sackl [:phlsa] from comment #15)
> > Quoting an email from Bryan:
> > > This URL will take you to the google homepage, but it’s also an empty search:
> > > https://www.google.com/search?q=&ie=utf-8&oe=utf-8&rls=org.mozilla:en-US:official&client=firefox-a&channel=sb&gfe_rd=ctrl&ei=scILU4r9BsqV8Qe40YD4BQ&gws_rd=cr
> 
> Yes, but that works just for some of our engines, see
> https://bugzilla.mozilla.org/show_bug.cgi?id=940685#c13

Ah, alright!
From the comment in the bug, it looks like we got the most relevant engines covered (Google, Bing, Yahoo).
I'm assuming BD is working on getting as many of those codes as possible.
(In reply to Marco Bonardo [:mak] from comment #14)
> (In reply to Philipp Sackl [:phlsa] from comment #13)
> > According to Bryan we have that code.
> 
> well, yes, for the submission url. Not for the search form page, afaik.

Yes and we can only apply our affiliate code on a search url; we can't apply it to other forms of a URL.  As bug 940685 comment 13 points out empty search submissions don't work as expected for every site, though keep in mind that the list goes from 99% usage for google to sub percent usage numbers for all the other sites. (not advocating breaking other engines, just a data point)

I'd agree with Gavin in comment 9 and recommend that we need an opt out system for certain search engines.  If an empty submission url doesn't work those search engines could provide a ResultDomain which gets used instead of their submission URL.  I suppose you could also do an opt in system but you'll need to guess TLD which perhaps is somewhat stable but is still an implicit system.
Flags: needinfo?(clarkbw)
We should probably just fix bug 557665, update the default plugins to use it where it makes sense, and then use .searchForm for this bug across the board.
(In reply to Bryan Clark (Firefox Search PM) [:clarkbw] from comment #18)
> Yes and we can only apply our affiliate code on a search url; we can't apply
> it to other forms of a URL.

Ok, this is good to know.

> As bug 940685 comment 13 points out empty
> search submissions don't work as expected for every site, though keep in
> mind that the list goes from 99% usage for google to sub percent usage
> numbers for all the other sites.

I agree that covering 99% is good, though with this UI it will be much easier for users to hit less used engines. For example I suppose any user soon or later types "www", after the first "w" we may suggest "Wikipedia", unfortunately the suggested link won't work.

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #19)
> We should probably just fix bug 557665, update the default plugins to use it
> where it makes sense, and then use .searchForm for this bug across the board.

Yes, that would allow to use an empty search where it works (with affiliate code) or the old plain searchURL where it doesn't work.
where by searchURL I actually meant searchForm :/
Attached patch more wip (obsolete) — Splinter Review
I should still write some tests for the new attribute in the search service and for the new module in Places.
This uses searchForm as the destination url, I think we can handle the affiliate issue apart (in bug 557665 probably)
Attachment #8388058 - Attachment is obsolete: true
Comment on attachment 8390081 [details] [diff] [review]
more wip

a few random drive-by comments:
- no need to update the sherlock parsing code for this attr, it's mostly dead (bug 862137)
- in the resultDomain getter, you might want to protect against URIs that have no hosts? I forget whether non-nsStandardURL URIs can get through to there...
(In reply to Marco Bonardo [:mak] from comment #20)
> > As bug 940685 comment 13 points out empty
> > search submissions don't work as expected for every site, though keep in
> > mind that the list goes from 99% usage for google to sub percent usage
> > numbers for all the other sites.
> 
> I agree that covering 99% is good, though with this UI it will be much
> easier for users to hit less used engines. For example I suppose any user
> soon or later types "www", after the first "w" we may suggest "Wikipedia",
> unfortunately the suggested link won't work.
Hm, perhaps we should just assume that a single »w« will autocomplete to the default search engine (most likely Google) unless frecency tells us otherwise. Typing »wi« would then autocomplete to wikipedia.
(In reply to Philipp Sackl [:phlsa] from comment #24)
> Hm, perhaps we should just assume that a single »w« will autocomplete to the
> default search engine (most likely Google) unless frecency tells us
> otherwise. Typing »wi« would then autocomplete to wikipedia.

I don't think it's a very good idea, per the way we always handled autocomplete queries (by ignoring www), I think most users are used to not type "www". Very non-technical people don't even use the location bar (my mom just types addresses in the search engine homepage, for example).
Moreover, such a very specific behavior would make the code a little awkward, it's hard to think of an API that matches at the beginning everything but "w". So I'd leave this out for second thoughts, once the basic stuff is in.

Btw, my point was not about wikipedia, was mostly about the fact we may be exposing non-working uris in a more prominent piece of the UI. though if we properly fix bug 557665 it won't be a problem.
Attached patch patch v1 (obsolete) — Splinter Review
Gavin, I'm asking review to you just cause you already took a look at the patch, but feel free to forward the request.
Attachment #8390081 - Attachment is obsolete: true
Attachment #8391479 - Flags: review?(gavin.sharp)
Comment on attachment 8391479 [details] [diff] [review]
patch v1

>diff --git a/netwerk/base/public/nsIBrowserSearchService.idl b/netwerk/base/public/nsIBrowserSearchService.idl

> interface nsISearchEngine : nsISupports

>+  /**
>+   * A string containing a root domain that can be used to match this search
>+   * engine in autocomplete text fields.
>+   * If the search engine definition doesn't contain a <ResultDomain>, this
>+   * will just return the host from the default submission url, modulo an
>+   * eventual "wwww." prefix, i.e. "google.com" for "http://www.google.com/".
>+   */
>+  readonly attribute AString resultDomain;

Comment referring to the autocomplete use case seems a bit too specific. How about:

"A string representing the hostname from which search results for the default text/html Url are returned, minus the leading "www." (if present). This can be specified in the engine description file, but will default to host from the <Url>'s template otherwise."

Actually, this makes me wonder whether this should just be a child element/attribute of the relevant <Url>. What do you think?

>diff --git a/toolkit/components/places/PriorityUrlProvider.jsm b/toolkit/components/places/PriorityUrlProvider.jsm

>+function SearchEnginesProvider(aCallback) {

nit: we're trying to move away from the "aParameter" convention. Can you avoid using it here?

>+  this._engines = new Map();
>+  Services.search.init( () => {

You should probably handle this returning a failure (it is passed an nsresult)

>+    Services.search.getVisibleEngines().forEach(this._addEngine.bind(this));

arrow function? pass this as second arg to "forEach"? so many options!

>+SearchEnginesProvider.prototype = {

>+  _addEngine: function (aEngine) {
>+    if (!this._engines.has(aEngine.name)) {

nit: early return and outdent the rest of the function?

>+      try {
>+        let match = { token: aEngine.resultDomain,
>+                      // TODO (bug 557665): searchForm should provide an usable
>+                      // url with affiliate code, if available.
>+                      url: aEngine.searchForm,
>+                      title: aEngine.name,
>+                      iconUrl: aEngine.iconURI ? aEngine.iconURI.spec : null,

We may need to use getIcons/getIconURLBySize to properly support hidpi? I guess ideally we would want a better API that picks the right size automatically (might be related to bug 795866). I guess it's probably best to defer this to a followup.

>+function promiseInitialized() {

>+  new SearchEnginesProvider( () => {

Seems a bit unusual/confusing to rely on the observer service reference to avoid this being GCed? Does that even work given that you're using a weak reference?

>+this.PriorityUrlProvider = Object.freeze({
>+  addMatch: function (aMatch) {
>+    matches.set(aMatch.token, aMatch);
>+  },
>+
>+  removeMatchByToken: function (aToken) {
>+    matches.delete(aToken);
>+  },

Is it useful to keep this separate from SearchEnginesProvider? Seems a bit overly complicated to have separate sets when it's this simple. But I don't feel strongly here.

>diff --git a/toolkit/components/places/tests/unit/test_priorityUrlProvider.js b/toolkit/components/places/tests/unit/test_priorityUrlProvider.js

>+add_task(function* no_match() {
>+  do_check_eq(yield PriorityUrlProvider.getMatchingSpec("test"));

Is omitting the second argument like this actually supported? Seems a bit confusing if so.

>+add_task(function* add_search_engine_match() {
>+  let promiseTopic = promiseSearchTopic("engine-added");
>+  Services.search.addEngineWithDetails("bacon", "", "bacon", "Search Bacon",
>+                                       "GET", "http://www.bacon.moz/?search={searchTerms}");
>+  yield promiseSearchTopic;
>+  let match = yield PriorityUrlProvider.getMatchingSpec("bacon");

How about checking that the match isn't there before you added the engine?

>+function promiseSearchTopic(aExpectedTopic) {
>+  let deferred = Promise.defer();
>+  Services.obs.addObserver( function observe(undefined, undefined, aVerb) {

This is a bit weird - redefining a parameter (and using a JS magical word?). Can you just use "subject" and "topic"? :) And change "expectedTopic" to "expectedVerb"

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

Changes here look good, but I think we should move to making resultDomain be a property of the URL rather than an attribute of the engine itself.

>diff --git a/toolkit/components/search/tests/xpcshell/test_resultDomain.js b/toolkit/components/search/tests/xpcshell/test_resultDomain.js

>+ * Test that currentEngine and defaultEngine properties can be set and yield the
>+ * proper events and behavior (search results)

Needs updating.

>+let waitForEngines = [ "Test search engine", "A second test engine", "bacon" ];

Make this a set to avoid needing to use splice() below?

>+function search_observer(aSubject, aTopic, aData) {
>+
>+}

What's this about?

All looks good, but enough changes suggested to warrant another review pass (particularly changes to the search service as suggested).
Attachment #8391479 - Flags: review?(gavin.sharp) → feedback+
Whiteboard: [search] p=5 s=it-30c-29a-28b.3 [qa-] → [search] p=5 s=it-31c-30a-29b.1 [qa-]
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #27)
> Actually, this makes me wonder whether this should just be a child
> element/attribute of the relevant <Url>. What do you think?

How would we represent that in the idl? if it's an attribute of the url, each url may have one, but we don't have a good hooking point to expose it for each url (nsISearchSubmission doesn't look like a good API to extend with this).
The API as-is better copes with a single value for the whole engine, and it makes sense to have just one resultDomain per its definition... Otherwise we'd need something like getResultDomain([optional] in AString responseType).
Sorry for nagging Gavin, I think it's worth to define the idl before I address the comments, we may eventually discuss this over irc, as you prefer.  Btw, neeinfo for comment 28.
And, if we go for the url property, I'd probably vote for an attribute like template, so resultdomain="somedomain.com"
Flags: needinfo?(gavin.sharp)
(In reply to Marco Bonardo [:mak] from comment #28)
> The API as-is better copes with a single value for the whole engine, and it
> makes sense to have just one resultDomain per its definition... Otherwise
> we'd need something like getResultDomain([optional] in AString responseType).

getResultDomain([optional] in AString responseType) sounds good to me.
Flags: needinfo?(gavin.sharp)
Attached patch patch v2Splinter Review
This should address all of the comments
Attachment #8391479 - Attachment is obsolete: true
Attachment #8393833 - Flags: review?(gavin.sharp)
Comment on attachment 8393833 [details] [diff] [review]
patch v2

Wonderful! One nit:

>diff --git a/toolkit/components/places/PriorityUrlProvider.jsm b/toolkit/components/places/PriorityUrlProvider.jsm

>+let SearchEnginesProvider = {
>+  init: function () {

>+        deferred.reject("Unable to initialize search service.");

IIRC new Error("msg") gives slightly more useful exceptions (line numbers etc.)?
Attachment #8393833 - Flags: review?(gavin.sharp) → review+
with nit fixed
https://hg.mozilla.org/integration/fx-team/rev/473edac990f3
Flags: in-testsuite+
Hardware: x86 → All
Target Milestone: --- → Firefox 31
Version: 28 Branch → Trunk
https://hg.mozilla.org/mozilla-central/rev/473edac990f3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Comment on attachment 8393833 [details] [diff] [review]
patch v2

Review of attachment 8393833 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/PriorityUrlProvider.jsm
@@ +104,5 @@
> +
> +let initialized = false;
> +function promiseInitialized() {
> +  if (initialized) {
> +    return Promise.resolve();

Drive-by comment: I don't know if this is relevant or intended here, but this pattern is subject to re-entrancy if the initialization method is called twice before it is completed.
(In reply to :Paolo Amadini from comment #35)

> ::: toolkit/components/places/PriorityUrlProvider.jsm
> @@ +104,5 @@
> > +
> > +let initialized = false;
> > +function promiseInitialized() {
> > +  if (initialized) {
> > +    return Promise.resolve();
> 
> Drive-by comment: I don't know if this is relevant or intended here, but
> this pattern is subject to re-entrancy if the initialization method is
> called twice before it is completed.

Yes, it may end up doing the init more than once in that case, in this case it shouldn't cause visible issues, but it's good to know for future changes, I may fix it in some of the other patches that will actually use this code.
Depends on: 990799
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Depends on: 991543
Blocks: 1039004
Blocks: 1039003
You need to log in before you can comment on or make changes to this bug.