1444579 - Places provider no longer accessible from topSites API

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

[Geoff Lankow (:darktrojan)]

Bug 1433133 removed the `NewTabUtils.placesProvider` reference to the provider, now `browser.topSites.get({providers: ['places']})` returns nothing.

Comment 1

[Ed Lee :Mardak]

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.

Comment 2

[Geoff Lankow (:darktrojan)]

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.

Comment 3

[:Gijs (he/him)]

(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. :-\

Comment 4

[Dão Gottwald [:dao]]

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?

Comment 5

[Ed Lee :Mardak]

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

Comment 6

[:Gijs (he/him)]

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...

Comment 7

[Ed Lee :Mardak]

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.

Comment 8

[:Gijs (he/him)]

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?

Comment 9

[Bob Silverberg [:bsilverberg]]

(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.

Comment 10

[Ed Lee :Mardak]

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?

Comment 11

[Geoff Lankow (:darktrojan)]

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.

Comment 12

[Mike Conca [:mconca]]

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.

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1444579 - Restore placesProvider as a member of NewTabUtils

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

Comment 14

[Geoff Lankow (:darktrojan)]

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 15

[Code Review Bot [:reviewbot]]

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 16

[Geoff Lankow (:darktrojan)]

Comment on attachment 8959024 [details]
Bug 1444579 - Restore placesProvider as a member of NewTabUtils

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

Comment 17

[Shane Caraveo (:mixedpuppy)]

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.

Comment 18

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1444579 - fix use of places provider for topSites api,

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

Comment 19

[Kris Maglione [:kmag]]

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.

Comment 20

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8959035 [details]
Bug 1444579 - fix use of places provider for topSites api,

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

Comment 21

[:Gijs (he/him)]

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

Comment 22

[Shane Caraveo (:mixedpuppy)]

(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.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f89982b63911
fix use of places provider for topSites api, r=Gijs,kmag

Comment 24

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/f89982b63911

Comment 25

[Shane Caraveo (:mixedpuppy)]

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

Comment 26

[Julien Cristau [:jcristau]]

Comment on attachment 8959035 [details]
Bug 1444579 - fix use of places provider for topSites api,

fix for topSites API, beta60+

Comment 27

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b9bc94d03e32