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

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: k88hudson, Assigned: k88hudson)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Summary: Add top sites and search UI/feeds to activity-stream system add-on → Add top sites and search to activity-stream system add-on
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → khudson
(Assignee)

Comment 2

2 years ago
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

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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
(Assignee)

Comment 5

2 years ago
mozreview-review-reply
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
(Assignee)

Comment 6

2 years ago
mozreview-review-reply
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

2 years ago
(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 hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
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.
Attachment #8861506 - Flags: review?(edilee) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 10

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

Updated

2 years ago
Keywords: checkin-needed

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a3b4b98b525b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

2 years ago
Blocks: 1363085

Updated

2 years ago
Depends on: 1350411
You need to log in before you can comment on or make changes to this bug.