Various reviewers have suggested that enum IconType should have no "holes" in their values, i.e. use contiguous enum values. The reason why powers of two are currently used are, presumably, because it is occasionally used as bitmask (even for persisting) and also for consistency with analogous enums: 1. blink::IconType (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/IconURL.h?l=40&rcl=cc237116c6ec17cd79cfa69278ed0c16132b93b5) 2. On iOS, web::FaviconURL::IconType. Interestingly, content::FaviconURL::IconType is already compact and an enum class, cleaned up in https://codereview.chromium.org/2918903002
Some more occurrences I came across: Java's FaviconHelper: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/favicon/FaviconHelper.java?l=24&rcl=cc237116c6ec17cd79cfa69278ed0c16132b93b5 Blink::WebIconURL: https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/WebIconURL.h?l=43&rcl=cc237116c6ec17cd79cfa69278ed0c16132b93b5 I can't tell why Blink has this enum twice.
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e commit e17ad367a6342c89ea68bc0a2220a4f33b51ec7e Author: Mikel Astiz <mastiz@chromium.org> Date: Fri Nov 03 11:27:28 2017 Adopt IconTypeSet instead of int bitmasks No behavioral change: we use of a set of enums to make the APIs more type-safe and as a prior step to adopting an enum class. I've considered introducing a home-grown container for IconTypeSet (e.g. based on std::bitset) but decided against for code simplicity. Besides, as of today, base::small_set does not exist, so we're using flat_set. Bug: 778551 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I64986271937bea040265f5d52e664be175b0fa85 Reviewed-on: https://chromium-review.googlesource.com/737638 Commit-Queue: Mikel Astiz <mastiz@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Brett Wilson <brettw@chromium.org> Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org> Cr-Commit-Position: refs/heads/master@{#513743} [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/chrome/android/java/src/org/chromium/chrome/browser/favicon/FaviconHelper.java [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/chrome/browser/android/favicon_helper.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/chrome/browser/android/favicon_helper.h [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/chrome/browser/bookmarks/bookmark_html_writer.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/chrome/browser/favicon/content_favicon_driver_browsertest.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/chrome/browser/history/android/android_provider_backend_unittest.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/chrome/browser/ui/webui/extensions/extension_icon_source.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/chrome/browser/ui/webui/favicon_source.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/favicon/core/favicon_driver_impl.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/favicon/core/favicon_handler.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/favicon/core/favicon_handler.h [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/favicon/core/favicon_handler_unittest.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/favicon/core/favicon_service.h [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/favicon/core/favicon_service_impl.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/favicon/core/favicon_service_impl.h [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/favicon/core/favicon_util.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/favicon/core/large_icon_service.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/favicon/core/large_icon_service.h [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/favicon/core/large_icon_service_unittest.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/favicon/core/test/mock_favicon_service.h [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/favicon_base/favicon_types.h [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/history/core/browser/android/favicon_sql_handler.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/history/core/browser/expire_history_backend_unittest.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/history/core/browser/history_backend.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/history/core/browser/history_backend.h [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/history/core/browser/history_backend_unittest.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/history/core/browser/history_service.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/history/core/browser/history_service.h [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/history/core/browser/thumbnail_database.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/history/core/browser/thumbnail_database.h [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/history/core/browser/thumbnail_database_unittest.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/components/sync_sessions/favicon_cache.cc [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/ios/chrome/browser/favicon/favicon_loader.h [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/ios/chrome/browser/favicon/favicon_loader.mm [modify] https://crrev.com/e17ad367a6342c89ea68bc0a2220a4f33b51ec7e/ios/chrome/browser/ui/tab_switcher/tab_switcher_utils.mm
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b commit a5ea255f5a86b495ed3caa6aa9590d0d03868d5b Author: Mikel Astiz <mastiz@chromium.org> Date: Mon Nov 06 22:50:15 2017 Adopt enum class for favicon_base::IconType Semi-automated patch composed almost entirely of renames, with the goal to provide stronger type safety via enum class and most notably prevent the enum from being used as bitmask (IconTypeSet should be used for this). The only non-trivial changes are in thumbnail_database.* where the enum needs to be casted to/from int. We introduce helper functions and the corresponding unit tests in preparation for future patches that will modify the enum values. Bug: 778551 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: Ide9a110a4d33ac5e94c6d113219631f24ef1ee1a Reviewed-on: https://chromium-review.googlesource.com/753361 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Brett Wilson <brettw@chromium.org> Commit-Queue: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#514270} [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/android/java/src/org/chromium/chrome/browser/favicon/FaviconHelper.java [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileRenderer.java [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageLoadTest.java [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NtpUiCaptureTestData.java [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupUnitTest.java [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/android/bookmarks/partner_bookmarks_reader.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/android/favicon_helper.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/android/large_icon_bridge.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/android/ntp/most_visited_sites_bridge.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/bookmarks/bookmark_html_writer.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/bookmarks/bookmark_html_writer_unittest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/extensions/extension_web_ui.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/favicon/content_favicon_driver_browsertest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/history/android/android_provider_backend.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/history/android/android_provider_backend_unittest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/sync/test/integration/bookmarks_helper.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/ui/search/ntp_user_data_logger_unittest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/ui/search/search_ipc_router_unittest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/ui/webui/extensions/extension_icon_source.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/browser/ui/webui/favicon_source.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/common/instant_struct_traits.h [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/chrome/renderer/searchbox/searchbox_extension.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/bookmarks/browser/bookmark_client.h [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/bookmarks/browser/bookmark_model.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/bookmarks/browser/bookmark_model.h [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/bookmarks/browser/bookmark_model_unittest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/bookmarks/browser/bookmark_node.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon/content/content_favicon_driver.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon/content/favicon_url_util.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon/core/favicon_driver_impl.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon/core/favicon_driver_observer.h [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon/core/favicon_handler.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon/core/favicon_handler.h [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon/core/favicon_handler_unittest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon/core/favicon_service.h [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon/core/favicon_service_impl.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon/core/favicon_url.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon/core/favicon_util.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon/core/favicon_util.h [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon/core/large_icon_service.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon/core/large_icon_service_unittest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon/ios/favicon_url_util.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon_base/favicon_types.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/favicon_base/favicon_types.h [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/history/core/browser/android/favicon_sql_handler.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/history/core/browser/expire_history_backend_unittest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/history/core/browser/history_backend.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/history/core/browser/history_backend.h [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/history/core/browser/history_backend_unittest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/history/core/browser/history_types.h [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/history/core/browser/thumbnail_database.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/history/core/browser/thumbnail_database.h [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/history/core/browser/thumbnail_database_unittest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/ntp_tiles/icon_cacher_impl.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/ntp_tiles/icon_cacher_impl_unittest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/ntp_tiles/metrics.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/ntp_tiles/metrics_unittest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/ntp_tiles/ntp_tile_impression.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/sync_bookmarks/bookmark_change_processor.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/components/sync_sessions/favicon_cache.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/ios/chrome/app/spotlight/spotlight_manager_unittest.mm [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/ios/chrome/browser/favicon/favicon_client_impl.mm [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/ios/chrome/browser/favicon/large_icon_cache_unittest.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/ios/chrome/browser/ui/history/favicon_view_provider_unittest.mm [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/ios/chrome/browser/ui/ntp/metrics.mm [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/ios/chrome/browser/ui/tab_switcher/tab_switcher_utils.mm [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/ios/web/public/favicon_url.cc [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/ios/web/public/favicon_url.h [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/ios/web/web_state/favicon_callbacks_inttest.mm [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/ios/web/web_state/ui/crw_web_controller.mm [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/ios/web/web_state/web_state_impl_unittest.mm [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/ios/web/web_state/web_state_observer_bridge_unittest.mm [modify] https://crrev.com/a5ea255f5a86b495ed3caa6aa9590d0d03868d5b/tools/metrics/histograms/enums.xml
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/26855edf326d83aaa387eeaa3570195e8396e941 commit 26855edf326d83aaa387eeaa3570195e8396e941 Author: Mikel Astiz <mastiz@chromium.org> Date: Wed Nov 15 21:59:49 2017 Use contiguous enum values for favicon_base::IconType This simplifies the integration with UMA and improves Mojo verification, which cannot otherwise detect invalid enum values. It should also help prevent Undefined Behavior cases in the future, with C++17. We update ThumbnailDatabase for backward compatibility because the values get persisted. There should be no other persistence of these enums since we have recently adopted enum classes to surface such cases. Bug: 778551 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I69fe20d64f30701a00cd798338ab74147b356b7f Reviewed-on: https://chromium-review.googlesource.com/756737 Reviewed-by: Brett Wilson <brettw@chromium.org> Commit-Queue: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#516844} [modify] https://crrev.com/26855edf326d83aaa387eeaa3570195e8396e941/components/favicon_base/favicon_types.cc [modify] https://crrev.com/26855edf326d83aaa387eeaa3570195e8396e941/components/favicon_base/favicon_types.h [modify] https://crrev.com/26855edf326d83aaa387eeaa3570195e8396e941/components/history/core/browser/thumbnail_database.cc [modify] https://crrev.com/26855edf326d83aaa387eeaa3570195e8396e941/components/history/core/browser/thumbnail_database_unittest.cc [modify] https://crrev.com/26855edf326d83aaa387eeaa3570195e8396e941/components/ntp_tiles/metrics.cc [modify] https://crrev.com/26855edf326d83aaa387eeaa3570195e8396e941/ios/web/public/favicon_url.h
Comment 1 by mastiz@chromium.org
, Oct 26 2017