Move browser-agnostic code from file_system_util to file_system_core_util.

Move browser-agnostic code from file_system_util to file_system_core_util.

This changelist moves browser-agnositc code
(i.e. code that doesn't need BrowserThread or Profile classes)
from file_system_util.h/.cc/_unittest.cc
into file_system_core_util.h/.cc/_unittest.cc.

Note that this is the final change needed to declare drive::FileSystem and its
dependencies as browser-agnostic (at least when considering the product code;
some tests still depend on browser-specific things).

Test steps:
$ GYP_DEFINES="use_goma=1 gomadir=... chromeos=1" gclient sync
$ ninja -C out/Debug -j 150 chrome unit_tests browser_tests interactive_ui_tests
$ out/Debug/unit_tests

BUG=498951
TEST=Please see "Test steps" above.

Committed: https://crrev.com/5da1d547c262af0b7d34c1a0ac71d2a4f0e197f4
Cr-Commit-Position: refs/heads/master@{#336410}

[Łukasz Anforowicz, 2015-06-16 22:07:08]

lukasza@chromium.org changed reviewers:
+ hashimoto@chromium.org

[Łukasz Anforowicz, 2015-06-16 22:07:09]

Ryo, could you please take a look?

[hashimoto, 2015-06-16 23:53:03]

How about creating the destination component directory now, so that we can
directly move the code to the final destination?

[Łukasz Anforowicz, 2015-06-17 00:10:46]

On 2015/06/16 23:53:03, hashimoto wrote:
> How about creating the destination component directory now, so that we can
> directly move the code to the final destination?

I recognize that there are some advantages of doing just one move, as you
suggest above.  The major advantage is avoiding repeatedly touching things
outside of chrome/browser/chromeos/drive/.

OTOH, I think I prefer to split now and componentize later (in a separate
changelist):

- file_system_browser_util.h would remain in the current location even after the
componentization

- Splitting now means that file_system_util.cc doesn't need an exception in DEPS
file (in crrev.com/1184273005)

- Componentization is complicated (and a bit scary) and I would prefer to do it
in several small steps (rather than for example bundling it with the splitting
of file_system_util).  For example, I would prefer each of the subitems below to
be a separate step / changelist:

  *) componentize chrome/browser/drive/ (leaving
drive_notification_manager_factory.cc behind / uncomponentized + not touching
the PowerSaveBlocker dependency)

  *) move chrome/browser/drive/drive_notification_manager_factory.cc somewhere
else (chrome/browser/chromeos/drive/ seems wrong as it is a CrOS-specific
directory)

  *) remove browser dependencies from test code in
chrome/browser/chromeos/drive/ (OTOH not sure if this is a prerequisite for
componentization)

  *) componentize most of the things from under chrome/browser/chromeos/drive/

  *) turn on building and testing of the components on Windows

[hashimoto, 2015-06-17 00:37:02]

On 2015/06/17 00:10:46, Łukasz Anforowicz wrote:
> On 2015/06/16 23:53:03, hashimoto wrote:
> > How about creating the destination component directory now, so that we can
> > directly move the code to the final destination?
> 
> I recognize that there are some advantages of doing just one move, as you
> suggest above.  The major advantage is avoiding repeatedly touching things
> outside of chrome/browser/chromeos/drive/.
> 
> OTOH, I think I prefer to split now and componentize later (in a separate
> changelist):
> 
> - file_system_browser_util.h would remain in the current location even after
the
> componentization
> 
> - Splitting now means that file_system_util.cc doesn't need an exception in
DEPS
> file (in crrev.com/1184273005)
> 
> - Componentization is complicated (and a bit scary) and I would prefer to do
it
> in several small steps (rather than for example bundling it with the splitting
> of file_system_util).  For example, I would prefer each of the subitems below
to
> be a separate step / changelist:
> 
>   *) componentize chrome/browser/drive/ (leaving
> drive_notification_manager_factory.cc behind / uncomponentized + not touching
> the PowerSaveBlocker dependency)
> 
>   *) move chrome/browser/drive/drive_notification_manager_factory.cc somewhere
> else (chrome/browser/chromeos/drive/ seems wrong as it is a CrOS-specific
> directory)
> 
>   *) remove browser dependencies from test code in
> chrome/browser/chromeos/drive/ (OTOH not sure if this is a prerequisite for
> componentization)
> 
>   *) componentize most of the things from under chrome/browser/chromeos/drive/
> 
>   *) turn on building and testing of the components on Windows

I'm not sure if I'm understanding your comment correctly, but I think we can get
the same benefit by creating the destination directory now.

What I'm proposing is that:
 1. Create the destination directory (say, components/drive).
 2. Move all functions which don't depend on browser code to
components/drive/file_system_util.cc

OTOH, what the uploaded CL does is:
 1. Move all browser-dependent functions to
chrome/browser/chromeos/drive/file_system_browser_util.cc
 2. (after this CL gets landed) Create the destination directory.
 3. Move chrome/browser/chromeos/drive/file_system_util.cc to components/drive

I think it's better to create the destination directory now, and starting
componentization with moving the utility functions as the first step, because
this way we don't have to deal with users of browser-dependent functions which
we are not interested in (i.e. no need to replace file_system_util.h include
with file_system_browser_util.h).

[Łukasz Anforowicz, 2015-06-17 15:21:20]

On 2015/06/17 00:37:02, hashimoto wrote:
> On 2015/06/17 00:10:46, Łukasz Anforowicz wrote:
> > On 2015/06/16 23:53:03, hashimoto wrote:
> > > How about creating the destination component directory now, so that we can
> > > directly move the code to the final destination?
> > 
> > I recognize that there are some advantages of doing just one move, as you
> > suggest above.  The major advantage is avoiding repeatedly touching things
> > outside of chrome/browser/chromeos/drive/.
> > 
> > OTOH, I think I prefer to split now and componentize later (in a separate
> > changelist):
> > 
> > - file_system_browser_util.h would remain in the current location even after
> the
> > componentization
> > 
> > - Splitting now means that file_system_util.cc doesn't need an exception in
> DEPS
> > file (in crrev.com/1184273005)
> > 
> > - Componentization is complicated (and a bit scary) and I would prefer to do
> it
> > in several small steps (rather than for example bundling it with the
splitting
> > of file_system_util).  For example, I would prefer each of the subitems
below
> to
> > be a separate step / changelist:
> > 
> >   *) componentize chrome/browser/drive/ (leaving
> > drive_notification_manager_factory.cc behind / uncomponentized + not
touching
> > the PowerSaveBlocker dependency)
> > 
> >   *) move chrome/browser/drive/drive_notification_manager_factory.cc
somewhere
> > else (chrome/browser/chromeos/drive/ seems wrong as it is a CrOS-specific
> > directory)
> > 
> >   *) remove browser dependencies from test code in
> > chrome/browser/chromeos/drive/ (OTOH not sure if this is a prerequisite for
> > componentization)
> > 
> >   *) componentize most of the things from under
chrome/browser/chromeos/drive/
> > 
> >   *) turn on building and testing of the components on Windows
> 
> I'm not sure if I'm understanding your comment correctly, but I think we can
get
> the same benefit by creating the destination directory now.
> 
> What I'm proposing is that:
>  1. Create the destination directory (say, components/drive).
>  2. Move all functions which don't depend on browser code to
> components/drive/file_system_util.cc
> 
> OTOH, what the uploaded CL does is:
>  1. Move all browser-dependent functions to
> chrome/browser/chromeos/drive/file_system_browser_util.cc
>  2. (after this CL gets landed) Create the destination directory.
>  3. Move chrome/browser/chromeos/drive/file_system_util.cc to components/drive
> 
> I think it's better to create the destination directory now, and starting
> componentization with moving the utility functions as the first step, because
> this way we don't have to deal with users of browser-dependent functions which
> we are not interested in (i.e. no need to replace file_system_util.h include
> with file_system_browser_util.h).

Thank you for clarifying (I initially misunderstood and thought that you are
asking to move all browser-agnostic parts, not only browser-agnostic parts of
file_system_util as you are suggesting).

Nevertheless, I think I still prefer to proceed with the current / originally
proposed CL:

- Moving browser-agnostic parts of file_system_util to components/ requires
first removing chrome/ dependencies from the part of
file_system_util_unittest.cc that tests browser-agnostic parts of
file_system_util.cc.  Examples of chrome/ dependencies:
chrome/browser/chromeos/drive/file_system_browser_util.h [for
GetDriveMountPointPath(Profile*)], chrome/test/base/testing_profile.h [for
TestingProfile].  Removing these dependencies is doable, but I would prefer to
do it in a separate changelist.

- Having a components/drive/filesystem/ with only a single file_system_util
compilation unit looks weird and IMHO is not something that we would want to
keep in the long-term.  On the other hand, I think the changelist under review
is something that we can keep in the long-term, even if we cannot finish
componentization soon (i.e. making tests browser-independent is more difficult
than anticipated OR I get hit by a proverbial bus OR I simply cannot spend time
on componentization efforts in the near term, but have to focus on first
providing more data on the viability of Dokan-over-Drive4CrOS).

- This CL addresses my immediate goals ( 1) flushing out changes I had to make
to drive libraries to build Dokan-over-Drive4CrOS and which are currently stored
only on my dev box, 2) preventing regressions [browser-independence via DEPS
file;  unfortunately no easy way to enforce building on Windows without lots of
refactoring of GYP files that probably only makes sense after
componentization]).  Componentization *is* on my list of long-term goals, but in
the short-term I would prefer to wrap-up what I have and move onto other
immediate goals, rather than spending time on partial, incomplete
componentization (of file_system_util).

[hashimoto, 2015-06-17 17:26:09]

On Wed, Jun 17, 2015 at 8:21 AM, <lukasza@chromium.org> wrote:

> On 2015/06/17 00:37:02, hashimoto wrote:
>
>> On 2015/06/17 00:10:46, Łukasz Anforowicz wrote:
>> > On 2015/06/16 23:53:03, hashimoto wrote:
>> > > How about creating the destination component directory now, so that
>> we can
>> > > directly move the code to the final destination?
>> >
>> > I recognize that there are some advantages of doing just one move, as
>> you
>> > suggest above.  The major advantage is avoiding repeatedly touching
>> things
>> > outside of chrome/browser/chromeos/drive/.
>> >
>> > OTOH, I think I prefer to split now and componentize later (in a
>> separate
>> > changelist):
>> >
>> > - file_system_browser_util.h would remain in the current location even
>> after
>> the
>> > componentization
>> >
>> > - Splitting now means that file_system_util.cc doesn't need an
>> exception in
>> DEPS
>> > file (in crrev.com/1184273005)
>> >
>> > - Componentization is complicated (and a bit scary) and I would prefer
>> to do
>> it
>> > in several small steps (rather than for example bundling it with the
>>
> splitting
>
>> > of file_system_util).  For example, I would prefer each of the subitems
>>
> below
>
>> to
>> > be a separate step / changelist:
>> >
>> >   *) componentize chrome/browser/drive/ (leaving
>> > drive_notification_manager_factory.cc behind / uncomponentized + not
>>
> touching
>
>> > the PowerSaveBlocker dependency)
>> >
>> >   *) move chrome/browser/drive/drive_notification_manager_factory.cc
>>
> somewhere
>
>> > else (chrome/browser/chromeos/drive/ seems wrong as it is a
>> CrOS-specific
>> > directory)
>> >
>> >   *) remove browser dependencies from test code in
>> > chrome/browser/chromeos/drive/ (OTOH not sure if this is a prerequisite
>> for
>> > componentization)
>> >
>> >   *) componentize most of the things from under
>>
> chrome/browser/chromeos/drive/
>
>> >
>> >   *) turn on building and testing of the components on Windows
>>
>
>  I'm not sure if I'm understanding your comment correctly, but I think we
>> can
>>
> get
>
>> the same benefit by creating the destination directory now.
>>
>
>  What I'm proposing is that:
>>   1. Create the destination directory (say, components/drive).
>>   2. Move all functions which don't depend on browser code to
>> components/drive/file_system_util.cc
>>
>
>  OTOH, what the uploaded CL does is:
>>   1. Move all browser-dependent functions to
>> chrome/browser/chromeos/drive/file_system_browser_util.cc
>>   2. (after this CL gets landed) Create the destination directory.
>>   3. Move chrome/browser/chromeos/drive/file_system_util.cc to
>> components/drive
>>
>
>  I think it's better to create the destination directory now, and starting
>> componentization with moving the utility functions as the first step,
>> because
>> this way we don't have to deal with users of browser-dependent functions
>> which
>> we are not interested in (i.e. no need to replace file_system_util.h
>> include
>> with file_system_browser_util.h).
>>
>
> Thank you for clarifying (I initially misunderstood and thought that you
> are
> asking to move all browser-agnostic parts, not only browser-agnostic parts
> of
> file_system_util as you are suggesting).
>
> Nevertheless, I think I still prefer to proceed with the current /
> originally
> proposed CL:
>
> - Moving browser-agnostic parts of file_system_util to components/ requires
> first removing chrome/ dependencies from the part of
> file_system_util_unittest.cc that tests browser-agnostic parts of
> file_system_util.cc.  Examples of chrome/ dependencies:
> chrome/browser/chromeos/drive/file_system_browser_util.h [for
> GetDriveMountPointPath(Profile*)], chrome/test/base/testing_profile.h [for
> TestingProfile].  Removing these dependencies is doable, but I would
> prefer to
> do it in a separate changelist.
>
My main concern about your CL is that it makes a lot of unnecessary
changes, and "browser" in the new file name "file_system_browser_util" is
totally meaningless (generally speaking, all files under chrome/browser are
related to browser).
If you really want to make two file_system_util files, how about naming the
one which hosts browser-independent code differently (e.g.
file_system_core_util) so that we can put browser-dependent code in the
existing file_system_util?

>
> - Having a components/drive/filesystem/ with only a single file_system_util
> compilation unit looks weird and IMHO is not something that we would want
> to
> keep in the long-term.  On the other hand, I think the changelist under
> review
> is something that we can keep in the long-term, even if we cannot finish
> componentization soon (i.e. making tests browser-independent is more
> difficult
> than anticipated OR I get hit by a proverbial bus OR I simply cannot spend
> time
> on componentization efforts in the near term, but have to focus on first
> providing more data on the viability of Dokan-over-Drive4CrOS).
>
Starting by creating a new component directory with a small number of files
is the proper way to start componentization (see
https://codereview.chromium.org/330233002 for example).

Having a code under chrome/browser which says it shouldn't depend on
chrome/browser is much weirder than having a small directory under
components.
I have to admit we have been in this state for years, but it's not what we
want to (and originally intended to) keep in long-term.

>
> - This CL addresses my immediate goals ( 1) flushing out changes I had to
> make
> to drive libraries to build Dokan-over-Drive4CrOS and which are currently
> stored
> only on my dev box, 2) preventing regressions [browser-independence via
> DEPS
> file;  unfortunately no easy way to enforce building on Windows without
> lots of
> refactoring of GYP files that probably only makes sense after
> componentization]).  Componentization *is* on my list of long-term goals,
> but in
> the short-term I would prefer to wrap-up what I have and move onto other
> immediate goals, rather than spending time on partial, incomplete
> componentization (of file_system_util).
>
 By moving the code out of chrome/browser/chromeos, you will get test
coverage on Windows buildbots which should benefit you too.
Componentization is not so difficult (the only additional cost is making a
new directory + moving files), so we'd really appreciate if you could
finish this task which has been our homework for years.

>
> https://codereview.chromium.org/1192493003/
>

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

[Łukasz Anforowicz, 2015-06-17 17:53:48]

On 2015/06/17 17:26:09, hashimoto wrote:
> On Wed, Jun 17, 2015 at 8:21 AM, <mailto:lukasza@chromium.org> wrote:
> 
> > On 2015/06/17 00:37:02, hashimoto wrote:
> >
> >> On 2015/06/17 00:10:46, Łukasz Anforowicz wrote:
> >> > On 2015/06/16 23:53:03, hashimoto wrote:
> >> > > How about creating the destination component directory now, so that
> >> we can
> >> > > directly move the code to the final destination?
> >> >
> >> > I recognize that there are some advantages of doing just one move, as
> >> you
> >> > suggest above.  The major advantage is avoiding repeatedly touching
> >> things
> >> > outside of chrome/browser/chromeos/drive/.
> >> >
> >> > OTOH, I think I prefer to split now and componentize later (in a
> >> separate
> >> > changelist):
> >> >
> >> > - file_system_browser_util.h would remain in the current location even
> >> after
> >> the
> >> > componentization
> >> >
> >> > - Splitting now means that file_system_util.cc doesn't need an
> >> exception in
> >> DEPS
> >> > file (in crrev.com/1184273005)
> >> >
> >> > - Componentization is complicated (and a bit scary) and I would prefer
> >> to do
> >> it
> >> > in several small steps (rather than for example bundling it with the
> >>
> > splitting
> >
> >> > of file_system_util).  For example, I would prefer each of the subitems
> >>
> > below
> >
> >> to
> >> > be a separate step / changelist:
> >> >
> >> >   *) componentize chrome/browser/drive/ (leaving
> >> > drive_notification_manager_factory.cc behind / uncomponentized + not
> >>
> > touching
> >
> >> > the PowerSaveBlocker dependency)
> >> >
> >> >   *) move chrome/browser/drive/drive_notification_manager_factory.cc
> >>
> > somewhere
> >
> >> > else (chrome/browser/chromeos/drive/ seems wrong as it is a
> >> CrOS-specific
> >> > directory)
> >> >
> >> >   *) remove browser dependencies from test code in
> >> > chrome/browser/chromeos/drive/ (OTOH not sure if this is a prerequisite
> >> for
> >> > componentization)
> >> >
> >> >   *) componentize most of the things from under
> >>
> > chrome/browser/chromeos/drive/
> >
> >> >
> >> >   *) turn on building and testing of the components on Windows
> >>
> >
> >  I'm not sure if I'm understanding your comment correctly, but I think we
> >> can
> >>
> > get
> >
> >> the same benefit by creating the destination directory now.
> >>
> >
> >  What I'm proposing is that:
> >>   1. Create the destination directory (say, components/drive).
> >>   2. Move all functions which don't depend on browser code to
> >> components/drive/file_system_util.cc
> >>
> >
> >  OTOH, what the uploaded CL does is:
> >>   1. Move all browser-dependent functions to
> >> chrome/browser/chromeos/drive/file_system_browser_util.cc
> >>   2. (after this CL gets landed) Create the destination directory.
> >>   3. Move chrome/browser/chromeos/drive/file_system_util.cc to
> >> components/drive
> >>
> >
> >  I think it's better to create the destination directory now, and starting
> >> componentization with moving the utility functions as the first step,
> >> because
> >> this way we don't have to deal with users of browser-dependent functions
> >> which
> >> we are not interested in (i.e. no need to replace file_system_util.h
> >> include
> >> with file_system_browser_util.h).
> >>
> >
> > Thank you for clarifying (I initially misunderstood and thought that you
> > are
> > asking to move all browser-agnostic parts, not only browser-agnostic parts
> > of
> > file_system_util as you are suggesting).
> >
> > Nevertheless, I think I still prefer to proceed with the current /
> > originally
> > proposed CL:
> >
> > - Moving browser-agnostic parts of file_system_util to components/ requires
> > first removing chrome/ dependencies from the part of
> > file_system_util_unittest.cc that tests browser-agnostic parts of
> > file_system_util.cc.  Examples of chrome/ dependencies:
> > chrome/browser/chromeos/drive/file_system_browser_util.h [for
> > GetDriveMountPointPath(Profile*)], chrome/test/base/testing_profile.h [for
> > TestingProfile].  Removing these dependencies is doable, but I would
> > prefer to
> > do it in a separate changelist.
> >
> My main concern about your CL is that it makes a lot of unnecessary
> changes, and "browser" in the new file name "file_system_browser_util" is
> totally meaningless (generally speaking, all files under chrome/browser are
> related to browser).
> If you really want to make two file_system_util files, how about naming the
> one which hosts browser-independent code differently (e.g.
> file_system_core_util) so that we can put browser-dependent code in the
> existing file_system_util?

Good point.  I also dislike the names of the files.  I'll go ahead and apply
your suggestion (to end up with file_system_core_util and file_system_util).
> >
> > - Having a components/drive/filesystem/ with only a single file_system_util
> > compilation unit looks weird and IMHO is not something that we would want
> > to
> > keep in the long-term.  On the other hand, I think the changelist under
> > review
> > is something that we can keep in the long-term, even if we cannot finish
> > componentization soon (i.e. making tests browser-independent is more
> > difficult
> > than anticipated OR I get hit by a proverbial bus OR I simply cannot spend
> > time
> > on componentization efforts in the near term, but have to focus on first
> > providing more data on the viability of Dokan-over-Drive4CrOS).
> >
> Starting by creating a new component directory with a small number of files
> is the proper way to start componentization (see
> https://codereview.chromium.org/330233002 for example).
> 
Ok, point taken.

> Having a code under chrome/browser which says it shouldn't depend on
> chrome/browser is much weirder than having a small directory under
> components.
> I have to admit we have been in this state for years, but it's not what we
> want to (and originally intended to) keep in long-term.
> 
> >
> > - This CL addresses my immediate goals ( 1) flushing out changes I had to
> > make
> > to drive libraries to build Dokan-over-Drive4CrOS and which are currently
> > stored
> > only on my dev box, 2) preventing regressions [browser-independence via
> > DEPS
> > file;  unfortunately no easy way to enforce building on Windows without
> > lots of
> > refactoring of GYP files that probably only makes sense after
> > componentization]).  Componentization *is* on my list of long-term goals,
> > but in
> > the short-term I would prefer to wrap-up what I have and move onto other
> > immediate goals, rather than spending time on partial, incomplete
> > componentization (of file_system_util).
> >
>  By moving the code out of chrome/browser/chromeos, you will get test
> coverage on Windows buildbots which should benefit you too.
> Componentization is not so difficult (the only additional cost is making a
> new directory + moving files), so we'd really appreciate if you could
> finish this task which has been our homework for years.

There are also other costs: 
1. moving drive stuff from chrome/*.gyp* files into components/
2. componentizing tests
2.a. figuring out if current test dependencies [on chrome/] or ok for
components/drive/...)
2.b. scaffolding for main() that runs the tests (maybe this is very simple, or
maybe Chrome dependency makes it more difficult;  I don't know)

If possible, I would like to avoid these costs in the short-term.

[hashimoto, 2015-06-17 22:57:44]

On Wed, Jun 17, 2015 at 10:53 AM, <lukasza@chromium.org> wrote:

> On 2015/06/17 17:26:09, hashimoto wrote:
>
>  On Wed, Jun 17, 2015 at 8:21 AM, <mailto:lukasza@chromium.org> wrote:
>>
>
>  > On 2015/06/17 00:37:02, hashimoto wrote:
>> >
>> >> On 2015/06/17 00:10:46, Łukasz Anforowicz wrote:
>> >> > On 2015/06/16 23:53:03, hashimoto wrote:
>> >> > > How about creating the destination component directory now, so that
>> >> we can
>> >> > > directly move the code to the final destination?
>> >> >
>> >> > I recognize that there are some advantages of doing just one move, as
>> >> you
>> >> > suggest above.  The major advantage is avoiding repeatedly touching
>> >> things
>> >> > outside of chrome/browser/chromeos/drive/.
>> >> >
>> >> > OTOH, I think I prefer to split now and componentize later (in a
>> >> separate
>> >> > changelist):
>> >> >
>> >> > - file_system_browser_util.h would remain in the current location
>> even
>> >> after
>> >> the
>> >> > componentization
>> >> >
>> >> > - Splitting now means that file_system_util.cc doesn't need an
>> >> exception in
>> >> DEPS
>> >> > file (in crrev.com/1184273005)
>> >> >
>> >> > - Componentization is complicated (and a bit scary) and I would
>> prefer
>> >> to do
>> >> it
>> >> > in several small steps (rather than for example bundling it with the
>> >>
>> > splitting
>> >
>> >> > of file_system_util).  For example, I would prefer each of the
>> subitems
>> >>
>> > below
>> >
>> >> to
>> >> > be a separate step / changelist:
>> >> >
>> >> >   *) componentize chrome/browser/drive/ (leaving
>> >> > drive_notification_manager_factory.cc behind / uncomponentized + not
>> >>
>> > touching
>> >
>> >> > the PowerSaveBlocker dependency)
>> >> >
>> >> >   *) move chrome/browser/drive/drive_notification_manager_factory.cc
>> >>
>> > somewhere
>> >
>> >> > else (chrome/browser/chromeos/drive/ seems wrong as it is a
>> >> CrOS-specific
>> >> > directory)
>> >> >
>> >> >   *) remove browser dependencies from test code in
>> >> > chrome/browser/chromeos/drive/ (OTOH not sure if this is a
>> prerequisite
>> >> for
>> >> > componentization)
>> >> >
>> >> >   *) componentize most of the things from under
>> >>
>> > chrome/browser/chromeos/drive/
>> >
>> >> >
>> >> >   *) turn on building and testing of the components on Windows
>> >>
>> >
>> >  I'm not sure if I'm understanding your comment correctly, but I think
>> we
>> >> can
>> >>
>> > get
>> >
>> >> the same benefit by creating the destination directory now.
>> >>
>> >
>> >  What I'm proposing is that:
>> >>   1. Create the destination directory (say, components/drive).
>> >>   2. Move all functions which don't depend on browser code to
>> >> components/drive/file_system_util.cc
>> >>
>> >
>> >  OTOH, what the uploaded CL does is:
>> >>   1. Move all browser-dependent functions to
>> >> chrome/browser/chromeos/drive/file_system_browser_util.cc
>> >>   2. (after this CL gets landed) Create the destination directory.
>> >>   3. Move chrome/browser/chromeos/drive/file_system_util.cc to
>> >> components/drive
>> >>
>> >
>> >  I think it's better to create the destination directory now, and
>> starting
>> >> componentization with moving the utility functions as the first step,
>> >> because
>> >> this way we don't have to deal with users of browser-dependent
>> functions
>> >> which
>> >> we are not interested in (i.e. no need to replace file_system_util.h
>> >> include
>> >> with file_system_browser_util.h).
>> >>
>> >
>> > Thank you for clarifying (I initially misunderstood and thought that you
>> > are
>> > asking to move all browser-agnostic parts, not only browser-agnostic
>> parts
>> > of
>> > file_system_util as you are suggesting).
>> >
>> > Nevertheless, I think I still prefer to proceed with the current /
>> > originally
>> > proposed CL:
>> >
>> > - Moving browser-agnostic parts of file_system_util to components/
>> requires
>> > first removing chrome/ dependencies from the part of
>> > file_system_util_unittest.cc that tests browser-agnostic parts of
>> > file_system_util.cc.  Examples of chrome/ dependencies:
>> > chrome/browser/chromeos/drive/file_system_browser_util.h [for
>> > GetDriveMountPointPath(Profile*)], chrome/test/base/testing_profile.h
>> [for
>> > TestingProfile].  Removing these dependencies is doable, but I would
>> > prefer to
>> > do it in a separate changelist.
>> >
>> My main concern about your CL is that it makes a lot of unnecessary
>> changes, and "browser" in the new file name "file_system_browser_util" is
>> totally meaningless (generally speaking, all files under chrome/browser
>> are
>> related to browser).
>> If you really want to make two file_system_util files, how about naming
>> the
>> one which hosts browser-independent code differently (e.g.
>> file_system_core_util) so that we can put browser-dependent code in the
>> existing file_system_util?
>>
>
> Good point.  I also dislike the names of the files.  I'll go ahead and
> apply
> your suggestion (to end up with file_system_core_util and
> file_system_util).
>
>> >
>> > - Having a components/drive/filesystem/ with only a single
>> file_system_util
>> > compilation unit looks weird and IMHO is not something that we would
>> want
>> > to
>> > keep in the long-term.  On the other hand, I think the changelist under
>> > review
>> > is something that we can keep in the long-term, even if we cannot finish
>> > componentization soon (i.e. making tests browser-independent is more
>> > difficult
>> > than anticipated OR I get hit by a proverbial bus OR I simply cannot
>> spend
>> > time
>> > on componentization efforts in the near term, but have to focus on first
>> > providing more data on the viability of Dokan-over-Drive4CrOS).
>> >
>> Starting by creating a new component directory with a small number of
>> files
>> is the proper way to start componentization (see
>> https://codereview.chromium.org/330233002 for example).
>>
>
> Ok, point taken.
>
>  Having a code under chrome/browser which says it shouldn't depend on
>> chrome/browser is much weirder than having a small directory under
>> components.
>> I have to admit we have been in this state for years, but it's not what we
>> want to (and originally intended to) keep in long-term.
>>
>
>  >
>> > - This CL addresses my immediate goals ( 1) flushing out changes I had
>> to
>> > make
>> > to drive libraries to build Dokan-over-Drive4CrOS and which are
>> currently
>> > stored
>> > only on my dev box, 2) preventing regressions [browser-independence via
>> > DEPS
>> > file;  unfortunately no easy way to enforce building on Windows without
>> > lots of
>> > refactoring of GYP files that probably only makes sense after
>> > componentization]).  Componentization *is* on my list of long-term
>> goals,
>> > but in
>> > the short-term I would prefer to wrap-up what I have and move onto other
>> > immediate goals, rather than spending time on partial, incomplete
>> > componentization (of file_system_util).
>> >
>>   By moving the code out of chrome/browser/chromeos, you will get test
>> coverage on Windows buildbots which should benefit you too.
>> Componentization is not so difficult (the only additional cost is making a
>> new directory + moving files), so we'd really appreciate if you could
>> finish this task which has been our homework for years.
>>
>
> There are also other costs:
> 1. moving drive stuff from chrome/*.gyp* files into components/
>
This is trivial.

> 2. componentizing tests
> 2.a. figuring out if current test dependencies [on chrome/] or ok for
> components/drive/...)
>
You can workaround this by moving the test files to the component directory
while leaving them in GYP files under chrome/ (examples
<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_test...>
).
This way you can skip removing chrome dependency from test files.

> 2.b. scaffolding for main() that runs the tests (maybe this is very
> simple, or
> maybe Chrome dependency makes it more difficult;  I don't know)
>
Tests will run as parts of unit_tests, browser_tests or component_unittests.
You don't have to worry about this.

>
> If possible, I would like to avoid these costs in the short-term.
>
Please note that correct componentization benefits everyone including you.
Continuous build & test coverage on Windows buildbots prevents you from
making mistakes and other Chromium developers can avoid making regression
with it.

>
>
> https://codereview.chromium.org/1192493003/
>

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

[Łukasz Anforowicz, 2015-06-18 18:29:24]

Ryo, could you please take a look at the latest patch set?

As you recommended, I moved browser-agnostic code into file_system_core_util and
left browser-dependent code in file_system_util.  Rediscovering
tools/git/mass-rename.sh helped a lot (this is what caused those extra sorting
changes in the gyp files).

I've tried moving file_system_core_util into components.  I've found that this
requires extra changes and therefore I would like to postpone this until a later
changelist.  The extra changes: 1) have to also move drive.proto, 2) drive.proto
means that BUILD.gn is also affected.

[hashimoto, 2015-06-19 01:03:24]

Sorry for being late to respond.

https://codereview.chromium.org/1192493003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/drive/file_system_core_util.cc (right):

https://codereview.chromium.org/1192493003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/drive/file_system_core_util.cc:119: base::FilePath
ExtractDrivePathFromFileSystemUrl(
Do you need this function in this file?
By omitting this, you can get rid of storage/ dependency IIUC.

[Łukasz Anforowicz, 2015-06-19 17:30:41]

Ryo, I've moved ExtractDrivePathFromFileSystemUrl back to file_system_util. 
Could you please take another look?

https://codereview.chromium.org/1192493003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/drive/file_system_core_util.cc (right):

https://codereview.chromium.org/1192493003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/drive/file_system_core_util.cc:119: base::FilePath
ExtractDrivePathFromFileSystemUrl(
On 2015/06/19 01:03:24, hashimoto wrote:
> Do you need this function in this file?
> By omitting this, you can get rid of storage/ dependency IIUC.

Done (moved back to file_system_util.cc).

Thanks for pointing this out - this is great feedback.  Honestly - I was mostly
focusing (in this CL and other CLs in this area) on chrome/ and content/
dependencies and it's possible there are more cases like the one you pointed
out.  I guess we'll find out when authoring a fresh DEPS file when
componentizing.

BTW: interestingly, there is also a single file that depends on storage/ in
chrome/browser/drive/:

  "drive_api_util\.cc": [
    "+storage/browser/fileapi/file_stream_reader.h"
  ],

(found out when authoring a fresh components/drive/service/DEPS in
https://codereview.chromium.org/1190203002 [componentization of
chrome/browser/drive];  this is still work in progress - have to at least add a
test_support target before asking for a review)

[hashimoto, 2015-06-19 17:43:37]

lgtm

[Łukasz Anforowicz, 2015-06-19 18:26:07]

lukasza@chromium.org changed reviewers:
+ mtomasz@chromium.org

[Łukasz Anforowicz, 2015-06-19 18:26:08]

Tomasz, could you please take a look for:

chrome/browser/chromeos/extensions/file_manager/
chrome/browser/chromeos/file_manager/
chrome/browser/chromeos/fileapi/
chrome/browser/extensions/api/file_system/

[mtomasz, 2015-06-21 23:58:50]

lgtm with minor nits

https://codereview.chromium.org/1192493003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/drive/file_system_core_util.cc (right):

https://codereview.chromium.org/1192493003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/drive/file_system_core_util.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
nit: ditto

https://codereview.chromium.org/1192493003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/drive/file_system_core_util.h (right):

https://codereview.chromium.org/1192493003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/drive/file_system_core_util.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
nit: This is a new file. (c) 2012 -> 2015

https://codereview.chromium.org/1192493003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/drive/file_system_core_util_unittest.cc (right):

https://codereview.chromium.org/1192493003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/drive/file_system_core_util_unittest.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
nit: 2015?

[Łukasz Anforowicz, 2015-06-22 19:37:13]

lukasza@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Łukasz Anforowicz, 2015-06-22 19:37:15]

Steven, could you please take a look for chrome/browser/chromeos/ and
chrome/browser/ui/ash/ ?

https://codereview.chromium.org/1192493003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/drive/file_system_core_util.cc (right):

https://codereview.chromium.org/1192493003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/drive/file_system_core_util.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2015/06/21 23:58:50, mtomasz wrote:
> nit: ditto

Done.

https://codereview.chromium.org/1192493003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/drive/file_system_core_util.h (right):

https://codereview.chromium.org/1192493003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/drive/file_system_core_util.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2015/06/21 23:58:50, mtomasz wrote:
> nit: This is a new file. (c) 2012 -> 2015

Done.

https://codereview.chromium.org/1192493003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/drive/file_system_core_util_unittest.cc (right):

https://codereview.chromium.org/1192493003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/drive/file_system_core_util_unittest.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2015/06/21 23:58:50, mtomasz wrote:
> nit: 2015?

Done.

[stevenjb, 2015-06-23 15:51:48]

rubber stamp lgtm, with #include audit.

https://codereview.chromium.org/1192493003/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right):

https://codereview.chromium.org/1192493003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_screenshot_grabber.cc:37: #include
"chrome/browser/chromeos/drive/file_system_util.h"
Do we still need all three includes?

[Łukasz Anforowicz, 2015-06-23 16:15:01]

lukasza@chromium.org changed reviewers:
+ mnissler@chromium.org

[Łukasz Anforowicz, 2015-06-23 16:15:02]

Thanks Steven.  Asanka - could you please take a look for
chrome/browser/download?

https://codereview.chromium.org/1192493003/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_screenshot_grabber.cc (right):

https://codereview.chromium.org/1192493003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_screenshot_grabber.cc:37: #include
"chrome/browser/chromeos/drive/file_system_util.h"
On 2015/06/23 15:51:47, stevenjb wrote:
> Do we still need all three includes?

I am not making changes to file_system_interface.h.  So that file has to stay.

Both file_system_core_util.h and file_system_util.h are needed:

- For example - drive::util::GetFileSystemByProfile is in file_system_util (it
is browser dependent, because it depends on the Profile class)

- For example - drive::util::IsUnderDriveMountPoint is in file_system_core_util
(no browser dependencies so it could be moved to the new file).

[Łukasz Anforowicz, 2015-06-23 16:15:57]

lukasza@chromium.org changed reviewers:
+ asanka@chromium.org

[Łukasz Anforowicz, 2015-06-23 16:15:58]

Actually adding Asanka to the reviewers list this time.

[asanka, 2015-06-25 14:50:17]

On 2015/06/23 at 16:15:58, lukasza wrote:
> Actually adding Asanka to the reviewers list this time.

lgtm

[Łukasz Anforowicz, 2015-06-25 16:48:27]

lukasza@chromium.org changed reviewers:
+ kalman@chromium.org

[Łukasz Anforowicz, 2015-06-25 16:48:28]

Benjamin, could you please take a look for
chrome/browser/extensions/webstore_installer.cc?

[not at google - send to devlin, 2015-06-25 18:16:54]

lgtm, but you can TBR me on #include changes.

[Łukasz Anforowicz, 2015-06-26 17:06:29]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2015-06-26 17:06:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
mtomasz@chromium.org, hashimoto@chromium.org, asanka@chromium.org,
stevenjb@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1192493003/#ps180001
(title: "Rebasing...")

[commit-bot: I haz the power, 2015-06-26 17:07:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1192493003/180001

[commit-bot: I haz the power, 2015-06-26 18:03:34]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-06-26 18:04:30]

Patchset 10 (id:??) landed as
https://crrev.com/5da1d547c262af0b7d34c1a0ac71d2a4f0e197f4
Cr-Commit-Position: refs/heads/master@{#336410}