Closed Bug 1343945 Opened 3 years ago Closed 3 years ago

Fix eslint errors in browser/base/content/browser-social.js

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: sfoster, Assigned: sfoster)

References

Details

Attachments

(1 file)

browser/base/content/browser-social.js is in the global .estlintignore file, and needs a little tidy-up to remove the exclusion.
Not sure of the component for this. 

Linting without the no-undef rule is straight-forward. To pass with no-undef I'll need some pointers on how to handle the many globals it uses?
Assignee: nobody → sfoster
Flags: needinfo?(jaws)
Blocks: 1342459
https://developer.mozilla.org/en-US/docs/ESLint#no-undef should cover most of this. Over IRC Standard8 said:
> sfoster: at a glance, I think you could add /* eslint-env mozilla/browser-window */ to the top of the file and be most of the way there (today’s tree you’ll need)
Flags: needinfo?(jaws)
Attachment #8843104 - Flags: review?(jaws)
Comment on attachment 8843104 [details]
Bug 1343945 - Fix lint errors from browser-social.js, remove from ignore list

https://reviewboard.mozilla.org/r/116864/#review118678

::: commit-message-c54c6:3
(Diff revision 2)
> +Bug 1343945 - Fix lint errors, remove from ignore list
> +* Added no-undef rule locally as we're not ready to turn on for the whole directory
> +* Adding "use strict"; inside the iife causes an exception - I'm not sure how to see it?

So the "use strict" exception is because in a "use strict" anonymous function, `this` is undefined. See http://stackoverflow.com/questions/9822561/why-is-this-in-an-anonymous-function-undefined-when-using-strict

The simplest way to fix this is by using .call(this) at the bottom of the file, like so:

> -})();

> +}).call(this);

As to "how to see it", I don't know of a good way. I commented out the body of SocialUI and SocialShare and still saw the error, so that left me with only about 10 lines of code that the issue could have been in.

::: browser/base/content/browser-social.js:403
(Diff revision 2)
>      // graphData is an optional param that either defines the full set of data
>      // to be shared, or partial data about the current page. It is set by a call
>      // in mozSocial API, or via nsContentMenu calls. If it is present, it MUST
>      // define at least url. If it is undefined, we're sharing the current url in
>      // the browser tab.
> -    let pageData = graphData ? graphData : this.currentShare;
> +    let sharedPageData = graphData ? graphData : this.currentShare;

Might as well change this line to:
let sharedPageData = graphData || this.currentShare;
Attachment #8843104 - Flags: review?(jaws) → review+
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/15f2040d22ff
Fix lint errors from browser-social.js, remove from ignore list r=jaws
https://hg.mozilla.org/mozilla-central/rev/15f2040d22ff
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.