|
|
Created:
3 years, 9 months ago by afakhry Modified:
3 years, 8 months ago CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, chfremer+watch_chromium.org, dcheng, dmazzoni+watch_chromium.org, stevenjb+watch_chromium.org, ios-reviews_chromium.org, aboxhall+watch_chromium.org, shuchen+watch_chromium.org, achuith+watch_chromium.org, nona+watch_chromium.org, je_julie, devtools-reviews_chromium.org, kalyank, alemate+watch_chromium.org, yuzo+watch_chromium.org, feature-media-reviews_chromium.org, oshima+watch_chromium.org, Matt Giuca, tfarina, nektar+watch_chromium.org, pfeldman, dtseng+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert Window Container Ids to an Enum
To make it less error-prone, and have the compiler assign values to
the various IDs, it's better to convert them into an enum.
BUG=None
Review-Url: https://codereview.chromium.org/2751523003
Cr-Commit-Position: refs/heads/master@{#464845}
Committed: https://chromium.googlesource.com/chromium/src/+/386772d9ce57793ad8110f3cd976336d4ed181ee
Patch Set 1 #Patch Set 2 : Rebase .... #Patch Set 3 : kFoo over FOO #
Total comments: 4
Patch Set 4 : Rebase ... #
Messages
Total messages: 33 (20 generated)
The CQ bit was checked by afakhry@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: This issue passed the CQ dry run.
The CQ bit was checked by afakhry@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: This issue passed the CQ dry run.
afakhry@chromium.org changed reviewers: + oshima@chromium.org, reveman@chromium.org, thestig@chromium.org, xiyuan@chromium.org
Hi all, This is just a mechanical change suggested during an earlier review to convert the container IDs to an enum to avoid duplicates and have the compiler generate the values. There's already a test I added earlier for detecting duplication. Reviewer per files: thestig: chrome/browser/chromeos/accessibility/chromevox_panel.cc chrome/browser/chromeos/bluetooth/bluetooth_pairing_dialog.cc chrome/browser/chromeos/display/overscan_calibrator.cc chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc chrome/browser/chromeos/input_method/candidate_window_controller_impl.cc chrome/browser/chromeos/input_method/mode_indicator_controller.cc chrome/browser/chromeos/login/ui/lock_window.cc chrome/browser/chromeos/login/ui/login_display_host_impl.cc chrome/browser/chromeos/set_time_dialog.cc chrome/browser/chromeos/ui/choose_mobile_network_dialog.cc chrome/browser/chromeos/ui/idle_app_name_notification_view.cc chrome/browser/chromeos/ui/kiosk_external_update_notification.cc chrome/browser/media/webrtc/desktop_media_list_ash.cc chrome/browser/ui/ash/app_list/app_list_interactive_uitest.cc chrome/browser/ui/ash/chrome_keyboard_ui.cc chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc chrome/browser/ui/ash/system_tray_client.cc chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc chrome/browser/ui/views/chrome_web_dialog_view.cc chrome/browser/ui/views/tabs/window_finder_ash.cc chrome/browser/ui/webui/chromeos/bluetooth_pairing_ui_browsertest-inl.h xiyuan: chrome/browser/chromeos/accessibility/chromevox_panel.cc chrome/browser/chromeos/bluetooth/bluetooth_pairing_dialog.cc chrome/browser/chromeos/display/overscan_calibrator.cc chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc chrome/browser/chromeos/input_method/candidate_window_controller_impl.cc chrome/browser/chromeos/input_method/mode_indicator_controller.cc chrome/browser/chromeos/login/ui/lock_window.cc chrome/browser/chromeos/login/ui/login_display_host_impl.cc chrome/browser/chromeos/set_time_dialog.cc chrome/browser/chromeos/ui/choose_mobile_network_dialog.cc chrome/browser/chromeos/ui/idle_app_name_notification_view.cc chrome/browser/chromeos/ui/kiosk_external_update_notification.cc chrome/browser/ui/ash/app_list/app_list_interactive_uitest.cc chrome/browser/ui/webui/chromeos/bluetooth_pairing_ui_browsertest-inl.h oshima: ash/accelerators/exit_warning_handler.cc ash/app_list/app_list_presenter_delegate.cc ash/app_list/app_list_presenter_delegate_unittest.cc ash/autoclick/autoclick_controller.cc ash/autoclick/mus/autoclick_application.cc ash/devtools/ash_devtools_dom_agent.cc ash/devtools/ash_devtools_unittest.cc ash/display/cursor_window_controller.cc ash/display/display_animator_chromeos.cc ash/drag_drop/drag_image_view.cc ash/extended_desktop_unittest.cc ash/first_run/desktop_cleaner.cc ash/first_run/first_run_helper_impl.cc ash/frame/default_header_painter_unittest.cc ash/frame/header_painter_util.cc ash/laser/laser_pointer_view.cc ash/metrics/pointer_metrics_recorder_unittest.cc ash/metrics/user_metrics_recorder.cc ash/mus/top_level_window_factory.cc ash/public/cpp/shell_window_ids.cc ash/public/cpp/shell_window_ids.h ash/root_window_controller.cc ash/root_window_controller_unittest.cc ash/shelf/overflow_bubble_view.cc ash/shelf/shelf_layout_manager.cc ash/shelf/shelf_layout_manager_unittest.cc ash/shelf/shelf_tooltip_manager.cc ash/shelf/shelf_window_watcher.cc ash/shelf/shelf_window_watcher_unittest.cc ash/shelf/wm_shelf.cc ash/shell.cc ash/shell/lock_view.cc ash/shell/window_watcher.cc ash/shell_port.cc ash/shell_unittest.cc ash/sticky_keys/sticky_keys_overlay.cc ash/system/ime_menu/ime_menu_tray.cc ash/system/palette/palette_tray.cc ash/system/session/logout_confirmation_dialog.cc ash/system/toast/toast_overlay.cc ash/system/tray/system_tray.cc ash/system/tray/system_tray_unittest.cc ash/system/tray/tray_event_filter.cc ash/system/user/user_view.cc ash/system/web_notification/ash_popup_alignment_delegate.cc ash/system/web_notification/ash_popup_alignment_delegate_unittest.cc ash/system/web_notification/web_notification_tray.cc ash/system/web_notification/web_notification_tray_unittest.cc ash/test/ash_test.h ash/touch/touch_observer_hud.cc ash/touch_hud/mus/touch_hud_application.cc ash/utility/screenshot_controller.cc ash/wallpaper/wallpaper_controller.cc ash/wallpaper/wallpaper_controller_unittest.cc ash/wm/always_on_top_controller.cc ash/wm/always_on_top_controller_unittest.cc ash/wm/ash_focus_rules.cc ash/wm/ash_focus_rules_unittest.cc ash/wm/container_finder.cc ash/wm/container_finder_unittest.cc ash/wm/drag_window_controller.cc ash/wm/drag_window_resizer_unittest.cc ash/wm/event_client_impl.cc ash/wm/focus_rules.cc ash/wm/fullscreen_window_finder.cc ash/wm/lock_layout_manager_unittest.cc ash/wm/maximize_mode/maximize_mode_window_manager.cc ash/wm/maximize_mode/workspace_backdrop_delegate.cc ash/wm/overview/window_grid.cc ash/wm/overview/window_selector.cc ash/wm/panels/panel_layout_manager.cc ash/wm/panels/panel_layout_manager.h ash/wm/panels/panel_layout_manager_unittest.cc ash/wm/panels/panel_window_resizer.cc ash/wm/panels/panel_window_resizer_unittest.cc ash/wm/root_window_layout_manager_unittest.cc ash/wm/screen_dimmer.cc ash/wm/screen_pinning_controller.cc ash/wm/session_state_animator_impl.cc ash/wm/session_state_animator_impl_unittest.cc ash/wm/stacking_controller_unittest.cc ash/wm/switchable_windows.cc ash/wm/system_modal_container_layout_manager.cc ash/wm/system_modal_container_layout_manager_unittest.cc ash/wm/toplevel_window_event_handler_unittest.cc ash/wm/window_cycle_controller.cc ash/wm/window_cycle_controller_unittest.cc ash/wm/window_cycle_list.cc ash/wm/wm_snap_to_pixel_layout_manager.cc ash/wm/workspace/multi_window_resize_controller.cc ash/wm/workspace/phantom_window_controller.cc ash/wm/workspace/phantom_window_controller.h ash/wm/workspace/workspace_layout_manager.cc ash/wm/workspace/workspace_layout_manager_keyboard_unittest.cc ash/wm/workspace/workspace_layout_manager_unittest.cc ash/wm/workspace/workspace_window_resizer.cc ash/wm/workspace_controller.cc ash/wm/workspace_controller_unittest.cc ash/wm_window.cc chrome/browser/chromeos/accessibility/chromevox_panel.cc chrome/browser/chromeos/bluetooth/bluetooth_pairing_dialog.cc chrome/browser/chromeos/display/overscan_calibrator.cc chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc chrome/browser/chromeos/input_method/candidate_window_controller_impl.cc chrome/browser/chromeos/input_method/mode_indicator_controller.cc chrome/browser/chromeos/login/ui/lock_window.cc chrome/browser/chromeos/login/ui/login_display_host_impl.cc chrome/browser/chromeos/set_time_dialog.cc chrome/browser/chromeos/ui/choose_mobile_network_dialog.cc chrome/browser/chromeos/ui/idle_app_name_notification_view.cc chrome/browser/chromeos/ui/kiosk_external_update_notification.cc chrome/browser/ui/ash/app_list/app_list_interactive_uitest.cc chrome/browser/ui/ash/chrome_keyboard_ui.cc chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc chrome/browser/ui/ash/system_tray_client.cc reveman: components/exo/display.cc components/exo/display_unittest.cc components/exo/pointer.cc components/exo/pointer_unittest.cc components/exo/shell_surface.cc components/exo/shell_surface_unittest.cc components/exo/test/exo_test_helper.cc components/exo/wayland/server.cc
Is my list suppose to overlap with xiyuan's? You could have condensed parts of the list. e.g. "reveman: components/exo/*"
On 2017/04/14 00:06:11, Lei Zhang wrote: > Is my list suppose to overlap with xiyuan's? You could have condensed parts of > the list. e.g. "reveman: components/exo/*" Noted, thanks! I just copied the output of git cl owners.
On 2017/04/14 00:12:49, Lei Zhang wrote: > Umm, did you see > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/5AqwVQt-Etg/dis... > ? Umm, no, I didn't. Done! :D That actually makes the change much smaller! --->> I hope you won't ask me to change kFoo_Bar to kFooBar!
The CQ bit was checked by afakhry@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...
On 2017/04/14 01:25:02, afakhry wrote: > On 2017/04/14 00:12:49, Lei Zhang wrote: > > Umm, did you see > > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/5AqwVQt-Etg/dis... > > ? > > Umm, no, I didn't. Done! :D > That actually makes the change much smaller! --->> I hope you won't ask me to > change kFoo_Bar to kFooBar! I won't. Someone else may auto-correct all of them or something. chrome/ lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2751523003/diff/40001/ash/autoclick/mus/autoc... File ash/autoclick/mus/autoclick_application.cc (right): https://codereview.chromium.org/2751523003/diff/40001/ash/autoclick/mus/autoc... ash/autoclick/mus/autoclick_application.cc:117: static_cast<int32_t>(ash::kShellWindowId_OverlayContainer)); i think this should be size_t ? https://codereview.chromium.org/2751523003/diff/40001/ash/touch_hud/mus/touch... File ash/touch_hud/mus/touch_hud_application.cc (right): https://codereview.chromium.org/2751523003/diff/40001/ash/touch_hud/mus/touch... ash/touch_hud/mus/touch_hud_application.cc:94: static_cast<int32_t>(ash::kShellWindowId_OverlayContainer)); ditto
BUG=None (or file a bug)
lgtm
Description was changed from ========== Convert Window Container Ids to an Enum To make it less error-prone, and have the compiler assign values to the various IDs, it's better to convert them into an enum. BUG= ========== to ========== Convert Window Container Ids to an Enum To make it less error-prone, and have the compiler assign values to the various IDs, it's better to convert them into an enum. BUG=None ==========
https://codereview.chromium.org/2751523003/diff/40001/ash/autoclick/mus/autoc... File ash/autoclick/mus/autoclick_application.cc (right): https://codereview.chromium.org/2751523003/diff/40001/ash/autoclick/mus/autoc... ash/autoclick/mus/autoclick_application.cc:117: static_cast<int32_t>(ash::kShellWindowId_OverlayContainer)); On 2017/04/14 03:46:06, oshima wrote: > i think this should be size_t ? size_t does't work, as it has no template specialization, so it tries to use the container one: https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/type_converter.... However, there is a specialization for int32_t: https://cs.chromium.org/chromium/src/services/ui/public/cpp/property_type_con... https://codereview.chromium.org/2751523003/diff/40001/ash/touch_hud/mus/touch... File ash/touch_hud/mus/touch_hud_application.cc (right): https://codereview.chromium.org/2751523003/diff/40001/ash/touch_hud/mus/touch... ash/touch_hud/mus/touch_hud_application.cc:94: static_cast<int32_t>(ash::kShellWindowId_OverlayContainer)); On 2017/04/14 03:46:06, oshima wrote: > ditto Similarly.
The CQ bit was checked by afakhry@chromium.org
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 afakhry@chromium.org
The CQ bit was checked by afakhry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, xiyuan@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2751523003/#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": 1492218515295120, "parent_rev": "f4f7b3aad31bbce44c84dd2cdbd2400a90f87d51", "commit_rev": "386772d9ce57793ad8110f3cd976336d4ed181ee"}
Message was sent while issue was closed.
Description was changed from ========== Convert Window Container Ids to an Enum To make it less error-prone, and have the compiler assign values to the various IDs, it's better to convert them into an enum. BUG=None ========== to ========== Convert Window Container Ids to an Enum To make it less error-prone, and have the compiler assign values to the various IDs, it's better to convert them into an enum. BUG=None Review-Url: https://codereview.chromium.org/2751523003 Cr-Commit-Position: refs/heads/master@{#464845} Committed: https://chromium.googlesource.com/chromium/src/+/386772d9ce57793ad8110f3cd976... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/386772d9ce57793ad8110f3cd976... |