Create Activity Stream browser extension

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Activity Streams: General
RESOLVED FIXED
2 months ago
2 months 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

2 months 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

2 months 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

2 months 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

2 months 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

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

Comment 10

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/97f9909c7851
Status: NEW → RESOLVED
Last Resolved: 2 months 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.