1475347 - Cleanup search API (camelCase, promise)

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement)

Description

[Mike Kaply [:mkaply]]

In bug 1352598 there were some requests to cleanup the API. They are probably right, so I'll be changing a few things.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Bug 1475347 - Cleanup search API - use camelCase and promise.

Review commit: https://reviewboard.mozilla.org/r/256670/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256670/

Comment 2

[kernp25]

> 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

Like with webRequest:
https://searchfox.org/mozilla-central/rev/46292b1212d2d61d7b5a7df184406774727085b8/toolkit/modules/addons/WebRequest.jsm#675

Comment 3

[kernp25]

But doesn't tabTracker.getTab[1] already throw?

[1] https://searchfox.org/mozilla-central/rev/46292b1212d2d61d7b5a7df184406774727085b8/browser/components/extensions/parent/ext-browser.js#371

Comment 4

[Mike Kaply [:mkaply]]

> But doesn't tabTracker.getTab[1] already throw?

Yep. I'll remove that.

> Like with webRequest:

I meant to do that and forgot. I'll fix.

Comment 5

[Mike Kaply [:mkaply]]

Comment on attachment 8991737 [details]
Bug 1475347 - Cleanup search API - use camelCase and promise.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256670/diff/1-2/

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8991737 [details]
Bug 1475347 - Cleanup search API - use camelCase and promise.

https://reviewboard.mozilla.org/r/256670/#review263770

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91cd5bee61ef
Cleanup search API - use camelCase and promise. r=aswan

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/91cd5bee61ef

Comment 9

[Worcester12345]

from the other bug:

(In reply to Mike Kaply [:mkaply] from comment #120)
> > 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 consistent 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 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.

(In reply to Tom Schuster [:evilpie] from comment #122)
> (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.

Should "chrome_settings_override" be trued up with using camel case, so EVERYTHING is consistent to one another? (Not a programmer, but there seems to be an inconsistency here.)

Comment 10

[Mike Kaply [:mkaply]]

> Should "chrome_settings_override" be trued up with using camel case, so EVERYTHING is consistent to one another? (Not a programmer, but there seems to be an inconsistency here.)

No, we should match Chrome in this regard.

Comment 11

[Tian, Sheng]

Should I add "search" to "permissions" in manifest.json?

Related MDN page told me "To use this API you need to have the "search" permission ( https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/search )." But "search" is not listed on permission page ( https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/permissions ). Testing on 63.0a1 (2018-07-16) (Windows 64-bit) shows me: Adding "search" will cause a warning, and API invoking would success if the permission is not listed in manifest.

Maybe the description of search API on MDN should be updated?

Comment 12

[Tian, Sheng]

search.search may only be called from a user input handler

That means following codes may not be functional (even in user input handler context) due to the await on tabs.create

    const newTab = await browser.tabs.create({ active: false, url: 'about:blank' });
    browser.search.search(searchProvider, searchTerms, newTab.id);

Is this the intended behavior? (And, should I submit it as another separated bug?)

Comment 13

[Tian, Sheng]

(In reply to 田生 from comment #12)
> search.search may only be called from a user input handler
> 
> That means following codes may not be functional (even in user input handler
> context) due to the await on tabs.create
> 
>     const newTab = await browser.tabs.create({ active: false, url:
> 'about:blank' });
>     browser.search.search(searchProvider, searchTerms, newTab.id);
> 
> Is this the intended behavior? (And, should I submit it as another separated
> bug?)

OK, Find it. This is a duplicate of bug 1398833...

Comment 14

[Mike Kaply [:mkaply]]

> Should I add "search" to "permissions" in manifest.json?

I didn't end up adding a permission for this.

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!