1359481 - Add top sites and search to activity-stream system add-on

Status: RESOLVED FIXED

(Firefox Graveyard :: Activity Streams: General, enhancement)

Description

[Kate Hudson :k88hudson]

This patch will include exported changes from github which add UI and supporting data feeds to the Activity Stream system add-on related to top sites and search.

Comment 1

[Kate Hudson :k88hudson]

Attachment: Bug 1359481 - Add top sites and search to activity-stream system add-on

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

Comment 2

[Kate Hudson :k88hudson]

This patch includes code exported from github at the following commit: 20e4b652eba5ec17295a40e0bd52df0d19810129 with a few additional changes:

- The line BROWSER_CHROME_MANIFESTS += ['test/functional/mochitest/browser.ini'] was not included in jar.mn
- The bundled js path was added to the general Firefox .eslintignore
- TelemetrySender.jsm was removed since it is not yet being used yet, and would therefore cause test failures

Comment 3

[Ed Lee :Mardak]

Comment on attachment 8861506 [details]
Bug 1359481 - Add top sites and search to activity-stream system add-on

https://reviewboard.mozilla.org/r/133480/#review136370

> - The line BROWSER_CHROME_MANIFESTS += ['test/functional/mochitest/browser.ini'] was not included in jar.mn
I'm assuming you meant `moz.build`? And that change didn't seem to make it into this patch?

::: .eslintignore:78
(Diff revision 1)
>  browser/extensions/pocket/content/panels/js/vendor/**
>  browser/locales/**
>  # vendor library files in activity-stream
>  browser/extensions/activity-stream/vendor/**
> +# bundled js in activity-stream
> +browser/extensions/activity-stream/data/content/activity-stream.bundle.js

We'll need to file an issue to remove the `eslintignore.diff` patching. Just to be clear, the patch will probably complain more forcefully with this conflict as it's actually different from how it's been patching (where an exact patch would whine about the patch already being applied). https://github.com/mozilla/activity-stream/blob/master/mozilla-central-patches/eslintignore.diff

::: browser/extensions/activity-stream/lib/ActivityStreamMessageChannel.jsm:35
(Diff revision 1)
>    pageURL: ABOUT_NEW_TAB_URL,
>    outgoingMessageName: "ActivityStream:MainToContent",
>    incomingMessageName: "ActivityStream:ContentToMain"
>  };
>  
> -this.ActivityStreamMessageChannel = class ActivityStreamMessageChannel {
> +class ActivityStreamMessageChannel {

Sorry, I missed this in the github backporting PR https://github.com/mozilla/activity-stream/pull/2448/files#diff-a12f94b26c6ac483de6e2a17547ced6bR201

I think we should keep what's already in m-c to match the `this.Foo = class Foo` pattern that other files have.

Comment 4

[Ursula Sarracini (:ursula)]

Comment on attachment 8861506 [details]
Bug 1359481 - Add top sites and search to activity-stream system add-on

https://reviewboard.mozilla.org/r/133480/#review136382

::: browser/extensions/activity-stream/lib/TopSitesFeed.jsm:34
(Diff revision 1)
> +    let screenshot = await PreviewProvider.getThumbnail(url);
> +    const action = {type: at.SCREENSHOT_UPDATED, data: {url, screenshot}};
> +    this.store.dispatch(ac.BroadcastToContent(action));
> +  }
> +  async getLinksWithDefaults(action) {
> +    let links = await PlacesProvider.links.getLinks();

Just a heads up - this will break as soon as Bug 1345122 lands because it gets rid of the PlacesProvider module altogether. Might be worth wrapping this in a try catch to avoid breakage for now. And we'll have to file a follow up bug to change this to be ```let links = await NewTabUtils.activityStreamLinks.getTopSites()``` when it lands

Comment 5

[Kate Hudson :k88hudson]

Comment on attachment 8861506 [details]
Bug 1359481 - Add top sites and search to activity-stream system add-on

https://reviewboard.mozilla.org/r/133480/#review136370

> We'll need to file an issue to remove the `eslintignore.diff` patching. Just to be clear, the patch will probably complain more forcefully with this conflict as it's actually different from how it's been patching (where an exact patch would whine about the patch already being applied). https://github.com/mozilla/activity-stream/blob/master/mozilla-central-patches/eslintignore.diff

Ah good point -- I will actually update this patch to match that one

Comment 6

[Kate Hudson :k88hudson]

Comment on attachment 8861506 [details]
Bug 1359481 - Add top sites and search to activity-stream system add-on

https://reviewboard.mozilla.org/r/133480/#review136370

Oops sorry, yeah -- I meant moz.build. I just mean that there's no change here (i.e. it's not added) despite it existing on master in github

Comment 7

[Ed Lee :Mardak]

(In reply to Ursula Sarracini (:ursula) from comment #4)
> > +    let links = await PlacesProvider.links.getLinks();
> Just a heads up - this will break as soon as Bug 1345122 lands because it
> gets rid of the PlacesProvider module altogether.
As noted on IRC, we'll just leave this as is and fix it in activity-stream repository to export soon after bug 1345122 lands. Tests aren't running yet anyway, so it won't require changes to the other bug.

I filed https://github.com/mozilla/activity-stream/issues/2488 for removing eslintignore.diff

and filed https://github.com/mozilla/activity-stream/issues/2489 about moz.build cleanup

Comment 8

[Kate Hudson :k88hudson]

Comment on attachment 8861506 [details]
Bug 1359481 - Add top sites and search to activity-stream system add-on

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

Comment 9

[Ed Lee :Mardak]

Comment on attachment 8861506 [details]
Bug 1359481 - Add top sites and search to activity-stream system add-on

https://reviewboard.mozilla.org/r/133480/#review136426

Looks good. Also just logging that we'll need to land the ActivityStreamMessageChannel change in activity-stream repository.

Comment 10

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3b4b98b525b75f3276f9e226781ee4bcc1a33c0
Bug 1359481 - Add top sites and search to activity-stream system add-on. r=Mardak

Comment 11

[Carsten Book [:Tomcat]]

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