Generated header files apparently require hard_dependency

Generated header files apparently necessitate hard_dependency

BUG=509277, 508897
TBR=msw@chromium.org,sadrul@chromium.org

Committed: https://crrev.com/a8c375bf808b8b169e4303deeac4cce20c786d29
Cr-Commit-Position: refs/heads/master@{#338690}

[Evan Stade, 2015-07-13 19:01:18]

estade@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-07-13 19:05:55]

https://codereview.chromium.org/1236863006/diff/1/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/1236863006/diff/1/ui/gfx/gfx.gyp#newcode417
ui/gfx/gfx.gyp:417: '<(vector_icons_cc_file)',
I think if you set process_outputs_as_sourceson the action, you don't need this

https://codereview.chromium.org/1236863006/diff/1/ui/gfx/gfx.gyp#newcode445
ui/gfx/gfx.gyp:445: 'hard_dependency': 1,
can you make a dedicated target for the generated files? there's no need to give
everything a hard dependency on ui/gfx if it's only needed for that one file.
(or do gfx headers include that generated .h file?)

is it possible to make it so that the .h file isn't generated?

[Evan Stade, 2015-07-13 19:18:41]

https://codereview.chromium.org/1236863006/diff/1/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/1236863006/diff/1/ui/gfx/gfx.gyp#newcode417
ui/gfx/gfx.gyp:417: '<(vector_icons_cc_file)',
On 2015/07/13 19:05:55, Nico wrote:
> I think if you set process_outputs_as_sourceson the action, you don't need
this

Done.

https://codereview.chromium.org/1236863006/diff/1/ui/gfx/gfx.gyp#newcode445
ui/gfx/gfx.gyp:445: 'hard_dependency': 1,
On 2015/07/13 19:05:55, Nico wrote:
> can you make a dedicated target for the generated files? there's no need to
give
> everything a hard dependency on ui/gfx if it's only needed for that one file.
> (or do gfx headers include that generated .h file?)

done

> 
> is it possible to make it so that the .h file isn't generated?

I don't think this is possible without defeating the purpose of this action.

[Nico, 2015-07-13 19:24:03]

stampy-ish lgtm

(If the header is included in headers in dependent projects, you also need
hard_dependency in direct_dependent_settings iirc – another reason for trying to
not have generated headers)

[Evan Stade, 2015-07-13 21:59:23]

estade@chromium.org changed reviewers:
+ sadrul@chromium.org

[Evan Stade, 2015-07-13 21:59:24]

On 2015/07/13 19:24:03, Nico wrote:
> stampy-ish lgtm
> 
> (If the header is included in headers in dependent projects, you also need
> hard_dependency in direct_dependent_settings iirc – another reason for
trying to
> not have generated headers)

In order to avoid that problem I've removed the vector_icons_public.h include
from other header files. PTAL

+sadrul

[Nico, 2015-07-13 22:06:12]

https://codereview.chromium.org/1236863006/diff/40001/ui/views/controls/image...
File ui/views/controls/image_view.h (right):

https://codereview.chromium.org/1236863006/diff/40001/ui/views/controls/image...
ui/views/controls/image_view.h:125: int vector_id_;
you can probably keep this a gfx::VectorIconId. Since the compiler knows that
it's an int (due to it being an enum class) it can allocate storage for it even
without having the definition of VectorIconId. (At least it works with clang
locally)

[Evan Stade, 2015-07-13 23:20:44]

https://codereview.chromium.org/1236863006/diff/40001/ui/views/controls/image...
File ui/views/controls/image_view.h (right):

https://codereview.chromium.org/1236863006/diff/40001/ui/views/controls/image...
ui/views/controls/image_view.h:125: int vector_id_;
On 2015/07/13 22:06:12, Nico wrote:
> you can probably keep this a gfx::VectorIconId. Since the compiler knows that
> it's an int (due to it being an enum class) it can allocate storage for it
even
> without having the definition of VectorIconId. (At least it works with clang
> locally)

hmm, when I tried this before I got

 error: field has incomplete type 'gfx::VectorIconId'

but I can't seem to figure out which compiler said that and maybe I had some
other error that caused it.

[Nico, 2015-07-13 23:22:03]

patch set 4 lgtm. If it doesn't build on some bots, patch set 3 lgtm.

[Evan Stade, 2015-07-14 01:41:55]

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

[commit-bot: I haz the power, 2015-07-14 01:43:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1236863006/100001

[commit-bot: I haz the power, 2015-07-14 01:59:34]

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

[commit-bot: I haz the power, 2015-07-14 01:59:35]

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

[Evan Stade, 2015-07-14 03:18:12]

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

[commit-bot: I haz the power, 2015-07-14 03:18:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1236863006/120001

[commit-bot: I haz the power, 2015-07-14 03:42:43]

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

[commit-bot: I haz the power, 2015-07-14 03:42:43]

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

(exceeded global retry quota)

[Evan Stade, 2015-07-14 05:24:01]

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

[commit-bot: I haz the power, 2015-07-14 05:24:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1236863006/140001

[Evan Stade, 2015-07-14 05:24:54]

going to TBR owners because this is actively breaking things. I will address any
feedback in a follow up CL.

[commit-bot: I haz the power, 2015-07-14 06:21:54]

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

[commit-bot: I haz the power, 2015-07-14 06:21:56]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[sadrul, 2015-07-14 08:07:15]

lgtm

Looks like you also need to add dep on gfx:geometry

https://codereview.chromium.org/1236863006/diff/140001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/1236863006/diff/140001/ui/gfx/gfx.gyp#newcode485
ui/gfx/gfx.gyp:485: },
Does something equivalent need to happen for gn?

[Evan Stade, 2015-07-14 14:01:53]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-07-14 14:01:53]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1236863006/#ps160001
(title: "geo")

[commit-bot: I haz the power, 2015-07-14 14:02:20]

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

[commit-bot: I haz the power, 2015-07-14 15:14:06]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-07-14 15:15:03]

Patchset 9 (id:??) landed as
https://crrev.com/a8c375bf808b8b169e4303deeac4cce20c786d29
Cr-Commit-Position: refs/heads/master@{#338690}

[Evan Stade, 2015-07-14 16:43:22]

https://codereview.chromium.org/1236863006/diff/140001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/1236863006/diff/140001/ui/gfx/gfx.gyp#newcode485
ui/gfx/gfx.gyp:485: },
On 2015/07/14 08:07:14, sadrul wrote:
> Does something equivalent need to happen for gn?

We* don't think so.

*thakis