Closed Bug 1475842 Opened Last year Closed Last year

browser.search.search: make name parameter optional

Categories

(WebExtensions :: General, enhancement, P3)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: kernp25, Assigned: robwu)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
What do you think?
Flags: needinfo?(mozilla)
Severity: normal → enhancement
Good idea, I wish it had been suggested before checkin :)
Assignee: nobody → mozilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(mozilla)
I don't like APIs that make the first parameter optional personally.
(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) {
> I think, the ordering of the parameters must change.

Yep, that was my plan.
I don't know, if it does matter, but can you also write the parameter as "engineName" and not "name"?
Can you also change the ordering here (name first)?     
{
 "type": "integer",
 "name": "tabId",
 "optional": true
}
Component: Untriaged → General
Priority: -- → P3
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.
Blocks: 1352598
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
Or what do you think about this:

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

?
Flags: needinfo?(mozilla)
Comment on attachment 9001255 [details]
Bug 1475842 - Change parameter format in browser.search API

Luca Greco [:rpl] has approved the revision.
Attachment #9001255 - Flags: review+
Attachment #9001255 - Attachment description: Bug 1475842 - Extra tests for browser.search + cleanup → Bug 1475842 - Change parameter format in browser.search API
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/6577561a493b
Change parameter format in browser.search API r=rpl
Flags: needinfo?(mozilla)
https://hg.mozilla.org/mozilla-central/rev/6577561a493b
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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!
Flags: needinfo?(mozilla)
Covered by automated tests.
Flags: needinfo?(mozilla) → qe-verify-
Assignee: mozilla → rob
Attachment #8993144 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.