Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2279713002: Make PassphraseType a "enum class" instead of "enum". (Closed)

Created:
1 year, 2 months ago by melandory
Modified:
1 year, 2 months ago
CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, sync-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make PassphraseType a "enum class" instead of "enum". Make PassphraseType a "enum class" instead of "enum" which enable forward declaration. BUG=638963 Committed: https://crrev.com/5c441aa82555fcc384e3e31fb90bdfb6888d4a0c Cr-Commit-Position: refs/heads/master@{#415202}

Patch Set 1 #

Patch Set 2 : fix changes in comments #

Patch Set 3 : fix java #

Unified diffs Side-by-side diffs Delta from patch set Stats (+314 lines, -229 lines) Patch
M chrome/browser/sync/profile_sync_service_android.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/options/sync_setup_handler.cc View 1 3 chunks +6 lines, -5 lines 0 comments Download
M chrome/browser/ui/webui/options/sync_setup_handler_unittest.cc View 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/settings/people_handler.cc View 3 chunks +8 lines, -6 lines 0 comments Download
M chrome/browser/ui/webui/settings/people_handler_unittest.cc View 3 chunks +5 lines, -4 lines 0 comments Download
M components/browser_sync/browser/profile_sync_service.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M components/sync/core/sync_encryption_handler.h View 1 1 chunk +1 line, -1 line 0 comments Download
M components/sync/core_impl/js_sync_encryption_handler_observer_unittest.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M components/sync/core_impl/sync_encryption_handler_impl.cc View 1 21 chunks +45 lines, -39 lines 0 comments Download
M components/sync/core_impl/sync_encryption_handler_impl_unittest.cc View 1 70 chunks +220 lines, -148 lines 0 comments Download
M components/sync/core_impl/sync_manager_impl_unittest.cc View 4 chunks +6 lines, -6 lines 0 comments Download
M components/sync/driver/glue/sync_backend_host_impl.cc View 2 chunks +4 lines, -3 lines 0 comments Download
M components/sync/driver/glue/sync_backend_host_mock.cc View 1 chunk +1 line, -1 line 0 comments Download
M components/sync/engine/sync_status.cc View 1 chunk +1 line, -1 line 0 comments Download
M components/sync/engine/sync_string_conversions.cc View 1 chunk +4 lines, -4 lines 0 comments Download
M components/sync/test/fake_sync_encryption_handler.cc View 3 chunks +3 lines, -3 lines 0 comments Download
Trybot results:  chromium_presubmit   chromium_presubmit   linux_android_rel_ng   mac_chromium_compile_dbg_ng   ios-device   ios-simulator   linux_chromium_chromeos_compile_dbg_ng   linux_chromium_asan_rel_ng   mac_chromium_rel_ng   chromeos_x86-generic_chromium_compile_only_ng   linux_chromium_rel_ng   chromium_presubmit   linux_chromium_compile_dbg_ng   chromeos_amd64-generic_chromium_compile_only_ng   chromeos_daisy_chromium_compile_only_ng   cast_shell_linux   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_clobber_rel_ng   win_clang   win_chromium_compile_dbg_ng   win_chromium_x64_rel_ng   win_chromium_rel_ng   linux_android_rel_ng   android_clang_dbg_recipe   cast_shell_android   android_compile_dbg   android_arm64_dbg_recipe   win_chromium_rel_ng   win_chromium_x64_rel_ng   linux_chromium_rel_ng   linux_chromium_chromeos_compile_dbg_ng   linux_chromium_asan_rel_ng   chromium_presubmit   chromeos_x86-generic_chromium_compile_only_ng   linux_chromium_compile_dbg_ng   chromeos_amd64-generic_chromium_compile_only_ng   chromeos_daisy_chromium_compile_only_ng   cast_shell_linux   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_clobber_rel_ng   win_clang   win_chromium_compile_dbg_ng   win_chromium_x64_rel_ng   win_chromium_rel_ng   mac_chromium_compile_dbg_ng   ios-device   ios-simulator   mac_chromium_rel_ng   linux_android_rel_ng   android_clang_dbg_recipe   cast_shell_android   android_arm64_dbg_recipe   android_compile_dbg   win_clang   win_chromium_compile_dbg_ng   win_chromium_x64_rel_ng   win_chromium_rel_ng   mac_chromium_compile_dbg_ng   ios-simulator   ios-device   mac_chromium_rel_ng   linux_android_rel_ng   android_clang_dbg_recipe   android_arm64_dbg_recipe   cast_shell_android   linux_chromium_chromeos_compile_dbg_ng   android_compile_dbg   linux_chromium_asan_rel_ng   linux_chromium_rel_ng   chromeos_x86-generic_chromium_compile_only_ng   chromium_presubmit   linux_chromium_compile_dbg_ng   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   chromeos_daisy_chromium_compile_only_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_clobber_rel_ng 

Dependent Patchsets:

Messages

Total messages: 37 (22 generated)
melandory
Non-tivials bit in this CL are: * chrome/browser/sync/profile_sync_service_android.cc:293 * components/sync/core_impl/sync_encryption_handler_impl.cc:254
1 year, 2 months ago (2016-08-26 01:05:10 UTC) #13
melandory
PTAL. Non-tivials bit in this CL are: * chrome/browser/sync/profile_sync_service_android.cc:293 * components/sync/core_impl/sync_encryption_handler_impl.cc:254 This CL is part ...
1 year, 2 months ago (2016-08-26 16:01:10 UTC) #15
Nicolas Zea
lgtm
1 year, 2 months ago (2016-08-26 17:50:27 UTC) #16
melandory
stevenjb@chromium.org: Please review changes in chrome/browser/ui/webui/*
1 year, 2 months ago (2016-08-26 18:25:53 UTC) #18
stevenjb
RS lgtm for c/b/ui/webui
1 year, 2 months ago (2016-08-26 20:47:46 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2279713002/40001
1 year, 2 months ago (2016-08-26 21:22:09 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/282454)
1 year, 2 months ago (2016-08-26 23:30:48 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2279713002/40001
1 year, 2 months ago (2016-08-29 16:25:55 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2279713002/40001
1 year, 2 months ago (2016-08-30 03:45:46 UTC) #28
dsansome
(I unset the commit bit on this change to try fix a stuck CQ - ...
1 year, 2 months ago (2016-08-30 03:54:49 UTC) #30
commit-bot: I haz the power
Committed patchset #3 (id:40001)
1 year, 2 months ago (2016-08-30 06:09:27 UTC) #31
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/5c441aa82555fcc384e3e31fb90bdfb6888d4a0c Cr-Commit-Position: refs/heads/master@{#415202}
1 year, 2 months ago (2016-08-30 06:12:52 UTC) #33
maxbogue
I'm late to the party on this, but could someone explain to me why we ...
1 year, 2 months ago (2016-08-30 16:39:16 UTC) #35
melandory
On 2016/08/30 16:39:16, maxbogue wrote: > I'm late to the party on this, but could ...
1 year, 2 months ago (2016-08-30 17:12:44 UTC) #36
maxbogue
1 year, 2 months ago (2016-08-30 19:57:43 UTC) #37
Message was sent while issue was closed.
On 2016/08/30 17:12:44, melandory wrote:
> On 2016/08/30 16:39:16, maxbogue wrote:
> > I'm late to the party on this, but could someone explain to me why we want
to
> be
> > able to forward declare PassphraseType?
> There are followup CL, which change how PassphraseType is used and I thought
it
> could be beneficial to move it so separate header.
> Some examples:
> 
> https://codereview.chromium.org/2279753003/
> https://codereview.chromium.org/2278043003/
> 
> If you fill strong about this CL, please tell, I'll postpone landing CLs which
> are based on this one and can revert this if we'll come conclusion that
ability
> to forward declare isn't really needed.

I'm definitely on board with moving it to a separate header; I was just unsure
why the ability to forward declare it was desirable. Move forward with your
changes and we can always change it back later if we want to.

Powered by Google App Engine
This is Rietveld efc10ee0f