Julian Descottes Assignee
	Description • 9 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.132 Safari/537.36

Steps to reproduce:

Follow-up to Bug 1164343.

Review styleinspector/rule-view.js and styleinspector/computed-view.js for eslint warnings and violations.

	Julian Descottes Assignee
	Comment 1 • 9 years ago

Only minor nits in this one : curly braces after line return, missing spaces ...

	Julian Descottes Assignee
	Comment 2 • 9 years ago

Adding missing globals (as well as some other minor nits).

Wonder if _Iterator should not be added to the eslint globals, as it seems to be provided by Loader.jsm ? Should be available in all modules ?

	Julian Descottes Assignee
	Comment 3 • 9 years ago

Removed all named functions for consistency : 

myFunction: function myModule_myFunction() { 

-> 

myFunction: function() {}

Didn't see this pattern elsewhere and eslint wants camelCased names for functions.

	Julian Descottes Assignee
	Comment 4 • 9 years ago

openStyleEditor method contained some dead code linked to a never used contentDoc variable.

	Julian Descottes Assignee
	Comment 5 • 9 years ago

(not related to eslint)

Promise was imported as promise (lowercase). 
For consistency with other modules, use Promise.

	Julian Descottes Assignee
	Comment 6 • 9 years ago

Consistent return in refreshMatchedSelectors.

Instead of doing a return; in the middle of a promise that should return a promise, return Promise.resolve().

To satisfy the no-else-return rule, the return value was also extracted to a "promise" variable.

	Julian Descottes Assignee
	Comment 7 • 9 years ago

Similar to previous patch. Return Promise.resolve() instead of return; for return consistency.

	Julian Descottes Assignee
	Comment 8 • 9 years ago

Just uploaded attachments for a first pass on computed-view. 

I deliberately didn't follow two rules too strictly : 

- max length 80 char. Not sure how hard we want to enforce this. It's a good reminder to avoid long, complicated lines. Here I applied it where I felt readability was bad, but skipped it otherwise.

- no-unused-var , for method arguments. Even if an argument is not used, I think it's good to keep it in the method signature if we know it is provided when the method is called (such as the event object for an event handler).

	Julian Descottes Assignee
	Comment 9 • 9 years ago

Import Promise as "Promise" instead of "promise"

	Julian Descottes Assignee
	Comment 10 • 9 years ago

Rule view : return consistency. Mostly return Promise.resolve(undefined); instead of return; when expecting a promise.

	Julian Descottes Assignee
	Comment 11 • 9 years ago

Extract function to avoid arguments shadowing arguments from upper scope.

	Gabriel [:gl] (ΦωΦ)
	Comment 12 • 9 years ago

Comment on attachment 8637456 [details] [diff] [review]
Bug1184746.ruleview.part1.v1.patch

Review of attachment 8637456 [details] [diff] [review]:
-----------------------------------------------------------------

I believe this change would be conflicting with natively implemented DOM Promise.

	Gabriel [:gl] (ΦωΦ)
	Comment 13 • 9 years ago

Comment on attachment 8637461 [details] [diff] [review]
Bug1184746.ruleview.part3.v1.patch

Review of attachment 8637461 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/styleinspector/rule-view.js
@@ +1933,5 @@
>    },
>  
> +  /**
> +   * Toggle the visibility of an expandable container
> +   * @param  {DOMNode}  twisty     clickable toggle element

We have some inconsistencies with how our jsdocs are formatted. Taking a quick look majority of them have the description on a new line.

Can you make sure the description is on a new line and align with the object type of the param?

	Gabriel [:gl] (ΦωΦ)
	Comment 14 • 9 years ago

The commit messages need to be formatted correctly. I only managed to find an old version of the docs, but what is said still holds true. https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions

The commit msg should have the following format:
Bug 123456 - <Summary> r=mikeratcliffe

Try to be a bit more descriptive with the summary of the bugs. For instance, part 1 of computed view could be: "Corrected formatting of function braces in computed-view.js"

Since we have multiple parts, you will also want to format it as the following: "Bug 123456 - Part 1: <Summary> r=mikeracliffe"

	Julian Descottes Assignee
	Comment 15 • 9 years ago

> I believe this change would be conflicting with natively implemented DOM Promise. 

Looking at other modules in the devtools, Promise is sometimes imported as "promise", sometimes as "Promise". Is it really an issue to shadow the DOM Promise here ? 

> Can you make sure the description is on a new line and align with the object type of the param? 

Ok will do.

> The commit messages need to be formatted correctly

I wanted to have small commits for review, and after the review, squash them and format them correctly. I don't think this bug is worth having 10 commits in the repository ?

	Gabriel [:gl] (ΦωΦ)
	Comment 16 • 9 years ago

(In reply to Julian Descottes from comment #15)
> > I believe this change would be conflicting with natively implemented DOM Promise. 
> 
> Looking at other modules in the devtools, Promise is sometimes imported as
> "promise", sometimes as "Promise". Is it really an issue to shadow the DOM
> Promise here ? 

Native implementation of DOM Promises are more recent. I suspect that is the reason for the disparity. I would defer to Mike's judgement here. I believe we shouldn't try to shadow native objects. In this case, we want to be clear if we are using DOM Promises or jsm promises.

> > Can you make sure the description is on a new line and align with the object type of the param? 
> 
> Ok will do.
> 
> > The commit messages need to be formatted correctly
> 
> I wanted to have small commits for review, and after the review, squash them
> and format them correctly. I don't think this bug is worth having 10 commits
> in the repository ?

From what I have seen, I would say it would be safe to have one big patch for all the eslint cleanup since the review for those are very quick. If you're refactoring a particular function, I would move that to a separate patch.

	Gabriel [:gl] (ΦωΦ)
	Comment 17 • 9 years ago

(In reply to Julian Descottes from comment #8)
> Just uploaded attachments for a first pass on computed-view. 
> 
> I deliberately didn't follow two rules too strictly : 
> 
> - max length 80 char. Not sure how hard we want to enforce this. It's a good
> reminder to avoid long, complicated lines. Here I applied it where I felt
> readability was bad, but skipped it otherwise.
> 
> - no-unused-var , for method arguments. Even if an argument is not used, I
> think it's good to keep it in the method signature if we know it is provided
> when the method is called (such as the event object for an event handler).

I believe the eventual goal is to have the try build process also run eslint and warn of lint errors. We should try to fix all the eslint errors the best we can.

BTW, thanks for the work so far! Don't hesitate to ask for feedback or review early.

	Julian Descottes Assignee
	Comment 18 • 9 years ago

@Gabriel : thanks for having a look at the patches ! I integrated your comments, just waiting for Mike's review before reuploading.

> We should try to fix all the eslint errors the best we can.

The two I mentioned are only warnings, so maybe we can leave them for now ? 

When choosing the eslint configuration, did you discuss about the unused-var rule applied to function arguments ? (found one PR discussing the topic on eslint's repo: https://github.com/eslint/eslint/issues/1939)

	Gabriel [:gl] (ΦωΦ)
	Comment 19 • 9 years ago

(In reply to Julian Descottes from comment #18)
> @Gabriel : thanks for having a look at the patches ! I integrated your
> comments, just waiting for Mike's review before reuploading.
> 
> > We should try to fix all the eslint errors the best we can.
> 
> The two I mentioned are only warnings, so maybe we can leave them for now ? 
> 
> When choosing the eslint configuration, did you discuss about the unused-var
> rule applied to function arguments ? (found one PR discussing the topic on
> eslint's repo: https://github.com/eslint/eslint/issues/1939)

I think eslint errors would be the first priority. I know there have been eslint discussions within the devtools team and should be noted in some bugs in which the configurations were committed. Regarding leaving eslint warnings, I would wait on Mike's review. I suspect it will be fine to keep eslint warnings, but there will always be that minor annoyance of seeing the lint warning and somebody will eventually try to fix it.

That said I believe there are also some inconsistencies already with keeping the unused event variable as well. So, it would be good to determine what we should do about it.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 20 • 9 years ago

Just to let you know: I would have been more than happy to review this as a single patch as it is fairly short.

	Julian Descottes Assignee
	Comment 21 • 9 years ago

@Mike : Sorry, I was afraid I was splitting too much this time ... Thanks for the review in any case.

What do you think about importing Promise.jsm as Promise instead of promise, is it fine with you ?
Gabriel was concerned about overshadowing the DOM Promise implementation.

I'll push this to try later today.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 22 • 9 years ago

I thought the same thing... seeing it used that way can easily confuse new contributors.

Lets import it as promise and use it that way throughout your patches... you don't need to ask for review for that change though, just make sure it runs through try.

	Julian Descottes Assignee
	Comment 23 • 9 years ago

Try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc065334412d

	Gabriel [:gl] (ΦωΦ)
	Comment 24 • 9 years ago

Julian, I think it is safe to set the "checkin-needed" flag in the "keywords" judging from the try build results.

	Gabriel [:gl] (ΦωΦ)
	Comment 25 • 9 years ago

The commit message just needs one adjustment (change : to -):
Bug 1184746 : <Msg> to Bug 1184746 - <Msg>

	Julian Descottes Assignee
	Comment 26 • 9 years ago

Ok, will do later today.

	Julian Descottes Assignee
	Comment 27 • 9 years ago

Updated commit comment.

	Julian Descottes Assignee
	Comment 30 • 9 years ago

Do not pay attention to v2 and v3, both were wrong uploads.

	Gabriel [:gl] (ΦωΦ)
	Comment 31 • 9 years ago

I went ahead and committed your changes Julian.

Good work and thank you for the patch!

https://hg.mozilla.org/integration/fx-team/rev/ffdd06074331

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/ffdd06074331