The default bug view has changed. See this FAQ.

Create Activity Stream browser extension

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Activity Streams: General
RESOLVED FIXED
25 days ago
18 days ago

People

(Reporter: ursula, Assigned: k88hudson)

Tracking

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

25 days ago
The Activity Stream UI and other bits will live in an extension in browser/extensions/activity-stream. This ticket will land the bootstrap.js, the jar.mn, the moz.build, the install.rdf.in, and a placeholder html page which will be hit when the pref browser.newtabpage.activity-stream.enabled is true.
Comment hidden (mozreview-request)

Comment 2

20 days ago
mozreview-review
Comment on attachment 8844971 [details]
Bug 1344319 - Create Activity Stream browser extension.

https://reviewboard.mozilla.org/r/118220/#review120198

This looks great! Just a few minor issues - the only one that you absolutely have to fix before landing is the eslint rule, which isn't at all your fault.

::: browser/extensions/activity-stream/bootstrap.js:1
(Diff revision 1)
> +/* globals Components, XPCOMUtils, Preferences, ActivityStream */

Please include a license header.

::: browser/extensions/activity-stream/bootstrap.js:54
(Diff revision 1)
> +}
> +
> +/**
> + * onPrefChanged - handler for changes to ACTIVITY_STREAM_ENABLED_PREF
> + *
> + * @param  {bool} isEnabled Determines whether

Finish sentence :)

::: browser/extensions/activity-stream/lib/ActivityStream.jsm:1
(Diff revision 1)
> +"use strict";

Please include a license header here too.

::: browser/extensions/activity-stream/lib/ActivityStream.jsm:25
(Diff revision 1)
> +  uninit() {
> +    this.initialized = false;
> +  }
> +}
> +
> +this.EXPORTED_SYMBOLS = ["ActivityStream"];

So this is not at all your fault, but this JSM is not going to pass eslint when you try to land it.

Would you mind including as part of your patch this change to the `./browser/extensions/.eslintrc.js` rule:

```
module.exports = { // eslint-disable-line no-undef
  "rules": {
    "no-unused-vars": ["error", {
      "vars": "local",
      "varsIgnorePattern": "^Cc|Ci|Cu|Cr|EXPORTED_SYMBOLS",
      "args": "none",
    }],
  }
};
```

This is what we use elsewhere in the tree (the new part is the `"vars": "local"` bit).

You can confirm it's working with `./mach eslint browser/extensions/activity-stream`
Attachment #8844971 - Flags: review?(rhelmer)
(In reply to Robert Helmer [:rhelmer] from comment #2)
> Comment on attachment 8844971 [details]
> This looks great! Just a few minor issues - the only one that you absolutely
> have to fix before landing is the eslint rule, which isn't at all your fault.

Well by "absolutely have to" I mean it will literally get backed out - please do fix the other things too :)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

19 days ago
mozreview-review-reply
Comment on attachment 8844971 [details]
Bug 1344319 - Create Activity Stream browser extension.

https://reviewboard.mozilla.org/r/118220/#review120198

> So this is not at all your fault, but this JSM is not going to pass eslint when you try to land it.
> 
> Would you mind including as part of your patch this change to the `./browser/extensions/.eslintrc.js` rule:
> 
> ```
> module.exports = { // eslint-disable-line no-undef
>   "rules": {
>     "no-unused-vars": ["error", {
>       "vars": "local",
>       "varsIgnorePattern": "^Cc|Ci|Cu|Cr|EXPORTED_SYMBOLS",
>       "args": "none",
>     }],
>   }
> };
> ```
> 
> This is what we use elsewhere in the tree (the new part is the `"vars": "local"` bit).
> 
> You can confirm it's working with `./mach eslint browser/extensions/activity-stream`

Weird... I don't see an .eslintrc.js in browser/extensions/, and I get 0 problems (0 errors, 0 warnings) when I run the eslint command in the activity-stream directory, am I looking in the wrong place maybe?
(In reply to Kate Hudson :k88hudson from comment #5)
> Comment on attachment 8844971 [details]
> Bug 1344319 - Create Activity Stream browser extension.
> 
> https://reviewboard.mozilla.org/r/118220/#review120198
> 
> > So this is not at all your fault, but this JSM is not going to pass eslint when you try to land it.
> > 
> > Would you mind including as part of your patch this change to the `./browser/extensions/.eslintrc.js` rule:
> > 
> > ```
> > module.exports = { // eslint-disable-line no-undef
> >   "rules": {
> >     "no-unused-vars": ["error", {
> >       "vars": "local",
> >       "varsIgnorePattern": "^Cc|Ci|Cu|Cr|EXPORTED_SYMBOLS",
> >       "args": "none",
> >     }],
> >   }
> > };
> > ```
> > 
> > This is what we use elsewhere in the tree (the new part is the `"vars": "local"` bit).
> > 
> > You can confirm it's working with `./mach eslint browser/extensions/activity-stream`
> 
> Weird... I don't see an .eslintrc.js in browser/extensions/, and I get 0
> problems (0 errors, 0 warnings) when I run the eslint command in the
> activity-stream directory, am I looking in the wrong place maybe?

Ugh apparently I have a version sitting there I haven't checked in - sorry for the false alarm.

Comment 7

19 days ago
mozreview-review
Comment on attachment 8844971 [details]
Bug 1344319 - Create Activity Stream browser extension.

https://reviewboard.mozilla.org/r/118220/#review120562
Attachment #8844971 - Flags: review?(rhelmer) → review+
Comment hidden (mozreview-request)

Comment 9

18 days ago
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/97f9909c7851
Create Activity Stream browser extension. r=rhelmer

Comment 10

18 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/97f9909c7851
Status: NEW → RESOLVED
Last Resolved: 18 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.