address style nits in all files located under prototypes/wcap

VERIFIED FIXED in 0.7

Status

Calendar
General
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Michael Büttner (no reviews TFN), Assigned: Michael Büttner (no reviews TFN))

Tracking

Dependency tree / graph
Bug Flags:
blocking-calendar0.7 +

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
In light of reviewing the prototype event dialog one particular issue is to address all the style nits throughout the code. This affects all files located under m/c/prototypes/wcap.
(Assignee)

Updated

11 years ago
Blocks: 370435
Flags: blocking-calendar0.7+
(Assignee)

Comment 1

11 years ago
Created attachment 273778 [details]
shell script

This script parses a source file and dumps all the relevant locations that need to be addressed. I received this script from philipp in order to automate the task of finding all style nits, thanks again for taking care. He said that I should mention that this script is "just a hack".
(Assignee)

Comment 2

11 years ago
The upcoming patch touches these files:

sun-calendar-customize-toolbar.js
sun-calendar-customize-toolbar.xul
sun-calendar-event-dialog-attendees.js
sun-calendar-event-dialog-attendees.xml
sun-calendar-event-dialog-attendees.xul
sun-calendar-event-dialog-freebusy.xml
sun-calendar-event-dialog-recurrence-datepicker.css
sun-calendar-event-dialog-recurrence-datepicker.xml
sun-calendar-event-dialog-recurrence-preview.css
sun-calendar-event-dialog-recurrence-preview.xml
sun-calendar-event-dialog-recurrence.js
sun-calendar-event-dialog-recurrence.xml
sun-calendar-event-dialog-recurrence.xul
sun-calendar-event-dialog-reminder.js
sun-calendar-event-dialog-reminder.xul
sun-calendar-event-dialog-timezone.js
sun-calendar-event-dialog-timezone.xul
sun-calendar-event-dialog.css
sun-calendar-event-dialog.js
sun-calendar-event-dialog.xul
(Assignee)

Comment 3

11 years ago
I tried to come up with a list of rules that we can use to check source code against. I also would like to have an agreed set of rules that we can use for future reference as I'm still unsure what's a style rule violation and what's not. So, here we go...

-> whitespace

no leading whitespace on empty lines
no trailing whitespace whatsoever

-> intendation

4 spaces for javascript, 2 spaces for xml
javascript inside xml (for bindings or such) should look like this:

<method name="methodName">
  <body><![CDATA[
    if (true) {
        return 42;
    }
  ]]></body>
</method>

cdata-tags (opening and closing) should be placed on the same line as the enclosing tag (body-tag in this case).

-> blanks

blank after keywords -> if, while, for each, for
blank around the following operators -> = + - * / % >= <= !=
blank after ',' separating function arguments

-> javascript objects

one property per line

var object = {
    a: 5,
    b: 42
}

-> brace placement

place opening braces on the same line as the statement (k&r style)
place the closing brace on the same column as the opening statement

if (true) {
    while (condition) {
        ...
    }
}

keywords directly following closing braces should be placed on the same line

do {
    ...
} while

if (true) {
    ...
} else {
    ...
}

I intentionally did not come up with a rule such as "no line longer than 80 chars". I feel this is antiquated and shouldn't be part of this rule set. Of course, we shouldn't have excessive lenghty lines, but religiously fighting for 80 chars is something I don't want to support any longer.

If anybody feels that this list is incomplete or should be corrected, please feel free to add your comment here.
(Assignee)

Comment 4

11 years ago
Created attachment 274276 [details] [diff] [review]
patch v1

This patch applies the above mentioned rules to the list of files that comprise the event dialog. Philipp, would you please be so kind to give this patch a high priority in your review queue as this bug essentially blocks any other patch for the event dialog. Furthermore, assuming that you find remaining style nits I might have missed, I would greatly appreciate that you just post a new version of the patch. Thanks in advance for taking care of this massive patch...
Attachment #274276 - Flags: review?(philipp)
Created attachment 274792 [details] [diff] [review]
patch v2

I found a bunch of other things and made massive use of regexes :)

I hope I didn't break anything on the way, please test before checking in. I'm requesting review to make sure its tested.
Attachment #274276 - Attachment is obsolete: true
Attachment #274792 - Flags: review?(michael.buettner)
Attachment #274276 - Flags: review?(philipp)
Blocks: 351084
Status: NEW → ASSIGNED
(Assignee)

Comment 6

11 years ago
Comment on attachment 274792 [details] [diff] [review]
patch v2

I didn't find anything else to complain about -> r=mickey.
Attachment #274792 - Flags: review?(michael.buettner) → review+
(Assignee)

Comment 7

11 years ago
I would like to note that Philipp was so kind to set up a new wiki page (see [1]) that summarizes all those rules above and adds a hell of a lot of other valuable stuff. I would like to establish this page as an established source of knowledge so that it serves as a general reference should any debate concerning style nits come up in the future.

[1] http://wiki.mozilla.org/Calendar:Style_Guide
(Assignee)

Comment 8

11 years ago
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7

Comment 9

11 years ago
Verified in lightning (build 2007080603) and sunbird (build 20070806) -> task is fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.