Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(106)

Issue 1739183003: Make extensions::DictionaryBuilder and extensions::ListValue unmovable. (Closed)

Created:
4 years, 10 months ago by dcheng
Modified:
4 years, 10 months ago
CC:
Aaron Boodman, abarth-chromium, asvitkine+watch_chromium.org, ben+mojo_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, davemoore+watch_chromium.org, derat+watch_chromium.org, extensions-reviews_chromium.org, felt, grt+watch_chromium.org, jam, kalyank, kinuko+fileapi, limasdf, media-router+watch_chromium.org, mlamouri+watch-notifications_chromium.org, nhiroki, oshima+watch_chromium.org, Peter Beverloo, qsr+mojo_chromium.org, rginda+watch_chromium.org, sadrul, James Su, tfarina, tzik, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make extensions::DictionaryBuilder and extensions::ListValue unmovable. There's no reason for these classes to be movable. std::move() is just being used as a synonym for Build(). In addition: - Build() is fewer characters than std::move(). - clang-format works better when builder syntax is consistently used, which makes it easier for readers to visually match up deeply nested builders. - It's surprising to see std::move() used with what looks like a temporary. BUG=none Committed: https://crrev.com/794d2bd77811b6d6b45048c19c287075cc9930db Cr-Commit-Position: refs/heads/master@{#378107}

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+1517 lines, -1286 lines) Patch
M chrome/browser/autocomplete/shortcuts_provider_extension_unittest.cc View 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/file_manager/file_tasks_unittest.cc View 6 chunks +212 lines, -166 lines 0 comments Download
M chrome/browser/chromeos/power/extension_event_observer_unittest.cc View 1 chunk +14 lines, -11 lines 0 comments Download
M chrome/browser/chromeos/power/renderer_freezer_unittest.cc View 2 chunks +21 lines, -15 lines 0 comments Download
M chrome/browser/chromeos/printer_detector/printer_detector_unittest.cc View 8 chunks +89 lines, -60 lines 0 comments Download
M chrome/browser/extensions/active_script_controller_unittest.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/extensions/active_tab_unittest.cc View 1 chunk +6 lines, -5 lines 0 comments Download
M chrome/browser/extensions/activity_log/activity_log_enabled_unittest.cc View 3 chunks +15 lines, -12 lines 0 comments Download
M chrome/browser/extensions/activity_log/activity_log_policy_unittest.cc View 1 chunk +2 lines, -3 lines 2 comments Download
M chrome/browser/extensions/activity_log/activity_log_unittest.cc View 3 chunks +15 lines, -12 lines 0 comments Download
M chrome/browser/extensions/activity_log/counting_policy_unittest.cc View 3 chunks +15 lines, -12 lines 0 comments Download
M chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc View 5 chunks +25 lines, -20 lines 0 comments Download
M chrome/browser/extensions/api/debugger/debugger_apitest.cc View 1 chunk +10 lines, -9 lines 0 comments Download
M chrome/browser/extensions/api/declarative_content/content_action_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/api/declarative_content/declarative_content_is_bookmarked_condition_tracker_unittest.cc View 1 chunk +6 lines, -5 lines 0 comments Download
M chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc View 4 chunks +14 lines, -12 lines 0 comments Download
M chrome/browser/extensions/api/developer_private/extension_info_generator_unittest.cc View 4 chunks +12 lines, -12 lines 0 comments Download
M chrome/browser/extensions/api/file_system/file_system_api_unittest.cc View 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/extensions/api/identity/identity_apitest.cc View 1 chunk +6 lines, -6 lines 0 comments Download
M chrome/browser/extensions/api/omnibox/omnibox_unittest.cc View 6 chunks +171 lines, -139 lines 0 comments Download
M chrome/browser/extensions/api/storage/settings_apitest.cc View 1 chunk +17 lines, -15 lines 0 comments Download
M chrome/browser/extensions/api/streams_private/streams_private_manifest_unittest.cc View 2 chunks +6 lines, -6 lines 0 comments Download
M chrome/browser/extensions/api/tabs/tabs_api_unittest.cc View 2 chunks +10 lines, -8 lines 0 comments Download
M chrome/browser/extensions/api/web_request/web_request_apitest.cc View 1 chunk +8 lines, -5 lines 0 comments Download
M chrome/browser/extensions/app_data_migrator_unittest.cc View 1 chunk +33 lines, -28 lines 0 comments Download
M chrome/browser/extensions/extension_action_manager_unittest.cc View 8 chunks +39 lines, -33 lines 0 comments Download
M chrome/browser/extensions/extension_action_test_util.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/extensions/extension_context_menu_model_unittest.cc View 2 chunks +4 lines, -5 lines 0 comments Download
M chrome/browser/extensions/extension_install_prompt_browsertest.cc View 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/browser/extensions/extension_install_prompt_unittest.cc View 3 chunks +27 lines, -25 lines 0 comments Download
M chrome/browser/extensions/extension_message_bubble_controller_unittest.cc View 6 chunks +32 lines, -25 lines 0 comments Download
M chrome/browser/extensions/extension_messages_apitest.cc View 2 chunks +10 lines, -8 lines 0 comments Download
M chrome/browser/extensions/extension_migrator_unittest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/extensions/extension_prefs_unittest.cc View 2 chunks +8 lines, -6 lines 0 comments Download
M chrome/browser/extensions/extension_reenabler_unittest.cc View 1 chunk +6 lines, -5 lines 0 comments Download
M chrome/browser/extensions/extension_service_sync_unittest.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/extensions/extension_web_ui_unittest.cc View 2 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/extensions/location_bar_controller_unittest.cc View 1 chunk +4 lines, -4 lines 0 comments Download
M chrome/browser/extensions/options_page_apitest.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/permission_messages_unittest.cc View 5 chunks +12 lines, -24 lines 0 comments Download
M chrome/browser/extensions/permissions_updater_unittest.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/extensions/process_manager_browsertest.cc View 1 chunk +5 lines, -4 lines 0 comments Download
M chrome/browser/extensions/scripting_permissions_modifier_unittest.cc View 1 chunk +11 lines, -11 lines 0 comments Download
M chrome/browser/extensions/shared_module_service_unittest.cc View 5 chunks +18 lines, -13 lines 0 comments Download
M chrome/browser/extensions/test_extension_environment.cc View 1 chunk +8 lines, -5 lines 0 comments Download
M chrome/browser/extensions/webstore_installer_browsertest.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/extensions/webstore_startup_installer_browsertest.cc View 1 chunk +12 lines, -10 lines 0 comments Download
M chrome/browser/metrics/extensions_metrics_provider_unittest.cc View 1 chunk +18 lines, -16 lines 0 comments Download
M chrome/browser/notifications/message_center_settings_controller_unittest.cc View 1 chunk +47 lines, -34 lines 0 comments Download
M chrome/browser/notifications/platform_notification_service_unittest.cc View 3 chunks +21 lines, -18 lines 0 comments Download
M chrome/browser/policy/managed_bookmarks_policy_handler_unittest.cc View 3 chunks +45 lines, -43 lines 0 comments Download
M chrome/browser/safe_browsing/incident_reporting/extension_data_collection_unittest.cc View 1 chunk +7 lines, -6 lines 0 comments Download
M chrome/browser/site_details_browsertest.cc View 2 chunks +15 lines, -13 lines 0 comments Download
M chrome/browser/sync_file_system/drive_backend/sync_worker_unittest.cc View 1 chunk +5 lines, -4 lines 0 comments Download
M chrome/browser/ui/ash/keyboard_controller_browsertest.cc View 1 chunk +5 lines, -4 lines 0 comments Download
M chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller_unittest.mm View 2 chunks +17 lines, -13 lines 0 comments Download
M chrome/browser/ui/location_bar/location_bar_browsertest.cc View 1 chunk +6 lines, -5 lines 0 comments Download
M chrome/browser/ui/toolbar/browser_actions_bar_browsertest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel_unittest.cc View 4 chunks +46 lines, -34 lines 0 comments Download
M chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc View 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/policy_ui_browsertest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/print_preview/extension_printer_handler.cc View 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/uber/uber_ui_browsertest.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/common/extensions/api/common_extension_api_unittest.cc View 2 chunks +20 lines, -13 lines 0 comments Download
M chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc View 4 chunks +27 lines, -19 lines 0 comments Download
M chrome/common/extensions/api/file_browser_handlers/file_browser_handler_manifest_unittest.cc View 5 chunks +76 lines, -63 lines 0 comments Download
M chrome/common/extensions/features/chrome_channel_feature_filter_unittest.cc View 3 chunks +16 lines, -15 lines 0 comments Download
M chrome/common/extensions/permissions/permission_set_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/extensions/permissions/permissions_data_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/pepper_permission_util_unittest.cc View 2 chunks +11 lines, -9 lines 0 comments Download
M chrome/renderer/extensions/renderer_permissions_policy_delegate_unittest.cc View 1 chunk +6 lines, -6 lines 0 comments Download
M extensions/browser/api/bluetooth/bluetooth_event_router_unittest.cc View 1 chunk +5 lines, -4 lines 0 comments Download
M extensions/browser/api/device_permissions_prompt_unittest.cc View 2 chunks +10 lines, -8 lines 0 comments Download
M extensions/browser/api_unittest.cc View 1 chunk +7 lines, -6 lines 0 comments Download
M extensions/browser/app_window/app_window_geometry_cache_unittest.cc View 2 chunks +6 lines, -4 lines 0 comments Download
M extensions/browser/lazy_background_task_queue_unittest.cc View 2 chunks +11 lines, -9 lines 0 comments Download
M extensions/browser/mojo/keep_alive_impl_unittest.cc View 2 chunks +22 lines, -13 lines 0 comments Download
M extensions/browser/runtime_data_unittest.cc View 1 chunk +6 lines, -6 lines 0 comments Download
M extensions/browser/updater/update_service_unittest.cc View 1 chunk +5 lines, -4 lines 0 comments Download
M extensions/browser/value_store/value_store_change_unittest.cc View 2 chunks +9 lines, -7 lines 0 comments Download
M extensions/common/extension_builder.h View 1 chunk +3 lines, -6 lines 0 comments Download
M extensions/common/extension_builder.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M extensions/common/features/base_feature_provider_unittest.cc View 2 chunks +17 lines, -12 lines 0 comments Download
M extensions/common/features/complex_feature_unittest.cc View 4 chunks +7 lines, -7 lines 0 comments Download
M extensions/common/manifest_handler_unittest.cc View 2 chunks +20 lines, -17 lines 0 comments Download
M extensions/common/test_util.cc View 1 chunk +19 lines, -15 lines 0 comments Download
M extensions/common/value_builder.h View 5 chunks +2 lines, -19 lines 0 comments Download
M extensions/common/value_builder.cc View 4 chunks +2 lines, -35 lines 0 comments Download
M extensions/common/value_builder_unittest.cc View 1 chunk +2 lines, -3 lines 0 comments Download
M extensions/shell/browser/api/identity/identity_api_unittest.cc View 1 chunk +11 lines, -8 lines 0 comments Download
M extensions/shell/browser/shell_native_app_window_aura_unittest.cc View 1 chunk +5 lines, -4 lines 0 comments Download

Messages

Total messages: 14 (5 generated)
dcheng
I noticed this while working on some other code today and though it looked a ...
4 years, 10 months ago (2016-02-27 01:36:32 UTC) #2
Ken Rockot(use gerrit already)
I agree! Lgtm, and thanks!
4 years, 10 months ago (2016-02-27 02:19:28 UTC) #3
dcheng
+thakis for //chrome OWNERS
4 years, 10 months ago (2016-02-27 02:34:28 UTC) #5
Nico
lgtm https://codereview.chromium.org/1739183003/diff/1/chrome/browser/extensions/activity_log/activity_log_policy_unittest.cc File chrome/browser/extensions/activity_log/activity_log_policy_unittest.cc (left): https://codereview.chromium.org/1739183003/diff/1/chrome/browser/extensions/activity_log/activity_log_policy_unittest.cc#oldcode54 chrome/browser/extensions/activity_log/activity_log_policy_unittest.cc:54: .release()); did you drop the release() intentionally?
4 years, 10 months ago (2016-02-27 03:33:00 UTC) #6
dcheng
https://codereview.chromium.org/1739183003/diff/1/chrome/browser/extensions/activity_log/activity_log_policy_unittest.cc File chrome/browser/extensions/activity_log/activity_log_policy_unittest.cc (left): https://codereview.chromium.org/1739183003/diff/1/chrome/browser/extensions/activity_log/activity_log_policy_unittest.cc#oldcode54 chrome/browser/extensions/activity_log/activity_log_policy_unittest.cc:54: .release()); On 2016/02/27 at 03:33:00, Nico wrote: > did ...
4 years, 10 months ago (2016-02-27 03:39:30 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739183003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739183003/1
4 years, 10 months ago (2016-02-27 03:39:57 UTC) #9
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 10 months ago (2016-02-27 03:51:42 UTC) #10
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/794d2bd77811b6d6b45048c19c287075cc9930db Cr-Commit-Position: refs/heads/master@{#378107}
4 years, 10 months ago (2016-02-27 03:53:11 UTC) #12
limasdf
4 years, 10 months ago (2016-02-27 07:15:36 UTC) #14
Message was sent while issue was closed.
This looks much better!
Thank you for doing this dcheng!

Powered by Google App Engine
This is Rietveld 408576698