New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 597020 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

content/shell depends on non-public content/browser files on Android

Project Member Reported by brettw@chromium.org, Mar 22 2016

Issue description

I'm trying to make "gn check" pass in content. On Android:

> gn check out/Default_android //content/shell:content_shell_lib
ERROR at //content/shell/browser/shell_mojo_test_utils_android.cc:12:11: Can't include this header from here.
#include "content/browser/mojo/service_registry_android.h"
          ^----------------------------------------------
The target:
  //content/shell:content_shell_lib
is including a file from the target:
  //content/browser:browser

It's usually best to depend directly on the destination target.
In some cases, the destination target is considered a subcomponent
of an intermediate target. In this case, the intermediate target
should depend publicly on the destination to forward the ability
to include headers.

Dependency chain (there may also be others):
  //content/shell:content_shell_lib -->
  //content/test:layouttest_support --[private]-->
  //content/browser:browser


This should use a public API or the file should be moved.
 

Comment 1 by jam@chromium.org, Mar 22 2016

Labels: -Pri-3 Pri-1
Owner: p...@chromium.org
Status: Assigned (was: Untriaged)
this was added in https://crrev.com/f581fe975fcb0039a5ae790b9dbb9b007c28b704

This is an incorrect dependency and should be removed as soon as possible.

Comment 2 by jam@chromium.org, Apr 18 2016

Cc: -jam@chromium.org sa...@chromium.org roc...@chromium.org yzshen@chromium.org amistry@chromium.org
Owner: ----
Status: Available (was: Assigned)
Removing ppi as he's not working on Chromium anymore.

Adding a few more folks.

Comment 3 by p...@chromium.org, Apr 18 2016

FYI, AFAIR, this was needed so that we can test content/common/mojo/service_registry_impl in content shell Java tests. Because of https://bugs.chromium.org/p/chromium/issues/detail?id=415945 , we couldn't put the native test hook into something test-specific, so it ended up in content/shell.
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/21c9135843f8d28df74b47f5c05486a59a01ae27

commit 21c9135843f8d28df74b47f5c05486a59a01ae27
Author: jam <jam@chromium.org>
Date: Tue Apr 19 18:28:15 2016

Fix content/shell including internals of content for service worker test.

BUG= 597020 

Review URL: https://codereview.chromium.org/1900903002

Cr-Commit-Position: refs/heads/master@{#388254}

[modify] https://crrev.com/21c9135843f8d28df74b47f5c05486a59a01ae27/chrome/chrome_gpu.gypi
[modify] https://crrev.com/21c9135843f8d28df74b47f5c05486a59a01ae27/chrome/gpu/BUILD.gn
[modify] https://crrev.com/21c9135843f8d28df74b47f5c05486a59a01ae27/components/components.gyp
[modify] https://crrev.com/21c9135843f8d28df74b47f5c05486a59a01ae27/components/contextual_search.gypi
[modify] https://crrev.com/21c9135843f8d28df74b47f5c05486a59a01ae27/content/common/mojo/service_registry_impl.cc
[modify] https://crrev.com/21c9135843f8d28df74b47f5c05486a59a01ae27/content/common/mojo/service_registry_impl.h
[modify] https://crrev.com/21c9135843f8d28df74b47f5c05486a59a01ae27/content/public/common/BUILD.gn
[modify] https://crrev.com/21c9135843f8d28df74b47f5c05486a59a01ae27/content/public/common/service_registry.h
[modify] https://crrev.com/21c9135843f8d28df74b47f5c05486a59a01ae27/content/shell/DEPS
[modify] https://crrev.com/21c9135843f8d28df74b47f5c05486a59a01ae27/content/shell/browser/shell_mojo_test_utils_android.cc
[modify] https://crrev.com/21c9135843f8d28df74b47f5c05486a59a01ae27/media/mojo/interfaces/mojo_bindings.gyp

Comment 5 by jam@chromium.org, Apr 19 2016

Owner: jam@chromium.org
Status: Fixed (was: Available)

Sign in to add a comment