New issue
Advanced search Search tips

Issue 862049 link

Starred by 1 user
Status: Fixed
Owner:
loyso@chromium.org
Closed: Jul 28
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Feature

Blocking:
issue 860581



Sign in to add a comment

Fix gn check errors for web_app.h in chrome/browser/ui

Project Member Reported by loyso@chromium.org, Jul 10 
To fix the gn check errors (see logs attached)
we use an explicit dependency so far:
//chrome/browser/web_applications:web_applications
//chrome/browser/web_applications:web_applications_on_extensions

`gn check` fails due to unconditionally included dependencies due to a conditionally-applied library dependency, it is usually a bug. The fix has traditionally been something like

#if defined(USE_AURA)
#include "ui/aura/window.h"  // nogncheck
#endif

But maybe `gn check` is getting smarter about this. E.g. if a dependency had to stay,

#if BUILDFLAG(ENABLE_EXTENSIONS)
#include ".. /web_app.h"
#endif

should fix the complaints from `gn check`

Since web_app _shouldn't_ have an extensions dependency, that might not be the right fix.

To avoid a mess to untangle later, one fix might be to add a header in the target you want to depend on that #includes web_app.h so there's some indirection.

Context:
https://chromium-review.googlesource.com/c/chromium/src/+/1128665/6/chrome/browser/ui/BUILD.gn#1732
gn_check_chromeos.log
4.9 KB View Download
gn_check_linux.log
2.8 KB View Download

Comment 1 by loyso@chromium.org, Jul 16

Owner: loyso@chromium.org
Status: Started (was: Available)

Comment 2 by loyso@chromium.org, Jul 16

Blocking: 860581
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 17 

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

commit 2f8bf8166fd41d1138adf5ac382171414882bb81
Author: Alexey Baskakov <loyso@chromium.org>
Date: Tue Jul 17 01:06:11 2018

WebApp: Remove chrome/browser/ui dependency on web_app.h

Most callers in chrome/browser/ui need just one helper:
GenerateApplicationNameFromExtensionId

All callers in chrome/browser/ui need just 3 helper functions from
chrome/browser/web_applications/web_app.h.
(See the CL code)

So let's move out these helpers. (No changes in behavior)

This change will allow us to break chrome/browser/ui dependencies on
chrome/browser/web_applications root target.

Also: this CL removes circular dependency between shell_integration_win.cc and
web_app.h

Next steps:
1) Remove web_app.h dependency in create_application_shortcut_view.h
2) Remove web_app.h dependency in shell_integration_linux.cc

Bug:  862049 
Change-Id: I53242317d73cc3df5aae0bb61d17aa288fd51821
Reviewed-on: https://chromium-review.googlesource.com/1137953
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575506}
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/app_controller_mac.mm
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/extensions/api/tabs/tabs_api.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/extensions/bookmark_app_navigation_throttle_browsertest.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/extensions/bookmark_app_navigation_throttle_utils.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/extensions/browsertest_util.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/extensions/tab_helper.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/renderer_context_menu/render_view_context_menu.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/shell_integration_linux_unittest.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/shell_integration_win.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/shell_integration_win_unittest.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/ash/launcher/browser_status_monitor.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/ash/launcher/launcher_controller_helper.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/browser_navigator.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/extensions/application_launch.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/extensions/hosted_app_browser_controller.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/views/apps/chrome_native_app_window_views_aura.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/views/frame/browser_window_property_manager_win.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/ui/views/frame/desktop_browser_frame_aura.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/web_applications/components/BUILD.gn
[add] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/web_applications/components/web_app_helpers.cc
[add] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/web_applications/components/web_app_helpers.h
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/web_applications/extensions/BUILD.gn
[add] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/web_applications/extensions/web_app_extension_helpers.cc
[add] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/web_applications/extensions/web_app_extension_helpers.h
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/web_applications/web_app.cc
[modify] https://crrev.com/2f8bf8166fd41d1138adf5ac382171414882bb81/chrome/browser/web_applications/web_app.h
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 28 

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

commit f302efe3d406be0c40a20886dde91cfaedbe7ff5
Author: Alexey Baskakov <loyso@chromium.org>
Date: Sat Jul 28 02:02:32 2018

WebApp: Split up web_app.h into extensions/ and components/

This is a move-and-rename CL, no behavior changes.

Results:
- web_app_shortcut.h contains extension-independent stuff.
- web_app_extension_shortcut.h contains extension-based code.
- All shortcut-related code moved from shell_integration_linux.cc
to web_app_shortcut_linux.cc (so no circular dependency on web_app.h)

This CL untangles all circular dependencies listed in
web_applications/BUILD.gn.

Modernize:
- Replace arraysize with base::size.
- UTF8String -> SysNSStringToUTF8.
- BindOnce.

Bug:  862049 
Change-Id: I9bf098d00f363aa8ed1a0c45514dc04cb9923e9e
Reviewed-on: https://chromium-review.googlesource.com/1138042
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578897}
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/app_controller_mac.mm
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/apps/app_shim/app_shim_interactive_uitest_mac.mm
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/apps/platform_apps/platform_app_navigation_redirector.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/apps/platform_apps/shortcut_manager.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/extensions/bookmark_app_helper.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/extensions/extension_browsertest.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/extensions/tab_helper.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/prefs/browser_prefs.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/shell_integration.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/shell_integration_linux.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/shell_integration_linux.h
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/shell_integration_linux_unittest.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/task_manager/task_manager_browsertest.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/ui/browser_navigator_browsertest.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/ui/startup/startup_browser_creator.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/ui/views/apps/chrome_native_app_window_views_aura.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/ui/views/create_application_shortcut_view.h
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/ui/views/frame/browser_window_property_manager_browsertest_win.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/ui/views/frame/browser_window_property_manager_win.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/ui/views/frame/desktop_browser_frame_aurax11.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/BUILD.gn
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/components/BUILD.gn
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/components/web_app_helpers.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/components/web_app_helpers.h
[add] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/components/web_app_shortcut.cc
[add] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/components/web_app_shortcut.h
[rename] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/components/web_app_shortcut_chromeos.cc
[add] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/components/web_app_shortcut_linux.cc
[add] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/components/web_app_shortcut_linux.h
[add] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/components/web_app_shortcut_linux_unittest.cc
[rename] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/components/web_app_shortcut_mac.h
[rename] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/components/web_app_shortcut_mac.mm
[rename] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/components/web_app_shortcut_mac_unittest.mm
[add] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/components/web_app_shortcut_unittest.cc
[rename] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/components/web_app_shortcut_win.cc
[add] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/components/web_app_shortcut_win.h
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/extensions/BUILD.gn
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/extensions/web_app_extension_helpers.cc
[modify] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/extensions/web_app_extension_helpers.h
[rename] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/extensions/web_app_extension_shortcut.cc
[add] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/extensions/web_app_extension_shortcut.h
[add] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/extensions/web_app_extension_shortcut_mac.h
[add] https://crrev.com/f302efe3d406be0c40a20886dde91cfaedbe7ff5/chrome/browser/web_applications/extensions/web_app_extension_shortcut_mac.mm
[delete] https://crrev.com/e60b716ff9d4e3a21c253be5630c1f7cf2042bd2/chrome/browser/web_applications/web_app.h
[delete] https://crrev.com/e60b716ff9d4e3a21c253be5630c1f7cf2042bd2/chrome/browser/web_applications/web_app_linux.cc
[delete] https://crrev.com/e60b716ff9d4e3a21c253be5630c1f7cf2042bd2/chrome/browser/web_applications/web_app_unittest.cc
[delete] https://crrev.com/e60b716ff9d4e3a21c253be5630c1f7cf2042bd2/chrome/browser/web_applications/web_app_win.h

Comment 5 by loyso@chromium.org, Jul 28

Status: Fixed (was: Started)
►

Sign in to add a comment