Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2820563004:
Create separate BUILD.gn for //apps/ui/views (Closed)
DescriptionCreate separate BUILD.gn for //apps/ui/views
This moves the views-related dependencies (including chrome/app/theme)
out of the top-level //apps static library.
Some DEPS includes were dropped entirely as they are no longer used in
//apps.
BUG=679971
R=benwells@chromium.org
Review-Url: https://codereview.chromium.org/2820563004
Cr-Commit-Position: refs/heads/master@{#469549}
Committed: https://chromium.googlesource.com/chromium/src/+/6d0b3151c08b8056637938953c936719e583dc54
Patch Set 1 #Patch Set 2 : handle is_chromecast :-/ #
Total comments: 3
Patch Set 3 : rebase #
Total comments: 2
Patch Set 4 : rebase #
Total comments: 4
Messages
Total messages: 30 (15 generated)
The CQ bit was checked by michaelpg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Description was changed from ========== Create separate BUILD.gn for //apps/ui/views This moves the views-related dependencies out of the top-level //apps static library. Some DEPS includes were dropped entirely as they are no longer used in //apps. BUG= R= ========== to ========== Create separate BUILD.gn for //apps/ui/views This moves the views-related dependencies out of the top-level //apps static library. Some DEPS includes were dropped entirely as they are no longer used in //apps. BUG=679971 R=benwells@chromium.org ==========
michaelpg@chromium.org changed reviewers: + benwells@chromium.org
ben: WDYT? I'm leaving the TODOs here, and I think they're still worth addressing, but at least this narrows the problem to the views code (which only chrome/browser/ui actually uses).
Description was changed from ========== Create separate BUILD.gn for //apps/ui/views This moves the views-related dependencies out of the top-level //apps static library. Some DEPS includes were dropped entirely as they are no longer used in //apps. BUG=679971 R=benwells@chromium.org ========== to ========== Create separate BUILD.gn for //apps/ui/views This moves the views-related dependencies (including chrome/app/theme) out of the top-level //apps static library. Some DEPS includes were dropped entirely as they are no longer used in //apps. BUG=679971 R=benwells@chromium.org ==========
Looks great, just one question as my DEPS foo is weak. https://codereview.chromium.org/2820563004/diff/20001/apps/ui/views/DEPS File apps/ui/views/DEPS (right): https://codereview.chromium.org/2820563004/diff/20001/apps/ui/views/DEPS#newc... apps/ui/views/DEPS:12: "!chrome/grit/theme_resources.h", What does the "!" do?
https://codereview.chromium.org/2820563004/diff/20001/apps/ui/views/DEPS File apps/ui/views/DEPS (right): https://codereview.chromium.org/2820563004/diff/20001/apps/ui/views/DEPS#newc... apps/ui/views/DEPS:12: "!chrome/grit/theme_resources.h", On 2017/04/18 07:08:17, benwells wrote: > What does the "!" do? It means something like "this is temporary", works the same as + but adds a PRESBUMIT warning when #include-ing it. https://chromium.googlesource.com/chromium/buildtools/+/master/checkdeps/READ...
lgtm https://codereview.chromium.org/2820563004/diff/20001/apps/ui/views/DEPS File apps/ui/views/DEPS (right): https://codereview.chromium.org/2820563004/diff/20001/apps/ui/views/DEPS#newc... apps/ui/views/DEPS:12: "!chrome/grit/theme_resources.h", On 2017/04/18 21:41:50, michaelpg wrote: > On 2017/04/18 07:08:17, benwells wrote: > > What does the "!" do? > > It means something like "this is temporary", works the same as + but adds a > PRESBUMIT warning when #include-ing it. > > https://chromium.googlesource.com/chromium/buildtools/+/master/checkdeps/READ... Oh, cool. TIL!
Description was changed from ========== Create separate BUILD.gn for //apps/ui/views This moves the views-related dependencies (including chrome/app/theme) out of the top-level //apps static library. Some DEPS includes were dropped entirely as they are no longer used in //apps. BUG=679971 R=benwells@chromium.org ========== to ========== Create separate BUILD.gn for //apps/ui/views This moves the views-related dependencies (including chrome/app/theme) out of the top-level //apps static library. Some DEPS includes were dropped entirely as they are no longer used in //apps. BUG=679971 R=benwells@chromium.org TBR=thakis@chromium.org,skyostil@chromium.org ==========
Description was changed from ========== Create separate BUILD.gn for //apps/ui/views This moves the views-related dependencies (including chrome/app/theme) out of the top-level //apps static library. Some DEPS includes were dropped entirely as they are no longer used in //apps. BUG=679971 R=benwells@chromium.org TBR=thakis@chromium.org,skyostil@chromium.org ========== to ========== Create separate BUILD.gn for //apps/ui/views This moves the views-related dependencies (including chrome/app/theme) out of the top-level //apps static library. Some DEPS includes were dropped entirely as they are no longer used in //apps. BUG=679971 R=benwells@chromium.org ==========
michaelpg@chromium.org changed reviewers: + skyostil@chromium.org, thakis@chromium.org
Adding DEPS target owners. Note the rules were moved into a deeper subdirectory (meaning are *more* restrictive than before).
I'm not an apps/OWNER. Did you mean tapted?
On 2017/04/21 20:42:58, Nico wrote: > I'm not an apps/OWNER. Did you mean tapted? I need OWNERs approval for adding the +ui rule to apps/ui/views/DEPS, according to the presubmit. skyostil: same for cc/paint. Thanks!
Ah, thanks for explaining. lgtm. https://codereview.chromium.org/2820563004/diff/40001/apps/ui/views/DEPS File apps/ui/views/DEPS (right): https://codereview.chromium.org/2820563004/diff/40001/apps/ui/views/DEPS#newc... apps/ui/views/DEPS:4: "+grit/theme_resources.h", You can probably remove this line, all the grit includes are now qualified and this rule shouldn't match anything any more. https://codereview.chromium.org/2820563004/diff/40001/apps/ui/views/DEPS#newc... apps/ui/views/DEPS:9: "+ui/strings/grit/ui_strings.h", You can remove this now.
Dep on cc/paint lgtm.
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benwells@chromium.org, thakis@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2820563004/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493939454405360,
"parent_rev": "3580fd5b0d3956acc69cf310669df6c202fac859", "commit_rev":
"6d0b3151c08b8056637938953c936719e583dc54"}
Message was sent while issue was closed.
Description was changed from ========== Create separate BUILD.gn for //apps/ui/views This moves the views-related dependencies (including chrome/app/theme) out of the top-level //apps static library. Some DEPS includes were dropped entirely as they are no longer used in //apps. BUG=679971 R=benwells@chromium.org ========== to ========== Create separate BUILD.gn for //apps/ui/views This moves the views-related dependencies (including chrome/app/theme) out of the top-level //apps static library. Some DEPS includes were dropped entirely as they are no longer used in //apps. BUG=679971 R=benwells@chromium.org Review-Url: https://codereview.chromium.org/2820563004 Cr-Commit-Position: refs/heads/master@{#469549} Committed: https://chromium.googlesource.com/chromium/src/+/6d0b3151c08b8056637938953c93... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6d0b3151c08b8056637938953c93...
Message was sent while issue was closed.
hugoh@opera.com changed reviewers: + hugoh@opera.com
Message was sent while issue was closed.
https://codereview.chromium.org/2820563004/diff/60001/apps/BUILD.gn File apps/BUILD.gn (right): https://codereview.chromium.org/2820563004/diff/60001/apps/BUILD.gn#newcode43 apps/BUILD.gn:43: "//extensions/browser", Somewhere here, I believe this dependency causes a little bit trouble for my team as we sometimes compile with !enable_extensions. Is it possible to add the |if (enable_extensions)| checks back again?
Message was sent while issue was closed.
https://codereview.chromium.org/2820563004/diff/60001/apps/BUILD.gn File apps/BUILD.gn (right): https://codereview.chromium.org/2820563004/diff/60001/apps/BUILD.gn#newcode43 apps/BUILD.gn:43: "//extensions/browser", On 2017/05/22 15:38:25, hugoh_UTC2 wrote: > Somewhere here, I believe this dependency causes a little bit trouble for my > team as we sometimes compile with !enable_extensions. > > Is it possible to add the |if (enable_extensions)| checks back again? Sure, just send me a patch.
Message was sent while issue was closed.
https://codereview.chromium.org/2820563004/diff/60001/apps/ui/views/BUILD.gn File apps/ui/views/BUILD.gn (right): https://codereview.chromium.org/2820563004/diff/60001/apps/ui/views/BUILD.gn#... apps/ui/views/BUILD.gn:8: assert(enable_extensions) Is this the part that caused problems when compiling? What gn args are you using?
Message was sent while issue was closed.
https://codereview.chromium.org/2820563004/diff/60001/apps/ui/views/BUILD.gn File apps/ui/views/BUILD.gn (right): https://codereview.chromium.org/2820563004/diff/60001/apps/ui/views/BUILD.gn#... apps/ui/views/BUILD.gn:8: assert(enable_extensions) On 2017/05/22 23:15:41, michaelpg wrote: > Is this the part that caused problems when compiling? What gn args are you > using? Yes, this assert (and some others of the same kind) fails when we build with enable_extensions=false. I've uploaded fixes for this at https://codereview.chromium.org/2904443004 - let's continue the discussion there! :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
