Just wanted to check in here. Is there a concrete plan to follow up with the concerns that have been raised here and make sure everyone's on the same page?
If there isn't something scheduled soon, unfortunately I think the right course of action is to revert the entire chain of CLs. Daniel On Fri, 28 Nov 2025 at 01:19, Daniel Cheng <[email protected]> wrote: > On Fri, 28 Nov 2025 at 00:23, Steinar H. Gunderson <[email protected]> > wrote: > >> On Thu, Nov 27, 2025 at 04:42:58PM -0800, Daniel Cheng wrote: >> > I think there's a balance to be struck here, and defaulting everything >> to a >> > member of LocalDOMWindow/Document/LocalFrame/Navigator doesn't really >> seem >> > like a maintainable approach, even if it's good for benchmarks. >> >> To be clear, I considered the change to increase maintainability and >> readability; benchmarks was only a part of the equation. Size decrease >> was a >> nice bonus. >> > > Based on the responses here, it does not seem like there is general > agreement that this increases maintainability and readability. > > >> >> These members were there all along. It's just that they are more visible >> now >> instead of being hidden away by compiler-generated code; I consider that a >> good thing. We haven't created more layer violations -- if A is not >> allowed >> to hold B, it shouldn't be allowed to do so through a Supplementable-like >> system either IMO (and Supplementable had big warnings on it that it was >> prone to type confusion if used with inheritance). >> > > Though `Supplementable` and `base::SupportsUserData` have sharp edges and > could use improvement, the underlying abstraction still has value. > > >> >> To put it another way: If you came to the current Blink code base with no >> knowledge of the past, would anyone say that we should take a lot of the >> members on ExecutionContext and stick them into an untyped hash table? >> (If so, should e.g. OriginTrialContext have been part of this table, >> which it wasn't before?) A lot of stuff was stuck into ExecutionContext's >> Supplementable without even being in different layers, and I'm not sure >> what >> distinguished them from the other Members that were there before. And I've >> honestly never seen this pattern recommended anywhere else before; it >> looked >> odd to me all along, which is why I invested time in removing it. >> > > It's not a common pattern to embed dozens of forward-declared pointer > fields in a class. It happens to work in Blink because the types live on > the Oilpan heap, but non-Oilpan types would need additional indirections > for this to work. In addition, the supplement pattern is widely used in > Chrome; it's not unique to Blink. It's used in //content (via > `base::SupportsUserData` in `WebContents`, `RenderFrameHost`, and > `RenderFrame`; //content also supports a similar primitive called > `DocumentUserData`); in addition, there's also a variant in //ui that > supports stronger typing at the cost of additional magic > <https://source.chromium.org/chromium/chromium/src/+/main:ui/base/unowned_user_data/README.md> > . > > Code search claims 290+ non-test subclasses of `WebContentsUserData`; I do > not think any //content/OWNER would approve a CL that added 290 opaque > pointer fields, getters, and setters to `content::WebContentsImpl`. > > >> But as others have pointed out, it's water under the bridge. I guess it >> _is_ >> possible to revert it still if you wish, but it would be very >> conflict-prone. >> > > Unfortunately, it seems multiple people (myself included) assumed that > this discussion had happened, and I apologize for that. But I think it was > a mistake to remove the abstraction without more discussion, and that > discussion should still happen. > > Daniel > > >> >> /* Steinar */ >> -- >> Homepage: https://www.sesse.net/ >> > -- You received this message because you are subscribed to the Google Groups "blink-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAF3XrKqZimbwW6Xs1EVzXnxisHN6Th%3DFfCtAtQZq2Oj3rNMxkA%40mail.gmail.com.
