Closed Bug 1444579 Opened 2 years ago Closed 2 years ago

Places provider no longer accessible from topSites API

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox58 unaffected, firefox59 unaffected, firefox60+ fixed, firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 + fixed
firefox61 --- fixed

People

(Reporter: darktrojan, Assigned: mixedpuppy)

References

Details

Attachments

(2 files)

Bug 1433133 removed the `NewTabUtils.placesProvider` reference to the provider, now `browser.topSites.get({providers: ['places']})` returns nothing.
Not entirely sure what providers are supposed to be available, but NewTabUtils.activityStreamLinks.getTopSites() resolves an array of link objects.

Gijs suggested in bug 1370930 comment 3 that the api should probably give what activity stream would use.
See Also: → 1370930
The API exists to allow extension writers to choose which providers they want links from. In my case, I want links from the places provider, but NOT from the activity stream provider.
(In reply to Geoff Lankow (:darktrojan) from comment #0)
> Bug 1433133 removed the `NewTabUtils.placesProvider` reference to the
> provider, now `browser.topSites.get({providers: ['places']})` returns
> nothing.

If this is expected to work then there should really be tests.

I also don't naively see why the removal of the public exposure of the provider would break this kind of query. IIRC the individual links objects have a provider identification, and shouldn't require the provider to be publicly exposed on NewTabUtils.

I also think it's a terrible idea to tie the webextensions code so directly into the implementation of 'top sites' - this type of API is effectively forcing us to not ever make internal code changes. :-\
Flags: in-testsuite?
I also wonder how we ended up with the "places" code name being exposed to webextensions. What is this even supposed to mean here? Is it something else than history?
Blocks: 1298211
Bug 1361899 added the provider option, and the tests there seem a bit odd in that they require attaching a new provider to the NewTabUtils object?

The top_sites.json schema seems to suggest "activityStream" is allowed, but that (nor "places") are tested in test_ext_topSites.js. I believe activityStreamProvider isn't actually registered as a provider.

https://hg.mozilla.org/mozilla-central/rev/3117287bcc9d#l2.18
Blocks: 1361899
Do we have any idea how many add-ons rely on this? Unless it's an insignificantly small number, I expect we'll need to track this for 60...
I think the fix should be to revert bug 1361899, i.e., remove the providers option. A topSites.get() based on NewTabUtils.activityStreamLinks.getTopSites would not include pinned sites.
Bob, who can help triage this and come to a resolution about what the desired approach is? I think a number of browser peers are surprised at the amount of implementation detail this API provides, and would like to move away from it (to the point they're suggesting reverting bug 1361899). Who makes decisions about this - is there still a webextensions triage group? When do they meet?
Flags: needinfo?(bob.silverberg)
(In reply to :Gijs from comment #8)
> Bob, who can help triage this and come to a resolution about what the
> desired approach is? I think a number of browser peers are surprised at the
> amount of implementation detail this API provides, and would like to move
> away from it (to the point they're suggesting reverting bug 1361899). Who
> makes decisions about this - is there still a webextensions triage group?
> When do they meet?

The old advisory group, to which I believe you are referring, and which you used to attend, doesn't really exist anymore. Mike Conca, to whom I'm forwarding the needinfo, would be the person to talk to about what APIs we need to provide, how they should look, and how much they're being used. I believe that Shane Caraveo is the one who implemented the topsites API, so he's probably a good person to talk to as well.
Flags: needinfo?(bob.silverberg) → needinfo?(mconca)
mixedpuppy, any thoughts on having `topSites.get()` ignore providers option as originally implemented but instead have it provide the frecency result instead of including pinned?
Flags: needinfo?(mixedpuppy)
The standard implementation also filters the result (one link per domain), but I think an extension should be able to decide for itself which links to keep. That was the main aim when the providers implementation was added.
Some data for reference:

* The topSites() is currently used by a total of 39 extensions on AMO (unknown number of unlisted extensions).
* It is unknown if any of those extensions are using the optional 'providers' parameter
* MDN never documented the optional 'providers' parameter

From a product perspective, it would be better to preserve the original behavior of topSites() since it has been in the wild and used by some extensions (despite the lack of documentation on MDN) for over a year. That said, it could be deprecated if the engineering burden to restore and/or maintain the original functionality is too great.  Awaiting info from :mixedpuppy.
Flags: needinfo?(mconca)
I've made a patch to restore the old behaviour, for now. If we're going to change the API (and I think we should - I didn't like it even at the time) we should at least not break Firefox 60.
Comment on attachment 8959024 [details]
Bug 1444579 - Restore placesProvider as a member of NewTabUtils

https://reviewboard.mozilla.org/r/227894/#review233710


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/test/xpcshell/test_ext_topSites.js:47
(Diff revision 1)
>      let data = [{url: "http://example1.com/", title: "site#1", frecency: 7, lastVisitDate: now},
>                  {url: "http://example2.com/", title: "site#2", frecency: 6, lastVisitDate: now}];
>      done(data);
>    });
>  
> +  Assert.ok('activityStreamProvider' in NewTabUtils);

Error: Strings must use doublequote. [eslint: quotes]

::: toolkit/components/extensions/test/xpcshell/test_ext_topSites.js:48
(Diff revision 1)
>                  {url: "http://example2.com/", title: "site#2", frecency: 6, lastVisitDate: now}];
>      done(data);
>    });
>  
> +  Assert.ok('activityStreamProvider' in NewTabUtils);
> +  Assert.ok('placesProvider' in NewTabUtils);

Error: Strings must use doublequote. [eslint: quotes]

::: toolkit/components/extensions/test/xpcshell/test_ext_topSites.js:49
(Diff revision 1)
>      done(data);
>    });
>  
> +  Assert.ok('activityStreamProvider' in NewTabUtils);
> +  Assert.ok('placesProvider' in NewTabUtils);
> +  Assert.ok(!('fakeProvider' in NewTabUtils));

Error: Strings must use doublequote. [eslint: quotes]
Comment on attachment 8959024 [details]
Bug 1444579 - Restore placesProvider as a member of NewTabUtils

I should have assigned this to myself.  This isn't quite the change I want.
Flags: needinfo?(mixedpuppy)
Attachment #8959024 - Flags: review?(gijskruitbosch+bugs) → review-
Assignee: nobody → mixedpuppy
Comment on attachment 8959035 [details]
Bug 1444579 - fix use of places provider for topSites api,

https://reviewboard.mozilla.org/r/227908/#review233724

::: toolkit/components/extensions/ext-topSites.js:19
(Diff revision 1)
> -            NewTabUtils.links.populateCache(function() {
> +            NewTabUtils.links.populateCache(async () => {
>                let urls;
>  
> -              if (options && options.providers && options.providers.length > 0) {
> -                let urlLists = options.providers.map(function(p) {
> -                  let provider = NewTabUtils[`${p}Provider`];
> +              // The placesProvider is a superset of activityStream if sites are blocked, etc.,
> +              // there is no need to attempt a merge of multiple provider lists.
> +              if (options && options.providers && options.providers.includes("places")) {

I'd rather we just make options `"default": {}` in the schema.

::: toolkit/components/extensions/test/xpcshell/test_ext_topSites.js:59
(Diff revision 1)
> -                   {url: "http://example0.com/", title: "site#0"},
> -                   {url: "http://example1.com/", title: "site#1"},
> -                   {url: "http://example2.com/", title: "site#2"},
> +    let result = extension.awaitMessage("sites");
> +    extension.sendMessage(providers);
> +    return result;

No need for the dance. Just:

    extension.sendMessage("providers");
    return extension.awaitMessage("sites");

No need for the function to be async, either.
Attachment #8959035 - Flags: review?(kmaglione+bmo) → review+
Blocks: 1445836
Comment on attachment 8959035 [details]
Bug 1444579 - fix use of places provider for topSites api,

https://reviewboard.mozilla.org/r/227908/#review233856

I still think the magical API strings here aren't a good fit for a webextensions API if we want to align with other browsers. If there are specific requirements about not wanting pinned / bookmarked sites or something, perhaps expressing those as options would make more sense than specifying you want a 'places' provider (because it's really not clear what that "ought" to mean). Anyway, in terms of just fixing this regression and ensuring we can't break this again without touching the test, rs=me
Attachment #8959035 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #21)
> Anyway, in terms of just fixing this regression and
> ensuring we can't break this again without touching the test, rs=me

IMO That is the only point of this bug.  I opened another for dealing with whatever is next.
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f89982b63911
fix use of places provider for topSites api, r=Gijs,kmag
Flags: qe-verify-
Flags: in-testsuite?
Flags: in-testsuite+
Priority: -- → P1
Component: WebExtensions: Untriaged → WebExtensions: General
https://hg.mozilla.org/mozilla-central/rev/f89982b63911
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8959035 [details]
Bug 1444579 - fix use of places provider for topSites api,

Approval Request Comment
[Feature/Bug causing the regression]: 1433133 
[User impact if declined]: topSites api was broken by changes in 1433133
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: improved tests, small changes.
[String changes made/needed]: none
Attachment #8959035 - Flags: approval-mozilla-beta?
Comment on attachment 8959035 [details]
Bug 1444579 - fix use of places provider for topSites api,

fix for topSites API, beta60+
Attachment #8959035 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
No longer blocks: 1445836
Depends on: 1445836
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.