Proposal: eliminate content-specific guidance on use of `const`

[Peter Kasting]

Hello content/ OWNERS,

Summary

The content README contains this bullet:

• The const identifier can be added to simple getter APIs implemented by content. Don‘t add const to interfaces implemented by the embedder, where we can’t make assumptions about what the embedder needs to implement it.

I propose eliminating this bullet, which would leave content/ governed by the same guidance on `const` as the rest of Chromium (See below). I am not proposing making any systematic implementation changes (e.g. an LSC to change how any APIs use `const`).

Please respond with any feedback you have about this. The remaining message below is just optional additional context.

***

Original motivation for content/ guidance

From jam@: "The motivation [for this guidance] was our repo. Generally we don't care about how our changes affect embedders in other repos... In the beginning of content/chrome split we had const, but would keep having to update many files each time an embedder changed its implementation of a delegate method that couldn't be const anymore. This was sometimes transitive too, e.g. class is calling another class in chrome, and a change there finds that it has to change content api and all the other implementations of that interface... When we added this [guidance] to content/public guidelines there was no chrome-wide guidance [about proper usage of `const`]."

Difference from the rest of Chrome

At this point the rest of Chromium and Blink use consistent guidance that, roughly, boils down to "use `const` to indicate logical constness", where "logically const" effectively means "should be threadsafe unless explicitly annotated otherwise, and should not be able to change the return value of a public const method, or provide the caller with the means to do so (e.g. by vending a pointer-to-non-const)". For the complete guidance, see the following links:

• Google Style Guide
• Chrome Dos And Don'ts summary
• Chrome "Using Const Correctly" doc

This basically means that, compared to what other Chrome code does, only content/ forbids marking logically const APIs as `const` unless they are simple getters. (On the contrary, Google/Chromium style "strongly encourages" such marking.)

Resulting challenges

Using the terminology of the "Using Const Correctly" doc, content/ is "const okay". This is challenging when hooked to "const correct" code, as it causes viral downgrades of pointers/refs to non-const whenever they'll be used to call such content/ APIs. We have in the past had to remove (semantically correct) `const`s across parts of Chrome due to calling a new content/ API.

We cannot safely address this by `const_cast<>`ing away `const` at content/ boundaries, as it's very easy to introduce silent UB this way.

Having different style rules for different parts of the codebase is also error-prone in both directions.

Known concerns

The primary feedback jam@ shared with me was that changes that merely fiddle with constness are low-value. He was concerned about any rule change here resulting in engineers wasting time "fixing" content/ const-correctness instead of doing something more useful.

I agree that this is not a change worth some sort of LSC to fix. I think it's unlikely any Googlers would take the time to do one. I hope that eliminating this style divergence actually reduces the total number of "fiddle with const" CLs over time (by eliminating a reason to change caller code).

PK

[Nasko Oskov]

Thanks Peter for a well written email!

One thing I personally am missing is the experience that lead early content OWNERS to avoid usage of const. It is not clear to me what are the bad patterns that resulted at the time and jam@'s synopsis included in this thread does include an example, but it is not clear if that's the extent of it. What I'd like to understand is that if we apply the very short guidance[1] from the documentation whether we will hit the pain points that were hit in the past.

[1] For safety and simplicity, don't return pointers or references to non-const objects from const methods. Within that constraint, mark methods as const where possible. Avoid const_cast to remove const, except when implementing non-const getters in terms of const getters.

In the case of delegate interfaces, would marking a method const still impose restrictions on how an embedder must implement their version? Do we need to be careful about some other cases?

John, would you or others that were involved in this work a while ago be able to dig through the history of the code and help clarify whether the guidance as it is in the paragraph above will avoid the pitfalls you hit?

Overall, I'm not opposed to doing better with const in //content/, but with the limited amount of time and energy we all have, I want us to have clear rules of when to use and when not to use. Ideally, OWNERS won't have to re-read the const guide every time they are reviewing an API change and the cost of getting something wrong and fixing it should be minimal.

--
You received this message because you are subscribed to the Google Groups "content-owners" group.
To unsubscribe from this group and stop receiving emails from it, send an email to content-owner...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAAHOzFAQeYZnHmjCcDpTwNoUAAtJ05Hsq10%3DHpyEjGTF_VVaow%40mail.gmail.com.

[Daniel Cheng]

On Fri, 15 Mar 2024 at 12:21, Nasko Oskov <na...@chromium.org> wrote:

Thanks Peter for a well written email!

One thing I personally am missing is the experience that lead early content OWNERS to avoid usage of const. It is not clear to me what are the bad patterns that resulted at the time and jam@'s synopsis included in this thread does include an example, but it is not clear if that's the extent of it. What I'd like to understand is that if we apply the very short guidance[1] from the documentation whether we will hit the pain points that were hit in the past.

[1] For safety and simplicity, don't return pointers or references to non-const objects from const methods. Within that constraint, mark methods as const where possible. Avoid const_cast to remove const, except when implementing non-const getters in terms of const getters.

In the case of delegate interfaces, would marking a method const still impose restrictions on how an embedder must implement their version? Do we need to be careful about some other cases?

If we mark a method `const`, but the embedder needs to internally use non-const methods to satisfy the implementation of the method, then the implementation may end up resorting to escape hatches like const_cast<T*>, mutable, et cetera.

Though I would kind of hope that encountering such a situation in Chrome would either prompt:

- removal of the const-qualification because it's not appropriate

- rethinking the embedder implementation to be const

I guess the tricky bit here is some of the existing non-constness might make things tricky—for example, RFH::GetParent() is const-qualified (though it returns a non-const pointer :), but RFH::GetMainFrame() is not. So you could imagine an embedder might run into a situation where fixing the const becomes quite viral.

Daniel

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAA%3DmyAt%2BOE-uPt5upS3eMVNRi46y5rmP8eO66Mnj1AxEmBpycA%40mail.gmail.com.

[John Abd-El-Malek]

On Fri, Mar 15, 2024 at 12:21 PM Nasko Oskov <na...@chromium.org> wrote:

Thanks Peter for a well written email!

One thing I personally am missing is the experience that lead early content OWNERS to avoid usage of const. It is not clear to me what are the bad patterns that resulted at the time and jam@'s synopsis included in this thread does include an example, but it is not clear if that's the extent of it. What I'd like to understand is that if we apply the very short guidance[1] from the documentation whether we will hit the pain points that were hit in the past.

[1] For safety and simplicity, don't return pointers or references to non-const objects from const methods. Within that constraint, mark methods as const where possible. Avoid const_cast to remove const, except when implementing non-const getters in terms of const getters.

In the case of delegate interfaces, would marking a method const still impose restrictions on how an embedder must implement their version? Do we need to be careful about some other cases?

John, would you or others that were involved in this work a while ago be able to dig through the history of the code and help clarify whether the guidance as it is in the paragraph above will avoid the pitfalls you hit?

Practically speaking I won't have time to go back ~10 years to dig into it :)

However the summary above is my recollection. I think if our code just used logical constness and not abused physical constness it would have been much smaller or a non-issue.

Peter made convincing arguments that it's better to have consistency codebase-wide, and that folks shouldn't be adding this as it's low impact, but in some places the status-quo adds friction.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAA%3DmyAt%2BOE-uPt5upS3eMVNRi46y5rmP8eO66Mnj1AxEmBpycA%40mail.gmail.com.

[Peter Kasting]

On Fri, Mar 15, 2024 at 12:21 PM Nasko Oskov <na...@chromium.org> wrote:

It is not clear to me what are the bad patterns that resulted at the time and jam@'s synopsis included in this thread does include an example, but it is not clear if that's the extent of it. What I'd like to understand is that if we apply the very short guidance[1] from the documentation whether we will hit the pain points that were hit in the past.

[1] For safety and simplicity, don't return pointers or references to non-const objects from const methods. Within that constraint, mark methods as const where possible. Avoid const_cast to remove const, except when implementing non-const getters in terms of const getters.

Informed speculation:

• APIs that were logically const (but not so marked) had to be pervasively changed every time people wanted to use const pointers/refs.
• APIs that were not logically const (but were marked as const because they were physically const) eventually caused impedance mismatches trying to access non-const methods, but because many other const methods called them, fixing them to not be const was hard/viral.

Both of these arise from not considering the semantics of an API. The real guidance is "mark methods as const iff they do not change the return values of future calls to other public const methods, or give callers a new handle/object that can be used to do so", but that's kinda fuzzy. Telling people never to return ptr-to-non-const from a const method and to use const where possible otherwise covers the common cases reasonably well, and is more memorable.

Unlike physical constness, logical constness generally does not change much over time, and if you consider it when writing the API initially, it will usually not need difficult/viral fixes in the future.

In the case of delegate interfaces, would marking a method const still impose restrictions on how an embedder must implement their version?

Yes, generally in ways that you want; that's the purpose of "const", after all. If you don't expect "give me the number of children" to actually add children, then marking it const helps make that both clear and enforceable. And if the implementer wants to lazy-add children on request, they need the actual children member to be mutable, which again informs readers that its value may change during const methods and probably needs to be updated before being queried.

with the limited amount of time and energy we all have, I want us to have clear rules of when to use and when not to use. Ideally, OWNERS won't have to re-read the const guide every time they are reviewing an API change and the cost of getting something wrong and fixing it should be minimal.

My hope is that sharing the same guidance as the rest of Chrome minimizes this cost. If Chrome's guidance can be improved, let's do that too.

I do think "consider whether this is logically const" imposes more upfront design burden than not considering it at all. But in exchange, implementation is generally safer, clearer, more maintainable, and interfaces better with non-content/ code.

PK

[Peter Kasting]

Friendly ping. Are there other questions or concerns here? Nasko, were your concerns addressed?

PK

[Nasko Oskov]

I don't have objections with this proposal, but I'm one of many OWNERS, so I would really like to hear what others have to say.

On Fri, 12 Apr 2024 at 19:14, Peter Kasting <pkas...@chromium.org> wrote:

Friendly ping. Are there other questions or concerns here? Nasko, were your concerns addressed?

PK

--

You received this message because you are subscribed to the Google Groups "content-owners" group.
To unsubscribe from this group and stop receiving emails from it, send an email to content-owner...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAAHOzFAWBqryZLEvY%3DphWtMu7TYC%3DOZzPLO%3DK0%2BnC8-WSHH9UA%40mail.gmail.com.

[Dave Tapuska]

I don't have any objections. I looked and I can find a handful of APIs that already violate this behavior in content/public. ([1], [2], [3]).. I stopped after finding 3 while some of these are pretty trivial examples, I feel where we would place const likely are trivial implementations as well.

With past experiences changing the content API both CEF, and Electron have always raised issues if there was a problem.

dave.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAA%3DmyAuQpD6jXZ5-YCP%3DEWALb-q3RwFB1QYuJyVQnLrufYc-cg%40mail.gmail.com.

[Avi Drissman]

While the current approach of content matches what our philosophy about const used to be, the world and we have moved on. I am in favor of aligning ourselves with the current global approaches.

On Tue, Apr 16, 2024 at 12:55 PM Nasko Oskov <na...@chromium.org> wrote:

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAA%3DmyAuQpD6jXZ5-YCP%3DEWALb-q3RwFB1QYuJyVQnLrufYc-cg%40mail.gmail.com.

[Peter Kasting]

On Friday, April 12, 2024 at 7:14:23 PM UTC-7 Peter Kasting wrote:

Friendly ping. Are there other questions or concerns here?

It's been several weeks, and multiple OWNERS have voiced support, with no objections. I consider this approved; I'll try and get to making this change (but someone else is welcome to if they get it first).

Thanks for the consideration! And thank you for one fewer thing Chromium devs need to remember :)

PK