872918 - Normalize comments style that include workaround referencing specific bugs

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Andrei Eftimie]

We have a number of comments that reference a specific bug.
They usually include a workaround that should be taken out of the codebase once the referenced bug has been dealt with.

At the moment they follow multiple styles, and should all adhere to the same template.

We have:
> 45 comments that include a bug reference
> 47 comments that include the "XXX" string (useful for finding all of them in a search)
> 
> 8 XXX comments without a bug reference
> 10 bug comments without XXX
>
> 31 instances where the description starts on the same line
> 16 instances where the description starts on a new line

We should set the template to be used, and change all instances to adhere to that template.
At the moment it might be confusing to add such a comment since it is evenly split on how to format it.

Comment 1

[Andrei Eftimie]

Styles used:

> // XXX: Bug 599702 doens't give enough information which type of notification

> // Bug 600502 : Add-ons in search view are not initialized correctly

> // XXX: Bug 668976
> //      No easy way to handle the multiple platform case. We have to filter
> //      by the final computed CSS style for the element

> // XXX Bug 634948
> // Regex objects are transformed to strings when evaluated in a sandbox
> // For now lets re-create the regex from its string representation

Question is, do we still need the XXX part?
Searching for "bug" yields now mostly the same results, making XXX redundant.

Then how should these comments look, should they be like the following example?
> // Bug 634948
> // Regex objects are transformed to strings when evaluated in a sandbox
> // For now lets re-create the regex from its string representation

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andrei Eftimie from comment #1)
> Then how should these comments look, should they be like the following
> example?
> > // Bug 634948
> > // Regex objects are transformed to strings when evaluated in a sandbox
> > // For now lets re-create the regex from its string representation

That looks right to me. I would really drop the 'XXX' part which is kinda unhelpful. If something is a todo we should mark it as such: 'TODO: description' so it can be picked up by editors.

Comment 3

[Andrei Eftimie]

Attachment: patch 1

Changed all comments to match the template.

Comment 4

[Jonathan French (:jfrench)]

It's totally minor, but I noticed that the original comment in lib/addons.js was misspelled. I suppose it could be corrected if another patch revision was required.

> // Doens't

Comment 5

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 750330 [details] [diff] [review]
patch 1

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

::: lib/addons.js
@@ +303,5 @@
>      var addon = spec.addon;
>      assert.ok(addon, arguments.callee.name + ": Add-on has been specified.");
>  
> +    // Bug 599702
> +    // Doens't give enough information which type of notification

As Jonathan pointed, lets correct this.

::: lib/modal-dialog.js
@@ +97,1 @@
>          // Remove extra checks once Mozmill 1.5.4 has been released

This is also a TODO item and will have to check if it's still available.

::: lib/software-update.js
@@ +195,5 @@
> +   // if (this.activeUpdate.patchCount == 2) {
> +   //   var patch0URL = this.activeUpdate.getPatchAt(0).URL;
> +   //   var patch1URL = this.activeUpdate.getPatchAt(1).URL;
> +   //     Test that the update snippet created by releng doesn't have the same
> +   //     url for both patches (bug 514040).

Please leave those // as they were, since this is a comment that need to be kept when we uncomment the block. Also, I would wrap these long blocks in /* and */

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 750330 [details] [diff] [review]
patch 1

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

::: lib/modal-dialog.js
@@ +97,1 @@
>          // Remove extra checks once Mozmill 1.5.4 has been released

Please file a new bug so we can kill the whole block and also remove the extra checks. Those are no longer necessary. Mozmill 1.5.4 is history for a long time. We can keep that and others in a single bug.

::: lib/software-update.js
@@ +195,5 @@
> +   // if (this.activeUpdate.patchCount == 2) {
> +   //   var patch0URL = this.activeUpdate.getPatchAt(0).URL;
> +   //   var patch1URL = this.activeUpdate.getPatchAt(1).URL;
> +   //     Test that the update snippet created by releng doesn't have the same
> +   //     url for both patches (bug 514040).

Those lines can also be removed now. Lets make it part of the above mentioned bug to file.

@@ +511,5 @@
>     * @param {MozMillController} browserController
>     *        Mozmill controller of the browser window
>     */
>    openDialog: function softwareUpdate_openDialog(browserController) {
> +    // TODO: After Firefox 4 has been released and we do not have to test any

Same here.

::: lib/tabs.js
@@ +478,5 @@
>  
>          // RTL-locales need to be treated separately
>          if (utils.getEntity(this.getDtds(), "locale.dir") == "rtl") {
> +          // Bug 537968
> +          // Workaround until bug 537968 has been fixed

Same here.

@@ +485,4 @@
>            this._controller.doubleClick(tabStrip, 100, 3);
>          } else {
> +          // Bug 537968
> +          // Workaround until bug 537968 has been fixed

Same here.

::: lib/tabview.js
@@ +524,5 @@
>          nodeCollector.queryNodes(".name");
>          break;
>        case "group_undoButton":
> +        // Bug 596504
> +        // No reference to the undo button

Same here. The bug has been fixed.

::: lib/ui/private-browsing.js
@@ +106,5 @@
>  
>      this._controller = utils.handleWindow("type", "navigator:browser", undefined, false);
>  
> +    // Bug 847991
> +    // URL does not always load when opening private browsing window

Please update the comment so its better understandable.

::: tests/functional/testTabbedBrowsing/testNewTab.js
@@ +69,5 @@
>    // Open a new tab and check that 'about:newtab' has been opened
>    tabBrowser.openTab(aEventType);
>  
> +  // Bug 716108
> +  // Remove once 716108 lands

Was fixed. Lets get it removed.

::: tests/remote/restartTests/testDiscoveryPane_installAddonWithEULA/test1.js
@@ +40,5 @@
>    controller.open(ADDON.page);
>    controller.waitForPageLoad();
>  
> +  // Bug 680045
> +  // Add elements to UI map for add-ons with EULA

This was wontfixed. Lets get it updated appropriately.

Comment 7

[Andrei Eftimie]

Attachment: patch v2

> ::: lib/software-update.js
> @@ +195,5 @@
> > +   // if (this.activeUpdate.patchCount == 2) {
> > +   //   var patch0URL = this.activeUpdate.getPatchAt(0).URL;
> > +   //   var patch1URL = this.activeUpdate.getPatchAt(1).URL;
> > +   //     Test that the update snippet created by releng doesn't have the same
> > +   //     url for both patches (bug 514040).
> 
> Please leave those // as they were, since this is a comment that need to be kept when we uncomment the block. Also, I would wrap these long blocks in /* and */

I just properly aligned them, I did not change any semantics here.
We use /* */ for block comments, // for inline comments and commented code


> ::: tests/remote/restartTests/testDiscoveryPane_installAddonWithEULA/test1.js
> @@ +40,5 @@
> >    controller.open(ADDON.page);
> >    controller.waitForPageLoad();
> >  
> > +  // Bug 680045
> > +  // Add elements to UI map for add-ons with EULA
> 
> This was wontfixed. Lets get it updated appropriately.

Removed the comment.

Opened bug 873473 to removed outdated workarounds and TODO's where needed.

Comment 8

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 751038 [details] [diff] [review]
patch v2

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

I haven't got the chance to review this, as it is a refactoring one, it was left at the bottom of my list. Could you please update it so I can land it today? Thanks.

Comment 9

[Andrei Eftimie]

Attachment: patch v3

Quick update.
Applies cleanly now.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 760782 [details] [diff] [review]
patch v3

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

Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/5989a11103a5 (default)

I think it's not that important to backport this now and just leave it to get merged in time.