blob: f23251b66baa7cbf0775b4b3fa4ab02de3edd2a1 [file] [log] [blame] [view]
dan sinclaire7033e72020-06-02 13:41:19 +00001# CONTRIBUTING
2In general, we follow the
Lei Zhangb7459e22021-04-12 18:10:06 +00003[Chromium Contributing](https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md)
dan sinclaire7033e72020-06-02 13:41:19 +00004guidelines in PDFium. The code review process, and the build tools are all very
5similar to Chromium. The PDFium
Lei Zhangb9037bd2021-06-30 17:02:33 +00006[README](https://pdfium.googlesource.com/pdfium/+/refs/heads/main/README.md)
dan sinclaire7033e72020-06-02 13:41:19 +00007outlines specific build and test information for PDFium.
8
9This document focuses on how the PDFium project operates and how wed like it
10to operate in the future. This is a living document, please file bugs if you
11think there are changes/updates which could be put in place to make it easier
12to contribute to PDFium.
13
14## Communication
15When writing a new feature or fixing an existing bug, get a second opinion
16before investing effort in coding. Coordinating up front makes it much easier
17to avoid frustration later on.
18
19If its a new feature, or updating existing code, first propose it to the
20[mailing list](https://groups.google.com/forum/#!forum/pdfium).
21
22 * If a change needs further context outside the CL, it should be tracked in
23 the [bug system](https://bugs.chromium.org/p/pdfium). Bugs are the right
24 place for long histories, discussion and debate, attaching screenshots, and
25 linking to other associated bugs. Bugs are unnecessary for changes isolated
26 enough to not need any of these.
dan sinclairae33e882020-06-03 20:52:57 +000027 * If the work being implemented is especially complex or large a design
28 document may be warranted. The document should be linked to the filled bug
29 and be set to publicly viewable.
dan sinclaire7033e72020-06-02 13:41:19 +000030 * If there isn't a bug and there should be one, please file a new bug.
31 * Just because there is a bug in the bug system doesn't necessarily mean that
32 a patch will be accepted.
33
34## Public APIs
35The public API of PDFium has grown over time. There are multiple mechanisms in
36place to support this growth from the stability requirements to the versioning
37fields. Along with those there are several other factors to be considered when
38adding public APIs.
39
40 * _Consistency_. We try to keep the APIs consistent with each other, this
41 includes things like naming, parameter ordering and how parameters are
42 handled.
43 * _Generality_. PDFium is used in several places outside the browser. This
44 could be server side, or in user applications. APIs should be designed to
45 work in the general case, or such that they can be expanded to the general
46 case if possible.
47 * _Documentation_. All public APIs should be documented to include information
48 on ownership of passed parameters, valid values being provided, error
49 conditions and return values.
50 * _Differentiate error conditions_. If at all possible, it should be possible
51 to tell the difference between a valid failure and an error.
52 * _Avoid global state_. APIs should receive the objects to be operated on
53 instead of assuming they exist in a global context.
54
55### Stability
56There are a lot of consumers of PDFium outside of Chromium. These include
57LibreOffice, Android and offline conversion tooling. As such, a lot of care is
58taken around the code in the
Lei Zhangb9037bd2021-06-30 17:02:33 +000059[public](https://pdfium.googlesource.com/pdfium/+/refs/heads/main/public/)
dan sinclaire7033e72020-06-02 13:41:19 +000060folder. When planning on changing the public API, the change should be preceded
61by a bug being created and an email to the mailing list to gather feedback from
62other PDFium embedders.
63
64The only stability guarantees that PDFium provides are around the APIs in the
65public folder. Any other interface in the system can be changed without notice.
66If there are features needed which are not exposed through the public headers
67you'll need to file a bug to get it added to the public APIs.
68
69#### Experimental
70All APIs start as Experimental. The experimental status is a documentation tag
71which is added to the API, the first line of the API documentation should be
72`// Experimental API.`
73
74Experimental APIs may be changed or removed entirely without formal notice to
75the community.
76
77#### Stable
78APIs eventually graduate to stable. This is done by removing the
79`// Experimental API.` marker in the documentation. We endeavor to not change
80stable APIs without notice to the community.
81
82NOTE, the process of migrating from experimental to stable isn’t well defined
83at this point. We have experimental APIs which have been that way for multiple
84years. We should work to better define how this transition happens.
85
86#### Deprecated
87If the API is retired, it is marked as deprecated and will eventually be removed.
88API deprecation should, ideally, come with a better replacement API and have a
896-12 months deprecation period. The pending removal should be recorded in the
90documentation comment for the API and should also be recorded in the README with
91the target removal timeframe. All deprecations should have an associated bug
92attached to them.
93
94### Versioning
95In order to allow the public API to expand there are `version` fields in some
96structures. When the versioned structures are expanded those version fields
97need to be incremented to cover the new additions. The code then needs to guard
98against the structure being received having the required version number in
99order to validate the new additions are available.
100
dan sinclairae33e882020-06-03 20:52:57 +0000101## Trybot Access
dan sinclair0d0186f2020-06-04 13:49:27 +0000102Changes must pass the try bots before they are merged into the repo. For your
dan sinclairae33e882020-06-03 20:52:57 +0000103first few CLs the try bots will need to be triggered by a committer. After
104you've submitted 2-3 CLs you can request try bot access by emailing one of the
105OWNERS and requesting try bot access. This will allow you to trigger the bots
106on your own changes without needing a committer.
107
dan sinclaire7033e72020-06-02 13:41:19 +0000108## Committers
109All changes committed to PDFium must be reviewed by a committer. Committers
110have done significant work in the PDFium code base and have a good overall
111understanding of the system.
112
113Contributors can become committers as they exhibit a strong understanding
114of the code base. There is a general requirement for ~10 non-trivial CLs to be
115written by the contributor before being considered for committership. The
116contributor is then nominated by an existing committer and if the nomination is
117accepted by two other committers they receive committer status.
118
119## OWNERS
120The OWNERS files list long time committers to the project and have a broad
121understanding of the code base and how the various pieces interact. In the
122event of a code review stalling with a committer, the OWNERS are the first line
123of escalation. The OWNERS files inherit up the tree, so an OWNER in a top-level
124folder has OWNERS in the folders subdirectories.
125
126There are a limited number of OWNERS files in PDFium at this time due to the
127inherent interconnectedness of the code. We are hoping to expand the number of
128OWNERS files to make them more targeted as the code quality progresses.
129
130Committers can be added to OWNERS files when they exhibit a strong
131understanding of the PDFium code base. This typically involves a combination of
132significant CLs, code review on other contributor CLs, and working with the
133other OWNERs to work through design and development considerations for the code.
134An OWNER must be committed to upholding the principles for the long term health
135of the project, take on a responsibility for reviewing future work, and
136mentor new contributors. Once you are a committer, you should feel free to reach
137out to the OWNERS who have reviewed your patches to ask what else theyd like to
138see from you to be comfortable nominating you as an OWNER. Once nominated,
139OWNERS are added or removed by rough consensus of the existing OWNERS.
140
141## Escalations
142There are times when reviews stall due to differences between reviewers,
143developers and OWNERS. If this happens, please escalate the situation to one of
144the people in the top-level OWNERS file (or another of the owners if already
145discussing with a top-level owner). If the disagreement has moved up through
146all the OWNERS files in the PDFium repo, the escalation should then proceed to
147the Chromium
John Abd-El-Malekb8f41c92023-01-06 20:18:38 +0000148[ATL_OWNERS](https://chromium.googlesource.com/chromium/src/+/refs/heads/main/ATL_OWNERS)
dan sinclaire7033e72020-06-02 13:41:19 +0000149as the final deciders.
150
151The
152[Standard of Code Review](https://google.github.io/eng-practices/review/reviewer/standard.html)
153document has some good guidance on resolving conflicts during code review.
154
155## CLA
156All contributors must complete the Google contributor license agreement. For
157individual contributors, please complete the
158[Individual Contributor License Agreement](https://cla.developers.google.com/about/google-individual?csw=1)
159online. Corporate contributors must fill out the
160[Corporate Contributor License Agreement](https://cla.developers.google.com/about/google-corporate?csw=1)
161and send it to us as described on that page.
162
163Your first CL should add yourself to the
Lei Zhangb9037bd2021-06-30 17:02:33 +0000164[AUTHORS](https://pdfium.googlesource.com/pdfium/+/refs/heads/main/AUTHORS)
dan sinclaire7033e72020-06-02 13:41:19 +0000165file (unless youre covered by one of the blanket entries).
166
167### External contributor checklist for reviewers
168Before LGTMing a change, ensure that the contribution can be accepted:
169 * Definition: The "author" is the email address that owns the code review
170 request on
171 [https://pdfium-review.googlesource.com](https://pdfium-review.googlesource.com/)
172 * Ensure the author is already listed in
Lei Zhangb9037bd2021-06-30 17:02:33 +0000173 [AUTHORS](https://pdfium.googlesource.com/pdfium/+/refs/heads/main/AUTHORS).
dan sinclaire7033e72020-06-02 13:41:19 +0000174 In some cases, the author's company might have a wildcard rule
175 (e.g. \*@google.com).
176 * If the author or their company is not listed, the CL should include a new
177 AUTHORS entry.
178 * Ensure the new entry is reviewed by a reviewer who works for Google.
179 * Contributor License Agreement can be verified by Googlers at
180 [http://go/cla](http://go/cla)
181 * If there is a corporate CLA for the author‘s company, it must list the
182 person explicitly (or the list of authorized contributors must say
183 something like "All employees"). If the author is not on their company’s
184 roster, do not accept the change.
185
186## Legacy Code
187The PDFium code base has been around in one form or another for a long time. As
188such, there is a lot of legacy hidden in the existing code. There are surprising
189interactions and untested corners of the code. We are actively working on
190increasing code coverage on the existing code, and especially welcome additions
191which move the coverage upwards. All new code should come with tests (either
192unit tests or integration tests depending on the feature).
193
194As part of this legacy nature, there is a good chance the code you’re working
195with wasn’t designed to do what you need it to do. There are often refactorings
196and bug fixes that end up happening along with feature development. Those
197fixes/refactorings should be pulled out to their own changes with the
198appropriate tests. This will make reviews a lot easier as, currently, it can be
199hard to tell if there are far reaching effects of a given change.
200
201There is a lot of existing technical debt that is being paid down in PDFium,
202anything we can do here to make future development easier is a great benefit to
203the project. This debt means means code reviews can take a bit longer if
204research is needed to determine how a feature change will interact with the
205rest of the system.