grit: qualify includes for shell_resources.h, virtual_driver_setup_resources.h, bluetooth_strings.h

grit: qualify includes for shell_resources.h, virtual_driver_setup_resources.h, bluetooth_strings.h, app_shell_resources.h

BUG=401588
TBR=fake virtual printer dep

Review-Url: https://codereview.chromium.org/2714393004
Cr-Commit-Position: refs/heads/master@{#453586}
Committed: https://chromium.googlesource.com/chromium/src/+/9b5475a957ae0a103d0c93e53355ad7c0d60cb31

[Nico, 2017-02-27 15:38:35]

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

[commit-bot: I haz the power, 2017-02-27 15:38:48]

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

[Nico, 2017-02-27 15:39:17]

Description was changed from

==========
grit: qualifiy includes for blu...

BUG=
==========

to

==========
grit: qualifiy includes for blu...

BUG=401588
==========

[Nico, 2017-02-27 15:40:08]

Description was changed from

==========
grit: qualifiy includes for blu...

BUG=401588
==========

to

==========
grit: qualifiy includes for shell_resources.h, virtual_driver_setup_resources.h,
bluetooth_strings.h, app_shell_resources.h

BUG=401588
==========

[Nico, 2017-02-27 15:42:09]

thakis@chromium.org changed reviewers:
+ derat@chromium.org, ortuno@chromium.org, pastarmovj@chromium.org,
slan@chromium.org

[Nico, 2017-02-27 15:42:09]

slan: chromecast/
pastarmovj: cloud_print/
ortuno: device/bluetooth
derat: extensions/shell

[Nico, 2017-02-27 15:42:26]

Description was changed from

==========
grit: qualifiy includes for shell_resources.h, virtual_driver_setup_resources.h,
bluetooth_strings.h, app_shell_resources.h

BUG=401588
==========

to

==========
grit: qualify includes for shell_resources.h, virtual_driver_setup_resources.h,
bluetooth_strings.h, app_shell_resources.h

BUG=401588
==========

[commit-bot: I haz the power, 2017-02-27 15:52:42]

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

[commit-bot: I haz the power, 2017-02-27 15:52:42]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[slan, 2017-02-27 16:01:34]

cast lgtm % nit. Thanks, Nico.

https://codereview.chromium.org/2714393004/diff/1/chromecast/app/BUILD.gn
File chromecast/app/BUILD.gn (right):

https://codereview.chromium.org/2714393004/diff/1/chromecast/app/BUILD.gn#new...
chromecast/app/BUILD.gn:109: # TODO(thakis): Consider removing this in favor of
the default directory.
Are you interested in moving this for consistency's sake, or is there another
advantage as well? If we do leave a TODO here, could we take out a bug as well?

[Nico, 2017-02-27 16:03:31]

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

[commit-bot: I haz the power, 2017-02-27 16:03:48]

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

[Nico, 2017-02-27 16:05:59]

https://codereview.chromium.org/2714393004/diff/1/chromecast/app/BUILD.gn
File chromecast/app/BUILD.gn (right):

https://codereview.chromium.org/2714393004/diff/1/chromecast/app/BUILD.gn#new...
chromecast/app/BUILD.gn:109: # TODO(thakis): Consider removing this in favor of
the default directory.
On 2017/02/27 16:01:34, slan wrote:
> Are you interested in moving this for consistency's sake, or is there another
> advantage as well? If we do leave a TODO here, could we take out a bug as
well?

There's another advantage: With this output_dir, once we make this target have
use_qualified_include=true too, the .h file needs to be included via #include
chromecast_strings/grit/chromecast_settings.h" which in turn requires a
+chromecast_strings entry in a DEPS file, which in return needs an LGTM from a
chromecast_strings/OWNER for the new dep -- but that file doesn't exist.

Using the default output dir nicely makes grit outputs match the existing DEPS
system.

Do you remember why this line was needed? Almost all grit() targets don't have
it.

[Daniel Erat, 2017-02-27 16:26:50]

lgtm for extensions/shell/

[slan, 2017-02-27 16:36:27]

On 2017/02/27 16:05:59, Nico wrote:
> https://codereview.chromium.org/2714393004/diff/1/chromecast/app/BUILD.gn
> File chromecast/app/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2714393004/diff/1/chromecast/app/BUILD.gn#new...
> chromecast/app/BUILD.gn:109: # TODO(thakis): Consider removing this in favor
of
> the default directory.
> On 2017/02/27 16:01:34, slan wrote:
> > Are you interested in moving this for consistency's sake, or is there
another
> > advantage as well? If we do leave a TODO here, could we take out a bug as
> well?
> 
> There's another advantage: With this output_dir, once we make this target have
> use_qualified_include=true too, the .h file needs to be included via #include
> chromecast_strings/grit/chromecast_settings.h" which in turn requires a
> +chromecast_strings entry in a DEPS file, which in return needs an LGTM from a
> chromecast_strings/OWNER for the new dep -- but that file doesn't exist.
> 
> Using the default output dir nicely makes grit outputs match the existing DEPS
> system.
> 
> Do you remember why this line was needed? Almost all grit() targets don't have
> it.

Frankly, no. This was designed to match the old GYP file, which was written in
the early hacky days :) 

Your reasoning SGTM. But we will need to update some things in our internal code
that depend in this location. I'll take care of it. Could you take out a bug
against me and tag it here?

Thanks!

[commit-bot: I haz the power, 2017-02-27 18:10:54]

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

[commit-bot: I haz the power, 2017-02-27 18:10:55]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-02-27 21:28:13]

lgtm

[pastarmovj, 2017-02-28 08:44:28]

lgtm

[Nico, 2017-02-28 12:17:55]

The CQ bit was checked by thakis@chromium.org

[Nico, 2017-02-28 12:17:56]

The patchset sent to the CQ was uploaded after l-g-t-m from slan@chromium.org
Link to the patchset: https://codereview.chromium.org/2714393004/#ps20001
(title: ".")

[commit-bot: I haz the power, 2017-02-28 12:18:17]

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-02-28 12:24:19]

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

[commit-bot: I haz the power, 2017-02-28 12:24:20]

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

[Nico, 2017-02-28 12:48:43]

Description was changed from

==========
grit: qualify includes for shell_resources.h, virtual_driver_setup_resources.h,
bluetooth_strings.h, app_shell_resources.h

BUG=401588
==========

to

==========
grit: qualify includes for shell_resources.h, virtual_driver_setup_resources.h,
bluetooth_strings.h, app_shell_resources.h

BUG=401588
TBrR=fake virtual printer dep
==========

[Nico, 2017-02-28 12:48:50]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2017-02-28 12:49:09]

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-02-28 12:55:01]

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

[commit-bot: I haz the power, 2017-02-28 12:55:03]

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

[Nico, 2017-02-28 13:23:00]

Description was changed from

==========
grit: qualify includes for shell_resources.h, virtual_driver_setup_resources.h,
bluetooth_strings.h, app_shell_resources.h

BUG=401588
TBrR=fake virtual printer dep
==========

to

==========
grit: qualify includes for shell_resources.h, virtual_driver_setup_resources.h,
bluetooth_strings.h, app_shell_resources.h

BUG=401588
TBR=fake virtual printer dep
==========

[Nico, 2017-02-28 13:23:08]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2017-02-28 13:23: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-02-28 13:28:34]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488288188609750,
"parent_rev": "db10455e00b03c01eadef0daa4f1327a625151d4", "commit_rev":
"9b5475a957ae0a103d0c93e53355ad7c0d60cb31"}

[commit-bot: I haz the power, 2017-02-28 13:29:15]

Description was changed from

==========
grit: qualify includes for shell_resources.h, virtual_driver_setup_resources.h,
bluetooth_strings.h, app_shell_resources.h

BUG=401588
TBR=fake virtual printer dep
==========

to

==========
grit: qualify includes for shell_resources.h, virtual_driver_setup_resources.h,
bluetooth_strings.h, app_shell_resources.h

BUG=401588
TBR=fake virtual printer dep

Review-Url: https://codereview.chromium.org/2714393004
Cr-Commit-Position: refs/heads/master@{#453586}
Committed:
https://chromium.googlesource.com/chromium/src/+/9b5475a957ae0a103d0c93e53355...
==========

[commit-bot: I haz the power, 2017-02-28 13:29:19]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/9b5475a957ae0a103d0c93e53355...