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/CAF3XrKrX%2B3B5vuTdjLb_SDBo%3D1Nva-U6rBCV6V9T6uCAr4tJ8Q%40mail.gmail.com.
