dan sinclair | e7033e7 | 2020-06-02 13:41:19 +0000 | [diff] [blame] | 1 | # CONTRIBUTING |
| 2 | In general, we follow the |
Lei Zhang | b7459e2 | 2021-04-12 18:10:06 +0000 | [diff] [blame] | 3 | [Chromium Contributing](https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md) |
dan sinclair | e7033e7 | 2020-06-02 13:41:19 +0000 | [diff] [blame] | 4 | guidelines in PDFium. The code review process, and the build tools are all very |
| 5 | similar to Chromium. The PDFium |
Lei Zhang | b9037bd | 2021-06-30 17:02:33 +0000 | [diff] [blame] | 6 | [README](https://pdfium.googlesource.com/pdfium/+/refs/heads/main/README.md) |
dan sinclair | e7033e7 | 2020-06-02 13:41:19 +0000 | [diff] [blame] | 7 | outlines specific build and test information for PDFium. |
| 8 | |
| 9 | This document focuses on how the PDFium project operates and how we’d like it |
| 10 | to operate in the future. This is a living document, please file bugs if you |
| 11 | think there are changes/updates which could be put in place to make it easier |
| 12 | to contribute to PDFium. |
| 13 | |
| 14 | ## Communication |
| 15 | When writing a new feature or fixing an existing bug, get a second opinion |
| 16 | before investing effort in coding. Coordinating up front makes it much easier |
| 17 | to avoid frustration later on. |
| 18 | |
| 19 | If it‘s 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 sinclair | ae33e88 | 2020-06-03 20:52:57 +0000 | [diff] [blame] | 27 | * 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 sinclair | e7033e7 | 2020-06-02 13:41:19 +0000 | [diff] [blame] | 30 | * 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 |
| 35 | The public API of PDFium has grown over time. There are multiple mechanisms in |
| 36 | place to support this growth from the stability requirements to the versioning |
| 37 | fields. Along with those there are several other factors to be considered when |
| 38 | adding 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 |
| 56 | There are a lot of consumers of PDFium outside of Chromium. These include |
| 57 | LibreOffice, Android and offline conversion tooling. As such, a lot of care is |
| 58 | taken around the code in the |
Lei Zhang | b9037bd | 2021-06-30 17:02:33 +0000 | [diff] [blame] | 59 | [public](https://pdfium.googlesource.com/pdfium/+/refs/heads/main/public/) |
dan sinclair | e7033e7 | 2020-06-02 13:41:19 +0000 | [diff] [blame] | 60 | folder. When planning on changing the public API, the change should be preceded |
| 61 | by a bug being created and an email to the mailing list to gather feedback from |
| 62 | other PDFium embedders. |
| 63 | |
| 64 | The only stability guarantees that PDFium provides are around the APIs in the |
| 65 | public folder. Any other interface in the system can be changed without notice. |
| 66 | If there are features needed which are not exposed through the public headers |
| 67 | you'll need to file a bug to get it added to the public APIs. |
| 68 | |
| 69 | #### Experimental |
| 70 | All APIs start as Experimental. The experimental status is a documentation tag |
| 71 | which is added to the API, the first line of the API documentation should be |
| 72 | `// Experimental API.` |
| 73 | |
| 74 | Experimental APIs may be changed or removed entirely without formal notice to |
| 75 | the community. |
| 76 | |
| 77 | #### Stable |
| 78 | APIs eventually graduate to stable. This is done by removing the |
| 79 | `// Experimental API.` marker in the documentation. We endeavor to not change |
| 80 | stable APIs without notice to the community. |
| 81 | |
| 82 | NOTE, the process of migrating from experimental to stable isn’t well defined |
| 83 | at this point. We have experimental APIs which have been that way for multiple |
| 84 | years. We should work to better define how this transition happens. |
| 85 | |
| 86 | #### Deprecated |
| 87 | If the API is retired, it is marked as deprecated and will eventually be removed. |
| 88 | API deprecation should, ideally, come with a better replacement API and have a |
| 89 | 6-12 months deprecation period. The pending removal should be recorded in the |
| 90 | documentation comment for the API and should also be recorded in the README with |
| 91 | the target removal timeframe. All deprecations should have an associated bug |
| 92 | attached to them. |
| 93 | |
| 94 | ### Versioning |
| 95 | In order to allow the public API to expand there are `version` fields in some |
| 96 | structures. When the versioned structures are expanded those version fields |
| 97 | need to be incremented to cover the new additions. The code then needs to guard |
| 98 | against the structure being received having the required version number in |
| 99 | order to validate the new additions are available. |
| 100 | |
dan sinclair | ae33e88 | 2020-06-03 20:52:57 +0000 | [diff] [blame] | 101 | ## Trybot Access |
dan sinclair | 0d0186f | 2020-06-04 13:49:27 +0000 | [diff] [blame] | 102 | Changes must pass the try bots before they are merged into the repo. For your |
dan sinclair | ae33e88 | 2020-06-03 20:52:57 +0000 | [diff] [blame] | 103 | first few CLs the try bots will need to be triggered by a committer. After |
| 104 | you've submitted 2-3 CLs you can request try bot access by emailing one of the |
| 105 | OWNERS and requesting try bot access. This will allow you to trigger the bots |
| 106 | on your own changes without needing a committer. |
| 107 | |
dan sinclair | e7033e7 | 2020-06-02 13:41:19 +0000 | [diff] [blame] | 108 | ## Committers |
| 109 | All changes committed to PDFium must be reviewed by a committer. Committers |
| 110 | have done significant work in the PDFium code base and have a good overall |
| 111 | understanding of the system. |
| 112 | |
| 113 | Contributors can become committers as they exhibit a strong understanding |
| 114 | of the code base. There is a general requirement for ~10 non-trivial CLs to be |
| 115 | written by the contributor before being considered for committership. The |
| 116 | contributor is then nominated by an existing committer and if the nomination is |
| 117 | accepted by two other committers they receive committer status. |
| 118 | |
| 119 | ## OWNERS |
| 120 | The OWNERS files list long time committers to the project and have a broad |
| 121 | understanding of the code base and how the various pieces interact. In the |
| 122 | event of a code review stalling with a committer, the OWNERS are the first line |
| 123 | of escalation. The OWNERS files inherit up the tree, so an OWNER in a top-level |
| 124 | folder has OWNERS in the folders subdirectories. |
| 125 | |
| 126 | There are a limited number of OWNERS files in PDFium at this time due to the |
| 127 | inherent interconnectedness of the code. We are hoping to expand the number of |
| 128 | OWNERS files to make them more targeted as the code quality progresses. |
| 129 | |
| 130 | Committers can be added to OWNERS files when they exhibit a strong |
| 131 | understanding of the PDFium code base. This typically involves a combination of |
| 132 | significant CLs, code review on other contributor CLs, and working with the |
| 133 | other OWNERs to work through design and development considerations for the code. |
| 134 | An OWNER must be committed to upholding the principles for the long term health |
| 135 | of the project, take on a responsibility for reviewing future work, and |
| 136 | mentor new contributors. Once you are a committer, you should feel free to reach |
| 137 | out to the OWNERS who have reviewed your patches to ask what else they’d like to |
| 138 | see from you to be comfortable nominating you as an OWNER. Once nominated, |
| 139 | OWNERS are added or removed by rough consensus of the existing OWNERS. |
| 140 | |
| 141 | ## Escalations |
| 142 | There are times when reviews stall due to differences between reviewers, |
| 143 | developers and OWNERS. If this happens, please escalate the situation to one of |
| 144 | the people in the top-level OWNERS file (or another of the owners if already |
| 145 | discussing with a top-level owner). If the disagreement has moved up through |
| 146 | all the OWNERS files in the PDFium repo, the escalation should then proceed to |
| 147 | the Chromium |
John Abd-El-Malek | b8f41c9 | 2023-01-06 20:18:38 +0000 | [diff] [blame] | 148 | [ATL_OWNERS](https://chromium.googlesource.com/chromium/src/+/refs/heads/main/ATL_OWNERS) |
dan sinclair | e7033e7 | 2020-06-02 13:41:19 +0000 | [diff] [blame] | 149 | as the final deciders. |
| 150 | |
| 151 | The |
| 152 | [Standard of Code Review](https://google.github.io/eng-practices/review/reviewer/standard.html) |
| 153 | document has some good guidance on resolving conflicts during code review. |
| 154 | |
| 155 | ## CLA |
| 156 | All contributors must complete the Google contributor license agreement. For |
| 157 | individual contributors, please complete the |
| 158 | [Individual Contributor License Agreement](https://cla.developers.google.com/about/google-individual?csw=1) |
| 159 | online. Corporate contributors must fill out the |
| 160 | [Corporate Contributor License Agreement](https://cla.developers.google.com/about/google-corporate?csw=1) |
| 161 | and send it to us as described on that page. |
| 162 | |
| 163 | Your first CL should add yourself to the |
Lei Zhang | b9037bd | 2021-06-30 17:02:33 +0000 | [diff] [blame] | 164 | [AUTHORS](https://pdfium.googlesource.com/pdfium/+/refs/heads/main/AUTHORS) |
dan sinclair | e7033e7 | 2020-06-02 13:41:19 +0000 | [diff] [blame] | 165 | file (unless you’re covered by one of the blanket entries). |
| 166 | |
| 167 | ### External contributor checklist for reviewers |
| 168 | Before 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 Zhang | b9037bd | 2021-06-30 17:02:33 +0000 | [diff] [blame] | 173 | [AUTHORS](https://pdfium.googlesource.com/pdfium/+/refs/heads/main/AUTHORS). |
dan sinclair | e7033e7 | 2020-06-02 13:41:19 +0000 | [diff] [blame] | 174 | 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 |
| 187 | The PDFium code base has been around in one form or another for a long time. As |
| 188 | such, there is a lot of legacy hidden in the existing code. There are surprising |
| 189 | interactions and untested corners of the code. We are actively working on |
| 190 | increasing code coverage on the existing code, and especially welcome additions |
| 191 | which move the coverage upwards. All new code should come with tests (either |
| 192 | unit tests or integration tests depending on the feature). |
| 193 | |
| 194 | As part of this legacy nature, there is a good chance the code you’re working |
| 195 | with wasn’t designed to do what you need it to do. There are often refactorings |
| 196 | and bug fixes that end up happening along with feature development. Those |
| 197 | fixes/refactorings should be pulled out to their own changes with the |
| 198 | appropriate tests. This will make reviews a lot easier as, currently, it can be |
| 199 | hard to tell if there are far reaching effects of a given change. |
| 200 | |
| 201 | There is a lot of existing technical debt that is being paid down in PDFium, |
| 202 | anything we can do here to make future development easier is a great benefit to |
| 203 | the project. This debt means means code reviews can take a bit longer if |
| 204 | research is needed to determine how a feature change will interact with the |
| 205 | rest of the system. |