1005331 - JSHint fixes for shared/js/media/

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Kevin Grandon :kgrandon]

We would like to have these files in this folder passing JSHint. Anyone is free to take this bug if it is unassigned.

Comment 1

[Mathieu Rhéaume]

Attachment: Github PR For Bug 1005331 with No errors in JsHint for shared/media

This is modification in the shared media js files so they all validate in JsHint.

The following changes were made :
1. Added /* global */ for variables that were from other scripts...
2. Added {} on one line if statement.
3. Added /* exported */ for functions that were declared but not used.
4. Added break; on switch statement that didn't have break;

Thanks for looking into it.

Comment 2

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8493343 [details] [review]
Github PR For Bug 1005331 with No errors in JsHint for shared/media

I think this was meant for djf.

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8493343 [details] [review]
Github PR For Bug 1005331 with No errors in JsHint for shared/media

I left a few things on github. Mostly concerned about the change of the self var which might cause it to leak into the global scope. You should definitely address that before landing I think. Overall looks good though, so I'll leave an F+. Thanks for the patch!

Comment 4

[Mathieu Rhéaume]

About var = self.

I tried removing the var statement and it broke Gu tests. 

If I leave the var statement, JsHint triggers a Unused variable. 

Not quite sure why it's there.

(In reply to Kevin Grandon :kgrandon from comment #3)
> Comment on attachment 8493343 [details] [review]
> Github PR For Bug 1005331 with No errors in JsHint for shared/media
> 
> I left a few things on github. Mostly concerned about the change of the self
> var which might cause it to leak into the global scope. You should
> definitely address that before landing I think. Overall looks good though,
> so I'll leave an F+. Thanks for the patch!

Comment 5

[Mathieu Rhéaume]

I just updated the PR.

Meanwhile to make sure we don't break anything. I put back the var self = this line as it was before and added a 

// jsHint ignore:true

So the line will be ignored when validating the javascript file.

I also applied the various nits from :kgrandon feedback.

(In reply to Mathieu Rhéaume from comment #4)
> About var = self.
> 
> I tried removing the var statement and it broke Gu tests. 
> 
> If I leave the var statement, JsHint triggers a Unused variable. 
> 
> Not quite sure why it's there.
> 
> (In reply to Kevin Grandon :kgrandon from comment #3)
> > Comment on attachment 8493343 [details] [review]
> > Github PR For Bug 1005331 with No errors in JsHint for shared/media
> > 
> > I left a few things on github. Mostly concerned about the change of the self
> > var which might cause it to leak into the global scope. You should
> > definitely address that before landing I think. Overall looks good though,
> > so I'll leave an F+. Thanks for the patch!

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8493343 [details] [review]
Github PR For Bug 1005331 with No errors in JsHint for shared/media

Could you file another bug, leave the bug # in the code saying that we should fix that |var self = this;|?

I have no idea why we need that either.

Comment 7

[Mathieu Rhéaume]

Hi I added the bug at :

https://bugzilla.mozilla.org/show_bug.cgi?id=1072784

I'm adding a comment with the bug id and checking in.

Thanks again.

(In reply to Tim Guan-tin Chien [:timdream] (OOO) (MoCo-TPE) (please ni?) from comment #6)
> Comment on attachment 8493343 [details] [review]
> Github PR For Bug 1005331 with No errors in JsHint for shared/media
> 
> Could you file another bug, leave the bug # in the code saying that we
> should fix that |var self = this;|?
> 
> I have no idea why we need that either.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Patch is rebased but need to wait for Gaia-Try to turn green.

https://tbpl.mozilla.org/?rev=5335c25a82f585995587caff80e6d1382b44b308&tree=Gaia-Try

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Mathieu, you got a linter error. Could you fix that and ask for review again?

Comment 10

[Mathieu Rhéaume]

Hi,

This line is the same problem as the Var self = this. As They are used to exports vars but declared as local variables. I added a Ignore:Line we'll see how it goes in gaia-try.

(In reply to Tim Guan-tin Chien [:timdream] (OOO) (MoCo-TPE) (please ni?) from comment #9)
> Mathieu, you got a linter error. Could you fix that and ask for review again?

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8493343 [details] [review]
Github PR For Bug 1005331 with No errors in JsHint for shared/media

I am not entire sure Gip error is caused by this patch.

https://tbpl.mozilla.org/?rev=662d7d5096029a4bbe0940c49ef7fc4c472c90df&tree=Gaia-Try

It's probably safer to prove if it's not before landing.

Comment 12

[Mathieu Rhéaume]

Comment on attachment 8493343 [details] [review]
Github PR For Bug 1005331 with No errors in JsHint for shared/media

Hi Tim,

You were right, I did broke some test while rebasing with gaia master branch. I fixed them up and reran gaia-try 2 times. Seems ok except Gij that failed on a intermitent bug one time and the other time it timedout. Not sure if it's related.

Please have a look and checkin if it's ok.

https://tbpl.mozilla.org/?tree=Gaia-Try&rev=0b026c8f1b23
https://tbpl.mozilla.org/?rev=528f7482dd5819ed9e6dd105ddf8233e57b1d757&tree=Gaia-Try

Thanks !

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8493343 [details] [review]
Github PR For Bug 1005331 with No errors in JsHint for shared/media

The error should be unrelated, and I am not sure why it got timed out.

I am re-triggering Gij for a few times just to make sure. After that you could add checkin-needed on the keyboard filed and people will help you out and get it landed.

Comment 14

[Carsten Book [:Tomcat]]

master https://github.com/mozilla-b2g/gaia/commit/b73b27fec2be9d945f963ebe33b4f7d8bcf52205

Comment 15

[David Flanagan [:djf]]

I'm pretty upset that this patch to the media apps shared files landed without review by anyone on the media team.  I was asked for review, and two days later that request was rerouted to someone else. I've just looked it over now while trying to figure out an (unrelated) regression. I would have totally given this an r- for the reasons I've described on github.

I'm thinking very seriously about backing this out and re-opening. Since Kevin filed the bug, though, I'll ask him first before I do.  Kevin: will it do any harm if I back this out?

Comment 16

[David Flanagan [:djf]]

Mathieu,

Thanks for your work on this. Overall, your patch is a helpful one. The review process didn't go as it should, however. I was the correct reviewer for the patch, since I am the author of most of the code in shared/js/media/. You shouldn't have switched the review request from me to Tim without first asking me.

Comment 17

[Kevin Grandon :kgrandon]

Yes, let's backout immediately and land a fixed version which you have reviewed. No need to ni? for backouts, if it causes a regression, let's back it out.

David - can you block this bug on any bugs created as a cause of it landing?

Backed out: https://github.com/mozilla-b2g/gaia/commit/539ac27c752dddbca30d43be34699fc1cb350daf

Mathieu - Do you have time to address David's concerns and submit another patch here? If so, let's make sure to get David's R+ here before landing next time. Thanks.

Comment 18

[Mathieu Rhéaume]

Hi,

Sorry if I didn't get the right owner for this bug. 

Kgrandon, I can indeed work with Djf feedback. I will update the code with djf comments and open a new PR then ask for review.

Djf, About the else if --> else, if in https://github.com/mozilla-b2g/gaia/commit/539ac27c752dddbca30d43be34699fc1cb350daf#diff-63f7c82399c6bbbc8d763cee1578f223R121. 

Would you rather ignore those lines instead and keep the semantics ? else we have to put the {} on every if and else.

I will look into modifying the files I added a lot of "use strict" to declare only one and export the funcs. It could indeed be more clean.

Thanks.

(In reply to Kevin Grandon :kgrandon from comment #17)
> Yes, let's backout immediately and land a fixed version which you have
> reviewed. No need to ni? for backouts, if it causes a regression, let's back
> it out.
> 
> David - can you block this bug on any bugs created as a cause of it landing?
> 
> Backed out:
> https://github.com/mozilla-b2g/gaia/commit/
> 539ac27c752dddbca30d43be34699fc1cb350daf
> 
> Mathieu - Do you have time to address David's concerns and submit another
> patch here? If so, let's make sure to get David's R+ here before landing
> next time. Thanks.

Comment 19

[David Flanagan [:djf]]

(In reply to Kevin Grandon :kgrandon from comment #17)
> Yes, let's backout immediately and land a fixed version which you have
> reviewed. No need to ni? for backouts, if it causes a regression, let's back
> it out.
> 

Thanks for backing it out, Kevin. Note, though, that it didn't actually cause a regression. I just really didn't like some of the changes!

Comment 20

[Kevin Grandon :kgrandon]

(In reply to Mathieu Rhéaume from comment #18)
> Would you rather ignore those lines instead and keep the semantics ? else we
> have to put the {} on every if and else.

Jshint can easily support `else if`, so you should probably just add braces where necessary.

Comment 21

[David Flanagan [:djf]]

(In reply to Mathieu Rhéaume from comment #18)
> Hi,
> 
> Sorry if I didn't get the right owner for this bug. 
> 
> Kgrandon, I can indeed work with Djf feedback. I will update the code with
> djf comments and open a new PR then ask for review.

Thanks, Mathieu!

> Djf, About the else if --> else, if in
> https://github.com/mozilla-b2g/gaia/commit/
> 539ac27c752dddbca30d43be34699fc1cb350daf#diff-
> 63f7c82399c6bbbc8d763cee1578f223R121. 
> 
> Would you rather ignore those lines instead and keep the semantics ? else we
> have to put the {} on every if and else.
> 

I don't really understand. I've got jshint 2.4.3 locally and it doesn't have any problem with else if constructs.  This code passes with no problems, for example:

var x, y, z;
if (x) {
  print(x);
}
else if (y) {
  print(y);
}
else {
  print(z);
}

Comment 22

[Sudheesh Singanamalla (:ShellHacker)]

I've seen the comments on the PR on github and noticed that this was reopened, Could I try and fix this ?

Comment 23

[David Flanagan [:djf]]

Sudheesh: that would be fine with me, but the person you should ask is the current assignee. 
Mathieu: are you still working on this, or can Sudheesh take it?

Comment 24

[Mathieu Rhéaume]

Sure david, assign it to somebody else.

No problems with this.

Comment 25

[Sudheesh Singanamalla (:ShellHacker)]

Thanks Mathieu, I am taking this.

Comment 26

[Sudheesh Singanamalla (:ShellHacker)]

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

No hint failures on shared/js/media/*
I hope this patch goes well.

Comment 27

[Kevin Grandon :kgrandon]

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

I think David should have the final say here, but the code generally looks good to me. Thank you!

Comment 28

[David Flanagan [:djf]]

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

Thanks for working on this Sudheesh.

I'm giving r- because you have added a break statement which alters the control flow in jpeg_metadata_parser.js. You added the break right before the comment /* fallthrough */ which is an old C convention for indicating that the programmer intentionally meant to omit the break and allow control flow to "fall through" to the next case.

Perhaps it was not wise to structure the code that way, but given that it is structured that way, you'll need to find a different way to appease jshint. I think you can use some kind of comment to tell jshint to be silent about that one line.

I also had a couple of nits, noted on github. In one case, I think using a comment to silence jshint would be better than moving 'var temp' up to the top of its function.

There are changes to the remote_controls.js file that look good to me, but I'm not certain that they are actually required by jshint. If they are not required, I'd like you to ask :dkuo to review that file.

And finally, I would guess that this patch should also include changes to the jshint exclude file (I'm not sure where it is) so that jshint starts getting automatically run on the files you've just cleaned up. Needinfo Kevin for his input on this point.  Kevin: I'm going to be out of office this week, so feel free to give a final review to this patch in my absence if Sudheesh addresses the points above.

Comment 29

[Kevin Grandon :kgrandon]

The xfail list can be found here: https://github.com/mozilla-b2g/gaia/blob/master/build/jshint/xfail.list#L362

Please remove the necessary files from that list as a part of this PR. Feel free to flag me for a final review pass and I will take a look. Thanks!

Comment 30

[Sudheesh Singanamalla (:ShellHacker)]

David,

Thanks for pointing things out, I've made the changes on the things you've pointed out, i've passed on the review of the updated PR to Kevin and :dkuo to look into the changes so that they could be merged.

Comment 31

[Kevin Grandon :kgrandon]

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

As in comment 29, these files still need to be removed from the xfail list. Please re-flag once they're removed. Thanks!

Comment 32

[Sudheesh Singanamalla (:ShellHacker)]

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

Fixed that small error of removing files from xfail.list, I was expecting them around .jshintignore and when i didn't find any, I thought it was all clear.

Cleared it now.

Comment 33

[Kevin Grandon :kgrandon]

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

I'm fine with reviewing all of the changes here. The changes to remote_control.js were also standard JSHint fixes, so going to take the full review here. Nice work!

Comment 34

[Kevin Grandon :kgrandon]

I've landed your commit into master and fixed the commit message: https://github.com/mozilla-b2g/gaia/commit/f11e0bcf952bcd8ad8a1d2e5c272dce3b4c1c120

Thank you for the contribution!

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Backed out for the same Linter failures that were visible in the last Gaia Try run in the pull request.
Master: https://github.com/mozilla-b2g/gaia/commit/401e981f51cf047292d101c785be8ec48bf75e8c

https://treeherder.mozilla.org/logviewer.html#?job_id=1170518&repo=b2g-inbound

Comment 36

[David Flanagan [:djf]]

I think the linter failure issue is because I landed a change to jpeg_metadata_parser.js between the time that this patch was reviewed and the time it was landed. My change caused another jshint error that was exposed when that file was removed from xfail.list

Personally, I can't get the /* falls through */ comment to work locally, even with an updated jshint, but, there is now another switch statement (near line 128) that is going to need the same jshint comment as you've got near line 96.  Commenting with // jshint ignore:line would be okay too if that works.

Sudheesh: do you want to take one more pass a this?

Comment 37

[Kevin Grandon :kgrandon]

Argh, sorry about that. Interesting that /* falls through */ would not work, seems we use that in several places.

Comment 38

[Sudheesh Singanamalla (:ShellHacker)]

Kevin, Thank you.

David, I think i'll give this another shot. By the way /* falls through */ has to be placed after the location where the falls through occurs and right before the next conditional statement
eg.
```
 case 0xE1:  // APP1 segment. Probably holds EXIF metadata
        parseAPP1(data);

      /* falls through */
      default:
        // A segment we don't care about, so just go on and read the next one
        if (isLastSegment) {
          metadataError('unexpected end of JPEG file');
          return;
        }
```
and not as
```
 case 0xE1:  // APP1 segment. Probably holds EXIF metadata
        parseAPP1(data);
        /* falls through */

      default:
        // A segment we don't care about, so just go on and read the next one
        if (isLastSegment) {
          metadataError('unexpected end of JPEG file');
          return;
        }
```
I had quite some learning about this while trying to fix this bug.

Comment 39

[Sudheesh Singanamalla (:ShellHacker)]

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

Outdates previous pull request. Fixes all jshint issues.

Comment 40

[Kevin Grandon :kgrandon]

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

Seems fine to me and I double checked that lint is passing on try. Let's do this again.

Comment 41

[Kevin Grandon :kgrandon]

In master: https://github.com/mozilla-b2g/gaia/commit/7238b35d272b65636bb2f55e49b9f789ec8a59d1

Comment 42

[Kevin Grandon :kgrandon]

Apparently sometimes JSHint has an issue with not having a break; before default, even if we have /* falls through */. You can see an example of this error here: http://docs.taskcluster.net/tools/task-graph-inspector/#lc77pfmCRlmca05DpbbAOw/DwcWBT6BSZaZt2CcHCGKxQ/0

Instead of backing this out, I think it would be more simple to just move the default statement to the end, and assume that the '1' will land in that path. I've landed a quick follow-up here: https://github.com/mozilla-b2g/gaia/commit/d8548eaa0a834da05de9dc302921bf0c45cf8042