DOM/WebIDL Review Checklist: Difference between revisions

From MozillaWiki
< DOM
Jump to navigation Jump to search
No edit summary
(Expand with a bunch of stuff we've discovered through experience. Remove some b2g-specific bits.)
Line 1: Line 1:
A review checklist for DOM Peers while doing WebIDL reviews. Not yet complete.
A review checklist for DOM Peers while doing WebIDL reviews. This is not exhaustive; things will be added as needed.
 
== IDL follows spec ==
 
* The IDL has a link to the relevant specification.  If there isn't one, the API must not be exposed to the web in general.
* The IDL matches the IDL in the spec.
** Double-check that our use of nullable sigils (<code>?</code>) matches the spec.
** Double-check that our <code>Exposed</code> values match the spec.
 
== Spec makes sense ==


* Assuming the API is exposed to the Web (so especially non-b2g-only APIs), make sure the API follows the spec.
* The API should be easy to understand and use, and consistent.
* The API should be easy to understand and use, and consistent.
* Whether things are nullable or not needs to make sense.
* Whether arguments and dictionary members are optional or not needs to make sense.
* <code>Exposed</code> values should make sense.
* The spec should almost certainly not be using the term "active document".  If it is, think extra carefully about what happens when that document is not same-origin with any of the other things the spec is talking about.
== Implementation is correct ==
* Check that the implementation follows the spec, as much as is possible without domain expertise.
* All functions with non-nullable return values must never return null.
* All functions that accept nullable or optional values (as arguments, dictionary members, etc) must properly handle the null or "not present" cases.
* Exceptions thrown from functions should generally be specified.  If there are non-specified exceptions being thrown, evaluate whether it's because Gecko has a state web specs assume can't arise (e.g. window without a document) or whether it's because the spec missed a case it should be handling.
* If the API has <code>[Exposed=SomethingOtherThanWindow]</code>:
* If the API has <code>[Exposed=SomethingOtherThanWindow]</code>:
** Audit the API implementation to make sure it doesn't assume the existence of a Window or a Document
** Audit the API implementation to make sure it doesn't assume the existence of a Window or a Document
** Require basic test coverage for each of the contexts in which the API is exposed.
** Require basic test coverage for each of the contexts in which the API is exposed.
* Use of <code>[CheckPermissions]</code> and <code>[AvailableIn]</code> should be preferred to doing permission checks or checks for privileged or certified apps in the implementation.
* Implementation should use the argument types described in [https://developer.mozilla.org/en/Mozilla/WebIDL_bindings our binding layer documentation], not whatever internal types bindings are using (e.g. no types in the <code>binding::detail</code> namespace, no <code>NonNull<T></code> arguments where <code>T&</code> would work, etc).
* If an API is only intended for privileged or certified apps, it should use <code>[AvailableIn]</code>, even if it is also controlled by a permission (in which case it requires a <code>[CheckPermissions]</code> in addition to <code>[AvailableIn]</code>).
* Check that the <code>GetParentObject()</code> methods of various objects, combined with how those objects get constructed, associate them with the correct global.
* Please take extra care reviewing things that are hidden behind <code>[Pref]</code>s.  If an API only has a <code>[Pref]</code> annotation, and it's supposed to be something only available to a subset of Firefox OS apps, they are probably doing things wrong, because <code>[Pref]</code> only gives us platform level granularity.  IOW, when they turn on the pref on b2g, the API will end up being exposed to all content there, which is usually not what we want.  Currently the only legitimate use case for only using <code>[Pref] without either <code>[CheckPermissions]</code> or <code>[AvailableIn]</code> is for APIs that are supposed to be exposed to all Web content but we're not turning them on yet for spec or implementation stability reasons.
 
* Consider whether APIs are acceptable for prerendering.  See https://wiki.mozilla.org/Gecko:Prerendering/API_Review_Guidelines for more information.
== Further reading ==
* Please refer to the [http://heycam.github.io/webidl/ WebIDL spec] or the [https://developer.mozilla.org/en/Mozilla/WebIDL_bindings documentation for our binding layer] in order to double check your understanding of the IDL as needed.
 
Please refer to the [http://heycam.github.io/webidl/ WebIDL spec] or the [https://developer.mozilla.org/en/Mozilla/WebIDL_bindings documentation for our binding layer] in order to double check your understanding of the IDL as needed.


[[Category:Web APIs]]
[[Category:Web APIs]]

Revision as of 15:58, 3 April 2017

A review checklist for DOM Peers while doing WebIDL reviews. This is not exhaustive; things will be added as needed.

IDL follows spec

  • The IDL has a link to the relevant specification. If there isn't one, the API must not be exposed to the web in general.
  • The IDL matches the IDL in the spec.
    • Double-check that our use of nullable sigils (?) matches the spec.
    • Double-check that our Exposed values match the spec.

Spec makes sense

  • The API should be easy to understand and use, and consistent.
  • Whether things are nullable or not needs to make sense.
  • Whether arguments and dictionary members are optional or not needs to make sense.
  • Exposed values should make sense.
  • The spec should almost certainly not be using the term "active document". If it is, think extra carefully about what happens when that document is not same-origin with any of the other things the spec is talking about.

Implementation is correct

  • Check that the implementation follows the spec, as much as is possible without domain expertise.
  • All functions with non-nullable return values must never return null.
  • All functions that accept nullable or optional values (as arguments, dictionary members, etc) must properly handle the null or "not present" cases.
  • Exceptions thrown from functions should generally be specified. If there are non-specified exceptions being thrown, evaluate whether it's because Gecko has a state web specs assume can't arise (e.g. window without a document) or whether it's because the spec missed a case it should be handling.
  • If the API has [Exposed=SomethingOtherThanWindow]:
    • Audit the API implementation to make sure it doesn't assume the existence of a Window or a Document
    • Require basic test coverage for each of the contexts in which the API is exposed.
  • Implementation should use the argument types described in our binding layer documentation, not whatever internal types bindings are using (e.g. no types in the binding::detail namespace, no NonNull<T> arguments where T& would work, etc).
  • Check that the GetParentObject() methods of various objects, combined with how those objects get constructed, associate them with the correct global.

Further reading

Please refer to the WebIDL spec or the documentation for our binding layer in order to double check your understanding of the IDL as needed.

Retrieved from "https://wiki.allizom.org/index.php?title=DOM/WebIDL_Review_Checklist&oldid=1167393"