842058 - Clean up function () code blocks in the repo

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Jonathan French (:jfrench)]

Begat from fixing bug 839705, I came upon functions in at least one test with closing braces not aligned to the block, similar to this

var foo = function () {
  }

..versus what the Mozmill Style Guide generally illustrates through its example code blocks.

var foo = function () {
}

In files like
tests\remote\testAddons\testSearchAddons.js

That seemed unusual to me. If you consider it a bug feel free to assign me and I will have a go at fixing it or any other tests which may have the same issue. I may be able to devise a clever way of pattern matching for them, or I may have to simply fix this one obvious test.

Comment 1

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

Looks like an oversight. So yeah, we might want to get this fixed but has low priority.

Comment 2

[Jonathan French (:jfrench)]

There were enough other functions() curly braces to tidy up, I've revised the title of the bug to be generic. As the Style Guide says, "be prepared to defend your reasons" so here are reasons for the proposed adjustments, which primarily consolidate the opening brace of the function, on to the same line as the function().

1) It reduces the number of lines in each file
2) Less likely to miss the opening brace on the same line when making edits
3) It makes these consistent in style with the other 97% of the repo
4) It makes these consistent within some individual tests, that use both styles
5) It is consistent with the Style 'Guide' examples
https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide#Naming

I'll attach a patch for default shortly if you wish to use it.

Comment 3

[Jonathan French (:jfrench)]

Attachment: clean up function code blocks (default)

Patch "clean up function code blocks (default)" for the default branch. Thirty files modified, which include some component files in lib.

Tested with Default Nightly 22.0a1 20130220031126. Tests pass where expected.

If you don't want the patch or to make these changes, no worries it was enjoyable to do.

Comment 4

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

Comment on attachment 716276 [details] [diff] [review]
clean up function code blocks (default)

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

A long overdue change. Thanks a lot for doing that! There are two nits I would like to see fixed before the check-in. r=me with those changes addressed. When you upload the new patch you can carry the r+ forward.

::: lib/software-update.js
@@ +59,5 @@
>            cancel: '{"dlgtype":"cancel"}',
>            finish: '{"dlgtype":"finish"}',
>            extra1: '{"dlgtype":"extra1"}',
>            extra2: '{"dlgtype":"extra2"}'
> +  }

Not sure why this got a 8 character indentation. Can you please also change that to our style guide which is 2 blanks?

::: tests/functional/testPrivateBrowsing/testAboutPrivateBrowsing.js
@@ +18,5 @@
>    pbWindow = new privateBrowsing.PrivateBrowsingWindow();
>  
>    prefs.preferences.setPref(PREF_PRIVATE_BROWSING_SUPPORT, LOCAL_TEST_PAGE);
>  }
> +

nit: trailing white-space issue

Comment 5

[Jonathan French (:jfrench)]

Sure, no problems. I have corrected the first one for a revised patch (I wasn't sure how aggressive to be with the code changes). As for the second one, that is fascinating, that was not in my original pull, someone must have introduced that to the file very recently.

Comment 6

[Jonathan French (:jfrench)]

Ok, to clarify testAboutPrivateBrowsing.js, I'm thinking you may have an old pull in default or something?(perhaps). The current version of the file in default branch - actually has no blank line between the setupModule and the teardownModule and my change corrects that by placing a blank line between them, so it is the same as other tests. 

I will be uploading a patch momentarily. In the patch, one file has since been corrected by someone else; testInstallation\testBreakpadInstalled, so this new patch omits that file.

Comment 7

[Jonathan French (:jfrench)]

Attachment: clean up function code blocks (default)

clean up function code blocks (default)

Patch "clean up function code blocks (default)" for the default branch. Twenty nine modified, which include some component files in lib. Carrying r+ review flag forward as requested.

This supersedes the originally submitted patch for default.

Comment 8

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

Comment on attachment 717138 [details] [diff] [review]
clean up function code blocks (default)

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

Looks fine! Landed as:

http://hg.mozilla.org/qa/mozmill-tests/rev/f1f141b9ff87 (default)

Not sure if we should spend the time on fixing that on the other branches. Looks very minor to me. Better to focus on other important bugs on default. Jonathan, if you want to backport feel free. If not we can close the bug as fixed.

Comment 9

[Jonathan French (:jfrench)]

That's fine so long as nobody ever "forward ports" files that would stomp on the fixes by porting an aurora/beta/release/esr17 file on to default? 

Does, or can that ever happen direction-wise? I don't mind doing the back port next week (after back porting bug 790597 first). Whatever you guys wish is fine though.

Comment 10

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

This will most likely never happen. We always backport, except something has to land on an older branch first for verification.

Bug 790597 is not about style changes and is worth backporting. But as discussed with Dave earlier this week, we might want to skip backports for styles from now on. There are more important other bugs to fix.

Thanks for working on that patch Jonathan!

Comment 11

[Jonathan French (:jfrench)]

Great, ok that is fine then. On to 790597.