API Change Request Review Process

From Tizen Wiki
Jump to: navigation, search

Overall Process

  1. A submitter submits an API change request (ACR) to tsg-arch-api@lists.tizen.org list.
  2. Designated ACR reviewers give comments on the ACR on the mailing list during review.
    If they agree on the ACR, add "Reviewed-by: reviewer name <reviewer's e-mail>" before ACR body.
  3. If the submitter gets "Reviewed-by" tag from all designated ACR reviewers, he/she can upload it to Tizen APIs JIRA.
    1. An ACR that does not get any comments from anyone of ACR reviewers for two weeks is considered to be implicitly rejected.
  4. Post activities of each ACR such as TC implementation, TC test results, related documents (developer's guide, API reference, tutorial and etc) will be tracked on Tizen APIs JIRA.
In JIRA workflows,
  • Reviewing: Means the submitter created an ACR and/or designated ACR reviewer is reviewing the ACR.
  • Approved: Means the ACR reviewer has approved the ACR.
  • Rejected: Means the ACR reviewer has rejected the ACR for some reason.
    Most likely this is rejected because the submitter didn't go through the ACR review process on the tsg-arch-api@lists.tizen.org.
  • Resolved: The submitter and co-workers have completed all the post activities, such as implementation of APIs, implementation of TCs, making all TC passed, and written related documents.
  • Closed: Means that QA team (or release engineer or someone else) has verified whether all the post activities have been done.

Designated ACR Reviewers

  • Designated ACR reviewers for Native APIs are:
    • Yoonsoo Kim<yesarang DOT kim AT gmail DOT com>, Hyunjin Choi
  • Designated ACR reviewers for Web Device APIs are:
    • Hobum Kwon, Sakari Poussa, Hyunjin Choi, Michael Leibowitz

Terminologies

  • ACR: API Change Request
  • N/P: Non-Privileged
  • RC: Release Candidate
  • TC: Test Case
  • DG: Developer Guide
  • ABC: Application Binary Compatibility

Details about Formatting the Email

The title of the email should be like the following:

  • Native API: [Native][DueDateOfAcrReview][Add|Modify|Remove|Deprecate|Versioning|Document] Brief summary of ACR
  • Web Device API: [WebDevice][DueDateOfAcrReivew][Add|Modify|Remove|Deprecate|Versioning|Document] Brief summary of ACR

On JIRA, "Brief summary of ACR" is sufficient for the title of the issue.
All ACR should be sent as plain text (no HTML).
All ACR should follow the following format.

ACR Format in E-mail

1. Target SDK Version
Format: x.y RCz
x, y, and z are digit(s)
Example: 3.0 RC1
You can get this information from release engineering team or JIRA for releases.

2. Privilege
Format: privilege name & privilege level
- Privilege name could be N/P for non-privileged APIs or URL style full privilege name for privileged APIs
- Privilege Level could be Public / Partner / Platform. If the submitter creates a new privilege, please explain why it is necessary.
Refer to the Security Architecture for detailed explanation on privileges.

3. Owner
Native API maintainer : maintainer name
Web API maintainer : maintainer name
Core framework maintainer : maintainer name

4. ACR Summary
If a new feature is requested for the first time, please mention its name.
Explain why the API change is necessary in detail. If the change comes from an officially planned feature (Tizen Features JIRA), you can just refer to it.
If this ACR is to fix a bug:
Specify Jira ID, bug detection date, related test case name, file path and etc.
If there is a discussion mail thread on some public mailing list inside Tizen, please link to it here.

5. Backward compatibility break
If the change breaks binary compatibility, behavioral compatibility, and/or source-level compatibility, specify the reason why the break is necessary precisely, and show the AS-IS Code and TO-BE code for comparison.

6. Description of ACR
Submitter must link to the specific patch for interface definition files in public gerrit review system. The patch should have the following information:

  • Function/class/interface signature (Be careful of typos, const (readonly), and pointer/reference)
  • Exceptions
  • API description
  • Method semantics and constraint
  • Input parameter range
  • Output value range
  • If there is pre-condition of the functions, specify it.

Please invite ACR reviewers(and/or maintainers) who are responsible for the git that you plan to add files to inside the gerrit review system.
If you can't specify some information in interface definition files, please describe such aspects here.

  • If the submitter adds a new interface:
    • we recommend it include an explanation for use cases and
    • a written interface-level description
  • If the submitter adds a new function (or method) because of deprecation, attach deprecation ACR approval or deprecated APIs in the same ACR.
  • If the submitter adds internal API, specify which module will use it.
    • Example 1: Most API will use it
    • Example 2: Tizen::Social will use it


7. ACR self-check result
Do ACR self check-list and specify if one does not follow the guide and the reason why not.

8. Test plan for ACR (in detail):

  • TC writer / executer / schedule / issue: This item should be created as a separate Tizen APIs JIRA item after ACR approval


9. Check-in Date

10. Document Update Date (Feature, ADS, DG, Tutorial and other corresponding docs)
All following fields are optional
Feature Jira link: link to Tizen Features JIRA item.
Design document link: link to design document(s) somewhere like wiki.tizen.org or git or Google Docs or Cloud storage
[DG link: Done / Date / N/A / : This item should be created as a separate Tizen APIs JIRA item after ACR approval]
[Tutorial link: Done / Date / N/A / : This item should be created as a separate Tizen APIs JIRA item after ACR approval and links to other docs.]

Detailed Guidelines on API Changes Including Deprecation, Removal, Versioning

When you want to change the existing APIs:

  • Specify the affected analysis results of applications that use previous APIs (functional and non-functional effects)
  • Specify the affected analysis results of platform inside. e.g., other module and API that has dependency on it.
  • In this case, peer review must be done with the affected module's owner and specify the result of the discussion (Issue/Discussion/Action item (w/R&R)/Schedule)
  • Core framework related changes
    • Specify the core framework owner and Tech Lead / core framework-related issue and discussion result
  • Compatibility test
    • Specify how to prepare a test case for the changed TCs when before and after behaviors are the same. Specify also schedule for the case.
  • API deprecation,
    • Specify deprecation reason
      • Examples: Defect fix, security issue, or low performance (in case of low performance, it must be serious one)
    • Suggestion of deprecation
      • Example: Add a new API
      • If you add a new API, consider the best API name (if you don't have, discuss through ACR review process).
      • If you add a new API, proceed separated ACR. In the ACR, refer to the deprecated ACR approval.
      • Example: Using existing APIs (or combination of it)
    • Give a usage guideline of Deprecated API (@remark)
  • Method Versioning:
    • Specify the frequency of the problem (one time, rare, frequent, or 100%)
    • Specify whether there is an app-level workaround.
    • If there is a workaround, specify whether it works well on up- version without 'code change'
    • Specify in which layer the actual versioning is and its reason.
    • If there are changes of registry, ini, data and protocol, please specify it too.

ACR Example for Native API

1. Target SDK Type and Version:
Tizen Public SDK / 3.0 RC1

2. Privilege
Privilege: N/P

3. Owner
OSP : John Kim(xxx@samsung.com)

4. ACR Summary
Official 3.0 feature. All the number classes(Int8, Integer, Integer8, Short & Long) supports Decode() API except LongLong class.
Decode() API decoded a string representing a numeric value to a signed long long value.

5. Backward compatibility break
It does not break application binary compatibility. I'm adding a new non-virtual function.

6. Description of ACR
https://review.tizen.org/gerrit/#/c/11319/

7. ACR self check-list.
All the class related check-list is followed.

8. Test plan for ACR
- Unit-TC. John Kim will write the Unit TCs and the task will be completed by 22/10/2013.

9. Check-in Date
-22/10/2013

10. Document Update Date
a. Feature Jira Link: N/A
b. Design document Link: N/A
c . DG: N/A
d. Tutorial : N/A

Reviewed-by tag

By offering Reviewed-by: tag, you state that:

           (a) I have carried out a technical review of this patch to
               evaluate its appropriateness and readiness for inclusion into
               Tizen native API(or Tizen Web Device API).
           (b) Any problems, concerns, or questions relating to the patch
               have been communicated back to the submitter.  I am satisfied
               with the submitter's response to my comments.
           (c) While there may be things that could be improved with this
               submission, I believe that it is, at this time, (1) a
               worthwhile modification to the Tizen native API(or Tizen Web Device API), and (2) free of known
               issues which would argue against its inclusion.
           (d) While I have reviewed the patch and believe it to be sound, I
               do not (unless explicitly stated elsewhere) make any
               warranties or guarantees that it will achieve its stated
               purpose or function properly in any given situation.

Native API ACR self checklist

Legend : [M] Mandatory, [R] Recommended
1. Class related

  • [M] Every public class MUST inherit from Tizen::Base::Object class unless it provides only static methods and will never provide instance methods later. Singleton and Interface classes are also exceptions.
  • [M] Every class MUST have “class extension pointer” (e.g., MyClassImpl*) for later extension without breaking ABC. (Note that the impl. pointer should be declared as placeholder even though there's no impl. class for now.)
  • [R] Every class SHOULD avoid having "I" as a prefix in its name since it may confuse developers with Tizen Interface classes.
  • [M] Every class MUST NOT inherit from multiple (non-interface) classes.
  • [M] Reserve "null-body" virtual methods with enough slots for later extension of the class/interface with "new" virtual methods without breaking ABC. (see Reserving dummy virtual methods for later extension without breaking ABC for more details.)
  • [M] Be careful to decide whether to make default constructor as private method or not.
  • [M] Provide a copy constructor and assignment operator only when necessary. If it’s not necessary, explicitly specify those as private.
  • [M] All public classes to be used by application SHOULD be exported through _OSP_EXPORT_ or similar compile macro.
  • [R] Do not define your class as template class unless there's a strong reason to do so.
  • [M] Wherever applicable, use class extension pointer (impl. class) to provide public methods to other classes for allowing access to your class's non-public members rather than declare other classes as 'friend' because this breaks the encapsulation (Make your impl. class to be your only friend class).
  • [M] Always override GetHashCode() when you override Equals(). If equality holds for two instances of same type, then GetHashCode() should return the same value. Note that the reverse does not always hold true.
  • [M] Think carefully whether to make your class as two-phase or one-phase: One-phase class cannot return an error in its construction, but gives the application simpler usage.

2. Interface related

  • [M] Every interface MUST have "I" as a prefix.
  • [M] Every interface MUST not have member variables.
  • [R] Every interface method SHOULD be pure virtual, except for the virtual destructor and reserved "null-body" virtual methods to prevent compile error for existing application in its re-compiling.
  • [M] Every root interface SHOULD declare "virtual" destructor explicitly.

3. Listener related

  • [M] Every listener interface MUST inherit from Tizen::Base::Runtime::IEventListener to explicitly declare the interface is following Tizen listener idiom and guideline.
  • [R] Every listener should have postfix of "EventListener" in its name, if its purpose is to listen to "external" event.
  • [R] Every listener should have postfix of "ResponseListener" in its name, if its purpose is to get callback for the corresponding "async" API.
  • [R] If listener's purpose is both "event" listening and getting "callback", figure out which type is dominant, and follow one of the above-mentioned naming rules.
  • [R] Every listener method corresponding to async. API SHOULD have "result" type input for carrying exceptions.
  • [R] Listener method name SHOULD include the "subject" part to differentiate it from other similar method in other listener.
    • E.g., IWifiManagerEventListener::OnWifiActivated() rather than OnActivated().
  • [R] AddListener/RemoveListener SHOULD return result instead of void (especially for RemoveListener, there is a case where app does not give correct listener pointer)
  • [M] AddListener vs. SetListener: The method name should be AddXxxListener(), but if and only if a class accepts only one listener, the proper method name is SetXxxListener().
  • [M] If your class requires app to set a listener as per your class's intention, specify the listener as an argument of class's Construct() methods (For example, PushManager::Construct (const IPushManagerListener&, const IPushEventListener&).
  • [R] For input arguments of listener methods, do not specify "const" unless there is a strong reason to do otherwise. Specifying input argument as "const" enforces application to cast the input argument to non-const type to invoke non-const methods in its callback implementation.
  • [R] Understand that there are two different types of interfaces for inversion of control: Listener interfaces for observer pattern, and Non-listener style interfaces for strategy pattern whose typical names are such as XxxProvider, XxxTranslator, and so on, whose method does not need to be like "OnXxx()". (E.g.,virtual int IListViewItemProvider::GetItemCoung(void), virtual bool IDataBindingDataValidator::ValidateDataToSource (...))

4. Method related

  • [M] Explicitly specify "void" for the API’s with no argument.
  • [M] An input argument forbidding NULL as its value MUST be a reference type instead of pointer.
  • [M] An input argument allowing NULL as its value MUST be a pointer type instead of reference.
  • [M] Make sure you properly specify "const"ness for every method. (const method, const argument, and const return type)
  • [R] Do not specify "const" for the method arguments and return type which are built-in type, enum type, and value object.
  • [M] Make sure you put "N" postfix for the methods which newly create an instance and return it to app with ownership transfer, which also applies to listener methods.
  • [R] Whenever applicable, prefer providing a set of distinct methods for each functionality to making a "big" method with multiple functionalities whose behavior is determined by the actual values of enum or String-type input argument. Making a feature-combined method make it very difficult to manage or analyze each functionality with different policy, like specifying @since, @deprecated, @compatibility, privilege, etc.
  • [R] Do not hesitate to make a looooong method name if its name well exposes the purpose of the method. Note that Tizen IDE supports auto-completion feature.
  • [R] Be careful that overloaded methods do not have same number of arguments, which can make compiler very confusing which method to invoke for some combination of specific input values, especially NULL/0 which can be implicitly type-casted to virtually all type Overloaded methods with different "output" arguments is an exception to this rule, since app does not give a value to these arguments.
  • [M] Every Tizen public methods should return "result" type exception starting with E_ unless there's strong reason to do otherwise.
  • [R] In defining types of input arguments of a method, specify their types to be concrete enough to filter out instance of unwanted type, generic enough to accept instances of permitted types.
    • e.g., IListItemEventListener::OnListItemChanged (Tizen::Ui::ListItem& listItem) is preferred to OnListItemChanged (Tizen::Ui::Control& listItem), if actual type of listItem is guaranteed to be of ListItem always.

5. Enum type and named constants related

  • [R] Before making namespace-specific enum types/constants, think twice whether this should be namespace-specific or common type which can be more properly defined in the basic & common namespace like Tizen::Base, for example.
  • [R] Every enum field MUST have enum type name as its prefix which can be abbreviated if too long, to prevent it from colliding with other enum types and to promote readability.

6. ABC related

  • [M] DO NOT assume that changing/removing your platform code will not affect the behavior of existing applications.
  • [M] DO NOT change order of virtual methods of classes/interfaces in header file, which have been already public and in use by 3rd-party application, which breaks ABC.

7. Doxygen tag related

  • [M] Make sure you put @since tag for every class/method/enum-type/enum-field/constant for the API’s which are newly introduced from specific Tizen SDK version.
  • [M] Make sure you specify appropriate privilege level and privilege for the API’s with privilege-based access control.
  • [M] Every public class/method/enum-type/enum-field/constant you want to specify compatibility between different bada API versions should have a @compatibility tag.
  • [M] Every public class/method/enum-type/enum-field/constant you want to deprecate should have a @deprecate tag.
  • [M] Always refer to the latest guide for doxygen comments managed by Technical Content team(Erin Choi).

8. English related

  • [M] Make sure there is no typo in the Open API's name or incorrect grammar in all sentences in the doxygen description

Web Device API ACR self checklist

TODO: write down the self checklist

Native API ABC (Application Binary Compatibility) Guide

  • Summary: YES means It breaks ABC
Classification Case ABC break
library remove an exported class YES
add new classes No
export a class that was not previously exported No
class change the size of enum YES
change the signature of an exported method YES
add new argument(s) to template class YES
delete argument(s) of the template class YES
change the access rights to the class YES
change size of an exported class YES
change size of a base class of an exported class YES
change the value of exception or error code YES
remove private static member if they are not called by any inline functions No
rename reserved member types No
member data add new static data members No
change the size of a member data of an exported class YES
change the type of a member data of an exported class YES
change the offset of a member data of an exported class YES
change the access rights of a member data of an exported class YES
delete member data from an exported class YES
all member functions inline existing function YES
change return type of a function YES
change the value of default argument(s) of a function No
add new default value of argument(s) of a function YES
add new argument(s) to the function YES
delete argument(s) of the function YES
change argument(s) type of the function YES
change the access rights of a member function only if the function was used internally No
changing the const/volatile qualifiers of the function YES
changing the const qualifiers of non-pointer primitive argument(s) for the function No
changing the const qualifiers of pointer/class argument(s) for the function YES
virtual member functions change virtual member function name No
change the order of virtual member functions YES
add new virtual function to an exported class YES
delete virtual function from an exported class YES
Non virtual functions add new non-virtual functions No
change non-virtual member function name YES
remove private non-virtual functions if they are not called by any inline function No
  • Introduction
    • Objective
      • This wiki page shall describe the guidelines with Dos and Don'ts for maintaining Application Binary Compatibility(ABC)
    • Background
      • Source Compatibility
        • Source is source compatible if a source compiles and runs on a system which is intended for other system
      • Binary Compatibility
        • Backward Binary Compatibility
          • Binary is backward binary compatible if a binary provides all the functionality of older version of binary
        • Forward Binary Compatibility
          • Binary is forward binary compatible if a binary can accept the data intended for a newer version
    • Policy
      • Our ABC policy is for Backward Binary Compatibility (BBC), term BBC used in relationship between two versions of a binary. Backward compatible binary allows applications which are developed using old binary to run using new binary without any change and recompilation
  • Dos & Don'ts: This section summarizes things to be taken care while modifying or updating source code of already released binary
    • Dos
      • add new classes
      • add new non-virtual functions
      • remove private non-virtual functions if they are not called by any inline functions
      • remove private static member if they are not called by any inline functions
      • add new static data members
      • export a class that was not previously exported
      • rename reserved member types
      • change the value of default argument(s) of a method
      • change the access rights of a member function only if the function was used internally
    • Don'ts
      • change the signature of an exported method
      • change the order of virtual member functions
      • add new virtual function to an exported class
      • delete virtual function from an exported class
      • change the size of enum
      • add new argument(s) to the virtual function
      • delete argument(s) of the virtual function
      • change argument(s) type of the virtual function
      • change non-virtual member function name
      • add new argument(s) to the non-virtual function
      • delete argument(s) of the non-virtual function
      • change argument(s) of the non-virtual function
      • change non-virtual member function name
      • add new argument(s) to the non-virtual function
      • delete argument(s) of the non-virtual function
      • change argument(s) type of the non-virtual function
      • remove an exported class
      • inline existing functions
      • change return type of a function
      • add new argument(s) to template class
      • delete argument(s) of the template class
      • change the access rights to the class
      • change the access rights of a member variable of an exported class
      • change the offset of a member variable of an exported class
      • change the type of a member variable of an exported class
      • change the size of a member variable of an exported class
      • delete member variable from an exported class
      • change size of an exported class
      • change size of a base class of an exported class
      • change the value of exception or error code
  • d-pointer : Tizen-Native framework uses d-pointer in all public classes derived from Tizen::Base::Object
    • d-pointer
      • d-pointer is the only private data member of the class and points to an instance of a class defined in class implementation file.
      • in order to easily maintain binary compatibility, private data members other than d-pointer itself should be omitted in a public class.
      • the name "d-pointer" stems from Trolltech's Arnt Gulbrandsen, who first introduced the technique into Qt, making it one of the first C++ GUI libraries to maintain binary compatibility even between bigger releases

File:ABC(ApplicationBinaryCompatibility) Guide.pdf