Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(12)

Issue 1236863006: Generated header files apparently require hard_dependency (Closed)

Created:
4 years, 4 months ago by Evan Stade
Modified:
4 years, 4 months ago
Reviewers:
Nico, sadrul
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 4

Patch Set 2 : separate build target #

Patch Set 3 : don't include header in header #

Total comments: 2

Patch Set 4 : forward declare #

Patch Set 5 : typo fix #

Patch Set 6 : attempt to fix ms #

Patch Set 7 : fix? #

Patch Set 8 : depend on skia #

Total comments: 2

Patch Set 9 : geo #

Unified diffs Side-by-side diffs Delta from patch set Stats (+56 lines, -36 lines) Patch
M chrome/browser/ui/views/profiles/profile_chooser_view.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M ui/gfx/gfx.gyp View 1 2 3 4 5 6 7 8 2 chunks +49 lines, -35 lines 0 comments Download
M ui/views/controls/image_view.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M ui/views/controls/image_view.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M ui/views/views.gyp View 1 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 30 (10 generated)
Evan Stade
4 years, 4 months ago (2015-07-13 19:01:18 UTC) #2
Nico
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, ...
4 years, 4 months ago (2015-07-13 19:05:55 UTC) #3
Evan Stade
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 ...
4 years, 4 months ago (2015-07-13 19:18:41 UTC) #4
Nico
stampy-ish lgtm (If the header is included in headers in dependent projects, you also need ...
4 years, 4 months ago (2015-07-13 19:24:03 UTC) #5
Evan Stade
On 2015/07/13 19:24:03, Nico wrote: > stampy-ish lgtm > > (If the header is included ...
4 years, 4 months ago (2015-07-13 21:59:24 UTC) #7
Nico
https://codereview.chromium.org/1236863006/diff/40001/ui/views/controls/image_view.h File ui/views/controls/image_view.h (right): https://codereview.chromium.org/1236863006/diff/40001/ui/views/controls/image_view.h#newcode125 ui/views/controls/image_view.h:125: int vector_id_; you can probably keep this a gfx::VectorIconId. ...
4 years, 4 months ago (2015-07-13 22:06:12 UTC) #8
Evan Stade
https://codereview.chromium.org/1236863006/diff/40001/ui/views/controls/image_view.h File ui/views/controls/image_view.h (right): https://codereview.chromium.org/1236863006/diff/40001/ui/views/controls/image_view.h#newcode125 ui/views/controls/image_view.h:125: int vector_id_; On 2015/07/13 22:06:12, Nico wrote: > you ...
4 years, 4 months ago (2015-07-13 23:20:44 UTC) #9
Nico
patch set 4 lgtm. If it doesn't build on some bots, patch set 3 lgtm.
4 years, 4 months ago (2015-07-13 23:22:03 UTC) #10
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1236863006/100001
4 years, 4 months ago (2015-07-14 01:43:02 UTC) #12
commit-bot: I haz the power
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_compile_dbg_32_ng/builds/72886)
4 years, 4 months ago (2015-07-14 01:59:35 UTC) #14
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1236863006/120001
4 years, 4 months ago (2015-07-14 03:18:41 UTC) #16
commit-bot: I haz the power
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_chromeos_compile_dbg_ng/builds/71303) (exceeded global ...
4 years, 4 months ago (2015-07-14 03:42:43 UTC) #18
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1236863006/140001
4 years, 4 months ago (2015-07-14 05:24:17 UTC) #20
Evan Stade
going to TBR owners because this is actively breaking things. I will address any feedback ...
4 years, 4 months ago (2015-07-14 05:24:54 UTC) #21
commit-bot: I haz the power
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_compile_dbg_ng/builds/58759)
4 years, 4 months ago (2015-07-14 06:21:56 UTC) #23
sadrul
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): ...
4 years, 4 months ago (2015-07-14 08:07:15 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1236863006/160001
4 years, 4 months ago (2015-07-14 14:02:20 UTC) #27
commit-bot: I haz the power
Committed patchset #9 (id:160001)
4 years, 4 months ago (2015-07-14 15:14:06 UTC) #28
commit-bot: I haz the power
Patchset 9 (id:??) landed as https://crrev.com/a8c375bf808b8b169e4303deeac4cce20c786d29 Cr-Commit-Position: refs/heads/master@{#338690}
4 years, 4 months ago (2015-07-14 15:15:03 UTC) #29
Evan Stade
4 years, 4 months ago (2015-07-14 16:43:22 UTC) #30
Message was sent while issue was closed.
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

Powered by Google App Engine
This is Rietveld 408576698