1475842 - browser.search.search: make name parameter optional

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[kernp25]

If the name parameter is not specified, it will use the default search engine.

Then a WebExtension can do:
> browser.search.search("I am looking for this");

instead of
> browser.search.search("Google", "I am looking for this");

Most people only use one (default) search engine.

Comment 1

[kernp25]

What do you think?

Comment 2

[Mike Kaply [:mkaply]]

Good idea, I wish it had been suggested before checkin :)

Comment 3

[Tom S. (please needinfo tschuster)]

I don't like APIs that make the first parameter optional personally.

Comment 4

[kernp25]

(In reply to Tom Schuster [:evilpie] from comment #3)
> I don't like APIs that make the first parameter optional personally.

I think, the ordering of the parameters must change.

> async search(name, searchTerms, tabId) {

will be come

> async search(searchTerms, name, tabId) {

Comment 5

[Mike Kaply [:mkaply]]

> I think, the ordering of the parameters must change.

Yep, that was my plan.

Comment 6

[Mike Kaply [:mkaply]]

Attachment: Bug 1475842 - Allow WebExtension search with default engine.

Comment 7

[kernp25]

I don't know, if it does matter, but can you also write the parameter as "engineName" and not "name"?

Comment 8

[kernp25]

Can you also change the ordering here (name first)?     
{
 "type": "integer",
 "name": "tabId",
 "optional": true
}

Comment 9

[Rob Wu [:robwu]]

Since the API has landed recently, is not documented yet and already shows an issue with optional parameters (here), can you change the API to take an object instead of positional parameters?

Most extension APIs take objects, which makes it easier to extend the API later *.
So change from

browser.search.search(name, searchTerms, tabId)

to

browser.search.search({
  name: "optional string",
  searchTerms: "required string",
  tabId: optional integer, defaulting to a new tab,
});


Examples of extensions that I can think of is to pass a cookieStoreId, so that extensions can easily start a search in a specific container tab.

Comment 10

[Rob Wu [:robwu]]

Attachment: Bug 1475842 - Change parameter format in browser.search API

API implementation changes:
- Rename "engineName" to "engine" in browser.search.search
- Simplify gBrowser getter. None of the "!gBrowser" conditionals are
  necessary, as demonstrated by the new options page+sidebar tests
  (which passed even after  removing the `if (!gBrowser ...)` blocks).
  Use windowTracker.topWindow for consistency with our other code.

Test changes:
- Remove unneeded name / "TEST_ID" in tests
- New test: Using API from options page.
- New test: Using API from sidebar.
- New test: Using API without explicit "engine" parameter.

Depends on D2230

Comment 11

[kernp25]

Or what do you think about this:

browser.search.search(searchTerms, {
  engineName: "optional string",
  tabId: "optional integer, defaulting to a new tab",
});

?

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 9001255 [details]
Bug 1475842 - Change parameter format in browser.search API

Luca Greco [:rpl] has approved the revision.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6577561a493b
Change parameter format in browser.search API r=rpl

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/6577561a493b

Comment 15

[CosminB]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!

Comment 16

[Rob Wu [:robwu]]

Covered by automated tests.