Cleanup search API (camelCase, promise)

RESOLVED FIXED in Firefox 63

Status

enhancement
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: mkaply, Assigned: mkaply)

Tracking

unspecified
mozilla63
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

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

Updated

11 months ago
Depends on: 1352598
Comment hidden (mozreview-request)

Comment 2

11 months ago
> 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
Assignee

Comment 4

11 months ago
> 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.
Assignee

Updated

11 months ago
Attachment #8991737 - Flags: review?(mixedpuppy)
Comment hidden (mozreview-request)

Comment 6

11 months ago
mozreview-review
Comment on attachment 8991737 [details]
Bug 1475347 - Cleanup search API - use camelCase and promise.

https://reviewboard.mozilla.org/r/256670/#review263770
Attachment #8991737 - Flags: review?(aswan) → review+

Comment 7

11 months ago
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/91cd5bee61ef
Cleanup search API - use camelCase and promise. r=aswan

Comment 8

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/91cd5bee61ef
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Comment 9

10 months ago
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.)
Assignee

Comment 10

10 months ago
> 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

10 months ago
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

10 months ago
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

10 months ago
(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...
Assignee

Comment 14

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

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

Updated

10 months ago
Blocks: 1352598
No longer depends on: 1352598

Comment 15

10 months ago
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)
Assignee

Updated

10 months ago
Flags: needinfo?(mozilla) → qe-verify-
You need to log in before you can comment on or make changes to this bug.