Adding a dependency from content/renderer to components/page_load_metrics/common

[Tom McKee]

Hi all,

Per the instructions in content/renderer/DEPS, I'm reaching out for opinions on whether it is okay or not to add a dependency to a component in content/renderer.

The component is in components/page_load_metrics (it's for collecting metrics related to page-load like First Contentful Paint and, more recently, initial interactivity like First Input Delay).

See my WIP CL for extra context but I'm hoping to define an interface in components/page_load_metrics/common/ that we'll dependency inject to the compositor (alongside the UkmRecorder that gets injected today).

Let me know if this seems like an egregious layering violation or business as usual :)

P.S. while trying to answer this question myself, I found a diagram that is labeled as out-of-date. Indeed, there's no mention of components/ or cc/ and blink is labeled as WebKit... do we know what the diagram should look like today? I'd be happy to whip up a diagram if someone could help sanity-check which labels should exist and at what layer.

Cheers,

- Tom

[John Abd-El-Malek]

On Tue, Mar 31, 2020 at 7:06 AM Tom McKee <tomm...@chromium.org> wrote:

Hi all,

Per the instructions in content/renderer/DEPS, I'm reaching out for opinions on whether it is okay or not to add a dependency to a component in content/renderer.

The component is in components/page_load_metrics (it's for collecting metrics related to page-load like First Contentful Paint and, more recently, initial interactivity like First Input Delay).

See my WIP CL for extra context but I'm hoping to define an interface in components/page_load_metrics/common/ that we'll dependency inject to the compositor (alongside the UkmRecorder that gets injected today).

Let me know if this seems like an egregious layering violation or business as usual :)

Generally PLM sits on top of content, so having content/ reach back to it would be a circular dependency (even if it works due to different GN targets).

I assume PLM would implement this interface later, and content would expose a way for it to set itself as the implementation right? Can we just define the interface in content/public/renderer? If components/PLM uses this interface on iOS as well, it could have an adapter to convert between the two interfaces.

 

P.S. while trying to answer this question myself, I found a diagram that is labeled as out-of-date. Indeed, there's no mention of components/ or cc/ and blink is labeled as WebKit... do we know what the diagram should look like today? I'd be happy to whip up a diagram if someone could help sanity-check which labels should exist and at what layer.

Yes that is quite out of date unfortunately. We have been moving towards putting more of the documentation in the README/DEPS files in content, components etc.

Most components/ depend on content and so content/ can't depend on them. There are also different kind of components, e.g. those that sit on top of content vs those that share code between content and blink or ios etc... So whether content/ can or can't depend on a component depends on why that component was created. 

Cheers,

- Tom

[Tom McKee]

On Tue, Mar 31, 2020 at 3:16 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Mar 31, 2020 at 7:06 AM Tom McKee <tomm...@chromium.org> wrote:

Hi all,

Per the instructions in content/renderer/DEPS, I'm reaching out for opinions on whether it is okay or not to add a dependency to a component in content/renderer.

The component is in components/page_load_metrics (it's for collecting metrics related to page-load like First Contentful Paint and, more recently, initial interactivity like First Input Delay).

See my WIP CL for extra context but I'm hoping to define an interface in components/page_load_metrics/common/ that we'll dependency inject to the compositor (alongside the UkmRecorder that gets injected today).

Let me know if this seems like an egregious layering violation or business as usual :)

Generally PLM sits on top of content, so having content/ reach back to it would be a circular dependency (even if it works due to different GN targets). 

I assume PLM would implement this interface later, and content would expose a way for it to set itself as the implementation right? Can we just define the interface in content/public/renderer? If components/PLM uses this interface on iOS as well, it could have an adapter to convert between the two interfaces.

 

For better or worse, PLM does not seem to be enabled for iOS, so no worries there. I tried moving it to content/renderer but it's also used in cc/ which, it seems, is even lower still in the hierarchy, beneath content. I've updated the linked CL with a version that declares the interface in cc/, we'll see what the reviewers think but, in hindsight, it does seem more natural to declare a dependency injection interface next to the injectee as opposed to the injector (does that make sense?).

 

P.S. while trying to answer this question myself, I found a diagram that is labeled as out-of-date. Indeed, there's no mention of components/ or cc/ and blink is labeled as WebKit... do we know what the diagram should look like today? I'd be happy to whip up a diagram if someone could help sanity-check which labels should exist and at what layer.

Yes that is quite out of date unfortunately. We have been moving towards putting more of the documentation in the README/DEPS files in content, components etc.

Gotcha. In future, I'll poke around those a bit more to help grok the dependency tree.

Most components/ depend on content and so content/ can't depend on them. There are also different kind of components, e.g. those that sit on top of content vs those that share code between content and blink or ios etc... So whether content/ can or can't depend on a component depends on why that component was created. 

 

Ah, that makes more sense now. Thanks!

Cheers,

- Tom

Thanks for your help!

- Tom

 

[John Abd-El-Malek]

On Thu, Apr 2, 2020 at 6:35 AM Tom McKee <tomm...@chromium.org> wrote:

On Tue, Mar 31, 2020 at 3:16 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Mar 31, 2020 at 7:06 AM Tom McKee <tomm...@chromium.org> wrote:

Hi all,

Per the instructions in content/renderer/DEPS, I'm reaching out for opinions on whether it is okay or not to add a dependency to a component in content/renderer.

The component is in components/page_load_metrics (it's for collecting metrics related to page-load like First Contentful Paint and, more recently, initial interactivity like First Input Delay).

See my WIP CL for extra context but I'm hoping to define an interface in components/page_load_metrics/common/ that we'll dependency inject to the compositor (alongside the UkmRecorder that gets injected today).

Let me know if this seems like an egregious layering violation or business as usual :)

Generally PLM sits on top of content, so having content/ reach back to it would be a circular dependency (even if it works due to different GN targets). 

I assume PLM would implement this interface later, and content would expose a way for it to set itself as the implementation right? Can we just define the interface in content/public/renderer? If components/PLM uses this interface on iOS as well, it could have an adapter to convert between the two interfaces.

 

For better or worse, PLM does not seem to be enabled for iOS, so no worries there. I tried moving it to content/renderer but it's also used in cc/ which, it seems, is even lower still in the hierarchy, beneath content. I've updated the linked CL with a version that declares the interface in cc/, we'll see what the reviewers think but, in hindsight, it does seem more natural to declare a dependency injection interface next to the injectee as opposed to the injector (does that make sense?).

yep makes sense, agreed this is a good layering now. src/cc doesn't depend on components other than viz.