mustash: Break dependency of //ash/public/interfaces on //ash

mustash: Break dependency of //ash/public/interfaces on //ash

In a future CL I need to use ash's mojom interfaces within ash. Trying
to do that now creates a dependency cycle due to typemaps.

* Create //ash/public/cpp and move shelf_types.h there, since it is a
public header used outside of ash
* Make //ash/public/interfaces typemaps depend on //ash/public/cpp instead
of //ash

The file move was mechanical:
tools/git/move_source_file.py ash/common/shelf/shelf_types.h \
    ash/public/cpp

BUG=none
TEST=compiles, checkdeps, gn check

Committed: https://crrev.com/d4649fa5f41c85707a7c1641e940d7a83549d454
Cr-Commit-Position: refs/heads/master@{#422149}

[James Cook, 2016-09-30 01:33:00]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 01:33:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-09-30 01:36:10]

jamescook@chromium.org changed reviewers:
+ msw@chromium.org, rockot@chromium.org

[James Cook, 2016-09-30 01:36:11]

rockot, please take a look at //ash/public/*

msw, can I get a rubber-stamp for the file move touching //c/b/ui/ash ?

sky, just FYI. I'm introducing a //ash/public/cpp directory. We might be able to
put code there instead of in //ash/shared.

https://codereview.chromium.org/2376103005/diff/20001/ash/public/interfaces/s...
File ash/public/interfaces/shelf.typemap (right):

https://codereview.chromium.org/2376103005/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/shelf.typemap:7: traits_headers = [
"//ash/public/interfaces/shelf_enum_traits.h" ]
Is it more idiomatic to leave the traits headers in the //foo/public/interfaces
directory or to move them to //foo/public/cpp ?

[commit-bot: I haz the power, 2016-09-30 01:37:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 01:37:33]

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...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-09-30 01:50:09]

lgtm, thanks! (ash/public/cpp seems better than ash/shared)

https://codereview.chromium.org/2376103005/diff/20001/ash/public/interfaces/s...
File ash/public/interfaces/shelf.typemap (right):

https://codereview.chromium.org/2376103005/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/shelf.typemap:7: traits_headers = [
"//ash/public/interfaces/shelf_enum_traits.h" ]
On 2016/09/30 01:36:10, James Cook wrote:
> Is it more idiomatic to leave the traits headers in the
//foo/public/interfaces
> directory or to move them to //foo/public/cpp ?

I'd guess interfaces, but could be wrong.

[sky, 2016-09-30 03:11:56]

Correct me if I'm wrong, but ash/shared and ash/public/cpp seem to be
slightly different use cases. ash/public/cpp is a client library
related to using the mojoms, where as ash/shared is for code that some
clients may want to make it easier to use certain features. Maybe I'm
splitting hairs and we should just combine...

On Thu, Sep 29, 2016 at 6:36 PM,  <jamescook@chromium.org> wrote:
> Reviewers: Ken Rockot, msw
> CL: https://codereview.chromium.org/2376103005/
>
> Message:
> rockot, please take a look at //ash/public/*
>
> msw, can I get a rubber-stamp for the file move touching //c/b/ui/ash ?
>
> sky, just FYI. I'm introducing a //ash/public/cpp directory. We might be
> able to
> put code there instead of in //ash/shared.
>
>
>
>
https://codereview.chromium.org/2376103005/diff/20001/ash/public/interfaces/s...
> File ash/public/interfaces/shelf.typemap (right):
>
>
https://codereview.chromium.org/2376103005/diff/20001/ash/public/interfaces/s...
> ash/public/interfaces/shelf.typemap:7: traits_headers = [
> "//ash/public/interfaces/shelf_enum_traits.h" ]
> Is it more idiomatic to leave the traits headers in the
> //foo/public/interfaces directory or to move them to //foo/public/cpp ?
>
> Description:
> mustash: Break dependency of //ash/public/interfaces on //ash
>
> In a future CL I need to use ash's mojom interfaces within ash. Trying
> to do that now creates a dependency cycle due to typemaps.
>
> * Create //ash/public/cpp and move shelf_types.h there, since it is a
> public header used outside of ash
> * Make //ash/public/interfaces typemaps depend on //ash/public/cpp instead
> of //ash
>
> The file move was mechanical:
> tools/git/move_source_file.py ash/common/shelf/shelf_types.h \
> ash/public/cpp
>
> BUG=none
> TEST=compiles, checkdeps, gn check
>
> Affected files (+51, -109 lines):
> M ash/BUILD.gn
> M ash/app_list/app_list_presenter_delegate.cc
> M ash/common/DEPS
> M ash/common/shelf/app_list_button.cc
> M ash/common/shelf/shelf_alignment_menu.cc
> M ash/common/shelf/shelf_background_animator.h
> M ash/common/shelf/shelf_layout_manager.h
> M ash/common/shelf/shelf_layout_manager_observer.h
> M ash/common/shelf/shelf_locking_manager.h
> D ash/common/shelf/shelf_types.h
> M ash/common/shelf/shelf_widget.h
> M ash/common/shelf/wm_shelf.h
> M ash/common/shelf/wm_shelf_observer.h
> M ash/common/shelf/wm_shelf_util.h
> M ash/common/system/cast/tray_cast.cc
> M ash/common/system/chromeos/session/logout_button_tray.cc
> M ash/common/system/overview/overview_button_tray.cc
> M ash/common/system/status_area_widget.h
> M ash/common/system/status_area_widget_delegate.h
> M ash/common/system/tray/system_tray_item.h
> M ash/common/system/tray/tray_background_view.h
> M ash/common/system/tray/tray_item_view.cc
> M ash/common/system/tray/tray_utils.h
> M ash/common/system/web_notification/ash_popup_alignment_delegate.h
> M ash/common/system/web_notification/ash_popup_alignment_delegate.cc
> M ash/common/wm/overview/window_grid.cc
> M ash/content/keyboard_overlay/keyboard_overlay_delegate_unittest.cc
> M ash/mus/context_menu_mus.cc
> M ash/mus/shelf_delegate_mus.h
> A + ash/public/cpp/BUILD.gn
> A + ash/public/cpp/shelf_types.h
> M ash/public/interfaces/shelf.typemap
> M ash/public/interfaces/shelf_enum_traits.h
> M ash/root_window_controller.h
> M ash/root_window_controller.cc
> M ash/shelf/shelf_window_targeter.cc
> M ash/shell.h
> M ash/shell/context_menu.cc
> M ash/system/overview/overview_button_tray_unittest.cc
> M ash/system/web_notification/ash_popup_alignment_delegate_unittest.cc
> M ash/wm/dock/docked_window_resizer_unittest.cc
> M ash/wm/immersive_fullscreen_controller_unittest.cc
> M ash/wm/panels/panel_layout_manager_unittest.cc
> M ash/wm/panels/panel_window_resizer_unittest.cc
> M chrome/browser/ui/BUILD.gn
> M chrome/browser/ui/ash/chrome_launcher_prefs.h
> M chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h
> M chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-09-30 03:14:47]

LGTM as non-owner.

I could see treating ash/shared as stuff which is shared between the client
library and the impl... but in the end there's no reason an impl can't use its
own client library so yeah, I'd probably prefer we merged things like that.

[James Cook, 2016-09-30 15:42:12]

I like the idea of combining ash/shared and ash/public/cpp into just
ash/public/cpp. That means we can tell other developers "look, you can only use
stuff in ash/public", which will get them both C++ and mojom.

I'll take that up in a separate CL, though.

Thanks for the review / feedback / help, guys!

[James Cook, 2016-09-30 16:28:08]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-09-30 16:28:09]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2376103005/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-30 16:28:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 17:50:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-30 17:52:47]

Description was changed from

==========
mustash: Break dependency of //ash/public/interfaces on //ash

In a future CL I need to use ash's mojom interfaces within ash. Trying
to do that now creates a dependency cycle due to typemaps.

* Create //ash/public/cpp and move shelf_types.h there, since it is a
public header used outside of ash
* Make //ash/public/interfaces typemaps depend on //ash/public/cpp instead
of //ash

The file move was mechanical:
tools/git/move_source_file.py ash/common/shelf/shelf_types.h \
    ash/public/cpp

BUG=none
TEST=compiles, checkdeps, gn check
==========

to

==========
mustash: Break dependency of //ash/public/interfaces on //ash

In a future CL I need to use ash's mojom interfaces within ash. Trying
to do that now creates a dependency cycle due to typemaps.

* Create //ash/public/cpp and move shelf_types.h there, since it is a
public header used outside of ash
* Make //ash/public/interfaces typemaps depend on //ash/public/cpp instead
of //ash

The file move was mechanical:
tools/git/move_source_file.py ash/common/shelf/shelf_types.h \
    ash/public/cpp

BUG=none
TEST=compiles, checkdeps, gn check

Committed: https://crrev.com/d4649fa5f41c85707a7c1641e940d7a83549d454
Cr-Commit-Position: refs/heads/master@{#422149}
==========

[commit-bot: I haz the power, 2016-09-30 17:52:48]

Patchset 4 (id:??) landed as
https://crrev.com/d4649fa5f41c85707a7c1641e940d7a83549d454
Cr-Commit-Position: refs/heads/master@{#422149}