Closed Bug 1315013 Opened 3 years ago Closed 3 years ago

Clean up comments that got made ugly by bug 1312486

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(2 files)

No description provided.
(clicked Save too soon).

Bug 1312486 required a space before comment text, but some comments were used as ASCII art to decorate the surrounding text.

Some examples are as following:
> 943	////////////////////////////////////////////////////////////////////////////////
> 944	//// PlacesMenuDNDHandler
became
> 943	// //////////////////////////////////////////////////////////////////////////////
> 944	// // PlacesMenuDNDHandler

The long line of slashes should get removed, and the following list should just become
> 944   // PlacesMenuDNDHandler

And
> //******** define a js object to implement nsITreeView
became
> //* ******* define a js object to implement nsITreeView

The extra asterisks should get removed and this should become
> // define a js object to implement nsITreeView

The following should be reverted, and have the eslint rule disabled for
> /****** BEGIN LICENSE BLOCK *****
became
> /* ***** BEGIN LICENSE BLOCK *****
because this is 3rd party code and it will make updating harder.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> The long line of slashes should get removed, and the following list should
> just become
> > 944   // PlacesMenuDNDHandler

in many cases this just comes before the equally-named object, and then the comment by itself is pointless.
The idea there was to provide visual spacing between large objects in large files, the // nameofobject comment by itself doesn't help much, maybe just a double newline?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> The following should be reverted, and have the eslint rule disabled for
> > /****** BEGIN LICENSE BLOCK *****
> became
> > /* ***** BEGIN LICENSE BLOCK *****
> because this is 3rd party code and it will make updating harder.

These lines were in fact not changed by 1312486 so they won't get changed here. They also do not look out place.

(In reply to Marco Bonardo [::mak] from comment #2)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> > The long line of slashes should get removed, and the following list should
> > just become
> > > 944   // PlacesMenuDNDHandler
> 
> in many cases this just comes before the equally-named object, and then the
> comment by itself is pointless.
> The idea there was to provide visual spacing between large objects in large
> files, the // nameofobject comment by itself doesn't help much, maybe just a
> double newline?

In the case where the comment didn't add much value I just removed the comment. I didn't add extra blank newlines because it didn't seem necessary to me (and I didn't see your comment until after I was finished making the patches). 

We should probably break up the large files in to more digestible sizes.
Comment on attachment 8807250 [details]
Bug 1315013 - part 2, remove unnecessary slash-star comments that don't add value and look out of place with the spaced-comment eslint rule.

https://reviewboard.mozilla.org/r/90480/#review90198
Attachment #8807250 - Flags: review?(dtownsend) → review+
Comment on attachment 8807249 [details]
Bug 1315013 - part 1, remove unnecessary double-slash comments that don't add value and look out of place with the spaced-comment eslint rule.

https://reviewboard.mozilla.org/r/90478/#review90206
Attachment #8807249 - Flags: review?(dtownsend) → review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57200c02575f
part 1, remove unnecessary double-slash comments that don't add value and look out of place with the spaced-comment eslint rule. r=mossop
https://hg.mozilla.org/integration/autoland/rev/1b91c5c012f8
part 2, remove unnecessary slash-star comments that don't add value and look out of place with the spaced-comment eslint rule. r=mossop
https://hg.mozilla.org/mozilla-central/rev/57200c02575f
https://hg.mozilla.org/mozilla-central/rev/1b91c5c012f8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.