Dependencies: //ui/base, //chromeos, //components

[Václav Brožek]

(for explicit Cc explanation, see P.S.)

Hi all,

I recently hit a dependency cycle when reviewing a CL which tried to use a flag from //ui/base in //components/autofill.

The cycle was as follows:

• (1)  //ui/base:base ->
• (2)  //chromeos:chromeos ->
• (3)  //components/password_manager/core/browser:hash_password_manager ->
• (4)  //components/password_manager/core/common:common ->
• (5)  //components/autofill/core/common:common ->
• (6)  //ui/base:base

The dependency (1) -> (2) is old (has been there since GYP times).

The dependency (2) -> (3) newer. I seems to make sense, as long as we want chromeos to depend on components.

The dependencies (3) -> (4) -> (5) are WAI, this is how the particular components and their layers were meant to work.

The dependency (5) -> (6) is using a UI flag in components, which is exactly what the CL introducing that flag foresaw.

Squashing the harmless dependencies, this is thus the triangle of: //ui/base -> //chromeos -> //components -> //ui/base. Is there a particular edge which should not be there? Or is the answer that for some of the big directories dependencies need to be more granular?

In my particular case, there is no real dependency cycle, because the part of //ui/base needed in (6) is disjoint from the one in (1) which uses (2). So it seems like one systematic way to fix this would be to reduce the dependency (1) -> (2) to only parts of //ui/base which need //chromeos-specific stuff. Looking at //ui/base, there is a 'ime/chromeos' subdirectory which seems to contain most of things requiring //chromeos (but not all).

//ui/base or //chromeos experts -- do you see a way to reduce (1) -> (2) as suggested?

The alternative might be the need to introduce a finer dependency structure inside the password_manager and autofill components, but before that I'd like to get a confirmation that the big triangle of dependencies mentioned above is WAI.

In the case of the CL linked at the beginning, we are lucky to avoid this cycle in the end, because the kExperimentalUi flag needed from //ui/base only exist on Win, Mac and Linux. But it seems just a random luck, so perhaps we still should consider a better solution in the near future?

Cheers,
Vaclav

P.S. I Cc-ed a few people who are experts in the touched areas and reviewed related CLs in the hope to get their opinion: xiyuan@ for //chromeos and (2) -> (3), thakis@ for //ui/base and (1) -> (2), and dvadym@ for hash_password_manager and (2) -> (3).

[Nico Weber]

ui/base is supposed to be usable from components, so either 1->2 or 2->3 is wrong -- chromeos/OWNERS probably know if chromeos:chromeos is meant to be a lower-level or a higher-level target. Maybe it needs to be split into two targets.

[Scott Violet]

My suspicion is 2->3 is wrong. One option is to put ui_base_features into it's own build targets.

  -Scott

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAN8iHej8ta-XQXazA35ZunXMjWpGGbv18pYSmXGAZJMquv6UzA%40mail.gmail.com.

[Xiyuan Xia]

I would like //chromeos:chromeos to be lower than //components. But it has deps on //components and some of them are not going to be removed. Most of these deps are clear because they don't have a long string of deps. The exception is hash_password_manager (i.e. 2->3) that seems to be hairy. The intention of the deps is to make UserContext carry the user password hash along the auth code path. It only needs "struct PasswordHashData". How about move the struct into its own build target.

[Václav Brožek]

Thanks all for the useful information!

Based on Xiyuan's suggestion, I created https://crrev.com/c/1092863.

Cheers,

Vaclav