Closed Bug 1344319 Opened 7 years ago Closed 7 years ago

Create Activity Stream browser extension

Categories

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

enhancement
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: ursula, Assigned: k88hudson)

References

Details

Attachments

(1 file)

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 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 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 on attachment 8844971 [details]
Bug 1344319 - Create Activity Stream browser extension.

https://reviewboard.mozilla.org/r/118220/#review120562
Attachment #8844971 - Flags: review?(rhelmer) → review+
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/97f9909c7851
Create Activity Stream browser extension. r=rhelmer
https://hg.mozilla.org/mozilla-central/rev/97f9909c7851
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Blocks: 1349288
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.