Constness in content/public/ APIs

[Peter Kasting]

Hey content/ OWNERS,

content/public/ disallows marking interfaces "const" via PRESUBMIT to avoid making assumptions about embedder implementation needs or exposing content implementation details ( https://chromium.googlesource.com/chromium/src/+/HEAD/content/public/README.md ).

Because this is in tension with both the style guide and our dos-and-don'ts, I assume this has been discussed and debated in greater detail before and there's probably more context about the specific problems seen, perceived tradeoffs, etc.  This rule is having viral effects on non-content/ code, hence my interest; I'd like to make sure I understand how we got here before I go any further.  Are there threads I should read, folks with strong opinions that want to elaborate, or other education I should get?

(What I know so far is that the API used to use const more heavily and significant work was put into removing that, so clearly that was seen as a problem worth engineering effort to solve.)

Thanks,

PK

[Alex Moshchuk]

I think +Lucas Gadani and +John Abd-El-Malek were the last to discuss this in this thread, with subsequent cleanup done in this bug.

--
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/CAAHOzFAeyF1daKoYqYH29rBghB6Gi_OZMW%3D9fMad0%2BjWKG-OtQ%40mail.gmail.com.

[Peter Kasting]

On Tue, Dec 15, 2020 at 1:45 PM Alex Moshchuk <ale...@chromium.org> wrote:

I think +Lucas Gadani and +John Abd-El-Malek were the last to discuss this in this thread, with subsequent cleanup done in this bug.

Thanks!

John, do you have a reference to that content change that resulted in a bunch of constness churn?  I'd like to see what sort of change it was and whether both the code before and the code after were logically or physically const.  I agree with the desire not to have implementation changes result in large viral constness propagation changes.

PK

[John Abd-El-Malek]

On Tue, Dec 15, 2020 at 1:52 PM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Dec 15, 2020 at 1:45 PM Alex Moshchuk <ale...@chromium.org> wrote:

I think +Lucas Gadani and +John Abd-El-Malek were the last to discuss this in this thread, with subsequent cleanup done in this bug.

Thanks!

John, do you have a reference to that content change that resulted in a bunch of constness churn?

I don't have links since this was 9-10 years ago when we started creating the public API. We got bit in two directions since some interfaces in content/public are implemented inside content while others are implemented by (many) embedders.

  I'd like to see what sort of change it was and whether both the code before and the code after were logically or physically const.  I agree with the desire not to have implementation changes result in large viral constness propagation changes.

PK

On Tue, Dec 15, 2020 at 12:12 PM Peter Kasting <pkas...@chromium.org> wrote:

(What I know so far is that the API used to use const more heavily and significant work was put into removing that, so clearly that was seen as a problem worth engineering effort to solve.)

I wouldn't say heavily; we had mostly not used const in content/public but it wasn't strictly enforced so it would leak in. Lucas cleaned it up and added the checks.

 

[Peter Kasting]

On Wed, Dec 16, 2020 at 9:53 AM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Dec 15, 2020 at 1:52 PM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Dec 15, 2020 at 1:45 PM Alex Moshchuk <ale...@chromium.org> wrote:

I think +Lucas Gadani and +John Abd-El-Malek were the last to discuss this in this thread, with subsequent cleanup done in this bug.

Thanks!

John, do you have a reference to that content change that resulted in a bunch of constness churn?

I don't have links since this was 9-10 years ago when we started creating the public API. We got bit in two directions since some interfaces in content/public are implemented inside content while others are implemented by (many) embedders.

Makes sense.

Is it possible that (a) confusion of logical and physical constness and (b) lack of const-correctness elsewhere were the major contributing factors here?  My hypothesis is that, assuming logical-const-correctness, using "const" in API definitions is helpful in defining how the API mutates the publicly-visible state of the world, without imposing constraints on the private implementation details behind it.  Physical-const-correctness, OTOH, definitely results in the problems this guideline looks to avoid; and lack of const-correctness elsewhere means that trying to uphold an API's constraints might require const-casts, which is unfortunate.

If my hypothesis is correct, then we might be able to remove this guideline and improve things for embedders (as we currently have the reverse problem, where const-correct code elsewhere can't interact easily with content/) as well as complying with the style guide and dos-and-don'ts page.  If it's wrong, then maybe we need to capture the reasons why as exceptions to our guidance elsewhere.

PK

[Gabriel Charette]

Food for thought: In some //base APIs what we do when const makes sense on the public API but the impl needs to use non-const state (e.g. a base::Lock) is to make those members "mutable" in the impl (can be accessed from the impl of a const method). That way we can still provide a public contract of "this call won't modify the state of this object" even though we now have to be more careful in the impl as we're asking the compiler to trust us...

PK

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

[Peter Kasting]

On Fri, Dec 18, 2020 at 6:29 AM Gabriel Charette <g...@chromium.org> wrote:

In some //base APIs what we do when const makes sense on the public API but the impl needs to use non-const state (e.g. a base::Lock) is to make those members "mutable" in the impl (can be accessed from the impl of a const method).

Right; while "mutable" can be misused, it's correct when it means "this object is an implementation detail that has no effect on externally-visible state".  Caches and locks are common cases.

Or to phrase in physical/logical constness terms, objects that affect only physical constness and not logical constness should be marked "mutable".

I have found some people view "mutable" as a hack or bug, akin to most cases of casting-away-const.  I think this springs from only thinking of constness in physical terms and not recognizing how logical constness is distinct.

PK

[Peter Kasting]

This seems to have gone silent.  Still hoping for some kind of feedback, since status quo is "content/ contradicts guidance elsewhere, necessity isn't clear, is causing negative effects downstream".  See relevant portion of previous email quoted below.

PK

[John Abd-El-Malek]

Apologies I missed this. I think the background I wrote earlier might not be coming through clearly over email; how about a video chat to go through the issues in high bandwidth?

[Avi Drissman]

Can you make the VC open? If there's a possibility of a decision being made, I'd like to attend.

--

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/CALhVsw18rHCoE1z604mrsJjniJGiciPLi_fe7WWOB_BBvs41%2Bw%40mail.gmail.com.

[Peter Kasting]

On Wed, Jan 20, 2021 at 12:31 PM John Abd-El-Malek <j...@chromium.org> wrote:

Apologies I missed this. I think the background I wrote earlier might not be coming through clearly over email; how about a video chat to go through the issues in high bandwidth?

Sure, can do that now or after 2 today, or else just throw something on my calendar.

Thanks!

PK

[Lucas Gadani]

Since I worked on making things consistent, I'll give my perspective on this. Initially, I looked into this rule because I had some push back adding a const method to the content public API. I started the thread on embedder-dev and John was the only one to voice an opinion on this list, supporting the existing behavior.

Personally, I don't have a strong opinion either way, but I support having clearly written and well enforced rules. There's no point in having rules that nobody follows. Given that, and the fact that there was a relatively low (manageable) number of methods that would need to be fixed in order to achieve consistency, I took on the work to make it happen.

[Avi Drissman]

My opinion is that, while the ban of const from the content API was an understandable reaction at the time to const issues during the development of the content API, it's not something that we should bring forward into the future. I support thoughtfully adding const-correctness to the API.

1. Establishing compiler-enforced guidelines as to what should and should not be changed makes the API stronger.

2. This is a deviation from the Chromium style guide that would not be accepted if proposed today.

3. Given that the content API is the only part of Chromium that bans const, this forces the use of const_cast, which is a UB footgun.

We should take this step along the path of moving from the API we have to the API we want.

Avi

--

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/CAEgCuoHqab1u2KDDwC5SXfFVt6G_9CD%2BPm6xyJemxZvexSCCGg%40mail.gmail.com.

[Tomasz Śniatowski]

Hi all,

As a chromium-downstream perspective, I recall that this rule led to changing the top-level content-client embedder APIs (such as //content/public/browser/content_browser_client.h) to avoid having const-qualified methods. I think that was useful -- it avoided flipping the qualifier back and forth depending on whether there were embedders under // that couldn't make do with a const client method. I think it makes sense to avoid const there and in cases like observer interfaces. Perhaps that was the motivation, but it got extended too widely.

I'd treat those (client and observer interfaces) at least somewhat separately from constness in actual content classes such as WebContents -- where the lack of any const methods is surprising by now. Are there guidelines on whether an observer interface should use const methods? It would seem to me that there should be no logical const assumed in general, and the rule may have been an attempt to stop using incidental physical constness of existing implementations as the determining factor on whether a client interface method should be const.

--
Tomasz Śniatowski

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

[Peter Kasting]

On Wed, Jan 20, 2021 at 1:38 PM Tomasz Śniatowski <tsnia...@vewd.com> wrote:

As a chromium-downstream perspective, I recall that this rule led to changing the top-level content-client embedder APIs (such as //content/public/browser/content_browser_client.h) to avoid having const-qualified methods. I think that was useful -- it avoided flipping the qualifier back and forth depending on whether there were embedders under // that couldn't make do with a const client method. I think it makes sense to avoid const there and in cases like observer interfaces. Perhaps that was the motivation, but it got extended too widely.

Do you know of cases where embedders today would struggle with a const-qualified client method (that we might plausibly want to make const)? I think Avi and I are of the opinion that no embedders should actually struggle with this -- anything that would get in the way should be mutable -- but seeing some illustrative examples would really help interrogate that opinion.

PK

[Tomasz Śniatowski]

I don't have a solid example right now. My main point is I hope this will follow what is logically const, and not what happens to be possible physically-const in the proverbial webview+cros+chrome at the time. I think things like the global content clients shouldn't be poisoning anyone with their non-constness (since they are non-const globals anyway), and forcing getters there to be const isn't too useful.

--
Tomasz Śniatowski

[Peter Kasting]

This meeting seems to have never happened.  John/Avi, are you still willing?  If so my Wednesday afternoon has some time, or maybe next week?

PK 

[Avi Drissman]

I'm still up for it.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFCe_uRfzzeQ3OELHNZ52C%2ByxXnmF_%3Dg8-aCXwX%3DsrpO-A%40mail.gmail.com.

[Alexander Timin]

Hey folks,

I wonder if there were any developments in this area?

I'm asking because I was reviewing a patch which had to add a const method to content-internal class (NavigationRequest), duplicating existing non-const one on //content/public one (NavigationHandle::IsServedFromBackForwardCache) to allow it to be used in places const NavigationRequest& is passed around in //content.

Given that marking NavigationHandle::IsServedFromBackForwardCache as const seems like the best option to me in this particular case, I wonder what's the latest thinking about the overall problem here?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAbBOHU67ufo6OautFpUJ-ETDQoAuoLbNkqcx6ONmLoT2g%40mail.gmail.com.

[Charlie Reis]

This came up again on another thread, and John offered to schedule a VC.  Sounds like at least John, Peter, and Avi are interested, and perhaps Alexander as well?  Any other content owners (or folks from earlier in the thread) want to participate?  We encourage weighing in on topics like this to keep the API rules living and useful.  :)

(John, feel free to add me as optional, but don't worry about finding a time that works with my shifted schedule.)

Thanks!

Charlie

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/CALHg4nkrE6UF_sqr9SFhSzHLUAP1AhfpOgxLoRzWQb2ADWPNbQ%40mail.gmail.com.

[Kinuko Yasuda]

I tend to be a fan of adding const when possible, while I think I also know the background of why we may not simply want to do so.  I'm interested in the discussion, while I'd probably add a constraint to the scheduling.  So same as Charlie- feel free to add me as optional as well, but don't worry about adjusting the time slot, I can follow up asyncly if the discussion outcome is shared.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%2B8MBY8%2B_4mMB2wiz2LjJAdm3FayasTcjvJ8y21HiKLKJVHqA%40mail.gmail.com.

[Scott Violet]

I'm interested in this topic, but I don't think I will add value beyond the other folks participating. Could someone send around a summary after the meeting?

Thanks,

  -Scott

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAH%2B8MBY8%2B_4mMB2wiz2LjJAdm3FayasTcjvJ8y21HiKLKJVHqA%40mail.gmail.com.

[John Abd-El-Malek]

Getting the most feedback from content owners is best. With the different timezones, it'll be impossible to schedule a meeting that works for everyone, so I take back my suggestion at a meeting and let's do this over email so that everyone has a chance to comment.

In general, I'm always supportive of not diverging in Chromium from Google style guide. This also applies to subsets of Chromium, e.g. Content API. The reason why we banned constness initially is that when we first added the API we ran into situations where changing implementation details of content OR embedder code resulted in having to add/remove const in methods across content internals, public APIs, and all embedders. There were two cases, so let's look at each separately:

1) APIs implemented by content: the issue we ran into was that methods on public interfaces were declared as const because the implementation (e.g. since we were just moving code from chrome/ to content/) was declared const. As the implementation changed, we'd then have to update all embedders as well (since if they held a const pointer they couldn't call the method anymore without const_cast). It could be argued that in those cases the method shouldn't have been declared const, but it's not always easy to know ahead of time this. The reason is that the interfaces in content/public back a complex system of interdependent objects. e.g. maybe there's a method on WebContents that looks like it should be const, but it goes through various layers of NavigationControllerImpl, RenderViewHostImpl, RenderProcessHostImpl, RenderFrameHostImpl etc... that through some refactoring now need to modify some state.

2) APIs implemented by embedders: since the interface is declared by content, it seemed like a layering violation that content would know whether all the embedders can implement a certain callback as const or not.

I don't see a way we can add const/ to 2) since there are many embedders but curious what others think. For 1), we could perhaps be thoughtful about picking which interfaces/methods we add const to. I also think originally some implementation methods were not logically const but were marked as const. This didn't affect things as much when there was only one user of these classes, before the content/chrome split.

[Peter Kasting]

On Wed, Mar 24, 2021 at 8:46 PM John Abd-El-Malek <j...@chromium.org> wrote:

Getting the most feedback from content owners is best.

I know from content side we've now heard from at least you and Avi, and from cxx, me.

It would be good to have other content and cxx folks weigh in with perspective.  Charlie, Kinuko, and Alex Moschuk in the former group have appeared on this thread already -- any of you want to chime in with where you think we should go from here?  Would also welcome any other members of either group speaking up.

PK

[Charlie Reis]

My two cents:

I like the idea of making the rules inside and outside content more consistent, especially if the attempt to avoid viral changes (by disallowing const) is causing viral changes of its own (in const correct code that is trying to interact with content).

I'm hearing from John and Tomasz that it may be worth avoiding const on APIs defined by content and implemented by embedders (e.g., observers), where we should not make assumptions about the embedder.  That makes sense to me, though I don't have experience with how other codebases handle that problem.

For APIs implemented by content, we could perhaps start allowing const to allow external const-correct and const-okay code to interact with content without dangerous casts (and to avoid the duplication Alexander mentioned).  Peter, would that change be sufficient for most of the problematic cases you've seen?

I encourage others to chime in as well!

Thanks,

Charlie

[Peter Kasting]

On Tue, Mar 30, 2021 at 11:25 PM Charlie Reis <cr...@chromium.org> wrote:

I like the idea of making the rules inside and outside content more consistent

It seems like there's a general consensus that it would be good to pursue at least some degree of this, so let's figure out what further steps are mutually agreeable.

I'm hearing from John and Tomasz that it may be worth avoiding const on APIs defined by content and implemented by embedders (e.g., observers), where we should not make assumptions about the embedder.  That makes sense to me, though I don't have experience with how other codebases handle that problem.

Something like an OnXYZHappened() callback should probably never be const.  So in general observer methods would largely not make sense to be const, I'd think.

Some delegate methods implemented by embedders may still make sense to make const if they are simple information-getters.  I don't have a specific example right now, but imagine that content defined an abstract interface with a method like GetOpenTabCount().  I think that method should be logically const, as asking for the number of open tabs should not result in externally-visible behavior changes -- even if for some reason counting were costly and the embedder wanted to cache the results internally, it should do so with a mutable member.  Does that make sense?

My proposal here is, allow const in "obviously safe" cases.

 

For APIs implemented by content, we could perhaps start allowing const to allow external const-correct and const-okay code to interact with content without dangerous casts (and to avoid the duplication Alexander mentioned).  Peter, would that change be sufficient for most of the problematic cases you've seen?

Probably the same rule could apply.  One way to start might be to examine what methods had previously been marked const, and assume we want a subset of those.

PK

[Charlie Reis]

Looks like there's a CL that would like to start introducing some const APIs, meeting the simple implemented-by-content case we described:

https://chromium-review.googlesource.com/c/chromium/src/+/2893803

Are we ok with removing the presubmit enforcement and adding a TODO on the const rule in the README, while it's still ambiguous what the rule will end up being?  It does seem like this case is likely to be acceptable, though the TODO leaves us in a gray area for upcoming CLs until we get a new rule nailed down.

Thoughts?

Charlie

[Charlie Reis]

I'm hearing some reasonable concerns about the ambiguity of a TODO in the API rules, which makes it unclear what's allowed in future CLs.

Can we propose a simple change to the const rule for now to cover the case in Dave's CL (and revisit any more complex cases later)?  I feel like that's better than asking Dave to remove the new consts in the CL, but either is better than holding up the CL.  :)  (Hopefully I can unblock it one way or another by end of Monday, as I'm signing off for the weekend now.)

Thanks,

Charlie

[Peter Kasting]

It's hard to know what would cause problems for embedders, so it's hard to phrase a rule. "Const qualifiers are allowed when they are const correct and do not impose an implementation burden on embedders"? Is that too vague?

PK

[Matt Falkenhagen]

After reading the thread, here are my thoughts as another content owner.

I'm not totally sure I understand why the content API is special here vs the rest of our codebase. I guess it's because it's the only large API in our codebase that has interfaces with both 1) implementations that are internal, and 2) implementations in the embedder, and also has multiple embedders.

I'm optimistic that the approach explained upthread is possible: const indicates logical constness, and where an implementation needs to use mutable it should. That seems to help overcome the concern about "content cannot assume the implementation details of the embedder".

At first glance Peter's proposed rule felt too vague, but it made more sense after reading the entire thread. We could potentially add a longer summary of this discussion, and capture the points about OnXYZHappened() vs GetOpenTabCount(), to the README.

It also seems like the two CLs raised so far concern the case of 1): interfaces that are implemented in content. Perhaps an initial unambiguous rule change, to unblock the CL, is to allow const in the case of 1) but not yet in the case of 2)?

2021年5月15日(土) 11:16 Peter Kasting <pkas...@chromium.org>:

--

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/CAAHOzFDk0uXDZf1avuRwuxYinAaOjfaHwSagKFax5MX4NwNo_g%40mail.gmail.com.

[Nasko Oskov]

I have been trying to make sense of all the data in this thread and I must admit that I'm slightly confused. I'm going to lay out some of my thoughts and please shout what I might be getting wrong or what I might be missing.

I'll also start a bit backwards. For 2), I would expect mostly agreement that we cannot dictate the implementation details of embedders, which means that we cannot use const in those cases. Since I'm making an assumption here, please shout if that is an incorrect one.

About 1). Adding const on an API that is implemented inside //content/ might seem simple on the surface, but I've tried reasoning about at least one existing case and have hard time figuring out whether it is safe or not - RenderFrameHost::GetProcess(). Simple logic tells me it should be safe, but digging into the implementation details ... it isn't so obvious. 

What worries me a bit more is that putting const on an API basically locks us into an implementation detail. In my mind, the main point of //content/public API is to isolate the rest of the codebase from the implementation details of //content/. If adding const on an API limits what we can do in the future, I'd consider this downside. Let's continue with the example above and rewind time about a year or so ago, before MBI existed. Let's also assume that RFH::GetProcess() was set to be const. Would it be possible to implement MBI (adding AgentSchedulingGroupHost in the middle) and not be limited by the const-ness? What about MPArch, which we are in the middle of?

We seem to consistently have big projects that shake up the assumptions we have made before - we only have RenderViewHost, we only have one FrameTree per WebContents, etc. Would we put more friction on such big changes by adding the additional constraint of keeping const satisfied when we might actually be changing the assumptions we have made in the past and that const might no longer be correct?

I honestly have a hard time understanding what usage of const will be safe in the future. As an //content/OWNER I will be very hesitant to approve const being added on a method implemented in //content/ because of the complex nature of reasoning about present and future. Maybe my C++-fu is weak, maybe there is some magic article that I can read on const that will enlighten me, it may be something else. But I'm mostly having a hard time with complexity, which is currently exploding in our codebase and adding const to the mix is adding another layer of complexity.

These are my raw thoughts. If we allow const in content, I would like us to have super crisp rules as to when it is ok and when it is not. Unfortunately, I don't know what those rules are. What Peter wrote is way too vague for me and I have a hard time parsing what it means.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAJ_xCi%3D%3DdViMRdJVuWf8jXF0-rZ0yekE4d4GbrusM9efQ40Ccw%40mail.gmail.com.

[Peter Kasting]

On Mon, May 17, 2021 at 5:38 PM Nasko Oskov <na...@chromium.org> wrote:

I'll also start a bit backwards. For 2), I would expect mostly agreement that we cannot dictate the implementation details of embedders, which means that we cannot use const in those cases. Since I'm making an assumption here, please shout if that is an incorrect one.

I think that assumption is incorrect.  Const in APIs is about API design, not implementation.  It marks when an API can neither modify visible state nor allow others to do so.  The visible state of an object is part of the contract, not an implementation detail.

About 1). Adding const on an API that is implemented inside //content/ might seem simple on the surface, but I've tried reasoning about at least one existing case and have hard time figuring out whether it is safe or not - RenderFrameHost::GetProcess(). Simple logic tells me it should be safe, but digging into the implementation details ... it isn't so obvious.

It cannot be const, because it vends a pointer-to-non-const.  No const method should vend a pointer-to-non-const.  (There are very, VERY rare exceptions, but this would definitely not be one of them.)

Let's continue with the example above and rewind time about a year or so ago, before MBI existed. Let's also assume that RFH::GetProcess() was set to be const. Would it be possible to implement MBI (adding AgentSchedulingGroupHost in the middle) and not be limited by the const-ness? What about MPArch, which we are in the middle of?

Sorry, I am too unfamiliar with the details of how those are implemented to answer your concerns.  However, since GetProcess() should not be const, perhaps this specific case is moot.

Would we put more friction on such big changes by adding the additional constraint of keeping const satisfied when we might actually be changing the assumptions we have made in the past and that const might no longer be correct?

The only case where this should occur is if you elect to expose previous private-and-mutable state as part of the public contract, for example exposing an object's internal caches as part of its public API.

Maybe my C++-fu is weak, maybe there is some magic article that I can read on const that will enlighten me, it may be something else.

I don't know, but have you already read our guidance on const?  It doesn't completely clear this up, but if it's not already familiar ground it might help.

But I'm mostly having a hard time with complexity, which is currently exploding in our codebase and adding const to the mix is adding another layer of complexity.

Hopefully our goals are aligned.  A system which delineates logically const methods as such should be safer to use and modify.

These are my raw thoughts. If we allow const in content, I would like us to have super crisp rules as to when it is ok and when it is not. Unfortunately, I don't know what those rules are. What Peter wrote is way too vague for me and I have a hard time parsing what it means.

Hmm, how about this:

You may only mark a method const when all of the following apply:

* The method changes no object state other than that marked mutable.

* The method does not return (directly or via outparam) pointers or references, except to const objects.

* The method does not call non-const methods on any other objects.

One can argue my second and third bullets are too restrictive, but in practice, there are few cases you'd want to relax them that cannot lead to trouble down the road.

Is this clear and scoped enough that it would reduce your concerns about future complexity?

PK

[Charlie Reis]

I think Nasko's high level concern is that we don't want the content implementation to be constrained by uses of const in the API, since content often goes through architecture changes where more and more things become swappable.  This was also John's original concern (for APIs implemented within content) as well.

The first and third bullets you propose are phrased in terms of what state the method changes or what methods it calls, but that's only true at one point in time.  I think we're struggling with the idea of reading it as, "The method changes no object state other than that marked mutable, *and it will never do so in the future.*"  If it did need to change visible object state or call non-const methods in the future, that would require updating a lot of embedder code.

I think the gap here might be our lack of intuition over what shouldn't be const.  You pointed out that RenderFrameHost::GetProcess() can't be const because it returns a non-const object, but what would have prevented having a const RenderProcessHost* GetProcess() const method (much like Dave's const WebView* GetWebView() const on RenderFrame here)?  Is there something wrong in that case?  If that were allowed and we later hypothetically changed RFH::GetProcess() to call SiteInstance::GetProcess() (which creates a process if one is missing), I think it would break the public const API.

I do appreciate the proposed rules, and maybe there's something that prevents the GetProcess() case without preventing the GetWebView() case?  I'd be happy to learn.

Thanks!

Charlie

[Peter Kasting]

On Mon, May 17, 2021 at 8:02 PM Charlie Reis <cr...@chromium.org> wrote:

The first and third bullets you propose are phrased in terms of what state the method changes or what methods it calls, but that's only true at one point in time.  I think we're struggling with the idea of reading it as, "The method changes no object state other than that marked mutable, *and it will never do so in the future.*"

This may be a feature rather than a bug -- that is, if "const" clearly delineates logically const operations and you change something to no longer be one, the compiler will now point out to you exactly which callers may have depended on the contract you just changed.  This potentially requires updating a lot of caller code, but if the original job was done right, this is code you likely had to update anyway; now the compiler can tell you where it is.  (This is a key reason Google and Chromium style guides recommend const APIs and variables where feasible.)

So don't think in terms of "will never change state in the future".  Think in terms of "if this method suddenly started mutating random world state, callers might care".  If callers might care, marking this as const lets them decide to care or not by using const or non-const handles to call methods on.

 

I think the gap here might be our lack of intuition over what shouldn't be const.  You pointed out that RenderFrameHost::GetProcess() can't be const because it returns a non-const object, but what would have prevented having a const RenderProcessHost* GetProcess() const method (much like Dave's const WebView* GetWebView() const on RenderFrame here)?  Is there something wrong in that case?  If that were allowed and we later hypothetically changed RFH::GetProcess() to call SiteInstance::GetProcess() (which creates a process if one is missing), I think it would break the public const API.

Not to bikeshed over naming, but perhaps SiteInstance::GetOrCreateProcess() would be a name that makes it more obvious that that kind of a method can never be const, and can never be called by a const method.  So any "RFH::GetProcess() const" method can't call it -- if no process exists already, it would have to return null.

Let's say you worry that, in the conceivable future, RFH::GetProcess() might be implemented in terms of a non-const method.  You have a number of choices:

• Do not expose a const GetProcess(), only a non-const one.  This makes sense if all callers of GetProcess() must be OK with any state mutations it might cause.  (Or to put it another way, you want to ban callers that can't afford to have calls affect things.)
• Expose const and non-const methods, but have the contract be that at least the const method may fail and return null (e.g. if the process does not exist).  This lets callers make a choice, at the risk of ambiguity/surprise.
• Rename/redesign the API, e.g. as GetOrCreateProcess().  (Maybe also add a const GetProcess() or GetProcessWithoutCreating() next to it.)  This maximizes clarity, at the cost of verbosity/non-parallel APIs.
• Expose both methods, have them be the same for now and omit any sort of future guarantee on behavior, and decide that if there is a future change like the above, you can deal with the associated refactoring.  This is the easy (and generally default) choice, and is fine in most common cases, especially when you have well-defined relationships that are unlikely to change later.

Any of these may be reasonable. All of them are better than a physically const API ("RPH* RFH::GetProcess() const").

In the end, there cannot be a magic rule that perfectly captures exactly when to do what any more than for most other things in programming.  (Though if you figure out the optimal rules for "passing arguments to functions" in C++, please teach them to me.)  I'm sure allowing const in content will at some point cost something.  The goal is to manage net costs so that the benefits outweigh them.

PK

[Charlie Reis]

Thanks-- this is the discussion I was looking for!  

On Mon, May 17, 2021 at 9:10 PM Peter Kasting <pkas...@chromium.org> wrote:

This potentially requires updating a lot of caller code, but if the original job was done right, this is code you likely had to update anyway; now the compiler can tell you where it is.  (This is a key reason Google and Chromium style guides recommend const APIs and variables where feasible.)

I like the view that these additional constraints make architecture changes easier (by exposing affected code) rather than harder (by having to update lots of callers).

Not to bikeshed over naming, but perhaps SiteInstance::GetOrCreateProcess() would be a name that makes it more obvious that that kind of a method can never be const, and can never be called by a const method. 

Yep, or https://crbug.com/952625.  But I digress.  :)

With the above in mind, let's revisit your proposed rule:

 

You may only mark a method const when all of the following apply:

* The method changes no object state other than that marked mutable.

* The method does not return (directly or via outparam) pointers or references, except to const objects.

* The method does not call non-const methods on any other objects.

 I think I can see the benefits to allowing those cases on APIs implemented by content (at least), and I would be in favor.

In APIs implemented by the embedder, is there a difference because they don't have the freedom to update callers inside content when embedder code changes?  It seems harder to reason about how an embedder might behave on a callback or observer (since we don't know what they'll need to do), making me suspect we might want fewer consts in those cases.  But maybe omitting const is the natural default if we don't know what the embedder will need?

Thanks,

Charlie

[Peter Kasting]

On Mon, May 17, 2021 at 9:55 PM Charlie Reis <cr...@chromium.org> wrote:

In APIs implemented by the embedder, is there a difference because they don't have the freedom to update callers inside content when embedder code changes?  It seems harder to reason about how an embedder might behave on a callback or observer (since we don't know what they'll need to do), making me suspect we might want fewer consts in those cases.  But maybe omitting const is the natural default if we don't know what the embedder will need?

Callbacks/observers should almost never be const, because their whole purpose is to change object state/perform some meaningful action in response to an event.   Only when you want to guarantee that firing an action can't have any effect would you make a callback for it const.  I can't think of a case like that I've seen in practice.

Similarly, in other cases, if a method implemented by an embedder *ought* to allow arbitrary side effects, it shouldn't be const, even if embedders don't do side effects today.

One heuristic is whether you're asking the embedder for something versus telling it about something.  Many of the former cases are const; very few of the latter are.

That's not really a property of embedders vs. other people, though; the same applies to the content layer itself (and any other object/subsystem boundary).

So you might make additions to the previous guidance:

* The method must not be some kind of callback, observer, or event handler.

* The method must not be plausibly renamable to "GetOrCreateXXX()" or the like.

* As a heuristic, the method's primary purpose should be to transfer information from callee to caller, not the other way around.

PK

[John Abd-El-Malek]

On Mon, May 17, 2021 at 10:54 PM Peter Kasting <pkas...@chromium.org> wrote:

On Mon, May 17, 2021 at 9:55 PM Charlie Reis <cr...@chromium.org> wrote:

In APIs implemented by the embedder, is there a difference because they don't have the freedom to update callers inside content when embedder code changes?  It seems harder to reason about how an embedder might behave on a callback or observer (since we don't know what they'll need to do), making me suspect we might want fewer consts in those cases.  But maybe omitting const is the natural default if we don't know what the embedder will need?

Callbacks/observers should almost never be const, because their whole purpose is to change object state/perform some meaningful action in response to an event.

Note this isn't necessarily true. The purpose of the API is to let embedders know of specific events, but content/ itself does not dictate or presume to know what embedders do. That's a big part of having a separation between content/ and its embedders.

   Only when you want to guarantee that firing an action can't have any effect would you make a callback for it const.  I can't think of a case like that I've seen in practice.

Similarly, in other cases, if a method implemented by an embedder *ought* to allow arbitrary side effects, it shouldn't be const, even if embedders don't do side effects today.

Per above, content/ can't make assumptions in general about what embedders do in any callback. I think it's clear from content/owners perspective we don't want to dictate/enforce any new restriction in that regard.

 

One heuristic is whether you're asking the embedder for something versus telling it about something.  Many of the former cases are const; very few of the latter are.

I'm not sure where the second statement comes from? e.g. not clear to me that we can use this (asking means it's const) as a guideline.

[Nasko Oskov]

On Mon, 17 May 2021 at 22:54, Peter Kasting <pkas...@chromium.org> wrote:

On Mon, May 17, 2021 at 9:55 PM Charlie Reis <cr...@chromium.org> wrote:

In APIs implemented by the embedder, is there a difference because they don't have the freedom to update callers inside content when embedder code changes?  It seems harder to reason about how an embedder might behave on a callback or observer (since we don't know what they'll need to do), making me suspect we might want fewer consts in those cases.  But maybe omitting const is the natural default if we don't know what the embedder will need?

Callbacks/observers should almost never be const, because their whole purpose is to change object state/perform some meaningful action in response to an event.   Only when you want to guarantee that firing an action can't have any effect would you make a callback for it const.  I can't think of a case like that I've seen in practice.

I think reasoning about embedders implementation of APIs is inherently hard, so I'd suggest we limit const only to APIs implemented inside //content/ and leave embedder implemented APIs as they are today.

 

Similarly, in other cases, if a method implemented by an embedder *ought* to allow arbitrary side effects, it shouldn't be const, even if embedders don't do side effects today.

One heuristic is whether you're asking the embedder for something versus telling it about something.  Many of the former cases are const; very few of the latter are.

That's not really a property of embedders vs. other people, though; the same applies to the content layer itself (and any other object/subsystem boundary).

So you might make additions to the previous guidance:

* The method must not be some kind of callback, observer, or event handler.

This is another way of saying "something implemented outside of content".

  

* The method must not be plausibly renamable to "GetOrCreateXXX()" or the like.

* As a heuristic, the method's primary purpose should be to transfer information from callee to caller, not the other way around.

 I think these two we can easily sum up in a single rule - "Only simple getter APIs can be const".

The overall addition of const has the side effect "a method that is const cannot be changed to be non-const without updating all callers". This can be considered a positive or negative depending on the viewpoint. From one perspective, it requires a lot of code to change to allow for that. On the other hand, it forces us to re-evaluate whether the new assumption is safe for existing code or not. Big projects such as MPArch, which are changing fundamental assumptions, already are doing audits of all callers of particular APIs, so we have to do the audit work regardless. I can see that const is a way to use the compiler to force us to do these audits. We have to accept that impactful projects will likely have to introduce non-const version of a const API and migrate code over to it ahead of time as opposed to today's world where we can enable the project and auditing can proceed in parallel.

If folks are ok with these trade offs, I'd be happy with the this guidance:

• const cannot be added on APIs implemented by embedders/outside of content.
• const can only be added on simple getter APIs

Thanks!

[John Abd-El-Malek]

This seems like a reasonable starting point, and we can see if it's still negatively impacting src/chrome after? Peter WDYT?

 

Thanks!

[Peter Kasting]

Do you have no embedder APIs that are of the form "simple getter on the embedder, for when content/ needs to know something"?  Is the information flow only ever one way?

I'm thinking of something like "bool IsZoomed()" or "int GetTabCount()" or the like.  I don't think it's appropriate to ban const on these simply because they're implemented on the embedder.  And because the embedder isn't allowed to make them const, they can't be called on the embedder side by the embedder's own const methods.  So you're not achieving an (IMO impossible) goal of "imposing nothing on the embedder".  You're imposing non-constness.  That was the whole point of raising this thread.

We can work around this by wrapping a public const method with a different name, but if the content/ API uses the obvious name for the method, that's an unfortunate workaround.

So in short, I think the first bullet above needs to be nixxed.  The guidance should be, you may only mark a function const if it's a simple getter and does not return a pointer or ref to non-const.  (And even then, that's a "can", not necessarily a "should".)

PK

[Daniel Cheng]

I don't think the first bullet point is set in stone; I think it's something that could reasonably be revisited. But as a starting point, I think it makes sense to start smaller. Even the Blink public API, which has had no guidance *against* using const, has very very few const-qualified methods: https://source.chromium.org/search?q=f:public%2Fweb%20virtual%5C%20%5Cw%2B%5C%20%5Cw%2B%5C(.%2B%5C)%5C%20const&sq=&ss=chromium

(and some of the ones that are const-qualified look a bit suspicious to me, e.g. AddAdditionalSchemes)

To me, this suggests that we often don't need const-qualified embedder-implemented methods, so maybe it's OK to omit for now.

If it is problematic, I think the embedder can always add an overload with the same name--just with an extra const qualifier.

Daniel

 

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/CAAHOzFAdBK6bij%2BuM2sBV%3DCto_kOt%3D2efCpbp6W51uAxHrz%2B6g%40mail.gmail.com.

[Charlie Reis]

Thanks everyone!

I think we're reaching a consensus that "const can be allowed on simple getter APIs implemented within content," at least as a starting point.  That's sufficient to unblock Dave's CL, and it allows some forward progress on const in the cases we seem to agree on.  I'll propose the updated rule text below to proceed and we can improve it as needed.  (Speak up if my impression is incorrect, though!)

"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."

Separately, I'm not opposed to considering more pros/cons in the future for allowing const on getter APIs implemented by the embedder (e.g., in delegate interfaces, rather than observer interfaces).  There are still things I'm learning there, like having const+nonconst overloads is only possible for methods with object return types and not primitive (int/bool/etc) return types, which restricts what an embedder can do for something like int GetFooCount() with no const.  In other words, there may be constraints either way (e.g., whether the embedder can use const in their code vs whether the embedder can mutate state in their code).  It might be nice to move towards a smaller delta from other style/API rules in general, but there also isn't a rush-- let's start with the easier change above and go from there.

Thanks!

Charlie

[Kinuko Yasuda]

I had a few draft emails that I didn't end up sending, but overall I feel happy with the latest proposal, it's a great progress with a clear and simpler addition with some reservation to revisit, thanks a lot for the discussion and resolution!

(I also liked the part that the discussion has us start towards the direction that "needing to revise constness due to architecture changes" is not a bug but a feature, while the cost that is needed to revise a lot of code needs to be also separately considered)

[dan...@chromium.org]

Thanks for keeping this ball moving forward everyone, and for encouraging our codebase to evolve and improve. +1 on the outcome.

Cheers,

Dana

[Peter Kasting]

On Tue, May 18, 2021 at 9:43 PM Charlie Reis <cr...@chromium.org> wrote:

"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."

Separately, I'm not opposed to considering more pros/cons in the future for allowing const on getter APIs implemented by the embedder (e.g., in delegate interfaces, rather than observer interfaces).  There are still things I'm learning there, like having const+nonconst overloads is only possible for methods with object return types and not primitive (int/bool/etc) return types, which restricts what an embedder can do for something like int GetFooCount() with no const.  In other words, there may be constraints either way (e.g., whether the embedder can use const in their code vs whether the embedder can mutate state in their code).  It might be nice to move towards a smaller delta from other style/API rules in general, but there also isn't a rush-- let's start with the easier change above and go from there.

SGTM

PK 

[John Abd-El-Malek]

On Tue, May 18, 2021 at 3:46 PM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, May 18, 2021 at 2:24 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, May 18, 2021 at 2:19 PM Nasko Oskov <na...@chromium.org> wrote:

If folks are ok with these trade offs, I'd be happy with the this guidance:

• const cannot be added on APIs implemented by embedders/outside of content.
• const can only be added on simple getter APIs

This seems like a reasonable starting point, and we can see if it's still negatively impacting src/chrome after? Peter WDYT?

Do you have no embedder APIs that are of the form "simple getter on the embedder, for when content/ needs to know something"?  Is the information flow only ever one way?

I'm thinking of something like "bool IsZoomed()" or "int GetTabCount()" or the like.  I don't think it's appropriate to ban const on these simply because they're implemented on the embedder.  And because the embedder isn't allowed to make them const, they can't be called on the embedder side by the embedder's own const methods.  So you're not achieving an (IMO impossible) goal of "imposing nothing on the embedder".  You're imposing non-constness.  That was the whole point of raising this thread.

Content embedder APIs are added for the service of content/ only, e.g. we specifically call out that methods shouldn't be added on embedder APIs to be called by the embedder in our API guidelines. Of course that can't stop embedders from calling existing functions, but in that case if something is const they can simply add a unix_hacker() getter for it.

[Charlie Reis]

Just a separate update to note that Dave's CL has landed, so the new rule is live:

https://chromium.googlesource.com/chromium/src/+/main/content/public/README.md

• 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.

Thanks for everyone's help in getting to that first outcome!

Feel free to continue discussing the embedder-implemented APIs question, though.  :)

Charlie