Move part of metrics from chrome/common to components

Move part of metrics from chrome/common to components

Chrome OS needs to have access to the metrics aggretion and serialization logic
to build a metric sender service replacing chrome. Protobuf definitions are
moved to components too.
The code remaining will be extrated in components/metrics and
components/variation during a refactoring later this year.

BUG=chromium:360183
TEST=run unittests
TBR=sky@chromium.org, benwells@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267709

[bsimonnet, 2014-04-15 21:23:22]

Here is the change that moves part of //chrome/common/metrics in
//components/metrics.
I'm trying to figure out if the linux_chromeos_asan failure is normal.

[Ilya Sherman, 2014-04-15 23:00:40]

Which parts of this CL are manual vs. automated?

I'm surprised not to see a DEPS file for the new component.  Is that
intentional?

https://codereview.chromium.org/239093004/diff/30001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/239093004/diff/30001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.cc:73: using metrics::MetricsLogBase;
nit: Alphabetize, please.

https://codereview.chromium.org/239093004/diff/30001/components/metrics.gypi
File components/metrics.gypi (right):

https://codereview.chromium.org/239093004/diff/30001/components/metrics.gypi#...
components/metrics.gypi:29: 'metrics/proto/user_action_event.proto',
The proto files should be in the proto target, not in the main target (same as
they were in the file you moved them from).

https://codereview.chromium.org/239093004/diff/30001/components/metrics/metri...
File components/metrics/metrics_log_base.cc (right):

https://codereview.chromium.org/239093004/diff/30001/components/metrics/metri...
components/metrics/metrics_log_base.cc:28: bool IsTestingID(const std::string&
id) { return id.size() < 16; }
nit: This should retain its previous formatting.  clang-format is being updated
to match this, but I guess that update hasn't been rolled out yet.

https://codereview.chromium.org/239093004/diff/30001/components/metrics/metri...
components/metrics/metrics_log_base.cc:138: }  // namespace metrics
nit: There should be a blank line above this.  (Please check for these
throughout the CL; I'll stop commenting explicitly for each.)

https://codereview.chromium.org/239093004/diff/30001/components/metrics/metri...
File components/metrics/metrics_log_base.h (right):

https://codereview.chromium.org/239093004/diff/30001/components/metrics/metri...
components/metrics/metrics_log_base.h:22: namespace metrics {
nit: There should be a blank line after this one.  It's only omitted for
forward-declarations.

https://codereview.chromium.org/239093004/diff/30001/components/metrics/metri...
components/metrics/metrics_log_base.h:106: }  // namespace metrics
nit: And, correspondingly, a blank line before this one.

https://codereview.chromium.org/239093004/diff/30001/components/metrics/proto...
File components/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/239093004/diff/30001/components/metrics/proto...
components/metrics/proto/omnibox_event.proto:195: // suggestion logging.
nit: Please revert the whitespace changes to this file.  If you're keen on
making them, please split that off to a separate CL.  That will help keep blame
history clearer.

https://codereview.chromium.org/239093004/diff/30001/components/metrics/proto...
File components/metrics/proto/perf_data.proto (right):

https://codereview.chromium.org/239093004/diff/30001/components/metrics/proto...
components/metrics/proto/perf_data.proto:185: optional uint64 sample_time = 5
[deprecated = true];
Ditto

https://codereview.chromium.org/239093004/diff/30001/components/metrics/proto...
File components/metrics/proto/profiler_event.proto (left):

https://codereview.chromium.org/239093004/diff/30001/components/metrics/proto...
components/metrics/proto/profiler_event.proto:13: 
Ditto

https://codereview.chromium.org/239093004/diff/30001/components/metrics/proto...
File components/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/239093004/diff/30001/components/metrics/proto...
components/metrics/proto/system_profile.proto:150: //
http://src.chromium.org/viewvc/chrome/trunk/src/content/public/common/gpu_per...
Ditto

https://codereview.chromium.org/239093004/diff/30001/components/metrics/proto...
components/metrics/proto/system_profile.proto:277: // log.
Ditto.  It does seem worthwhile to clean up the protobuf file formatting, but
let's do that in a separate CL.  Thanks!

[Ilya Sherman, 2014-04-15 23:41:03]

Btw, I'm seeing the same compile failures on the linux_chromeos_asan bot for an
unrelated CL, so I bet you can safely ignore those.

[bsimonnet, 2014-04-16 00:06:44]

On 2014/04/15 23:00:40, Ilya Sherman wrote:
> Which parts of this CL are manual vs. automated?
The code is mainly a combination of mv, git cl format, clang-format.

The manual part is:
* the namespacing (the code has to be in the metrics namespace),
* changing include statements from chrome/browser/metrics to components/metrics
* moving the logic that populates the channel in system_profile into
metrics_log.cc
* adding a test to check that the moved logic is still working.

> I'm surprised not to see a DEPS file for the new component.  Is that
> intentional?

There was no DEPS in the first version. So far, the component only depends on
itself, base and protobuf.
None of those need to be in a DEPS, right? Should we add one anyway (even if
empty)?

[Alexei Svitkine (slow), 2014-04-16 15:19:32]

I think we want to eventually merge MetricsLogBase and MetricsLog.

So I'm not sure that it makes sense to move MetricsLogBase to components while
moving some stuff from MetricsLogBase that depends on chrome/ to MetricsLog is
useful in the long term, given that we'll want both to be in components/
eventually.

Although I can persuaded that this is a useful stepping stone, I think we need
to think about the path to the final state here. (e.g. how to make the code in
MetricsLog that depends on chrome/ to not do so, so that it can be moved to
components/).

[bsimonnet, 2014-04-16 16:39:59]

On 2014/04/16 15:19:32, Alexei Svitkine wrote:
> I think we want to eventually merge MetricsLogBase and MetricsLog.
> 
> So I'm not sure that it makes sense to move MetricsLogBase to components while
> moving some stuff from MetricsLogBase that depends on chrome/ to MetricsLog is
> useful in the long term, given that we'll want both to be in components/
> eventually.
> 
> Although I can persuaded that this is a useful stepping stone, I think we need
> to think about the path to the final state here. (e.g. how to make the code in
> MetricsLog that depends on chrome/ to not do so, so that it can be moved to
> components/).

I agree. I am doing the easy part of the metrics componentization (moving files)
while pushing the real work (refactoring metrics_log to remove the dependency)
on
the side.
metrics_log_base is a very simple class. metrics_log seems to contain a lot more
logic and refactoring it would probably require refactoring of the rest of
metrics.

This changed seemed like a good middleground as:
* it gives ChromeOS the ability to access the metrics protobufs and basic
histogram
serialization mechanism before the full componentization is done.
* it is not making things worse for chrome in ease of componentization. It
arguably
helps a little by grouping the chrome specific logic in one place (metrics_log).

I don't think I am experienced enough with metrics (or C++) to do a good job at
refactoring metrics_log but I am willing to help with the review when that
happens.

[Alexei Svitkine (slow), 2014-04-16 17:01:00]

Fair enough. I'm okay with this if it's a useful stepping stone towards the
end goal, just wanted to make sure we're on the same page regarding the
eventual goal.


On Wed, Apr 16, 2014 at 12:40 PM, <bsimonnet@chromium.org> wrote:

> On 2014/04/16 15:19:32, Alexei Svitkine wrote:
>
>> I think we want to eventually merge MetricsLogBase and MetricsLog.
>>
>
>  So I'm not sure that it makes sense to move MetricsLogBase to components
>> while
>> moving some stuff from MetricsLogBase that depends on chrome/ to
>> MetricsLog is
>> useful in the long term, given that we'll want both to be in components/
>> eventually.
>>
>
>  Although I can persuaded that this is a useful stepping stone, I think we
>> need
>> to think about the path to the final state here. (e.g. how to make the
>> code in
>> MetricsLog that depends on chrome/ to not do so, so that it can be moved
>> to
>> components/).
>>
>
> I agree. I am doing the easy part of the metrics componentization (moving
> files)
> while pushing the real work (refactoring metrics_log to remove the
> dependency)
> on
> the side.
> metrics_log_base is a very simple class. metrics_log seems to contain a
> lot more
> logic and refactoring it would probably require refactoring of the rest of
> metrics.
>
> This changed seemed like a good middleground as:
> * it gives ChromeOS the ability to access the metrics protobufs and basic
> histogram
> serialization mechanism before the full componentization is done.
> * it is not making things worse for chrome in ease of componentization. It
> arguably
> helps a little by grouping the chrome specific logic in one place
> (metrics_log).
>
> I don't think I am experienced enough with metrics (or C++) to do a good
> job at
> refactoring metrics_log but I am willing to help with the review when that
> happens.
>
> https://codereview.chromium.org/239093004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bsimonnet, 2014-04-18 18:55:52]

I am rerunning trybots on android_dbg and win_rel, the fail seem to be unrelaed.
chromeos_asan seems to be failing for everyone.

Do you have more comments ?

[Ilya Sherman, 2014-04-18 19:03:06]

Sorry for the delay, I'd missed your reply.

On 2014/04/16 00:06:44, bsimonnet wrote:
> There was no DEPS in the first version. So far, the component only depends on
> itself, base and protobuf.
> None of those need to be in a DEPS, right? Should we add one anyway (even if
> empty)?

The DEPS file should enforce that nothing that we *don't* want is included.

[Ilya Sherman, 2014-04-18 19:07:09]

On 2014/04/18 19:03:06, Ilya Sherman wrote:
> Sorry for the delay, I'd missed your reply.

Looking back at the Gmail thread, it looks like Rietveld never sent out an email
for your reply.  Very odd.  :/

[Ilya Sherman, 2014-04-19 03:44:51]

LGTM with a final nit.  For future reference, please do respond "Done." for
comments that you've addressed -- it helps your reviewers keep track of the
state of the review.  Thanks! :)

https://codereview.chromium.org/239093004/diff/90001/components/metrics/DEPS
File components/metrics/DEPS (right):

https://codereview.chromium.org/239093004/diff/90001/components/metrics/DEPS#...
components/metrics/DEPS:4: "-content",
nit: Please alphabetize.

[Ilya Sherman, 2014-04-19 07:02:48]

https://chromiumcodereview.appspot.com/239093004/diff/90001/components/metric...
File components/metrics/DEPS (right):

https://chromiumcodereview.appspot.com/239093004/diff/90001/components/metric...
components/metrics/DEPS:2: "-chrome",
nit: No need to add this either, actually.  Components are never allowed to
depend on Chrome, so this isn't unique to this particular component.

[bsimonnet, 2014-04-21 15:17:05]

Perfect, Thanks for the great feedback!

https://codereview.chromium.org/239093004/diff/90001/components/metrics/DEPS
File components/metrics/DEPS (right):

https://codereview.chromium.org/239093004/diff/90001/components/metrics/DEPS#...
components/metrics/DEPS:2: "-chrome",
On 2014/04/19 07:02:49, Ilya Sherman wrote:
> nit: No need to add this either, actually.  Components are never allowed to
> depend on Chrome, so this isn't unique to this particular component.

Done.

https://codereview.chromium.org/239093004/diff/90001/components/metrics/DEPS#...
components/metrics/DEPS:4: "-content",
On 2014/04/19 03:44:52, Ilya Sherman wrote:
> nit: Please alphabetize.

Done.

[bsimonnet, 2014-04-21 15:17:24]

The CQ bit was checked by bsimonnet@chromium.org

[commit-bot: I haz the power, 2014-04-21 15:17:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bsimonnet@chromium.org/239093004/110001

[commit-bot: I haz the power, 2014-04-21 15:17:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-21 15:17:52]

Failed to apply patch for chrome/chrome_browser.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/chrome_browser.gypi
  Hunk #1 FAILED at 33.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/chrome_browser.gypi.rej

Patch:       chrome/chrome_browser.gypi
Index: chrome/chrome_browser.gypi
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index
38ee9e32429a701b61c069d96f9684ee6a3117b3..48d65cbeac419c9f5c2e4867b1c2a54c2534e204
100644
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -33,6 +33,7 @@
         '../components/components.gyp:data_reduction_proxy_browser',
         '../components/components.gyp:domain_reliability',
         '../components/components.gyp:favicon_core',
+        '../components/components.gyp:metrics',
         '../components/components.gyp:navigation_metrics',
         '../components/components.gyp:os_crypt',
         '../components/components.gyp:password_manager_core_browser',

[bsimonnet, 2014-04-22 15:26:38]

The CQ bit was checked by bsimonnet@chromium.org

[commit-bot: I haz the power, 2014-04-22 15:27:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bsimonnet@chromium.org/239093004/130001

[Alexei Svitkine (slow), 2014-04-22 15:28:09]

https://codereview.chromium.org/239093004/diff/110001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_log_unittest.cc (right):

https://codereview.chromium.org/239093004/diff/110001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log_unittest.cc:654: TEST_F(MetricsLogTest,
ChromeVersionWritten) {
The name of this test should say "channel" somewhere.

https://codereview.chromium.org/239093004/diff/110001/components/metrics.gypi
File components/metrics.gypi (right):

https://codereview.chromium.org/239093004/diff/110001/components/metrics.gypi...
components/metrics.gypi:28: 'target_name': 'component_metrics_proto',
I think you will need to add an explicit dependency on this sub-library to
targets that include the proto headers. Transitivity doesn't work in this case.

This is confusing but basically, it's because the gyp dependency graph specifies
dependencies that are needed for linking, rather than compiling. So without the
explicit dependency, code that includes histogram_event.pb.h will start to be
compiled before histogram_event.pb.h is generated.

[bsimonnet, 2014-04-22 15:31:30]

The CQ bit was unchecked by bsimonnet@chromium.org

[bsimonnet, 2014-04-22 16:52:42]

https://codereview.chromium.org/239093004/diff/110001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_log_unittest.cc (right):

https://codereview.chromium.org/239093004/diff/110001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log_unittest.cc:654: TEST_F(MetricsLogTest,
ChromeVersionWritten) {
On 2014/04/22 15:28:10, Alexei Svitkine wrote:
> The name of this test should say "channel" somewhere.

Done.

https://codereview.chromium.org/239093004/diff/110001/components/metrics.gypi
File components/metrics.gypi (right):

https://codereview.chromium.org/239093004/diff/110001/components/metrics.gypi...
components/metrics.gypi:28: 'target_name': 'component_metrics_proto',
I added a dependency on components.gyp:component_metrics_proto for
chrome_browser.
Do we need the same for chrome_tests_unit.gyp ?
(chrome/browser/metrics/extension_metrics_unittest.cc include a protobuf file)
It depends on chromium_browser_dependencies currently. Would it have the same
problem ?

On 2014/04/22 15:28:10, Alexei Svitkine wrote:
> I think you will need to add an explicit dependency on this sub-library to
> targets that include the proto headers. Transitivity doesn't work in this
case.
> 
> This is confusing but basically, it's because the gyp dependency graph
specifies
> dependencies that are needed for linking, rather than compiling. So without
the
> explicit dependency, code that includes histogram_event.pb.h will start to be
> compiled before histogram_event.pb.h is generated.

[Alexei Svitkine (slow), 2014-04-22 20:51:16]

LGTM

https://codereview.chromium.org/239093004/diff/110001/components/metrics.gypi
File components/metrics.gypi (right):

https://codereview.chromium.org/239093004/diff/110001/components/metrics.gypi...
components/metrics.gypi:28: 'target_name': 'component_metrics_proto',
On 2014/04/22 16:52:43, bsimonnet wrote:
> I added a dependency on components.gyp:component_metrics_proto for
> chrome_browser.
> Do we need the same for chrome_tests_unit.gyp ?
> (chrome/browser/metrics/extension_metrics_unittest.cc include a protobuf file)
> It depends on chromium_browser_dependencies currently. Would it have the same
> problem ?
> 
> On 2014/04/22 15:28:10, Alexei Svitkine wrote:
> > I think you will need to add an explicit dependency on this sub-library to
> > targets that include the proto headers. Transitivity doesn't work in this
> case.
> > 
> > This is confusing but basically, it's because the gyp dependency graph
> specifies
> > dependencies that are needed for linking, rather than compiling. So without
> the
> > explicit dependency, code that includes histogram_event.pb.h will start to
be
> > compiled before histogram_event.pb.h is generated.
> 

It's probably needed. See some of the latter commits in http://crbug.com/266007
where I encountered this when making variations a component.

I think if you clean your output dir then build the specific target you're
wondering about, you can check whether it fails or not.

https://codereview.chromium.org/239093004/diff/140001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (left):

https://codereview.chromium.org/239093004/diff/140001/chrome/chrome_common.gy...
chrome/chrome_common.gypi:44: '<(DEPTH)/components/components.gyp:variations',
Does anything in chrome/common/metrics still include the UMA proto headers? If
so, the dependency needs to be added here too. (If not, then feel free to ignore
this comment.)

[bsimonnet, 2014-04-23 00:08:00]

https://codereview.chromium.org/239093004/diff/140001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (left):

https://codereview.chromium.org/239093004/diff/140001/chrome/chrome_common.gy...
chrome/chrome_common.gypi:44: '<(DEPTH)/components/components.gyp:variations',
Everything was extracted, chrome/common/metrics dont need any proto from
components/metrics
On 2014/04/22 20:51:17, Alexei Svitkine wrote:
> Does anything in chrome/common/metrics still include the UMA proto headers? If
> so, the dependency needs to be added here too. (If not, then feel free to
ignore
> this comment.)

[bsimonnet, 2014-04-24 14:55:33]

On 2014/04/23 00:08:00, bsimonnet wrote:
>
https://codereview.chromium.org/239093004/diff/140001/chrome/chrome_common.gypi
> File chrome/chrome_common.gypi (left):
> 
>
https://codereview.chromium.org/239093004/diff/140001/chrome/chrome_common.gy...
> chrome/chrome_common.gypi:44: '<(DEPTH)/components/components.gyp:variations',
> Everything was extracted, chrome/common/metrics dont need any proto from
> components/metrics
> On 2014/04/22 20:51:17, Alexei Svitkine wrote:
> > Does anything in chrome/common/metrics still include the UMA proto headers?
If
> > so, the dependency needs to be added here too. (If not, then feel free to
> ignore
> > this comment.)

chromeos_asan still failing. Win_chromium_rel seems unrelated. I'm commiting.

[bsimonnet, 2014-04-24 14:55:43]

The CQ bit was checked by bsimonnet@chromium.org

[commit-bot: I haz the power, 2014-04-24 14:56:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bsimonnet@chromium.org/239093004/180001

[commit-bot: I haz the power, 2014-04-24 15:02:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-24 15:02:35]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[rkc, 2014-04-24 19:11:12]

lgtm

[blundell, 2014-04-28 07:54:36]

not LGTM pending resolution of the below question

Wasn't there an issue with dependencies, wherein you wanted to be able to ensure
that specific bits of the metrics component didn't pull in dependencies that
other bits were going to pull in? Nothing in the structure that I see here seems
like it will cleanly support that.

[blundell, 2014-04-30 14:53:58]

Ok, my understanding from offline discussion is the following:

- Everything that you're putting in the metrics component now is the code that
will only be able to depend on //base
- You'd prefer to just put it into a simple structure now and have us figure out
where it should finally end up when we're doing the larger metrics
componentization later this spring
- You're OK with this code moving from its current location sometime later on
this spring, but you'd like it to stay in a single directory if possible

Is that right? If so, I'm OK with this approach at the current time.

[bsimonnet, 2014-04-30 15:47:41]

On 2014/04/30 14:53:58, blundell wrote:
> Ok, my understanding from offline discussion is the following:
> 
> - Everything that you're putting in the metrics component now is the code that
> will only be able to depend on //base
> - You'd prefer to just put it into a simple structure now and have us figure
out
> where it should finally end up when we're doing the larger metrics
> componentization later this spring
> - You're OK with this code moving from its current location sometime later on
> this spring, but you'd like it to stay in a single directory if possible
> 
> Is that right? If so, I'm OK with this approach at the current time.

It is :)

[blundell, 2014-04-30 15:51:16]

Could you add a README explaining the current structure of this component and
detailing what structure you would like to keep in place when the component has
to be refactored?

[blundell, 2014-04-30 15:51:58]

Other than that, rubberstamp LGTM from me.

[bsimonnet, 2014-04-30 23:21:36]

sky, benwells, can you sign on it too?

[benwells, 2014-05-01 00:58:14]

lgtm.

BTW TBR is fine for this kind of refactoring fallout, where someone has reviewed
the main change.

[sky, 2014-05-01 03:23:23]

What do you need me to review here?

[sky, 2014-05-01 03:23:43]

Also, if it's just a bunch of include changes you need me to review, feel free
to TBR it.

[bsimonnet, 2014-05-01 16:13:44]

On 2014/05/01 03:23:43, sky wrote:
> Also, if it's just a bunch of include changes you need me to review, feel free
> to TBR it.

It was. I was not sure about the TBR. I added you and sky to it :)

[bsimonnet, 2014-05-01 22:13:54]

On 2014/05/01 16:13:44, bsimonnet wrote:
> On 2014/05/01 03:23:43, sky wrote:
> > Also, if it's just a bunch of include changes you need me to review, feel
free
> > to TBR it.
> 
> It was. I was not sure about the TBR. I added you and sky to it :)

The trybot failure dont seem to be linked to this CL. I'm commiting.

[bsimonnet, 2014-05-01 22:14:40]

The CQ bit was checked by bsimonnet@chromium.org

[commit-bot: I haz the power, 2014-05-01 22:15:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bsimonnet@chromium.org/239093004/220001

[commit-bot: I haz the power, 2014-05-01 22:19:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-01 22:19:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[bsimonnet, 2014-05-01 22:24:45]

The CQ bit was checked by bsimonnet@chromium.org

[commit-bot: I haz the power, 2014-05-01 22:26:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bsimonnet@chromium.org/239093004/220001

[Ilya Sherman, 2014-05-01 22:28:26]

You'll want to TBR someone for the
chrome/browser/autocomplete/autocomplete_provider.h change as well -- I think
that's why the presubmit step is failing for you.

[bsimonnet, 2014-05-01 22:31:22]

On 2014/05/01 22:28:26, Ilya Sherman wrote:
> You'll want to TBR someone for the
> chrome/browser/autocomplete/autocomplete_provider.h change as well -- I think
> that's why the presubmit step is failing for you.

Yeah I added the owner to the TBR field of the commit message of the last patch
but it looks like i need to update the description here manually.

[commit-bot: I haz the power, 2014-05-02 00:59:11]

Change committed as 267709

[blundell, 2014-05-05 08:16:50]

https://codereview.chromium.org/239093004/diff/220001/components/metrics/README
File components/metrics/README (right):

https://codereview.chromium.org/239093004/diff/220001/components/metrics/READ...
components/metrics/README:21: It is a plus if the component stays in a single
directory as it would be easier
nit: To avoid confusion, I would say "if the code currently in the component
(i.e., the code that can depend only on //base)" instead of "if the component".