Move a subset of chrome/browser/chromeos/drive into components/drive

Move a subset of chrome/browser/chromeos/drive into components/drive

Moved: drive_pref_names.*, drive_test_util.*, file_change.*,
file_error.*, local_file_reader.*

Other changes:
- Removed unnecessary include/dependency inside file_change.h
- ChromeOS if-def guards in drive_test_util.h
- Necessary DEPS tweaks

Test steps:
1. Verify that things still build via GYP (and unit tests pass).
   $ GYP_DEFINES="use_goma=1 gomadir=... chromeos=1" gclient sync
   $ ninja -C out/Debug -j 150 chrome unit_tests \
         interactive_ui_tests browser_tests drive
   $ out/Debug/unit_tests

2. git cl try

TBR=satorux@chromium.org,benchan@chromium.org
TEST=Please see "Test steps" above.
BUG=257943, 498951
Committed: https://crrev.com/76b4a982964ffefeffa73af775f92d4c22d48791
Cr-Commit-Position: refs/heads/master@{#342492}

[Łukasz Anforowicz, 2015-07-21 20:38:58]

Patchset #1 (id:1) has been deleted

[Łukasz Anforowicz, 2015-07-22 17:16:15]

Patchset #1 (id:20001) has been deleted

[Łukasz Anforowicz, 2015-07-22 17:16:24]

Patchset #1 (id:40001) has been deleted

[Łukasz Anforowicz, 2015-07-22 17:25:53]

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

[Łukasz Anforowicz, 2015-07-22 17:25:54]

Ryo and Tomasz - could you please take a look?

https://codereview.chromium.org/1246753003/diff/60001/chrome/chrome_browser_c...
File chrome/chrome_browser_chromeos.gypi (left):

https://codereview.chromium.org/1246753003/diff/60001/chrome/chrome_browser_c...
chrome/chrome_browser_chromeos.gypi:114:
'browser/chromeos/drive/drive_pref_names.h',
Removed entries are covered by the already-existing dependency from
browser_chromeos to components.gyp:drive

https://codereview.chromium.org/1246753003/diff/60001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1246753003/diff/60001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:531:
'browser/ui/webui/chromeos/bluetooth_pairing_ui_browsertest-inl.h',
Sorting order enforced by tools/git/mass-rename.sh

https://codereview.chromium.org/1246753003/diff/60001/components/drive/drive_...
File components/drive/drive_test_util.h (right):

https://codereview.chromium.org/1246753003/diff/60001/components/drive/drive_...
components/drive/drive_test_util.h:34: const int64 kLotsOfSpace = 3ul * 1024ul *
1024ul * 1024ul;  // 3GB
Replying to mtomasz@'s comment from crrev.com/1215503010 (bigger CL that got
split into smaller ones).

> It's quite cryptic. Can we use ... * 1024 * 1024 instead?

Done.

> Also, is this value correct for other OS'es?

This value is only used by tests - therefore I think the value is okay.

[hashimoto, 2015-07-23 06:01:15]

lgtm with a couple of nits.

https://codereview.chromium.org/1246753003/diff/60001/components/drive/BUILD.gn
File components/drive/BUILD.gn (right):

https://codereview.chromium.org/1246753003/diff/60001/components/drive/BUILD....
components/drive/BUILD.gn:55: visibility = [ ":*" ]
Why do we need to maintain this line?

https://codereview.chromium.org/1246753003/diff/60001/components/drive/drive_...
File components/drive/drive_test_util.h (right):

https://codereview.chromium.org/1246753003/diff/60001/components/drive/drive_...
components/drive/drive_test_util.h:34: const int64 kLotsOfSpace = 3ul * 1024ul *
1024ul * 1024ul;  // 3GB
On 2015/07/22 17:25:54, Łukasz Anforowicz wrote:
> Replying to mtomasz@'s comment from crrev.com/1215503010 (bigger CL that got
> split into smaller ones).
> 
> > It's quite cryptic. Can we use ... * 1024 * 1024 instead?
> 
> Done.
> 
> > Also, is this value correct for other OS'es?
> 
> This value is only used by tests - therefore I think the value is okay.

nit: cryptohome::kMinFreeSpaceInBytes*10 = 5GB.
I think it's better to use the same value across all platforms.

https://codereview.chromium.org/1246753003/diff/60001/components/drive/file_c...
File components/drive/file_change.h (left):

https://codereview.chromium.org/1246753003/diff/60001/components/drive/file_c...
components/drive/file_change.h:14: #include
"storage/browser/fileapi/file_system_url.h"
nice :)

[mtomasz, 2015-07-23 09:37:31]

lgtm with DEPS comment

https://codereview.chromium.org/1246753003/diff/60001/components/drive/DEPS
File components/drive/DEPS (right):

https://codereview.chromium.org/1246753003/diff/60001/components/drive/DEPS#n...
components/drive/DEPS:27: r"(drive_test_util\.h"
Is this r" correct?

[Łukasz Anforowicz, 2015-07-27 17:53:27]

Mattias, could you please take a look at 1)
chrome/browser/download/download_dir_policy_handler and 2) chrome/browser/policy
(+ at the associated DEPS changes).

https://codereview.chromium.org/1246753003/diff/60001/components/drive/BUILD.gn
File components/drive/BUILD.gn (right):

https://codereview.chromium.org/1246753003/diff/60001/components/drive/BUILD....
components/drive/BUILD.gn:55: visibility = [ ":*" ]
On 2015/07/23 06:01:15, hashimoto wrote:
> Why do we need to maintain this line?

This means that BUILD.gn targets from outside of components/drive directory
cannot directly depend on components/drive:proto.  Maybe this is not really
needed (i.e. the :proto target is not really "internal" to components/drive)? 
WDYT?

https://codereview.chromium.org/1246753003/diff/60001/components/drive/DEPS
File components/drive/DEPS (right):

https://codereview.chromium.org/1246753003/diff/60001/components/drive/DEPS#n...
components/drive/DEPS:27: r"(drive_test_util\.h"
On 2015/07/23 09:37:31, mtomasz wrote:
> Is this r" correct?

The "r" is correct (i.e. see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pol...).

One reason for using "r" here and not below, is to make sure I have unique keys
(i.e. the key on line 27 and 34 are different).  This works for now and will
start to look better once I add more files to these 2 cases.  (i.e. compare with
the end goal here:
https://codereview.chromium.org/1215503010/patch/1/5657903226683392).

https://codereview.chromium.org/1246753003/diff/60001/components/drive/drive_...
File components/drive/drive_test_util.h (right):

https://codereview.chromium.org/1246753003/diff/60001/components/drive/drive_...
components/drive/drive_test_util.h:34: const int64 kLotsOfSpace = 3ul * 1024ul *
1024ul * 1024ul;  // 3GB
On 2015/07/23 06:01:15, hashimoto wrote:
> On 2015/07/22 17:25:54, Łukasz Anforowicz wrote:
> > Replying to mtomasz@'s comment from crrev.com/1215503010 (bigger CL that got
> > split into smaller ones).
> > 
> > > It's quite cryptic. Can we use ... * 1024 * 1024 instead?
> > 
> > Done.
> > 
> > > Also, is this value correct for other OS'es?
> > 
> > This value is only used by tests - therefore I think the value is okay.
> 
> nit: cryptohome::kMinFreeSpaceInBytes*10 = 5GB.
> I think it's better to use the same value across all platforms.

Done.  Initially I shied away from values greater or equal to 4GB, to make sure
that "ul" prefix in integer literals is sufficient (because I see that "ull"
prefix has been covered by the standard starting in C++11 -
http://en.cppreference.com/w/cpp/language/integer_literal).  OTOH, I see that
"[0-9]ull\b" is quite widespread in Chromium so it seems safe.

[Łukasz Anforowicz, 2015-07-27 18:10:56]

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

[Łukasz Anforowicz, 2015-07-27 18:10:57]

This time I am actually adding Mattias to the Reviewers field...

[Mattias Nissler (ping if slow), 2015-07-28 11:40:22]

policy changes LGTM

[Łukasz Anforowicz, 2015-07-28 14:13:31]

lukasza@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[Łukasz Anforowicz, 2015-07-28 14:13:32]

Pawel, could you please take a look at the dependency from components/drive/DEPS
into content/public/test/test_utils.h?

[hashimoto, 2015-07-29 02:07:08]

https://codereview.chromium.org/1246753003/diff/60001/components/drive/BUILD.gn
File components/drive/BUILD.gn (right):

https://codereview.chromium.org/1246753003/diff/60001/components/drive/BUILD....
components/drive/BUILD.gn:55: visibility = [ ":*" ]
On 2015/07/27 17:53:27, Łukasz Anforowicz wrote:
> On 2015/07/23 06:01:15, hashimoto wrote:
> > Why do we need to maintain this line?
> 
> This means that BUILD.gn targets from outside of components/drive directory
> cannot directly depend on components/drive:proto.  Maybe this is not really
> needed (i.e. the :proto target is not really "internal" to components/drive)? 
> WDYT?

This "visibility" line was added by you in
https://codereview.chromium.org/1243853002/ (sorry, I didn't notice it in the
review and I'm not sure why you added it).
If you see no reason to maintain this line, please remove it.

[Paweł Hajdan Jr., 2015-07-30 12:18:03]

On 2015/07/28 at 14:13:32, lukasza wrote:
> Pawel, could you please take a look at the dependency from
components/drive/DEPS into content/public/test/test_utils.h?

Could you briefly describe why it's necessary and why allowing it temporarily is
the best solution?

[Łukasz Anforowicz, 2015-08-03 21:30:48]

https://codereview.chromium.org/1246753003/diff/60001/components/drive/BUILD.gn
File components/drive/BUILD.gn (right):

https://codereview.chromium.org/1246753003/diff/60001/components/drive/BUILD....
components/drive/BUILD.gn:55: visibility = [ ":*" ]
On 2015/07/29 02:07:08, hashimoto wrote:
> On 2015/07/27 17:53:27, Łukasz Anforowicz wrote:
> > On 2015/07/23 06:01:15, hashimoto wrote:
> > > Why do we need to maintain this line?
> > 
> > This means that BUILD.gn targets from outside of components/drive directory
> > cannot directly depend on components/drive:proto.  Maybe this is not really
> > needed (i.e. the :proto target is not really "internal" to
components/drive)? 
> > WDYT?
> 
> This "visibility" line was added by you in
> https://codereview.chromium.org/1243853002/ (sorry, I didn't notice it in the
> review and I'm not sure why you added it).
> If you see no reason to maintain this line, please remove it.

Done.  I don't see a compelling reason to keep this line.

[Łukasz Anforowicz, 2015-08-03 21:32:30]

On 2015/07/30 12:18:03, Paweł Hajdan Jr. wrote:
> On 2015/07/28 at 14:13:32, lukasza wrote:
> > Pawel, could you please take a look at the dependency from
> components/drive/DEPS into content/public/test/test_utils.h?
> 
> Could you briefly describe why it's necessary and why allowing it temporarily
is
> the best solution?

We are trying to ensure that drive libraries (most of what used to live under
c/b/drive and c/b/chromeos/drive) are browser-agnostic and platform-agnostic
(i.e. work not only on ChromeOS but also on Windows).  This work is being
tracked
by crbug.com/498951 and crbug.com/257943.  At this point the product code *is*
browser-agnostic and platform-agnostic, but to ensure that it stays this way
we want to move it to components/drive where we can have stricter DEPS and where
we can build and run unit tests outside of chromeos platform.

Now - to answer your questions:

1. Moving c/b/drive and c/b/chromeos/drive into components/drive is not
"necessary",
   but is still desirable (for overall code hygiene, even if reusing this code
   on Windows platform [for an internal project] won't work out in the
long-term).

2. Restricting dependencies of the product code is an important first step.
   Restricting dependencies of the test code is desirable, but seems to be lower
   priority.  

To elaborate on (2) a bit more:
- We have a plan for getting rid of
+storage/browser/fileapi/file_stream_reader.h dependency from the product code.
- We have a not-so-good-but-acceptable plan for getting rid of
+content/public/browser/power_save_blocker.h from the product code
  (ideally power_save_blocker would be componentized but this is difficult
because of dependencies of content/browser/power_save_blocker_android.cc;  a bit
icky but acceptable solution is to add one more abstraction/indirection layer in
drive libraries)
- We have not yet started looking at the work required to restrict test
dependencies.  Componentizing product code and moving test code would indeed
(temporarily?) leave a few test dependencies in components/drive/DEPS - see
https://codereview.chromium.org/1215503010/patch/1/5657903226683392 for details
(this is from a big CL that is unwieldy and is therefore being split into
smaller CLs like the current one under review).

[Paweł Hajdan Jr., 2015-08-04 09:11:12]

On 2015/08/03 at 21:32:30, lukasza wrote:
> On 2015/07/30 12:18:03, Paweł Hajdan Jr. wrote:
> > On 2015/07/28 at 14:13:32, lukasza wrote:
> > > Pawel, could you please take a look at the dependency from
> > components/drive/DEPS into content/public/test/test_utils.h?
> > 
> > Could you briefly describe why it's necessary and why allowing it
temporarily is
> > the best solution?
> 
> Now - to answer your questions:
> 
> 1. Moving c/b/drive and c/b/chromeos/drive into components/drive is not
"necessary",
>    but is still desirable (for overall code hygiene, even if reusing this code
>    on Windows platform [for an internal project] won't work out in the
long-term).

That's totally fine. I actually like the cleanup.

> 2. Restricting dependencies of the product code is an important first step.
>    Restricting dependencies of the test code is desirable, but seems to be
lower
>    priority.  

Sure. Sorry for a vague question. I'd like to clarify - could you briefly
explain what depends on content/public/test/test_utils.h and what things from
there are needed? Have you considered other solutions than whitelisting it for
now (which could be fine), and if so, why would you say whitelisting is the best
one at this point? Note that I don't have a strong opinion yet either way, and
would just like to understand it better.

[Łukasz Anforowicz, 2015-08-04 20:23:38]

On 2015/08/04 09:11:12, Paweł Hajdan Jr. wrote:
> Sure. Sorry for a vague question. I'd like to clarify - could you briefly
> explain what depends on content/public/test/test_utils.h and what things from
> there are needed? 

Drive tests depend on content/public/test/... mostly for managing test
threads.  In the current CL, components/drive/drive_test_util.h depends on
content/public/test/test_util.h for content::RunAllBlockingPoolTasksUntilIdle.
Other tests (currently still left under c/b/chromeos/drive) depend on
content::TestBrowserThreadBundle.

> Have you considered other solutions than whitelisting it for
> now (which could be fine)

The only whitelisting alternative I see is to get rid of the content/public/test
dependendency today (most likely by introducing a DriveTestThreadBundle with its
equivalents of the threads and blocking pool that are relevant to Drive
libraries).

> and if so, why would you say whitelisting is the best
> one at this point? 

Whitelisting allows us to move the code to components/drive today.
Moving the code automatically helps avoid build regressions (before the move,
the code is only built for ChromeOS because of the dirname substring).
Moving the code is also a prerequisite for enabling running Drive tests outside
of ChromeOS.

Whether whitelisting is "best" probably depends on the relative importance
of 1) preventing Drive build and test regressions on Windows today vs 2) making
sure we have all the right long-term dependencies from the beginning.

[Paweł Hajdan Jr., 2015-08-05 19:10:47]

LGTM, thanks for explaining.

[Łukasz Anforowicz, 2015-08-05 21:44:08]

lukasza@chromium.org changed reviewers:
+ benchan@chromium.org, satorux@chromium.org

[Łukasz Anforowicz, 2015-08-05 21:44:09]

Thanks Pawel.

Ben, Satoru - I am adding you to TBR, because I believe changes I am making in
your directories are mechanical (and therefore TBR should be sufficient).  In
particular:

1. Changes in
    chrome/browser/chromeos/DEPS
    chrome/browser/chromeos/preferences.cc
are a mechanical consequence from moving
    c/b/chromeos/drive/drive_pref_names.h
into
    components/drive/drive_pref_names.h.

2. Dependency from
    components/drive/DEPS
into
    third_party/cros_system_api/constants/cryptohome.h
is retained from files being moved from
    c/b/chromeos/drive/...
into 
    components/drive/...
(i.e. drive_test_util.h in the current CL).  This dependency should be fine for
[platform-agnostic] components/drive, because all references to
third_party/cros_system_api are guarded by ifdef(OS_CHROMEOS).

[Łukasz Anforowicz, 2015-08-05 23:46:32]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2015-08-05 23:46:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
mtomasz@chromium.org, hashimoto@chromium.org, mnissler@chromium.org,
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1246753003/#ps140001
(title: "Rebasing...")

[commit-bot: I haz the power, 2015-08-05 23:46:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246753003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1246753003/140001

[commit-bot: I haz the power, 2015-08-05 23:59:44]

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

[commit-bot: I haz the power, 2015-08-05 23:59:46]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Łukasz Anforowicz, 2015-08-06 00:04:25]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2015-08-06 00:05:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246753003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1246753003/140001

[commit-bot: I haz the power, 2015-08-06 00:21:18]

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

[commit-bot: I haz the power, 2015-08-06 00:21:20]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Łukasz Anforowicz, 2015-08-07 23:28:27]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2015-08-07 23:28:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
hashimoto@chromium.org, mnissler@chromium.org, mtomasz@chromium.org,
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1246753003/#ps200001
(title: "Fixed incorrect reference to a GYP dependency.")

[commit-bot: I haz the power, 2015-08-07 23:28:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246753003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1246753003/200001

[commit-bot: I haz the power, 2015-08-08 00:36:49]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2015-08-08 00:37:26]

Patchset 8 (id:??) landed as
https://crrev.com/76b4a982964ffefeffa73af775f92d4c22d48791
Cr-Commit-Position: refs/heads/master@{#342492}

[mtomasz, 2015-08-08 01:15:47]

On 2015/08/08 00:37:26, commit-bot: I haz the power wrote:
> Patchset 8 (id:??) landed as
> https://crrev.com/76b4a982964ffefeffa73af775f92d4c22d48791
> Cr-Commit-Position: refs/heads/master@{#342492}

Wohoo!