canmove, Confirmed users
1,448
edits
AMO:Editors/EditorGuide/AddonReviews: Difference between revisions
AMO:Editors/EditorGuide/AddonReviews (view source)
Revision as of 21:04, 19 January 2011
, 19 January 2011→Step 3: Code Review
(Policies and actions for validation step) |
|||
| Line 239: | Line 239: | ||
=== Step 3: Code Review === | === Step 3: Code Review === | ||
Every line of add-on code must be reviewed. The code validator can't detect all possible security or code quality issues, so we must always be in the lookout for bad code. For versions in the Preliminary Review queue, the code review should be shallow and just ensure the add-on's safety. | Every line of add-on code must be reviewed. The code validator can't detect all possible security or code quality issues, so we must always be in the lookout for bad code. For versions in the Preliminary Review queue, the code review should be shallow and just ensure the add-on's safety. | ||
All review pages have a View Contents link that take you to the code browser page. Pending updates also have a Compare with Public Version link next to it, which will show you the code with the changed sections highlighted. These links appear near the top of the review page.<br> | All review pages have a View Contents link that take you to the code browser page. Pending updates also have a Compare with Public Version link next to it, which will show you the code with the changed sections highlighted. These links appear near the top of the review page.<br> | ||
| Line 249: | Line 249: | ||
In the case of comparing updates, the files and folders that have had any changes will have their names in italics. The image above shows changes in all files and folders.<br> | In the case of comparing updates, the files and folders that have had any changes will have their names in italics. The image above shows changes in all files and folders.<br> | ||
During the code review, you should check all code files, and you should look for all of the following:<br> | ==== Policies and Actions ==== | ||
{| width="650" cellspacing="0" cellpadding="1" border="1" | |||
|- | |||
! scope="col" | Policy | |||
! scope="col" | Action | |||
! scope="col" | Notes | |||
|- | |||
| Remote code download or execution, custom updates | |||
| Reject | |||
| | |||
|- | |||
| Security violations | |||
| Reject | |||
| Sending passwords in clear text or in GET requests. Using HTTP for logins or secure operations. | |||
|- | |||
| Bad or no namespacing | |||
| Preliminary Review | |||
| | |||
|- | |||
| Preference names without "extensions." prefix" | |||
| Preliminary Review | |||
| | |||
|- | |||
| Performance problems | |||
| Preliminary Review | |||
| Synchronous requests, inefficient code, multiple overlay scripts with lots of code. | |||
|- | |||
| Not using prefwindow for preferences | |||
| Add Note | |||
| | |||
|} | |||
<br> During the code review, you should check all code files, and you should look for all of the following:<br> | |||
*Namespacing. All scripts that are included in the main window overlay should have proper namespacing to avoid name conflicts with other add-ons. The name should normally correspond to the add-on name in order to guarantee its uniqueness.<br> | *Namespacing. All scripts that are included in the main window overlay should have proper namespacing to avoid name conflicts with other add-ons. The name should normally correspond to the add-on name in order to guarantee its uniqueness.<br> | ||