Closed Bug 1352598 Opened 7 years ago Closed 6 years ago

Add an API to list installed search engines and execute searches using only those engines

Categories

(WebExtensions :: Untriaged, enhancement, P3)

Product:
WebExtensions
Component:
Untriaged
Type:
enhancement

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: andy+bugzilla, Assigned: mkaply)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, helpwanted, Whiteboard: [design-decision-approved])

Attachments

(2 files, 2 obsolete files)

Bug 1352598 - Add WebExtension API for access to search.
(deleted), text/x-review-board-request
aswan
: review+
Details
Latest patch
(deleted), patch
Details | Diff | Splinter Review 
This is a spin off from bug 1268401 comment 16. Let's create an API to list the installed search engines and the relevant APIs from that. It will return something like: icon, prefix URL, search engine name and if its the default.
Priority: -- → P3
Whiteboard: triaged, possibly management API
I think that management.getall should be able to return search engines.
Why the spin off?
(In reply to Carlos [nohamelin] from comment #2)
> Why the spin off?

I'm going to guess a read-only API is less problematic than a read-write one, as search hijacking is one of the most common attacks against browsers.

https://github.com/benbasson/contextsearch is also blocked on this.
Not sure if use cases are relevant, but just in case: this could be useful to Vimium as well: https://github.com/philc/vimium/issues/2598

(And if it's easier: just being able to pass a search string to be sent to the default search engine would probably be fine as well.)
Extensions like https://addons.mozilla.org/de/firefox/addon/quickcontextsearch/ require such an API to be ported to WebExtensions.
(In reply to kirill.rakhman from comment #6)
> Extensions like
> https://addons.mozilla.org/de/firefox/addon/quickcontextsearch/ require such
> an API to be ported to WebExtensions.

This is also why I'm here. Following.
Is there any chance this can be done and added for Firefox 57 or should I inform my users that their add-on will be disappearing for a while? It would be useful to get a steer. This is now the only actual blocking bug I think I'm waiting for before I can port Context Search.
(In reply to Ben Basson from comment #8)
> Is there any chance this can be done and added for Firefox 57 or should I
> inform my users that their add-on will be disappearing for a while? It would
> be useful to get a steer. This is now the only actual blocking bug I think
> I'm waiting for before I can port Context Search.

The cutoff for new features to be added for 57 has already gone by so this won't be in 57.
I'm not aware of anybody actively working on this so it would need a patch from a contributor to make 58 or 59.
(In reply to Worcester12345 from comment #7)
> (In reply to kirill.rakhman from comment #6)
> > Extensions like
> > https://addons.mozilla.org/de/firefox/addon/quickcontextsearch/ require such
> > an API to be ported to WebExtensions.
> 
> This is also why I'm here. Following.

Also following this bug for "Duplicate Tabs Closer" extension compatibility.
(In reply to Worcester12345 from comment #10)
> Also following this bug for "Duplicate Tabs Closer" extension compatibility.

Why do you need this in Duplicate Tabs Closer?
(In reply to Ben Basson from comment #8)
> Is there any chance this can be done and added for Firefox 57 or should I
> inform my users that their add-on will be disappearing for a while? It would
> be useful to get a steer. This is now the only actual blocking bug I think
> I'm waiting for before I can port Context Search.

Saw this in another bug:
November 14 WebExtensions
APIs triage meeting.

Here’s a quick overview of what to expect at the triage: 
* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a
proposal; we won't be going deep into implementation details

Relevant Links: 
* Wiki for the meeting: https://wiki.mozilla.org/Add-ons/Contribute/Triage
* Meeting agenda:
https://docs.google.com/document/d/1g3RMfKZ3671NcusMqkoOiKwfPekRe-VI7Rzqxo6F_Ao/edit#
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision
Hm, the bug doesn't seem to be on the list for the meeting. Is there a way we can sneak it in?

Requirements for the API, as I see it:

- List all search engines with name, url and favicon
- Alternative to getting the url is an API for performing a search with a given search engine
The agenda for tomorrow's meeting has already been set.  To get something on the agenda for a future meeting, the first step is to add design-decision-needed to the whiteboard, as I've done for this bug.
Whiteboard: triaged, possibly management API → design-decision-needed
Because the search engines can use POST method [1], returning the URL is not enough. The old API allowed you to get a submission object [2] but if we want to keep the API simple, method for performing the search should suffice.

[1]: http://www.opensearch.org/Specifications/OpenSearch/Extensions/Parameter
[2]: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsISearchEngine#getSubmission()
(In reply to Jan Tojnar from comment #15)
> method for performing the search should suffice.

Agreed, on the assumption that either you can choose where that search happens (e.g. new tab, current tab, new window) or that there are sufficient mechanisms in place to open a new tab/window and then perform the search in that context.
In case this makes a future prioritisation meeting, just want to sum up the things I need (I probably can't attend the meeting as I'm usually travelling at the time it happens):

1. List *active* search engines.
2. Get icon for each search engine as base64 so I can show the icon on the context menu
3a. Get a search submission (or equivalent) and allow me to execute (choosing destination for search, new tab, current tab, new window)
- or - 
3b. Get a search submission (or equivalent) and allow me to pass it to the "new tab"/"window" relevant API methods.

As mentioned above, URL is not sufficient to support all installed engines.
Attached patch Draft for browser.searchEngines (obsolete) (deleted) — — Splinter Review 
I started working on a very basic API for this. Basically you get browser.searchEngines.get which resolves to a list of search engines, where the engines look like search_provider entries in chrome_settings_overrides. I also added browser.searchEngines.search(name, searchText) to bypass the complexity of constructing your own functioning search URL and postData. The code is pretty bad and I am not sure if I am going to continue work on this.
Attached patch Draft for browser.searchEngines (obsolete) (deleted) — — Splinter Review 

        Attachment #8933951 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Tom Schuster [:evilpie] from comment #19)
> Created attachment 8933952 [details] [diff] [review]
> Draft for browser.searchEngines

Is this getting reviewed anywhere?
Whiteboard: design-decision-needed → [design-decision-needed]

    
    

    
  
+1

Some comments:

1. Required for improvement in this addon: https://addons.mozilla.org/en-US/firefox/addon/swift-selection-search/

2. Wouldn't it be easier to store these SE settings as special "bookmarks" items and be able to list these search engines via the existig bookmarks api? Before rejecting this, consider that bookmarks with a "keyword" are already considered as search engines in Firefox and work the same.

    
    

    
  
I mean that I'm a bit dissatisfied with the default search engines widget since it doesn't have full support of OpenSearch spec. So I guess we just need a fully custom widget and the default one not to be an inherent part of the browser. But what is a search widget? It's just a textbox on a toolbar. Unfortunately the webext API disallows placing a frame with arbitrary HTML on a toolbar. So in factt we need to fix the following, not to introduce new API for search:

0 Create an api allowing to create arbitrary browser widgets on toolbars with HTML.
1 Move all search engines functionality from the browser into an addon.
2 Ship this addon with the browser.
3 A user should be able to disable/remove that addon and install a custom one.

    
    

    
  
(In reply to KOLANICH from comment #22)
> I mean that I'm a bit dissatisfied with the default search engines widget
> since it doesn't have full support of OpenSearch spec. So I guess we just
> need a fully custom widget and the default one not to be an inherent part of
> the browser. But what is a search widget? It's just a textbox on a toolbar.
> Unfortunately the webext API disallows placing a frame with arbitrary HTML
> on a toolbar. So in factt we need to fix the following, not to introduce new
> API for search

I have no intention of doing anything with the browser toolbar, so I *do* need new APIs for search. If you want to request toolbar enhancements you should raise a separate bug.

    
    

    
  
This has been added to the agenda for the January 9, 2018 WebExtensions APIs triage meeting. All who are interested in this bug are welcome to attend. 

Here’s a quick overview of what to expect at the triage: 

* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a proposal; we won't be going deep into implementation details

Relevant Links: 

* Wiki for the meeting: https://wiki.mozilla.org/WebExtensions/Triage#Next_Meeting
* Meeting agenda: https://docs.google.com/document/d/15JYw3L1490dKbr6yTLz1uirH8rfhcSiLJCubSWDlnfs/edit#
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision

    
    

    
  
There's another implementation of this here:
https://github.com/Smile4ever/webext-experiment-searchengines
Flags: needinfo?(amckay)
Whiteboard: [design-decision-needed] → [design-decision-approved]

    
    

    
  
Notes from the conversation: this seemed to make sense to everyone. The only question was the .search API evilpie suggested in comment 18, which also looks fine if the implementor wanted to do it, although its not needed.
Flags: needinfo?(amckay)

    
    

    
  
(In reply to Andy McKay [:andym] from comment #26)
> Notes from the conversation: this seemed to make sense to everyone. The only
> question was the .search API evilpie suggested in comment 18, which also
> looks fine if the implementor wanted to do it, although its not needed.

Though .search does have the advantage that it would obey the browser.search.context.loadInBackground pref like regular context search.

    
    

    
  
I thought the biggest reason for having the search function is that postData is complicated to implement otherwise. (I guess you would have to fake a post submission somehow?)

    
    

    
  
(In reply to Ian Moody [:Kwan] (UTC+0) from comment #27)
> (In reply to Andy McKay [:andym] from comment #26)
> > Notes from the conversation: this seemed to make sense to everyone. The only
> > question was the .search API evilpie suggested in comment 18, which also
> > looks fine if the implementor wanted to do it, although its not needed.
> 
> Though .search does have the advantage that it would obey the
> browser.search.context.loadInBackground pref like regular context search.

Does it have to? That may not be desirable in all use-cases? What if I want to specifically open a new window, or use the current tab (for example).

    
    

    
  
(In reply to Geoffrey De Belie (Smile4ever) from comment #11)
> (In reply to Worcester12345 from comment #10)
> > Also following this bug for "Duplicate Tabs Closer" extension compatibility.
> 
> Why do you need this in Duplicate Tabs Closer?

My mistake. 

Are there currently two different patches for this bug? Any way to reconcile things? Would be nice to put this one to bed. Looking forward to Context Search working again!

    
    

    
  
(In reply to Worcester12345 from comment #30)
> Are there currently two different patches for this bug? Any way to reconcile
> things? Would be nice to put this one to bed. Looking forward to Context
> Search working again!

If somebody has a completed patch ready, I or any of the webextensions peers can review it but we currently have our hands full with other projects and don't have the time to finish any of the existing patches.

    
    

    
  
Can someone add the "helpwanted" key word?
Thanks.

    
    

    
  
(In reply to Worcester12345 from comment #32)
> Can someone add the "helpwanted" key word?
> Thanks.

    
    

    
  
(In reply to Worcester12345 from comment #32)
> Can someone add the "helpwanted" key word?
> Thanks.

Added.
Keywords: helpwanted

    
    

    
  
So I like this general idea. A couple thoughts.

1. The search API should just return the results of the getSubmission and let the extension do what it wants with it. There's no need to interact with windows.

2. You should be able to call the search directly on the engine that comes back from the list. I don't know how to implement that though.

3. I'm not sold on searchEngines as the name of the permissions. Maybe search? and the mapping would be similar to what we have. search.getEngine. or search.getEngineByName.

    
    

    
  
(In reply to Mike Kaply [:mkaply] from comment #35)
> 1. The search API should just return the results of the getSubmission and
> let the extension do what it wants with it. There's no need to interact with
> windows.

This would be my preference. I can't see any way of actually using a submission right now though, do we need to file a bug to have this added to the tabs/window APIs? e.g. so the result of this bug is useful? :-)

> 2. You should be able to call the search directly on the engine that comes
> back from the list. I don't know how to implement that though.

How would this behave? Open the result in the current active tab? If so, then there could be a new function added to the search submission object returned.

    
    

    
  
(In reply to Ben Basson from comment #36)
> (In reply to Mike Kaply [:mkaply] from comment #35)
> > 1. The search API should just return the results of the getSubmission and
> > let the extension do what it wants with it. There's no need to interact with
> > windows.
> 
> This would be my preference. I can't see any way of actually using a
> submission right now though, do we need to file a bug to have this added to
> the tabs/window APIs? e.g. so the result of this bug is useful? :-)
>

The submission contains a URI (uri.spec) and postData. You could just return those two values (url and string)

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsISearchSubmission

 
> > 2. You should be able to call the search directly on the engine that comes
> > back from the list. I don't know how to implement that though.
> 
> How would this behave? Open the result in the current active tab? If so,
> then there could be a new function added to the search submission object
> returned.

the more I think about this, the more I'm wrong. I think the API should be "getSubmission" with an engine name and text.

IT returns a URL and postData that the extension can then open in a separate window or tab. This keeps the search code concentrated on just returning the correct data.

    
    

    
  
(In reply to Mike Kaply [:mkaply] from comment #37)
> The submission contains a URI (uri.spec) and postData. You could just return
> those two values (url and string)
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsISearchSubmission

Oh sure, that's what I used in the old world. But that was in conjunction with tab browser functions that accepted the post data. So do we need to file a separate bug to enhance the tab API to make having this information useful? The post data isn't much good if it can't be posted.

    
    

    
  
> Oh sure, that's what I used in the old world. But that was in conjunction with tab browser functions that accepted the post data. So do we need to file a separate bug to enhance the tab API to make having this information useful? The post data isn't much good if it can't be posted.

Good point. I think the answer is we should figure out a way to have the search API to the work. I'm thinking I want to avoid add-ons messing with search directly. I'm still working all this out.

    
    

    
      
  


  

    
    

    
  
Comment on attachment 8961558 [details]
Bug 1352598 - Add WebExtension API for access to search.

https://reviewboard.mozilla.org/r/230422/#review236664

looks like a good start, how about some tests?

::: toolkit/components/extensions/ext-search.js:48
(Diff revision 1)
> +          let engines = Services.search.getEngines();
> +          return engines.filter(engine => !engine.hidden).map(engine => {
> +            let searchEngine = {
> +              name: engine.name,
> +              is_default: engine === Services.search.currentEngine,
> +              // This could be a resource URI. What should we do?

"could be"?  under what circumstances is it (or is it not) a resource url?

::: toolkit/components/extensions/schemas/search.json:22
(Diff revision 1)
> +      }
> +    ]
> +  },
> +  {
> +    "namespace": "search",
> +    "description": "Use chrome.search to nteract with search engines.",

chrome -> browser, nteract -> interact

::: toolkit/components/extensions/schemas/search.json:52
(Diff revision 1)
> +            "name": "callback",
> +            "type": "function",
> +            "parameters": [
> +              {
> +                "name": "results",
> +                "type": "array",
> +                "items": {
> +                  "$ref": "SearchEngine"
> +                }
> +              }
> +            ]
> +          }

This api doesn't exist in google chrome does it?  If we don't need the callback form for compatibility, just async please.

::: toolkit/components/extensions/schemas/search.json:73
(Diff revision 1)
> +        "type": "function",
> +        "requireUserInput": true,
> +        "description": "Perform a search.",
> +        "parameters": [
> +          {
> +            "name": "name",

nit: can we call this engineName or something to be clear?

::: toolkit/components/extensions/schemas/search.json:77
(Diff revision 1)
> +          {
> +            "name": "name",
> +            "type": "string"
> +          },
> +          {
> +            "name": "data",

nit: can we call this `query` or `terms` or something

        Attachment #8961558 -
        Flags: review?(aswan) → review-

    
    

    
  
as original poster of bug 1450119, i will weigh in with my requirements. I need the equivalent of the old .getEngineByAlias() method. To be clear again, I need the alias of the search engine looked up and get the engine object back. I then open the search query from my addon. I don't want to activate anything in the search provider, just need the URI and name of the search engine based on the alias. I will use the URI to open the search; if that can't currently be done for POST data, then I need that functionality as well. I don't really need to set search engine entries or change them at this time.

    
    

    
  
Can you please explain why you need aliases versus name? What's the use case? If I provided it as part of list of engines would that be enough? Then you could use the alias to find the engine name.

My plan is not to provide the URL but simply to provide a way for any WebExtension to execute the search (and possibily specify the location).

There's shouldn't be a reason for a WebExtension to need the actual URL.

    
    

    
  
My extension establishes hot keys for search engines based on the aliases. I could match them programatically if the aliases are returned along with the search engine names and references to execute the search from. It's just that the old interface returned a search submission object based on the name or alias of the search engine.

I don't really care about the URL. I just want to be able direct the search to open in a new tab or the current tab. I used to do it using the URI because I could load it into any tab I chose from an e10s mainscript with .loadOneTab() or .loadURI() and the URI string.
The URI used to be able to be derived from the search submission object. I vaguely remember that it was noticably faster and simpler to do it that way rather than having the search manager submit the search. I'm not sure if that approach would have worked with POST data but I never encountered a Search provider it didn't work on. It would be better if it worked even with POST data. I don't really remember what the difference between get and post searches is, but using the uri string always worked in my addmittedly handful of test cases.

There's no reason I can see to hide the URI from the extension. If the extension alters the URI before the search, then I think the search fails and so does the extension. Maybe the extension wants to add some custom fields to the URI, I don't see the security issue in letting the extension read the URI's. I think it can hijack the search even if doesn't read the URI because it could substitute a bogus submission object in place of the returned one and execute the search on that. It seems that the real danger is allowing extensions to intercept a search query by trapping its event or perhaps recognizing its page load and substituting behaviour the user did not intend. Well, I've rambled on enough about this, I'm no expert on securtiy or FF.

    
    

    
  
Why do you match hotkeys based on the aliases intead of the actual name of the engines? I guess I'm missing something.

If you present UI saying "this key matches this engine" why does the alias matter?

As far as the search URL goes, I want to avoid WebExtensions having to worry anything about the internals of search.

    
    

    
  
>>Why do you match hotkeys based on the aliases intead of the actual name of the engines? I guess I'm missing something.

using the alias just provides a built-in FF dialog to configure the hotkeys, which are aliases, to the seatch engines. That makes my extension smaller and simpler and avoids duplicating a feature that is already present in FF. My extensions are actually demonstrations of simple, small and efficient coding that I hope is very useful to the users. Reusing the FF dialog probably cuts the size of the extension in half. The names of search engines are long strings, oftem with multiple words, and do not make for easy mapping and remembering to hotkeys. I encourage the users to just make the aliases one character long such as G for Google or U for Youtube, although the extension can handle multiple character aliases.

 >>If you present UI saying "this key matches this engine" why does the alias matter?

as above, I dont' present that interface, you do.

>>As far as the search URL goes, I want to avoid WebExtensions having to worry anything about the internals of search.

that's fine, but opening a tab to a URL is not any longer or more complicated to the developer than asking FF to do the same thing. As I mentioned before, I don't really remember the difference ( if any) in handling a get vs post search, I would say that if it takes me more than 1 or 2 lines of code to handle the difference, I would rather that FF do it for me.

    
    

    
  
thinking about permissions, reading search engines and submitting searches does not really seem to need a permissions unless this is traditional on mobile platforms. Adding, Removing or especially Altering, search providers should definitely need a permission. as FF currently stands it is virtually inpossibled for the user to add a search provider, this used to be easy and useful to powerusers. third parties have an easier time to add a search engine than the user him/her self.

after reading through the thread, I now see that the problem is that WX will not automatically handle the post data and that you want to avoid altering the WX tab manager to fix this. you are opting for building post data into the search handler to get around this problem. i see why you want the search handler to perform the search in that case. i think just provide an argument that accepts a tab object for the search to open in. The default, if the object is not present, is set by the pref mentioned above as current or new tab in current window. handle the post data problem in the search manager. my preference is the old way where tabs could handle the post data situation, but if that's not feasible I don't see a problem with handling it in the search manager.

    
    

    
  
Comment on attachment 8961558 [details]
Bug 1352598 - Add WebExtension API for access to search.

https://reviewboard.mozilla.org/r/230422/#review236664

> "could be"?  under what circumstances is it (or is it not) a resource url?

Resource images work fine in a WebExtension, so I think we're good here.

    
    

    
  
thinking about the interface for this... if you are going to supply a method that returns a search engine object based on its name, then it is trivial to make one based on the alias since you should be able to reuse virtually all of the code. the only differnece is in the reference to what field is searched from the search engine array. that should be just a line of code to handle the alias based function, combined with extracting the searching and object return as a subroutine from the name based function. i'm pretty sure this is why the old interface implemented both as searches. it should be very easy.

    
    

    
  
I'm only planning to give you a list of all engines with all their information (including aliases). I'm not going to have the concept of individual engine objects.

When you want to execute a search, you'll search based on the name of the engine. (browser.search.search("Google", "Search string")

The reason I'm doing it this way is because the Webextension APIs don't have a concept of calling APIs on an object returned by an API. So I couldn't do engine.search("search string")

So the concept of an engine object doesn't get us anything like it did with old extensions.

    
    

    
      
  

    
    

    
  
Comment on attachment 8961558 [details]
Bug 1352598 - Add WebExtension API for access to search.

https://reviewboard.mozilla.org/r/230422/#review239710


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/parent/ext-search.js:21
(Diff revision 2)
> +var searchInitialized2 = () => {
> +  if (Services.search.isInitialized) {
> +    return;
> +  }
> +  return new Promise(resolve => {
> +    const SEARCH_SERVICE_TOPIC = "browser-search-service";    

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/components/extensions/parent/ext-search.js:56
(Diff revision 2)
> +            return searchEngine;
> +          });
> +        },
> +
> +        async search(name, data) {
> +          await searchInitialized();

Error: 'searchinitialized' is not defined. [eslint: no-undef]

::: toolkit/components/extensions/test/browser/browser_ext_search.js:32
(Diff revision 2)
> +  });
> +  await extension.startup();
> +
> +  let addonEngines = await extension.awaitMessage("engines");
> +  let engines = Services.search.getEngines().filter(engine => !engine.hidden);
> +  is(addonEngines.length, engines.length, "Engine lengths are the same.")

Error: Missing semicolon. [eslint: semi]

    
    

    
  
Mike, thanks for doing this, I like the general direction. Would it be possible for the search() method to take in an arg so we can choose a new tab or new window as the destination of the search? If it's a right faff then I'd be more than happy with what you've done.

    
    

    
  
> Mike, thanks for doing this, I like the general direction. Would it be possible for the search() method to take in an arg so we can choose a new tab or new window as the destination of the search? If it's a right faff then I'd be more than happy with what you've done.

I'm curious as to the use case for opening a new window?

It's been suggested I allow the passing of a tab object to specify the specific tab. I'm honestly not sure how window would work.

I can't find any existing WebExtensions APIs that do anythign similar for me to copy.

    
    

    
  
(In reply to Ben Basson from comment #54)
> Mike, thanks for doing this, I like the general direction. Would it be
> possible for the search() method to take in an arg so we can choose a new
> tab or new window as the destination of the search? If it's a right faff
> then I'd be more than happy with what you've done.

And preferably whether the new window or new tab should be active or not.

    
    

    
  
Comment on attachment 8961558 [details]
Bug 1352598 - Add WebExtension API for access to search.

https://reviewboard.mozilla.org/r/230422/#review239860

Looking good, but the test doesn't yet cover browser.search.search()

::: toolkit/components/extensions/parent/ext-search.js:8
(Diff revision 2)
> +// The ext-* files are imported into the same scopes.
> +/* import-globals-from ext-toolkit.js */

please remove this, this is handled from .eslintrc.js now

::: toolkit/components/extensions/parent/ext-search.js:16
(Diff revision 2)
> +"use strict";
> +
> +ChromeUtils.defineModuleGetter(this, "Services",
> +                               "resource://gre/modules/Services.jsm");
> +
> +var searchInitialized2 = () => {

why the 2?

::: toolkit/components/extensions/parent/ext-search.js:20
(Diff revision 2)
> +  return new Promise(resolve => {
> +    const SEARCH_SERVICE_TOPIC = "browser-search-service";    
> +    Services.obs.addObserver(function observer(subject, topic, data) {
> +      if (data != "init-complete") {
> +        return;
> +      }
> +
> +      Services.obs.removeObserver(observer, SEARCH_SERVICE_TOPIC);
> +      resolve();
> +    }, SEARCH_SERVICE_TOPIC);
> +  });

you can use `ExtensionUtils.promiseObserved()` for this

::: toolkit/components/extensions/parent/ext-search.js:45
(Diff revision 2)
> +            let searchEngine = {
> +              name: engine.name,
> +              is_default: engine === Services.search.currentEngine,
> +              alias: engine.alias,
> +              favicon_url: engine.iconURI.spec,
> +            };
> +            return searchEngine;

nit: shorten this to
```
return {
 ...
};
```

::: toolkit/components/extensions/parent/ext-search.js:49
(Diff revision 2)
> +          return engines.filter(engine => !engine.hidden).map(engine => {
> +            let searchEngine = {
> +              name: engine.name,
> +              is_default: engine === Services.search.currentEngine,
> +              alias: engine.alias,
> +              favicon_url: engine.iconURI.spec,

You said previously that this is a resource: url right?  That may happen to work right now but we shouldn't be exposing those to extensions.
How about converting them to data: urls?

::: toolkit/components/extensions/parent/ext-search.js:70
(Diff revision 2)
> +          };
> +          if (Services.prefs.getBoolPref("browser.search.context.loadInBackground")) {
> +            params.inBackground = true;
> +          }
> +
> +          let window = context.currentWindow || Services.wm.getMostRecentWindow("navigator:browser");

Since this function requires user input, we should get the window where the actual user input occurred, like this:
https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/toolkit/components/extensions/parent/ext-permissions.js#44

::: toolkit/components/extensions/parent/ext-search.js:76
(Diff revision 2)
> +          let observer = (subject, topic, data) => {
> +            fire.async(data);
> +          };

It looks like `data` just tells you what sort of change took place (ie, added, removed) but nothing about the actual change (ie the name/details of the engine that was added/removed)

This event also doesn't have any tests.  Maybe it would be easier to land the rest of the API without onChanged and add the event in a follow-up?

::: toolkit/components/extensions/schemas/search.json:14
(Diff revision 2)
> +      {
> +        "$extend": "OptionalPermission",
> +        "choices": [{
> +          "type": "string",
> +          "enum": [
> +            "search"

Do we want users to see a prompt for extensions that request this permission?  If so, an appropriate string needs to be added to browser.properties (and unfortunately duplicated for mobile since this is in toolkit)

::: toolkit/components/extensions/schemas/search.json:22
(Diff revision 2)
> +      }
> +    ]
> +  },
> +  {
> +    "namespace": "search",
> +    "description": "Use chrome.search to nteract with search engines.",

Please call this browser.search not chrome.search.  And also typo in interact.
(I think I commented on this before and the issue was marked resolved even though it wasn't addressed)

::: toolkit/components/extensions/test/browser/browser_ext_search.js:25
(Diff revision 2)
> +        },
> +      },
> +      name: TEST_ID,
> +      permissions: ["search"],
> +    },
> +    background: `(${background})()`,

Just `background,` should be sufficient here

        Attachment #8961558 -
        Flags: review?(aswan) → review-

    
    

    
  
(In reply to Mike Kaply [:mkaply] from comment #55)
> I'm curious as to the use case for opening a new window?

That's what my legacy add-on allowed as a feature option, ideally I'd be able to preserve that, but tabs would be good enough to cover what most people want. As I say, nice to have.

    
    

    
  
(In reply to Ben Basson from comment #58)
> (In reply to Mike Kaply [:mkaply] from comment #55)
> > I'm curious as to the use case for opening a new window?
> 
> That's what my legacy add-on allowed as a feature option, ideally I'd be
> able to preserve that, but tabs would be good enough to cover what most
> people want. As I say, nice to have.

If you can pass a tab object into the search method, it would be up to your extension which tab object to pass. that would incluce a new tab created in foreground or background, the current tab or a tab in another window ( existing or new). it would be nice if we get a read only pass through of the new/current tab search preference through the search manager, since presumably that pref is now hidden from us.
Blocks: 1443843

    
    

    
  
(In reply to Mike Kaply [:mkaply] from comment #55)
> I'm curious as to the use case for opening a new window?
This would be useful to Tridactyl (https://addons.mozilla.org/en-US/firefox/addon/tridactyl-vim/) although, as previously mentioned, extensions could just open a new tab in a new window and then have the search happen in this new window.

    
    

    
  
(In reply to snuz.gary@gmail.com from comment #59)
> If you can pass a tab object into the search method, it would be up to your
> extension which tab object to pass. that would incluce a new tab created in
> foreground or background, the current tab or a tab in another window (
> existing or new). 

This seems like an elegant solution and one that I would be 100% happy with.

    
    

    
      
  

    
    

    
  
Updated patch.

Everything has been resolved per the review except for the resource URL situation. I can't find an easy way to convert these icons over to data URLs. I'm still investigating.

The API has been updated to take a tab.

There is a test for search.

I've removed onChanged for now so I can focus on the core idea.

    
    

    
  
Comment on attachment 8961558 [details]
Bug 1352598 - Add WebExtension API for access to search.

https://reviewboard.mozilla.org/r/230422/#review243514

Getting closer but we still need to deal with icons.
There's also no test coverage of search() with no tabId argument or of search() with engines that require post data.

::: browser/components/extensions/parent/ext-browser.js:150
(Diff revision 3)
> +// This promise is used to wait for the search service to be initialized.
> +// None of the code in the WebExtension modules requests that initialization.
> +// It is assumed that it is started at some point. If tests start to fail
> +// because this promise never resolves, that's likely the cause.
> +let searchInitialized = () => {
> +  if (Services.search.isInitialized) {
> +    return;
> +  }
> +  return ExtensionUtils.promiseObserved("browser-search-service", (_, data) => data == "init-complete");
> +};

Please make this a lazy getter that is a single Promise

::: browser/components/extensions/parent/ext-chrome-settings-overrides.js:53
(Diff revision 3)
>      let item = await ExtensionSettingsStore.getSetting(
>        DEFAULT_SEARCH_STORE_TYPE, ENGINE_ADDED_SETTING_NAME, id);
>      if (item) {
>        ExtensionSettingsStore.removeSetting(
>          id, DEFAULT_SEARCH_STORE_TYPE, ENGINE_ADDED_SETTING_NAME);
> -      await searchInitialized();
> +      await global.searchInitialized();

global. should not be necessary here

::: browser/components/extensions/parent/ext-search.js:34
(Diff revision 3)
> +              favicon_url: engine.iconURI ? engine.iconURI.spec : null,
> +            };
> +          });
> +        },
> +
> +        async search(name, data, tabId) {

Please change data to searchTerms to be consistent with the schema

::: browser/components/extensions/parent/ext-search.js:41
(Diff revision 3)
> +          let engine = Services.search.getEngineByName(name);
> +          if (!engine) {
> +            throw new ExtensionError(`${name} was not found`);
> +          }
> +          let submission = engine.getSubmission(data, null, "webextension");
> +          if (tabId) {

We don't allow 0 as a valid tabId, nevertheless this is bad style, please do a strict check against null.

::: browser/components/extensions/parent/ext-search.js:43
(Diff revision 3)
> +            throw new ExtensionError(`${name} was not found`);
> +          }
> +          let submission = engine.getSubmission(data, null, "webextension");
> +          if (tabId) {
> +            let tab = tabTracker.getTab(tabId);
> +            tab.linkedBrowser.loadURI(submission.uri.spec);

What if the search requires post data?

::: browser/components/extensions/parent/ext-search.js:51
(Diff revision 3)
> +              postData: submission.postData,
> +            };
> +            let browser = context.pendingEventBrowser || context.xulBrowser;
> +            let {gBrowser} = browser.ownerGlobal;
> +            let nativeTab = gBrowser.addTab(submission.uri.spec, options);
> +            if (!Services.prefs.getBoolPref("browser.search.context.loadInBackground")) {

please make a lazyPreferenceGetter for this

::: browser/components/extensions/test/browser/browser_ext_search.js:9
(Diff revision 3)
> +
> +add_task(async function test_search() {
> +  const TEST_ID = "test_search@tests.mozilla.com";
> +
> +  async function background() {
> +    browser.search.get().then((engines) => {

This can just be
```
let engines = await browser.search.get();
browser.test.sendMessage("engine", engines);
```
It could even be a one-liner if you want but the above is fine too

::: browser/components/extensions/test/browser/browser_ext_search.js:13
(Diff revision 3)
> +  async function background() {
> +    browser.search.get().then((engines) => {
> +      browser.test.sendMessage("engines", engines);
> +    });
> +    browser.browserAction.onClicked.addListener(tab => {
> +      chrome.tabs.onUpdated.addListener(function(tabId, info, changedTab) {

chrome -> browser

::: browser/components/extensions/test/browser/browser_ext_search.js:24
(Diff revision 3)
> +      applications: {
> +        gecko: {
> +          id: TEST_ID,
> +        },
> +      },

This appears to be unnecessary

::: browser/components/extensions/test/browser/browser_ext_search.js:39
(Diff revision 3)
> +          "name": "Search Test",
> +          "search_url": "https://localhost/",
> +        },
> +      },
> +    },
> +    background: background,

nit: just `background,` is sufficient

::: browser/components/extensions/test/browser/browser_ext_search.js:46
(Diff revision 3)
> +  });
> +  await extension.startup();
> +
> +  let addonEngines = await extension.awaitMessage("engines");
> +  let engines = Services.search.getEngines().filter(engine => !engine.hidden);
> +  is(addonEngines.length, engines.length, "Engine lengths are the same.");

What about the individual properties here?  How about at least testing they have the right types, only one entry has is_default == true, etc

        Attachment #8961558 -
        Flags: review?(aswan) → review-

    
    

    
  
(In reply to Andrew Swan [:aswan] from comment #64)
> Comment on attachment 8961558 [details]
> Bug 1352598 - Add WebExtension API for access to search.
> 
Is someone working on the fixes mentioned in this comment, to resubmit?
Thanks.
Flags: needinfo?(evilpies)

    
  

        Attachment #8933952 -
        Attachment is obsolete: true
Flags: needinfo?(evilpies)

    
    

    
  
I am not working on this.
Flags: needinfo?(mozilla)

    
    

    
  
Yes, sorry. Sidetracked by 60. Still on my radar.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(mozilla)

    
    

    
  
(In reply to Mike Kaply [:mkaply] from comment #67)
> Yes, sorry. Sidetracked by 60. Still on my radar.

Great, thanks. Would be nice to get this one put away.

    
    

    
  
(In reply to Mike Kaply [:mkaply] from comment #67)
> Yes, sorry. Sidetracked by 60. Still on my radar.

Any progress at least getting the fixes submitted for testing?

    
    

    
      
  

    
    

    
  
Worcester12345 - thanks for the reminder.

Updated patch with most things addressed. Open issues:

1. I'm getting:

browser/components/extensions/test/browser/browser_ext_search.js
  FAIL Found an unexpected tab at the end of test run: https://localhost/ -
browser/components/extensions/test/browser/test-oop-extensions/browser_ext_search.js
  FAIL Found an unexpected tab at the end of test run: https://localhost/ -

at the end of the test even though I'm explicitly closing the open tab.

2. I haven't figured out how to test POST engines.

3. I'm not sure how to convert the resource URLs to data URLs. Are we sure that the resource URLs won't be allowed at some point?

    
    

    
  
(In reply to Mike Kaply [:mkaply] from comment #71)
> Worcester12345 - thanks for the reminder.
> 
> Updated patch with most things addressed. Open issues:
> 
> 1. I'm getting:
> 
> browser/components/extensions/test/browser/browser_ext_search.js
>   FAIL Found an unexpected tab at the end of test run: https://localhost/ -
> browser/components/extensions/test/browser/test-oop-extensions/
> browser_ext_search.js
>   FAIL Found an unexpected tab at the end of test run: https://localhost/ -
> 
> at the end of the test even though I'm explicitly closing the open tab.

From a quick glance I think probably you need to await the result of browser.tabs.remove()

> 2. I haven't figured out how to test POST engines.

I think easiest would be to use a .sjs file, it is covered here:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest

> 3. I'm not sure how to convert the resource URLs to data URLs. Are we sure
> that the resource URLs won't be allowed at some point?

I don't understand this question

    
    

    
  
(In reply to Andrew Swan [:aswan] from comment #72)
> > 3. I'm not sure how to convert the resource URLs to data URLs. Are we sure
> > that the resource URLs won't be allowed at some point?
> 
> I don't understand this question

Sorry, what I meant was, are we sure the resource URLs should not be allowed? Is it documented anywhere that a WebExtension can't use a resource URL for an image.

And is there some clever way to convert the resource image to a data URL without doing a canvas.

    
    

    
  
I'm not sure if this qualifies as clever, but in the past I have embedded images in js by converting them to base64 using an online thingy and then embedding the data in html/js by pasting the text result into the <data>.

    
    

    
  
(In reply to Mike Kaply [:mkaply] from comment #73)
> Sorry, what I meant was, are we sure the resource URLs should not be
> allowed? Is it documented anywhere that a WebExtension can't use a resource
> URL for an image.

I don't know if there is explicit documentation but resource: urls are part of the browser implementation and I don't believe there are other places where they are exposed to content or extensions.

> And is there some clever way to convert the resource image to a data URL
> without doing a canvas.

Why do you need a canvas?  I was imagining something like (off the top of my head, untested):
```
let response = await fetch("resource:whatever");
let type = response.headers.get("content-type");
return `data:${type};base64,${btoa(response.text)}`;

```

    
    

    
  
I was thinking I would have to draw it to a canvas and then pull it as a base64.

Your idea is quite interesting, but unfortunately results in:

InvalidCharacterError: String contains an invalid character

on the btoa.

I'm looking into it.

    
    

    
  
(In reply to Mike Kaply [:mkaply] from comment #76)
> I was thinking I would have to draw it to a canvas and then pull it as a
> base64.
> 
> Your idea is quite interesting, but unfortunately results in:
> 
> InvalidCharacterError: String contains an invalid character
> 
> on the btoa.
> 
> I'm looking into it.

https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/btoa#Unicode_strings

    
    

    
  
(In reply to kernp25 from comment #77)
> (In reply to Mike Kaply [:mkaply] from comment #76)
> > I was thinking I would have to draw it to a canvas and then pull it as a
> > base64.
> > 
> > Your idea is quite interesting, but unfortunately results in:
> > 
> > InvalidCharacterError: String contains an invalid character
> > 
> > on the btoa.
> > 
> > I'm looking into it.
> 
> https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/
> btoa#Unicode_strings

From that link:
"A better, more faithful and less expensive solution is to use typed arrays to do the conversion."

    
    

    
      
  

    
    

    
  
Comment on attachment 8961558 [details]
Bug 1352598 - Add WebExtension API for access to search.

https://reviewboard.mozilla.org/r/230422/#review253780

::: browser/components/extensions/parent/ext-search.js:28
(Diff revision 5)
> +  let response = await fetch(resourceURI);
> +  let buffer = await response.arrayBuffer();
> +  // Remove charset from content type
> +  let contentType = response.headers.get("content-type").split(",");
> +  let bytes = new Uint8Array(buffer);
> +  let str = "";

let str = String.fromCharCode.apply(null, bytes);

    
    

    
  
(In reply to Tom Schuster [:evilpie] from comment #80)
> Comment on attachment 8961558 [details]
> Bug 1352598 - Add WebExtension API for access to search.
> 
> https://reviewboard.mozilla.org/r/230422/#review253780
> 
> ::: browser/components/extensions/parent/ext-search.js:28
> (Diff revision 5)
> > +  let response = await fetch(resourceURI);
> > +  let buffer = await response.arrayBuffer();
> > +  // Remove charset from content type
> > +  let contentType = response.headers.get("content-type").split(",");
> > +  let bytes = new Uint8Array(buffer);
> > +  let str = "";
> 
> let str = String.fromCharCode.apply(null, bytes);

let str = String.fromCharCode(...bytes);

Also seems to work :)

    
    

    
  
(In reply to kernp25 from comment #81)
> (In reply to Tom Schuster [:evilpie] from comment #80)
> > Comment on attachment 8961558 [details]
> > Bug 1352598 - Add WebExtension API for access to search.
> > 
> > https://reviewboard.mozilla.org/r/230422/#review253780
> > 
> > ::: browser/components/extensions/parent/ext-search.js:28
> > (Diff revision 5)
> > > +  let response = await fetch(resourceURI);
> > > +  let buffer = await response.arrayBuffer();
> > > +  // Remove charset from content type
> > > +  let contentType = response.headers.get("content-type").split(",");
> > > +  let bytes = new Uint8Array(buffer);
> > > +  let str = "";
> > 
> > let str = String.fromCharCode.apply(null, bytes);
> 
> let str = String.fromCharCode(...bytes);
> 
> Also seems to work :)

This will work, but is worse in SpiderMonkey. This might lead to stackoverflow or similar, because it actually pushes all those bytes onto the stack as parameters to fromCharCode.

    
    

    
  
Comment on attachment 8961558 [details]
Bug 1352598 - Add WebExtension API for access to search.

Mike says he's working on a revision, clearing r? until that is ready

        Attachment #8961558 -
        Flags: review?(aswan)

    
    

    
      
  

    
    

    
  
aswan:

With regards to tests for POSTS, I'm going to need to punt on that for two reasons.

1. WebExtensions don't support POST search engines so I can't easily add them to a manifest.

2. The search service doesn't support adding POST search engines using addEngineWithDetails, so I can't easily manually add them for testing.

POST search engines are a very edge case, and I have verified that they do work with the patch (I tracked one down from http://mycroftproject.com/).

For the icon conversion, I ended up keeping the base64 conversion using btoa instead of FileReader.

    
    

    
  
First off, this looks great!  Tried it out on a local build and it was very quick to make an extension that replaces my most-missed one since 57.

Secondly, looks like you're suffering from bug 1465164, don't think all those package-lock.json changes should be in the patch.

    
    

    
  
Comment on attachment 8961558 [details]
Bug 1352598 - Add WebExtension API for access to search.

https://reviewboard.mozilla.org/r/230422/#review254842

::: browser/components/extensions/parent/ext-browser.js:216
(Diff revision 6)
> +XPCOMUtils.defineLazyGetter(global, "searchInitialized", () => {
> +  if (Services.search.isInitialized) {
> +    return;
> +  }
> +  return ExtensionUtils.promiseObserved("browser-search-service", (_, data) => data == "init-complete");
> +});

why isn't eslint complaining about inconsistent returns here?

::: browser/components/extensions/parent/ext-search.js:29
(Diff revision 6)
> +  let buffer = await response.arrayBuffer();
> +  // Remove charset from content type
> +  let contentType = response.headers.get("content-type").split(",");
> +  let bytes = new Uint8Array(buffer);
> +  let str = String.fromCharCode.apply(null, bytes);
> +  return `data:${contentType};base64,${btoa(str)}`;

this doesn't look right, contentType is an array...

::: browser/components/extensions/parent/ext-search.js:40
(Diff revision 6)
> +      search: {
> +        async get() {
> +          await searchInitialized;
> +          let engines = Services.search.getEngines();
> +          let visibleEngines = engines.filter(engine => !engine.hidden);
> +          // Can't use map because it doesn't support Promises

how about `Promise.all()`

::: browser/components/extensions/parent/ext-search.js:51
(Diff revision 6)
> +            };
> +            if (visibleEngine.alias) {
> +              engine.alias = visibleEngine.alias;
> +            }
> +            if (visibleEngine.iconURI) {
> +              if (visibleEngine.iconURI.spec.startsWith("resource:")) {

I don't know the search service at all, what are all the possible schemes that can be returned here?

::: browser/components/extensions/parent/ext-search.js:73
(Diff revision 6)
> +          let submission = engine.getSubmission(searchTerms, null, "webextension");
> +          let options = {
> +            postData: submission.postData,
> +          };
> +          if (tabId == null) {
> +            let browser = context.pendingEventBrowser || context.xulBrowser;

Should check that browser is not null here.  There are lots of cases where an extension is considered handlingUserInput but we can't find an enclosing browser chrome (e.g., popups, sidebars, options pages, etc)

        Attachment #8961558 -
        Flags: review?(aswan) → review-

    
    

    
  
Comment on attachment 8961558 [details]
Bug 1352598 - Add WebExtension API for access to search.

https://reviewboard.mozilla.org/r/230422/#review254842

> why isn't eslint complaining about inconsistent returns here?

I don't know that answer, but I'm wondering what the right thing is to return here in this case. Creating a promise to then immediately resolve seems overkill.

> how about `Promise.all()`

I guess I would then have to loop through and store all the promises and then Promise.all on the promises that happened?

I think that's probably overkill for what we're doing here (just to avoid the for loop). Unless I'm missing something...

> I don't know the search service at all, what are all the possible schemes that can be returned here?

https://searchfox.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#1679

We theoretically allow resource and chrome for built in engines, but only use resource. data/http/https/ftp can come from the engine itself.

So I should probably rename the function and allow it to take chrome and resource. Is that what you were thinking?

> Should check that browser is not null here.  There are lots of cases where an extension is considered handlingUserInput but we can't find an enclosing browser chrome (e.g., popups, sidebars, options pages, etc)

Any thoughts as to what to do in that case? Should I go back to Services.wm.getMostRecentWindow("navigator:browser") and getting a gBrowser from there? I don't see anything similar in existing code.

    
    

    
  
> > how about `Promise.all()`
> 
> I guess I would then have to loop through and store all the promises and
> then Promise.all on the promises that happened?
> 
> I think that's probably overkill for what we're doing here (just to avoid
> the for loop). Unless I'm missing something...

How about await Promise.all(visibleEngines.map(async x => { ... }))

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Latest patch (deleted)
        — 
          — Splinter Review
      

    


  
mozreview won't let me do anything with this since aswan is not taking reviews, but I need to put it somewhere.

    
  
Product: Toolkit → WebExtensions

    
    

    
      
  

    
  

        Attachment #8961558 -
        Flags: review?(kmaglione+bmo)

    
    

    
      
  

    
    

    
  
Comment on attachment 8961558 [details]
Bug 1352598 - Add WebExtension API for access to search.

https://reviewboard.mozilla.org/r/230422/#review260544

r=me with comments addressed

::: browser/components/extensions/parent/ext-browser.js:229
(Diff revision 8)
> +// None of the code in the WebExtension modules requests that initialization.
> +// It is assumed that it is started at some point. If tests start to fail
> +// because this promise never resolves, that's likely the cause.
> +XPCOMUtils.defineLazyGetter(global, "searchInitialized", () => {
> +  if (Services.search.isInitialized) {
> +    return Promise.resolve(true);

nit: do you actually care about the resolution value here?  ie, can this just be `return Promise.resolve();`

::: browser/components/extensions/parent/ext-search.js:69
(Diff revision 8)
> +          if (!engine) {
> +            throw new ExtensionError(`${name} was not found`);
> +          }
> +          let submission = engine.getSubmission(searchTerms, null, "webextension");
> +          let options = {
> +            postData: submission.postData,

We should be passing a triggeringPrincipal as well (which you can get from `context.principal`).

::: browser/components/extensions/test/browser/browser_ext_search.js:38
(Diff revision 8)
> +  let addonEngines = await extension.awaitMessage("engines");
> +  let engines = Services.search.getEngines().filter(engine => !engine.hidden);
> +  is(addonEngines.length, engines.length, "Engine lengths are the same.");
> +  let defaultEngine = addonEngines.filter(engine => engine.is_default === true);
> +  is(defaultEngine.length, 1, "One default engine");
> +  is(defaultEngine[0].name, "Google", "Google is default");
> +  await clickBrowserAction(extension);
> +  let url = await extension.awaitMessage("searchLoaded");
> +  is(url, "https://localhost/", "Loaded page matches search");
> +  await extension.unload();

I'm a little surprised this isn't already breaking, but as a general rule, a browser test should leave the browser in the same state it found it in.  So this should either open a new tab to use for the search (and then close it before finishing) or navigate the tab back to where it started before exiting.  Preferrably the former.

::: browser/components/extensions/test/browser/browser_ext_search.js:43
(Diff revision 8)
> +  let addonEngines = await extension.awaitMessage("engines");
> +  let engines = Services.search.getEngines().filter(engine => !engine.hidden);
> +  is(addonEngines.length, engines.length, "Engine lengths are the same.");
> +  let defaultEngine = addonEngines.filter(engine => engine.is_default === true);
> +  is(defaultEngine.length, 1, "One default engine");
> +  is(defaultEngine[0].name, "Google", "Google is default");

what is the purpose of testing this here?

::: browser/components/extensions/test/browser/browser_ext_search.js:46
(Diff revision 8)
> +  let defaultEngine = addonEngines.filter(engine => engine.is_default === true);
> +  is(defaultEngine.length, 1, "One default engine");
> +  is(defaultEngine[0].name, "Google", "Google is default");
> +  await clickBrowserAction(extension);
> +  let url = await extension.awaitMessage("searchLoaded");
> +  is(url, "https://localhost/", "Loaded page matches search");

why is the search term not present in the url?

::: browser/components/extensions/test/browser/browser_ext_search.js:55
(Diff revision 8)
> +      browser.tabs.onCreated.addListener(function(tab) {
> +        browser.tabs.onUpdated.addListener(async (tabId, info, changedTab) => {

This looks racy -- the tab could finish loading before you get a chance to add the listener.  Could you get away with just the onUpdated listener?

        Attachment #8961558 -
        Flags: review?(aswan) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/879fbe1b07f4
Add WebExtension API for access to search. r=aswan

    
    

    
  
Maybe you can use
> XPCOMUtils.defineLazyGlobalGetters(this, ["fetch", "btoa"]);

instead of

> Cu.importGlobalProperties(["fetch", "btoa"]);

for better performance?

https://bugzilla.mozilla.org/show_bug.cgi?id=1464548

    
    

    
  
Also, i would write the following like so:
> let contentType = response.headers.get("content-type").split(",")[0];

instead of

> let contentType = response.headers.get("content-type").split(",");

    
    

    
  
I would write the following code like so:  
> if (engine.iconURI.schemeIs("resource") ||

instead of

> if (engine.iconURI.spec.startsWith("resource:") ||

https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/netwerk/base/nsIURI.idl#182

    
    

    
  
I've relanded this hoping to see how intermittent it is.

I couldn't get any failures on try.

Thanks for the feedback. I'll see about doing some cleanup in a later patch.
Flags: needinfo?(mozilla)

    
    

    
  
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/f5bfc9971285
Add WebExtension API for access to search. r=aswan

    
    

    
  
Backed out changeset f5bfc9971285 (bug 1352598) for browser chrome failures on browser_ext_search.js

Backout: https://hg.mozilla.org/integration/autoland/rev/6e281abe4d39f6d1d3c40fdaca47c009db6720de

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f5bfc9971285d5edcf12e4610d521eb4bedf7a59&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&selectedJob=186240176

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=186240176&repo=autoland&lineNumber=3252

[task 2018-07-03T18:38:32.101Z]     INFO - TEST-START | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_search.js
[task 2018-07-03T18:39:17.110Z]     INFO - TEST-INFO | started process screentopng
[task 2018-07-03T18:39:17.618Z]     INFO - TEST-INFO | screentopng: exit 0
[task 2018-07-03T18:39:17.620Z]     INFO - Buffered messages logged at 18:38:32
[task 2018-07-03T18:39:17.621Z]     INFO - Entering test bound test_search
[task 2018-07-03T18:39:17.621Z]     INFO - Extension loaded
[task 2018-07-03T18:39:17.622Z]     INFO - Console message: Warning: attempting to write 15503 bytes to preference extensions.webextensions.uuids. This is bad for general performance and memory usage. Such an amount of data should rather be written to an external file. This preference will not be sent to any content processes.
[task 2018-07-03T18:39:17.624Z]     INFO - Console message: Warning: attempting to write 9820 bytes to preference browser.uiCustomization.state. This is bad for general performance and memory usage. Such an amount of data should rather be written to an external file. This preference will not be sent to any content processes.
[task 2018-07-03T18:39:17.625Z]     INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_search.js | Engine lengths are the same. - 
[task 2018-07-03T18:39:17.626Z]     INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_search.js | One default engine - 
[task 2018-07-03T18:39:17.628Z]     INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_search.js | Default engine is correct - 
[task 2018-07-03T18:39:17.630Z]     INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_search.js | Expect widget not to be overflowed - 
[task 2018-07-03T18:39:17.630Z]     INFO - Buffered messages finished
[task 2018-07-03T18:39:17.631Z]     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_search.js | Test timed out -
Flags: needinfo?(mozilla)

    
    

    
      
  

    
    

    
  
Mike, should it not be:
let contentType = response.headers.get("content-type").split(";")[0];

Because, you split the "," and not the ";" in the content type?

It says:
> // Remove charset from content type

Content type header looks like this:
application/x-javascript;charset=UTF-8

?

Is the split also needed here, because "data:application/x-javascript;charset=UTF-8;base64," also works.

> let contentType = response.headers.get("content-type"); // should also work

Also, what if the content-type header doesn't exists (is that possible?)?
https://developer.mozilla.org/en-US/docs/Web/API/Headers/get says it will return null.

    
    

    
  
Actually I'm thinking too hard about this. It's an image. It will never have a charset. Just getting the contenttype is enough.
Flags: needinfo?(mozilla)

    
    

    
      
  

    
    

    
  
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/68a6a71686fa
Add WebExtension API for access to search. r=aswan

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/68a6a71686fa
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox63:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

    
    

    
  
I don't understand this:
"Search the active tab for "banana", log the number of matches, and highlight them:"?

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/search/get#Basic_examples
Flags: needinfo?(geoffreydebelie)

    
    

    
  
Maybe you can also write here, if the search engine could not be found, it will throw an error?

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/search/search

    
    

    
  
(In reply to kernp25 from comment #111)
> I don't understand this:
> "Search the active tab for "banana", log the number of matches, and
> highlight them:"?
> 
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/search/
> get#Basic_examples

Fixed, was copied from a different page

(In reply to kernp25 from comment #112)
> Maybe you can also write here, if the search engine could not be found, it
> will throw an error?
> 
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/search/search

I wrote "If the search engine name you specify doesn't exist, the promise will be rejected."
Flags: needinfo?(geoffreydebelie)

    
    

    
  
(In reply to Geoffrey De Belie (Smile4ever) from comment #113)
> I wrote "If the search engine name you specify doesn't exist, the promise
> will be rejected."

But "browser.search.search" doesn't return a Promise?
Flags: needinfo?(geoffreydebelie)

    
    

    
  
Can "browser.search.search" also return a Promise?

Then it can reject with an error message.
Flags: needinfo?(mozilla)

    
    

    
  
(In reply to kernp25 from comment #115)
> Can "browser.search.search" also return a Promise?
> 
> Then it can reject with an error message.

@mkaply Is it perhaps cleaner to return a promise here?
Flags: needinfo?(geoffreydebelie)

    
    

    
  
(In reply to Geoffrey De Belie (Smile4ever) from comment #116)
> (In reply to kernp25 from comment #115)
> > Can "browser.search.search" also return a Promise?
> > 
> > Then it can reject with an error message.
> 
> @mkaply Is it perhaps cleaner to return a promise here?

Or for example, if you pass an invalid tabId to "browser.search.search" then it can reject with an error message
"tab could not be found" or something.

    
    

    
  
That's probably fair. Let's document as is and open a followup bug. I've got some other work I'm going to do as well.
Flags: needinfo?(mozilla)

    
    

    
  
Shouldn't the search engine object properties be named using camel case, just like it's done for tab properties (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab)? So for example favicon_url would be favIconUrl instead. Sure it's not a big deal, but it would be more consitent to follow the convention.

    
    

    
  
> Shouldn't the search engine object properties be named using camel case, just like it's done for tab properties (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab)? So for example favicon_url would be favIconUrl instead. Sure it's not a big deal, but it would be more consitent to follow the convention.

I'm actually matching the convention in chrome_settings_override:

https://developer.chrome.com/extensions/settings_override

that's the only place these search attributes are defined in WebExtensions.

    
    

    
  
In (at least some) other situations like this they are mapped to camelCase: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contentScripts/register.

    
    

    
  
(In reply to Will Bamberg [:wbamberg] from comment #121)
> In (at least some) other situations like this they are mapped to camelCase:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contentScripts/
> register.

This is enough to convince me that we should change it. Better to be consistent with everything else.

    
  
Summary: Add an API to list installed search engines → Add an API to list installed search engines and execute searches using only those engines

    
    

    
  
I'm also seeing that "alias" and "favicon_url" will be null sometimes.
But should these not be undefined instead of null?

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/search/get

    
    

    
  
(In reply to robbendebiene from comment #119)
> Shouldn't the search engine object properties be named using camel case,
> just like it's done for tab properties
> (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab)? So
> for example favicon_url would be favIconUrl instead. Sure it's not a big
> deal, but it would be more consitent to follow the convention.

"searchTerms" from browser.search.search[1] also is in camelCase.


[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/search/search

    
    

    
  
Please take any new comments to this bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=1475347
Blocks: 1475347

    
    

    
  
Adding some links to other follow-up (implementation) bugs.

Before writing documentation, please see those bugs (the API signature and permission requirements may change).
No longer blocks: 1475347
Depends on: 1477248, 1477271, 1475842, 1476415, 1474941, 1475347

    
    

    
  
Smile4ever, thanks for writing some docs for this API! I'm going to wait until things settle down before finishing them off though.
Flags: needinfo?(wbamberg)

    
  
Depends on: 1479689

    
    

    
  
Hi,

I created a new bug that is more an evolution of the search.get() API.
with an example to illustrate 

https://bugzilla.mozilla.org/show_bug.cgi?id=1479689

if someone can add it in "Depends on"

Thank's

Christophe

    
    

    
  
Is manual QA needed on this bug? If yes, please provide a test webextension and some STRs, else add the "qe-verify-" flag.
Flags: needinfo?(mozilla)

    
    

    
  
I've updated the docs written by :Smile4ever:

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/search
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/search/get
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/search/search

Please let me know if this covers it. I'm working on an example extension too.

One thing that occurred to me: since search engines can be added and removed, I wonder if this API should have an onInstalled (and onRemoved) event that fires when this happens? That would give an extension like https://addons.mozilla.org/de/firefox/addon/quickcontextsearch/ a nice way to update the menu item when the search engine list changes.

    
    

    
  
Maybe you can write it like so:

browser.search.search(
  searchProperties       // object
)

instead of

browser.search.search({
  query: "the search query",    // string
  engine: "the search engine",  // optional string
  tabId:  1                     // optional integer
)

?

Also, you missing a "}" at the end.
Flags: needinfo?(wbamberg)

    
    

    
  
browser.search.search({ engine: searchProvider, query: searchTerms });(In reply to Will Bamberg [:wbamberg] from comment #130)
> One thing that occurred to me: since search engines can be added and
> removed, I wonder if this API should have an onInstalled (and onRemoved)
> event that fires when this happens? That would give an extension like
> https://addons.mozilla.org/de/firefox/addon/quickcontextsearch/ a nice way
> to update the menu item when the search engine list changes.

But this extension is not compatible with Firefox Quantum?

https://addons.mozilla.org/de/firefox/addon/simple-context-search/ uses the menus.onShown event
to get the installed search engines.

    
    

    
  
Please ignore the "browser.search.search({ engine: searchProvider, query: searchTerms });" part, i forgot to remove it.

    
    

    
  
> Maybe you can write it like so:

I've done this.

> But this extension is not compatible with Firefox Quantum?

I know: I was just using it as an example of an extension that could use this API. It was mentioned above as a potential user of this API.

> https://addons.mozilla.org/de/firefox/addon/simple-context-search/ uses the menus.onShown event
to get the installed search engines.

Then I guess it isn't needed. It seemed better to me to update when the list changes, not every time the menu is shown. But it's just a suggestion.
Flags: needinfo?(wbamberg)

    
    

    
  
(In reply to Will Bamberg [:wbamberg] from comment #134)
> It seemed better to me to update when the list
> changes, not every time the menu is shown. But it's just a suggestion.

For better performance it makes sense, yes.

What do you think?
Flags: needinfo?(rob)

    
    

    
  
My original plan was to do update messages, but it was delaying the feature. I'll open a new bug.
Flags: needinfo?(rob)

    
    

    
  
(In reply to Will Bamberg [:wbamberg] from comment #130)
> I wonder if this API should have an onInstalled (and onRemoved) event
> that fires when this happens? That would give an extension like
> https://addons.mozilla.org/de/firefox/addon/quickcontextsearch/ a nice way
> to update the menu item when the search engine list changes.

It may also be edited: For example, the changing the keyword by user, or auto update(?). So, maybe another onChange is required. Maybe only adding an onChange instead of onChange + onInstalled + onRemoved?

    
    

    
  
Thanks for implementing this API!
There is just one thing that bothers me.

> Also, you can only call this function from inside the handler for a user action, such as
>    clicking the extension's browser action or page action
>    selecting its context menu item
>    activating a keyboard shortcut defined by the extension (note: this is not currently supported in Firefox)
>    clicking a button in a page bundled with the extension.
(from: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/search/search)

This basically means that add-ons who want to trigger a search from a content script via background messages cannot do so. I'm the developer of Gesturefy and wanted to use this API to display a popup list containing the users search engines and allow the user to execute a search with the current text selection as the query. Why does this restriction exist? It would make more sense to me if it would behave like the clipboardWrite API, which can be used without the clipboardWrite permissions if the fuction is called inside a handler. If not the clipboardWrite permission is required. However the search API requires both.

    
    

    
  
(In reply to robbendebiene from comment #138)
> Thanks for implementing this API!
> There is just one thing that bothers me.
> 
> > Also, you can only call this function from inside the handler for a user action, such as
> >    clicking the extension's browser action or page action
> >    selecting its context menu item
> >    activating a keyboard shortcut defined by the extension (note: this is not currently supported in Firefox)
> >    clicking a button in a page bundled with the extension.
> (from:
> https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/
> search/search)
> 
> This basically means that add-ons who want to trigger a search from a
> content script via background messages cannot do so. I'm the developer of
> Gesturefy and wanted to use this API to display a popup list containing the
> users search engines and allow the user to execute a search with the current
> text selection as the query. Why does this restriction exist? It would make
> more sense to me if it would behave like the clipboardWrite API, which can
> be used without the clipboardWrite permissions if the fuction is called
> inside a handler. If not the clipboardWrite permission is required. However
> the search API requires both.

https://bugzilla.mozilla.org/show_bug.cgi?id=1477248

    
    

    
  
(In reply to robbendebiene from comment #138)
> Thanks for implementing this API!
> There is just one thing that bothers me.
> 
> > Also, you can only call this function from inside the handler for a user action, such as
> >    clicking the extension's browser action or page action
> >    selecting its context menu item
> >    activating a keyboard shortcut defined by the extension (note: this is not currently supported in Firefox)
> >    clicking a button in a page bundled with the extension.
> (from:
> https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/
> search/search)
> 
> This basically means that add-ons who want to trigger a search from a
> content script via background messages cannot do so. I'm the developer of
> Gesturefy and wanted to use this API to display a popup list containing the
> users search engines and allow the user to execute a search with the current
> text selection as the query. Why does this restriction exist? It would make
> more sense to me if it would behave like the clipboardWrite API, which can
> be used without the clipboardWrite permissions if the fuction is called
> inside a handler. If not the clipboardWrite permission is required. However
> the search API requires both.

For the record, I have the exact same problem. I see the new bug is addressing it.

    
    

    
  
I think this is good via automated tests.
Flags: needinfo?(mozilla) → qe-verify-

    
  
Depends on: 1525729

    
    

    
  
Ben Basson, is this holding up Context Search any longer?


Flags: needinfo?(bugzilla)

    
    

    
  
I created a Context Search (https://addons.mozilla.org/en-US/firefox/addon/contextual-search/) extension to replace Ben Basson's excellent extension which was no longer available with the implementation of the new Web extension API. I've been receiving very bad reviews b/c I don't offer users the possibility to import the search engines they already have in Firefox to Context Search. I'm not able to do so b/c Mozilla doesn't give me access to the search query string url. I'm not handling POST requests b/c I don't know how this can be done, as each search engine has different data I believe.


Anyway, to summarize: Context Search would be greatly improved if we had access to the search query string url of search engines in Firefox through the Search API.



    
    

    
  
You shouldn't need the query string because you can just execute .search using the engine.



    
    

    
  
(In reply to Olivier de Broqueville from comment #143)




I I'm not able to do so b/c Mozilla doesn't give me access to the search query string url.




As long as you required search, <all_urls>, webRequest, and, webRequestBlocking permissions, you can trigger a search, block the request, and know how the URL / Post body works. (finally remember to cancel it.) I know its an ugly solution. But it should work. (I had not tested this.)


(In reply to Mike Kaply [:mkaply] from comment #144)




You shouldn't need the query string because you can just execute .search using the engine.




    

  1. Such limitation is meaningless. As there would always be a walk around. It only making developers requiring more unnecessary permissions to make it work.
    2. 

  2. It could be useful. Olivier de Broqueville had provided a use case where they want import this settings to their extension. So user may maintain two different searching list.
    3. 




    
    

    
  
I'm not the only add-on dev needing this. The following 2 extensions adopt a very un-elegant way (and not so secure I guess) to import Firefox search engines by accessing the mozlz4 file from a user's Firefox profile.



In the Options page of our extensions, we list search engines along with the search query string url that can be customised by the user. This gives the user the most flexibility to meet his or her needs.


Not being able to import search query string urls would force add-on devs to implement 2 different ways of handling search engines, which in my view is a mess and would be confusing to the user. A bit of effort to understand devs views would be nice! It shouldn't be asking for so much to make a query url property available in addition to the existing properties (name, isDefault, alias and FavIconUrl) when the HTTP request method is GET.



    
    

    
  
Please open a new bug for your request.



    
    

    
  
(In reply to Mike Kaply [:mkaply] from comment #147)




Please open a new bug for your request.




I have opened bug 1643772


Thank you.



    
    

    
  
Perhaps now is a good time to just add this very useful code into Firefox itself once and for all. Olivier, I hope you consider this avenue. I used to use Context Search a LOT before Mozilla broke it.



    
  
Flags: needinfo?(olivier.debroqueville)

    
    

    
  
Clearing NI. This bug is closed.


Flags: needinfo?(olivier.debroqueville)
Flags: needinfo?(bugzilla)

    
    

    
  
I have created a new bug to address what was left out in this bug.


https://bugzilla.mozilla.org/show_bug.cgi?id=1700129


Duplicate of this bug: 1804357

    
  
No longer duplicate of this bug: 1804357

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: