guest_view component violates layering rule
Reported by
most...@opera.com,
May 26 2017
|
||||||
Issue descriptionGuestView appears to be specific to the chrome layer, and depends upon //extensions. I believe this violates the layering rule that components should only depend on lower level targets. Also, since it's chrome-specific, shouldn't this code live in //chrome/ ? Reference: https://chromium.googlesource.com/chromium/src/+/master/components/README CC Jochen for //components OWNER input... Also, I notice that //components/guest_view/ imports //extensions/features/features.gni and asserts that extensions are enabled, but has no other references to extensions in either code or build configuration. This seems a little strange, not sure if this also needs followup.
,
May 26 2017
,
May 26 2017
Issue 726690 has been merged into this issue.
,
May 26 2017
,
May 26 2017
I introduced the //extensions/features/features.gni import and the assert(enable_extensions) in https://codereview.chromium.org/2874833002/ . The motivation was just to prevent guest_view from being accidentally built for android, since guest_views are (currently) only used by extensions. It looks like these are the only references to extensions in components/guest_view, so I can modify the assert to not reference extensions. guest_views were moved to components here: https://bugs.chromium.org/p/chromium/issues/detail?id=444869 .
,
May 29 2017
Perhaps you can simply assert(!is_android) ? Or maybe change the visibility of guest_view so that it's only dependable upon from extensions?
,
May 29 2017
This CL is in development: https://codereview.chromium.org/2907863002/
,
May 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f0a499017741777102444b10bff254e347f96571 commit f0a499017741777102444b10bff254e347f96571 Author: mcnee <mcnee@chromium.org> Date: Tue May 30 16:35:38 2017 Remove references to extensions from //components/guest_view/. We had an assert(enable_extensions) in //components/guest_view/ to prevent the accidental building of GuestViews on mobile, since extensions are currently the only thing using GuestViews. However, referencing extensions here violates layering. This modifies the assert to not reference extensions. BUG= 726694 Review-Url: https://codereview.chromium.org/2907863002 Cr-Commit-Position: refs/heads/master@{#475554} [modify] https://crrev.com/f0a499017741777102444b10bff254e347f96571/components/BUILD.gn [modify] https://crrev.com/f0a499017741777102444b10bff254e347f96571/components/guest_view/browser/BUILD.gn [modify] https://crrev.com/f0a499017741777102444b10bff254e347f96571/components/guest_view/renderer/BUILD.gn
,
May 30 2017
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by jochen@chromium.org
, May 26 2017