1084243 - Fix remaining hint problems in shared/js

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Shared, defect)

Comment 1

[Sudheesh Singanamalla (:ShellHacker)]

Fixed all files that are not in shared/js/media or shared/js/fb

Having trouble with shared/js/keyboard_helper.js which keeps complaining for possible strict violation

Comment 2

[Kevin Grandon :kgrandon]

Do you want to upload the in-progress pull request first? Maybe we can land that and handle keyboard_helper.js by itself at the end? Seems like we need to find a solution, and it may be the same as bug 1084242?

Comment 3

[Sudheesh Singanamalla (:ShellHacker)]

Yeah lets land the in progress PR first. Yup, you're right, looks like the same issue as that of bug 1084242

Comment 4

[Kevin Grandon :kgrandon]

Hey Sudheesh - any chance you could upload the pull request with most of the files passing JSHint? It would help. Thank you!

Comment 5

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/25714

I am extremely sorry for the delay Kevin. I have attached the PR containing the patch to this.

Comment 6

[Kevin Grandon :kgrandon]

Comment on attachment 8515413 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/25714

Thank you for the patch. I think we're getting close here, but there are a few things we need to investigate before we can land this. Please address and re-flag me when ready. Thanks!

Comment 7

[Sudheesh Singanamalla (:ShellHacker)]

Updated the patch. Hopefully this should fix it. Thanks !

Comment 8

[Kevin Grandon :kgrandon]

Looking good, but we're still seeing several linter failures here: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=ec83d5536a5e

Can you look into it? Thanks!

Comment 9

[Sudheesh Singanamalla (:ShellHacker)]

The only issue I see is the Strict violation. I am unable to figure out what to exactly do. I am sorry for the delay in replying here, Had my routine mid semester examinations this whole week

Comment 10

[Kevin Grandon :kgrandon]

Hey Sudheesh - I think generally this is because we try to call a function with .call() or .apply(). To let jshint know this is ok, I think we can annotate these functions with a jshint tag. Can you try placing the following tag inside each function that needs this, and let me know if it works?

/* jshint validthis: true */

Comment 11

[Sudheesh Singanamalla (:ShellHacker)]

In fb_data_reader.js, the file when modified to be

-  var fb = this.fb || {};
-  this.fb = fb;

(function() {
+  var fb = this.fb || {};
+  this.fb = fb;
  

Works perfectly, In the rest of the files, the this.fb is at a global scope and jshint validthis:true can only be used at a local scope. On using it, it provides the following error.

Option 'validthis' can't be used in a global scope.

Comment 12

[Sudheesh Singanamalla (:ShellHacker)]

That is the same problem with the other files since most of them have `this` in global scope.

Comment 13

[Kevin Grandon :kgrandon]

(In reply to Sudheesh Singanamalla (:ShellHacker) [Exams Nov 18 - Dec 5] from comment #12)
> That is the same problem with the other files since most of them have `this`
> in global scope.

Hmm, this is going to need some investigation. I *think* what we want to do is use the `self` variable, instead of `this`, but I need to do some investigation first.

We also have bug 1084642 filed which is similar - to devise a simple pattern for all of the scripts in shared/ to use.

Comment 14

[Sudheesh Singanamalla (:ShellHacker)]

Any progress with this Kevin ?

Comment 15

[Kevin Grandon :kgrandon]

Not too much movement on this yet, sorry. We may want to get the files that don't have this problem landed while we investigate a solution. (So everything except fb_data_reader.js maybe?)

Recently I've been thinking about having each file export an ES6 module, but this is going to take a while to implement.

Comment 16

[Kevin Grandon :kgrandon]

We should try using `self` or perhaps the jshint `worker` option here for the time being.

Comment 17

[Kevin Grandon :kgrandon]

Sudheesh - do you have interest in finishing this up?

I think using the `self` var from a worker context would be our best path forward here. Let me know what you think. Thanks!

Comment 18

[Sudheesh Singanamalla (:ShellHacker)]

Kevin, I am slightly busy right now, I would fix this if its left but that'd take time, in case this is a blocker, it'd be great if you could go ahead with this.

Comment 19

[Kevin Grandon :kgrandon]

Thank you very much for the info and contribution. I'll see if we can take what you have and rebase it against master to get it landed.

Comment 20

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:bug_1084243_shared_jshint > mozilla-b2g:master

Comment 21

[Kevin Grandon :kgrandon]

Finally in master: https://github.com/mozilla-b2g/gaia/commit/f45e662fb738dc0c699a25a8e7e742eb8d196c3d