[webkit-reviews] review granted: [Bug 68045] Generate WebKit team's page out of committers.py : [Attachment 107509] Used JSON instead of eval and querySelector instead of getElementsByClassName

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 68045: Generate WebKit team's page out of committers.py
https://bugs.webkit.org/show_bug.cgi?id=68045

Attachment 107509: Used JSON instead of eval and querySelector instead of
getElementsByClassName
https://bugs.webkit.org/attachment.cgi?id=107509&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=107509&action=review


This patch doesn't add a hyperlink to this page and hence you can only access
it directly by its name.  I take it that you plan to work on this some more
before adding a hyperlink to this page from the public website/wiki?

Additionally, as Patrick Gansterer implied by his question in comment 30, it
would be great if we provided some kind of fall back when JavaScript is either
disabled or the client lacks sufficient JavaScript support. We may not be
interested in explicitly supporting the latter group.

> Websites/webkit.org/team.html:30
> +    'rim.com': 'Research in Motion',

Nit: Research in Motion => Research In Motion. That being said, we should fix
this on the wiki as well.

> Websites/webkit.org/team.html:49
> +	   if (match = contributorLine.exec(lines[i])) {

It seems sufficient to define variable match as a local variable instead of a
global. That is, I would write this line as:

var match = contributorLine.exec(lines[i]);
if (match) {
...

You may also want to consider using an early-continue style. This may make the
code flow a bit easier to follow given that there is a "continue" statement in
the "catch" clause of the try-block. Using an early-continue style would also
remove one level of indentation and make the two continue statements
left-align.

> Websites/webkit.org/team.html:81
> +    return affiliations ? affiliations.join(' / ') : null;

Notice that affiliations.join(' / ') == "" (the empty string) when affiliations
== [] and the empty string evaluates to false in a boolean context.

Therefore, I would write this line as:

return affiliations.join(' / ');

> Websites/webkit.org/team.html:94
> +function pupulateContributorListItem(listItem, contributor) {

pupulateContributorListItem => populateContributorListItem

(Notice the 'o' in populate)

> Websites/webkit.org/team.html:115
> +	   pupulateContributorListItem(listItem, contributorsOfKind[i]);

Ditto.

> Websites/webkit.org/team.html:130
> +	   if (nameContainer &&
nameContainer.textContent.toLowerCase().indexOf(contributor.name.toLowerCase())
>= 0)

The expression "contributor.name.toLowerCase()" evaluates to the same result
for each iteration of this loop. I suggest storing the result of this
expression in a variable, say contributorName or lowercaseContributorName,
outside of the for-loop instead of re-evaluting this expression on each
iteration.

> Websites/webkit.org/team.html:133
> +	   var nicksInContainer = nicksInListItem(listChildren[i]);
> +	   if (nicksInContainer && contributor.nicks) {

It's unnecessary to call nicksInListItem() if contributor.nicks is null. That
being said, I take it you ordered it this way since the majority of people list
a nick on the Trac wiki page; => less likely contributor.nicks is null; => less
likely that this if-condition will evaluate to false.

If this was your intention then, for your consideration, you may want to add a
comment here that explains this assumption.

> Websites/webkit.org/team.html:151
> +	       pupulateContributorListItem(listItem, contributor);

pupulateContributorListItem => populateContributorListItem

(Notice the 'o' in populate)

> Websites/webkit.org/team.html:176
> +	       // Strip special html characters, and match lines like *
'''Ryosuke Niwa''' (rniwa) ''Google''

html => HTML

(because it's an acronym)

> Websites/webkit.org/team.html:177
> +	       match = lines[i].replace(/[\{\}<>"%;\&+\/]/g,
'').match(/^\s+\*\s+\'\'\'([^']+)\'\'\'\s*(\(([^']+)\)\s*)?(\'\'([^']+)\'\')?\s
*$/);

>From my understanding, the only characters that need to be escaped within a
character class expression (i.e. [ ... ]) are '\', ']', and '-' since they
represent the escape character, end of the character class, and the class range
separator, respectively. That is, it's unnecessary to escape the literals '{',
'}', '&', and '/' inside a character class. So,
"lines[i].replace(/[\{\}<>"%;\&+\/]/g, '')" can be simplified to:

lines[i].replace(/[{}<>"%;&+/]/g, '').

Also, a ' (single-quote) doesn't need to be escaped within a regular
expression. So, the last regular expression on this line can be written as:

/^\s+\*\s+'''([^']+)'''\s*(\(([^']+)\)\s*)?(''([^']+)'')?\s*$/;

You may also want to define a variable for this expression with a more
meaningful name outside of this for-loop, maybe
webkitTeamWikiNameAndNickAndAffliationRegEx? or webkitTeamWikiNameRegEx? or
nameAndNickAndAffliationRegEx?

> Websites/webkit.org/team.html:188
> +    xhr.onerror = function () { alert('Could not obtain
http://trac.webkit.org/wiki/WebKit%20Team'); };

How did you come to the decision to use a JavaScript alert() for displaying the
error message? Can we write this error message to the page instead of
displaying a modal dialog alert()?

Writing the error message to the page would also be consistent with the
approach you've taken when the committers.py file cannot be obtained on line
206.