|
|
| (90 intermediate revisions by 9 users not shown) |
| Line 1: |
Line 1: |
| = Add-on Reviews<br> =
| | #REDIRECT [[AMO/Reviewers/Guide]] |
| | |
| == The Queues<br> ==
| |
| | |
| The add-on queues are sorted by waiting time, with the longest waiting add-on at the top. They show the same information for every pending review:<br>
| |
| | |
| *Add-on name and version. This is a link to the add-on version review page.<br>
| |
| *Add-on type.<br>
| |
| *Waiting time.<br>
| |
| *Flags. At the moment the only possible flag is the Admin Review flag; it's the green add-on icon with a yellow warning sign on top. Only admin editors can review add-ons with this flag.<br>
| |
| *Applications. Application icons of the applications this add-on supports.<br>
| |
| *Additional information. This can indicate if the add-on is platform-specific, site-specific, or requires external software.<br>
| |
| | |
| If an add-on version is supported on more than one platform, but not all of them, an entry will appear in the queue for each platform. All entries link to the same review page.'''<br>'''
| |
| | |
| '''You should always try to review the add-ons near the top of queue.''' Always favor the add-ons that have been waiting for the longest time. Having said that, the few add-ons that are at the very top and have been waiting for a much longer time than the rest (sometimes multiple days more than the next one) are likely to be add-ons that require special review from an admin editor, or are awaiting a response from the developer. Try to focus on the first 10 or 15 add-ons in the queues. There should be a couple there that you can review.<br>
| |
| | |
| == Performing a Review<br> ==
| |
| | |
| Before getting started, here's an important legal note: reviews should not include checking for possible copyright or trademark violations and you should not take any action on an add-on because you suspect it copies code from others without permission or may otherwise infringe someone else's copyright or trademark. The DMCA is a law that gives us legal protection from being held responsible for copyright infringement by users who post content to our site, but only if our conduct qualifies us for such protection and we follow exactly the procedures laid out in the DMCA. Determining copyright or trademark infringement is complicated and you will not have enough information to make those determinations. We have a robust DMCA process where copyright or trademark holders can contact Mozilla legal to address any potential problems. More about this in a section below.
| |
| | |
| '''Editor Tour:''' Select a review in the queue and click on its link. This will take you to the add-on review page.<br>
| |
| | |
| === Possible resolutions<br> ===
| |
| | |
| The grey box near the top of the review page shows the possible resolutions for a review.<br>
| |
| | |
| [[Image:Resolutions.png|center|Possible review resolutions]]
| |
| | |
| Clicking on any of them will open a form below it.
| |
| | |
| The comments textbox is where you should write all of your review notes. There's also a canned response list below it, that contains useful reusable snippets of text for your notes. Selecting any of them will just add the text to the textbox. You can use as many as you need. Take some time to familiarize yourself with the canned response list. It can give you a good idea of the issues we frequently encounter and what we have to say about it.<br>
| |
| | |
| You should normally use one of the first three resolutions:
| |
| | |
| *Push to Public: the version doesn't have any problems and it's OK for the public.
| |
| * Grant Preliminary Review: the version has problems that prevent us from making it fully public, but it's still safe to use.
| |
| *Reject: the version has security issues and must be rejected.
| |
| | |
| The other two are for special situations:
| |
| | |
| *Request More Information: this allows you to ask the author for information necessary to perform the review, without removing the add-on from the queue. A very common case for this is when the add-on is site-specific and the author didn't provide a test account. Since we can't be registering to every site on the web just to test, we always require authors to provide the necessary information. Replies from the author will appear in the review page.
| |
| *Request Super-Review: this is for very special cases where you think an admin editor should review this add-on. It can be because you think there's some malicious intent from the author; in this case you should notify the mailing list about it. It can also be because you are aware Mozilla has received a DMCA notice about this add-on. It is critical that you do '''not''' try to resolve this issue yourself. Copyright complaints should be escalated to admins and, at most, you can point the person making the complaint to the Digital Millennium Copyright Act Notice section of our [http://www.mozilla.com/en-US/about/legal.html legal notices page] for an explanation of the protocol to follow. The add-on will remain in the queue, and in this case the comments aren't sent to the author. They're only readable from the review page.
| |
| | |
| While you perform your review, you should be keeping notes of everything you noticed about the add-on: validator flags, errors in the error console, bugs, areas for improvement, etc. You can either use a note-taking program, or just type your notes in the comments textbox of the review page. Changing the resolution won't clear your notes. Just make sure you selected the right one when you're ready to submit!<br>
| |
| | |
| '''Editor Tour:''' an admin editor should have given you a specific add-on to review. Also, don't submit your first review without admin approval!<br>
| |
| | |
| === Step 1: Review Add-on Info<br> ===
| |
| | |
| The review page has most of the information you need to review an add-on.<br>
| |
| | |
| ==== Metadata ====
| |
| | |
| The top section displays the add-on name, version, authors, categories and compatibility information. <br>
| |
| | |
| *If the name is missing, it's very likely that the author assigned a name to the add-on in a locale different from English and forgot to include an English translation. In this case, an information request should be sent to the author to correct this. If there are other reasons to reject the add-on, the name issue should be included as one of the things to fix.<br>
| |
| *Make sure the compatiblity information is up to date. Add-ons should be compatible with the latest stable versions of the applications they support, and around the time of a new major version release it's worthwhile to give a note to authors about updating compatibility.<br>
| |
| | |
| ==== Descriptions ====
| |
| | |
| The middle section includes lots of information that you should always read:<br>
| |
| | |
| *Nomination message: this is the message written by the author when the add-on was first nominated. It can contain important information about testing or source code availability.<br>
| |
| *Notes to reviewer: also a note from the author to AMO Editors, but it can change for each version. It often contains information about the changes in this version.<br>
| |
| *Summary, description, developer notes: the same data that appears in the public add-on page. This information should clearly say what the add-on does and how to use it. Most add-ons should be easily usable with no additional explanation, but others may require it, and this is where everything should be explained.<br>
| |
| *Version notes: this is very important when reviewing updates. It'll help you understand what the code changes are supposed to do.
| |
| *Privacy Policy and EULA: they most clearly state what the add-on does with private user data or any usage restrictions it may have.
| |
| | |
| This should all give you a good picture of what the add-on does.
| |
| | |
| ==== Add-on Relevance ====
| |
| | |
| For nominations, at this point you should ask yourself if the add-on is relevant enough to be public on AMO. We have a very low admission threshold, but we don't want to publish add-ons that will get at most a couple hundred downloads and will be mostly ignored during their lifetime. In that case, the add-on should at best receive a preliminary review approval.
| |
| | |
| Add-ons that commonly fall into this category are:
| |
| | |
| *Add-ons that help access very specific webpages. For example, an add-on that makes it easier to use an internal business site or a university library.
| |
| *Add-ons targeted to a limited geographical area. For example, an add-on for a local city newspaper or radio station.
| |
| *Add-ons that add a toolbar or menu filled with website links.
| |
| *Add-ons that download video files from Flash video sites like YouTube. We already have too many add-ons like these, with little to no distinction between them. Unless the add-on provides something really innovative to video downloading, it should be rejected.
| |
| | |
| Assessing the relevance of an add-on can be difficult. In these cases you should go to the add-on page and see how many reviews and downloads the add-on has. The add-on page is accessible using the Item Overview link near the top of the review page.
| |
| | |
| [[Image:Item-overview.png|center|Item overview link]]
| |
| | |
| Small numbers (less than a couple hundred downloads, few or no reviews) should be an indication that the add-on should be rejected. When in doubt, leave the review to somebody else or ask in the mailing list.
| |
| | |
| ==== Review History and Editor Comments ====
| |
| | |
| Below the descriptions you should see the add-on version review history. The review history contains all previous reviews peformed to the add-on, including the editor notes for each one of them.
| |
| | |
| This is very valuable information, and you should always read at least the most recent reviews. If the last review was a rejection, you should verify that all issues noted in the previous review were addressed. If an editor made a request for information that hasn't been answered yet, you should not review that add-on. Most add-ons in the nomination queue will have this section empty because they haven't been reviewed before.
| |
| | |
| Editor comments are rarely used, but if there are any, you should read them. They are only used for internal communication between editors. You can add notes about this review, like if the source viewer or diff tool didn't work for you, but you can use the mailing list for that as well.
| |
| | |
| === Step 2: Automatic validation ===
| |
| | |
| We have a extensive set of static tests that identify common bad practices and possible security problems with add-on code. You must always run the code validator and inspect the results when performing a review.<br>
| |
| | |
| The code validation link is located near the bottom of the review page, between the add-on descriptions and the review history.<br>
| |
| | |
| [[Image:Validation-link.png|center|Add-on validation link]]<br>
| |
| | |
| Clicking on the link will take you to the validation page, where the automatic code validator will be run on the latest version of the add-on and then the results will be displayed.<br>
| |
| | |
| '''Note:''' The validator can sometimes hang and require a refresh for the results to appear. If the results never appear, even after multiple tries, it's likely that the add-on package is too large (too many files) and a memory error is occurring. In those cases it's better to leave the add-on to an admin.<br>
| |
| | |
| The [https://addons.mozilla.org/en-US/firefox/pages/validation Validator Help Page] explains in detail what every possible warning means and how serious each is.
| |
| | |
| ==== Result interpretation ====
| |
| | |
| An add-on version should be rejected if any of the following appear:
| |
| | |
| *The add-on is malformed or missing. In this case an error will appear and the validation won't run, or the validation page will say "File not found".<br>
| |
| *''eval'' or ''Function()'' usage. The ''eval'' function must not be used to parse remote JavaScript code, or even JSON code.
| |
| *Unprotected ''browser'' or ''iframe'' nodes in XUL documents.<br>
| |
| *Remote script insertion. Add-ons should not load remote code since it hasn't been reviewed, and inserting remote code in websites can lead to breakage and security problems.<br>
| |
| | |
| If an add-on is flagged as a Conduit toolbar, you should first check the add-on authors. If the add-on has the [https://addons.mozilla.org/en-US/firefox/user/4959120 CONDUIT-AMO] account as an author, it should be flagged for admin review. If it doesn't, it should be rejected and the author told to contact Conduit about listing the add-on on AMO.<br>
| |
| | |
| If the add-on contains binary, obfuscated or minified code that is very hard or impossible to read, the author will need to send the readable source code to an admin. If the author provides the source code packaged with the add-on, or through a link in the descriptions, the add-on should be flagged for admin review. Otherwise, the add-on should be rejected, with a request for the source code.
| |
| | |
| An add-on version should only receive receive a preliminary review if any of the following appear:
| |
| | |
| * Expanding on the eval usage, we strongly discourage its use in any of its forms. This blog post is a good reference on the dangers of ''eval'' and alternatives for it: [http://adblockplus.org/blog/five-wrong-reasons-to-use-eval-in-an-extension Five wrong reasons to use eval() in an extension].
| |
| **One case that is generally accepted is when ''eval'' is used to replace existing Firefox functions. This is very common for add-ons that change bookmarking or tabbing behavior. <br>
| |
| **It's also allowed in known libraries like jQuery.<br>
| |
| *Library checksum errors. This happens when the add-on includes a modified file from a known JS library. The library files should be unchanged, to avoid having to review all of their code. The filename shouldn't be changed by the author either, and this should be checked during code review.
| |
| *Using the ''codebase_principal_support'' preference and ''enablePrivilege'' function. If you're reviewing an update '''and''' this code wasn't inserted as part of the update '''and''' no editor has noted this before, the update can be allowed with the condition that the code needs to be removed on the next update. In all other cases the add-on version should be rejected.<br>
| |
| *Native object prototype extension. This can lead to serious compatibility problems. We normally don't allow the Prototype library in add-ons because of this.
| |
| *Changing preferences related to the automatic update service. Changing any application preferences in the default preferences file should normally be enough for a rejection as well.
| |
| *Storing passwords in the preferences.
| |
| | |
| Most of the other flags are not that important, but should still be fully read and understood. When in doubt, check the help page or ask in the mailing list.
| |
| | |
| === 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.<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>
| |
| | |
| [[Image:View-contents-links.png|center|View contents links]]For updates, the compare link should be used. It can sometimes fail to work for very large add-ons. In that case, you can either review the full source code, or download the new and public files to your system and compare them using tools like [http://code.google.com/p/amo-editor-tools/source/browse/trunk/sh/xpidiff.sh xpidiff.sh] or [https://wiki.mozilla.org/AMO:Editors/WinMerge WinMerge].<br>
| |
| | |
| [[Image:Code-browser.png|center|Add-on code browser]]The box on the top left allows you to browse through the add-on code. You can drag this box from the title bar to any part of the page. Folders and JAR files appear highlighted in bold, and can be expanded by clicking on them. If a JAR file fails to expand, please notify it on the mailing list.<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>
| |
| | |
| *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>
| |
| *Default add-on preferences should use the "extensions." prefix, and should also have a reasonable namespace. Application preferences should not be changed in the default preferences file.<br>
| |
| *Remote code downloading or execution. Add-ons should not download remote code in any way. They are free to interact with web APIs as long as all that is being transmitted is data, not code. Add-ons are allowed to insert local scripts into webpages (within reason), but are not allowed to insert remote scripts.<br>
| |
| *Privacy issues. An add-on can claim to work with a popular website like Twitter, but then send the user data through some other site, most likely owned by the developer. There needs to be a justified reason to handle user data in this manner, and the privacy policy and add-on descriptions need to be very clear about this. Passwords should never be handled in this way, and they should only be transmitted directly to the original API provider (for example, Twitter).<br>
| |
| *Custom updaters. Add-ons are not allowed to have custom update mechanisms or tamper with the update mechanism in the application. Any code that indicates the add-on is fetching or installing updates should be rejected.<br>
| |
| *Possible performance problems. Running inefficient code during important events like startup or page load should be discouraged, but it's not a reason for rejection, unless the problem is serious or it has been noted to the author before.<br>
| |
| *Using [https://developer.mozilla.org/en/XUL/prefwindow prefwindow]. If an add-on has a preferences window, we recommend that authors do 2 things: 1) use the ''prefwindow'' element, 2) add the line in install.rdf that enables this window to be opened from the Add-ons Manager window. It's not mandatory to do this, but it's always worth noting to the author.<br>
| |
| *Binary, obfuscated and minified code. Some of it may not appear in the validator, so keep an eye for it. Known libraries that pass the checksum should be no concern. In the case of updates, if none of the binary, obfuscated or minified files have been updated and were previously approved, it's OK to approve if everything else passes review.<br>
| |
| *In general you should apply your judgement and try to identify code that may appear suspicious or out of place. Try to understand what everything does and how it all fits together.<br>
| |
| | |
| By the end of the code review, you may have a series of notes that will require the add-on to be significantly rewritten. If you think that's the case, it's OK to reject the add-on without proceeding with the next step.<br>
| |
| | |
| === Step 4: Feature Review<br> ===
| |
| | |
| The last step in a review is to install and test the add-on.
| |
| | |
| ==== Testing setup ====
| |
| | |
| These are a few settings and tools you should use or consider using when setting up your add-on testing environment:
| |
| | |
| *'''Always use a separate profile for testing''', never your main profile. See [https://developer.mozilla.org/en/Setting_up_extension_development_environment Setting up an extension development environment].
| |
| *Ideally you should perform your tests in a virtual machine. It is always useful in case you need to test in multiple platforms. [http://www.virtualbox.org/ VirtualBox] is free and supports most platforms.
| |
| *Enable full Error Console reporting, as described in [https://developer.mozilla.org/en/Setting_up_extension_development_environment#Development_preferences the Development Preferences section].
| |
| *These add-ons help you inspect add-on behavior and find possible solutions for problems:
| |
| **[https://addons.mozilla.org/en-US/firefox/addon/6622 DOM Inspector]. Analyze XUL and HTML layout, CSS and even JS objects.
| |
| **[https://addons.mozilla.org/en-US/firefox/addon/1815 Console<sup>2</sup>] for detailed Console logging.
| |
| **[https://addons.mozilla.org/en-US/firefox/addon/966 Tamper Data] or [https://addons.mozilla.org/en-US/firefox/addon/3829 Live HTTP Headers] for HTTP traffic analysis.
| |
| **[https://addons.mozilla.org/en-US/firefox/addon/2490 Leak Monitor] to detect some types of memory leak. See [https://wiki.mozilla.org/MozillaQualityAssurance:Home_Page:Firefox_3.0_TestPlan:Leaks:LeakTesting-How-To#Leak_Gauge Leak Gauge] for a more general solution.
| |
| *To test Fennec add-ons, you'll need to [https://wiki.mozilla.org/Fennec#Test_Builds install Fennec]. It is preferred that you test in a supported mobile device. If a Fennec nomination has been waiting for long, it's OK to test with the desktop XULRunner application.
| |
| *For online malware scanning, you can use [http://www.virustotal.com/ Virus Total], [http://www.kaspersky.com/scanforvirus Kaspersky online scan] and [http://virusscan.jotti.org Jotti online scan]. AMO performs virus checks, and binary add-ons should be admin-reviewed anyway, but if you suspect anything, those are good places to use.<br>
| |
| | |
| ==== Installing and testing<br> ====
| |
| | |
| Add-ons are normally cross-platform, so there will only be a single file to review, linked with the name ''ALL'' (as in all platforms). If the add-on is offered for a limited number of platforms, there will be individual links for each one of them. In this case all supported platforms should be tested.<br>
| |
| | |
| Regarding applications, you don't need to test the add-on for all applications it supports. If the add-on supports Firefox and others, it's OK to only test on Firefox. If, however, an add-on update introduces Fennec or other application support, the add-on should be tested on it. The applications we support for reviews are listed on [https://addons.mozilla.org/en-US/firefox/pages/appversions this page].
| |
| | |
| The link points directly to the add-on file.<br>
| |
| | |
| [[Image:Install-link.png|center|Install link]]And these are the things that you should always check for:<br>
| |
| | |
| *Keep the Error Console open at all times. JS errors in chrome scripts are unnaceptable. If you see any errors, make sure to copy the full error and include it in the message to the author, with indications on how to reproduce, when possible.<br>
| |
| *The startup experience is very important. Many add-ons open dialogs or new tabs at startup, mostly offering information on getting started. This is useful, but it shouldn't block the user from using the browser. Opening modal (blocking) dialogs at startup is not allowed. Non-modal dialogs, separate windows or new tabs are allowed.<br>
| |
| ** One important thing to keep in mind is that most startup flows happen only once, and add-ons set preferences not to run them again. Since many add-ons leave their preferences after uninstall, you need to clean up your testing profile every now and then to get rid of lingering prefs and test all add-ons like it's the first time.
| |
| *If the add-on is difficult to use, there should be instructions included in the add-on descriptions, or in a startup page or window. If the add-on is too difficult or impossible to figure out, it should be rejected.<br>
| |
| *The [https://addons.mozilla.org/en-US/developers/docs/policies/reviews#section-defaults No Surprises policy] must be respected at all times. Changes to the homepage, default search engine, or other default settings without user consent is strictly forbidden. Whenever that decision is presented to the user, the default should always be not to perform any changes.<br>
| |
| *Visit HTTPS sites like addons.mozilla.org and make sure the identity button is unchanged. This is specially important for add-on that insert scripts into sites. If site identity is broken, the add-on must be rejected.<br>
| |
| | |
| [[Image:Identity-button.png|center|Identity button]]
| |
| | |
| *Visit a very simple website like example.org and inspect its DOM, looking for any changes. Again, this is particularly important for extensions that insert scripts or make DOM changes.<br>
| |
| *Open the add-on's preferences window, from the Add-ons Manager and elsewhere, and verify that preference changes apply properly. Make sure the window fits all of its contents (a common problem in Mac OS).<br>
| |
| *Test all add-on features, within reason. If there too many, focus on the main features.<br>
| |
| *If the add-on adds toolbars or toolbar buttons, try removing them using the toolbar customization dialog and then restarting Firefox. If they reappear, the add-on should be rejected for not respecting user choice.<br>
| |
| *Affiliate linking. Some add-ons add affiliate codes to Amazon links (or similar) in order to make money. At the moment we allow this as long as (1) the add-on follows the No Surprises policy, (2) the feature doesn't replace or remove any existing affiliate codes, (3) the affiliate codes aren't inserted in the merchant website's links (inserting Amazon affiliate codes in Amazon.com pages).<br>
| |
| *If the add-on requires third-party software to be installed in order to function, flag it for admin review.
| |
| *[[AMO:Editors/InfoEditors#Geolocation|Geolocation policies]], when applicable.<br>
| |
| | |
| === Step 5: Resolution<br> ===
| |
| | |
| Choose the appropriate resolution and include all of your notes. Make sure you use a corteous and professional tone, and be as helpful as you can when pointing out problems or areas for improvement. If you are pushing the add-on public, thank the author for the time and effort they have put in. Once you're ready, click the ''Process Action'' button.<br>
| |
| | |
| The submission can fail for a number of reasons, so it is highly recommended that you select all of your notes and copy them before you click on the submit button. You can also use add-ons like [https://addons.mozilla.org/en-US/firefox/addon/5761 Textarea Cache] for this.
| |
| | |
| '''Editor tour:''' remember to ask the admin editor to review your response before sending it.
| |
| | |
| [[AMO:Editors/EditorGuide/MailingList|Next: The Mailing List]] | |