Compile volume related targets in file_manager/background.

Compile volume related targets in file_manager/background.

BUG=636289
TEST=run_compiler.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2636183002
Cr-Commit-Position: refs/heads/master@{#444306}
Committed: https://chromium.googlesource.com/chromium/src/+/b0588d590175d3afc652c5777bd9f2f4493871e5

[oka, 2017-01-17 14:07:23]

Description was changed from

==========
Compile volume related targtes in file_manager/background.

BUG=636289
TEST=run_compiler.
==========

to

==========
Compile volume related targtes in file_manager/background.

BUG=636289
TEST=run_compiler.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[oka, 2017-01-17 14:08:21]

.

[oka, 2017-01-17 14:09:07]

Description was changed from

==========
Compile volume related targtes in file_manager/background.

BUG=636289
TEST=run_compiler.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Compile volume related targtes in file_manager/background.

BUG=636289
TEST=run_compiler.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[oka, 2017-01-17 14:09:17]

Description was changed from

==========
Compile volume related targtes in file_manager/background.

BUG=636289
TEST=run_compiler.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Compile volume related targets in file_manager/background.

BUG=636289
TEST=run_compiler.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[oka, 2017-01-17 14:16:31]

oka@chromium.org changed reviewers:
+ dbeam@chromium.org, fukino@chromium.org

[oka, 2017-01-17 14:16:32]

PTAL.

dbeam@ for ui/webui/resources/js/cr/ui/compiled_resources2.gyp.
cr.ui.ArrayDataModel is used by Files app's VolumeInfoListImpl, so I want to add
the target array_data_model.

fukino@ for other files.

[Dan Beam, 2017-01-17 19:24:08]

lgtm

https://codereview.chromium.org/2636183002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/compiled_resources2.gyp (right):

https://codereview.chromium.org/2636183002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/compiled_resources2.gyp:124:
'../../../../webui/resources/js/cr/ui/compiled_resources2.gyp:array_data_model',
i think it might be worth using <(DEPTH)/ui/webui/resources/js/ here instead of
a relative path, but that depends on whether you think that ui/webui and
ui/file_manager are "the same package" or something

i would argue the same about externs

it's ultimately up to you guys.

my thinking is that if file manager got moved, all the paths that are relative
should still work (because in theory that code would be moved with it in one big
unit).  so only use relative paths for things you consider in the "same
package".

unfortunately, you can't use <(DEPTH) for 'includes' paths

[fukino, 2017-01-18 00:15:08]

https://codereview.chromium.org/2636183002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/compiled_resources2.gyp (right):

https://codereview.chromium.org/2636183002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/compiled_resources2.gyp:124:
'../../../../webui/resources/js/cr/ui/compiled_resources2.gyp:array_data_model',
On 2017/01/17 19:24:08, Dan Beam wrote:
> i think it might be worth using <(DEPTH)/ui/webui/resources/js/ here instead
of
> a relative path, but that depends on whether you think that ui/webui and
> ui/file_manager are "the same package" or something
> 
> i would argue the same about externs
> 
> it's ultimately up to you guys.
> 
> my thinking is that if file manager got moved, all the paths that are relative
> should still work (because in theory that code would be moved with it in one
big
> unit).  so only use relative paths for things you consider in the "same
> package".
> 
> unfortunately, you can't use <(DEPTH) for 'includes' paths

I agree with Dan, and I avoid relative paths to specify dependencies in other
compiled_resources2.gyp in file_manager. (I failed to do so in some lines,
though)
oka@, could you use absolute paths?

[fukino, 2017-01-18 00:19:52]

On 2017/01/18 00:15:08, fukino wrote:
>
https://codereview.chromium.org/2636183002/diff/20001/ui/file_manager/file_ma...
> File ui/file_manager/file_manager/background/js/compiled_resources2.gyp
(right):
> 
>
https://codereview.chromium.org/2636183002/diff/20001/ui/file_manager/file_ma...
> ui/file_manager/file_manager/background/js/compiled_resources2.gyp:124:
>
'../../../../webui/resources/js/cr/ui/compiled_resources2.gyp:array_data_model',
> On 2017/01/17 19:24:08, Dan Beam wrote:
> > i think it might be worth using <(DEPTH)/ui/webui/resources/js/ here instead
> of
> > a relative path, but that depends on whether you think that ui/webui and
> > ui/file_manager are "the same package" or something
> > 
> > i would argue the same about externs
> > 
> > it's ultimately up to you guys.
> > 
> > my thinking is that if file manager got moved, all the paths that are
relative
> > should still work (because in theory that code would be moved with it in one
> big
> > unit).  so only use relative paths for things you consider in the "same
> > package".
> > 
> > unfortunately, you can't use <(DEPTH) for 'includes' paths
> 
> I agree with Dan, and I avoid relative paths to specify dependencies in other
> compiled_resources2.gyp in file_manager. (I failed to do so in some lines,
> though)
> oka@, could you use absolute paths?

Regarding the dependencies in file_manager (not webui), relative paths make
sense.
Let's use
- Relative paths for files under ui/file_manager
- Absolute paths for other files.

[oka, 2017-01-18 05:40:58]

The CQ bit was checked by oka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 05:41:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oka, 2017-01-18 05:54:34]

Used abs path for webui.

[oka, 2017-01-18 05:56:02]

sort.

[oka, 2017-01-18 05:58:52]

Use target name directly if the target is in the same file.

[oka, 2017-01-18 05:58:59]

PTAL.

https://codereview.chromium.org/2636183002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/compiled_resources2.gyp (right):

https://codereview.chromium.org/2636183002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/compiled_resources2.gyp:124:
'../../../../webui/resources/js/cr/ui/compiled_resources2.gyp:array_data_model',
On 2017/01/18 00:15:07, fukino wrote:
> On 2017/01/17 19:24:08, Dan Beam wrote:
> > i think it might be worth using <(DEPTH)/ui/webui/resources/js/ here instead
> of
> > a relative path, but that depends on whether you think that ui/webui and
> > ui/file_manager are "the same package" or something
> > 
> > i would argue the same about externs
> > 
> > it's ultimately up to you guys.
> > 
> > my thinking is that if file manager got moved, all the paths that are
relative
> > should still work (because in theory that code would be moved with it in one
> big
> > unit).  so only use relative paths for things you consider in the "same
> > package".
> > 
> > unfortunately, you can't use <(DEPTH) for 'includes' paths
> 
> I agree with Dan, and I avoid relative paths to specify dependencies in other
> compiled_resources2.gyp in file_manager. (I failed to do so in some lines,
> though)
> oka@, could you use absolute paths?

Done.

[fukino, 2017-01-18 06:16:19]

lgtm. Thanks!

[oka, 2017-01-18 08:17:05]

The CQ bit was checked by oka@chromium.org

[oka, 2017-01-18 08:17:05]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2636183002/#ps80001
(title: "Use target name directly if the target is in the same file.")

[commit-bot: I haz the power, 2017-01-18 08:17:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 08:51:17]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484727425405320,
"parent_rev": "dac00e30c79dd56cbe8ba441bd99e27f69ea533c", "commit_rev":
"b0588d590175d3afc652c5777bd9f2f4493871e5"}

[commit-bot: I haz the power, 2017-01-18 08:54:55]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484727425405320,
"parent_rev": "dac00e30c79dd56cbe8ba441bd99e27f69ea533c", "commit_rev":
"b0588d590175d3afc652c5777bd9f2f4493871e5"}

[commit-bot: I haz the power, 2017-01-18 08:55:02]

Description was changed from

==========
Compile volume related targets in file_manager/background.

BUG=636289
TEST=run_compiler.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Compile volume related targets in file_manager/background.

BUG=636289
TEST=run_compiler.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2636183002
Cr-Commit-Position: refs/heads/master@{#444306}
Committed:
https://chromium.googlesource.com/chromium/src/+/b0588d590175d3afc652c5777bd9...
==========

[commit-bot: I haz the power, 2017-01-18 08:55:03]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b0588d590175d3afc652c5777bd9...