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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 months ago by melandory
Modified:
10 months, 3 weeks 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 
Commit queue not available (can’t edit this change).

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
11 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 ...
10 months, 4 weeks ago (2016-08-26 16:01:10 UTC) #15
Nicolas Zea
lgtm
10 months, 4 weeks ago (2016-08-26 17:50:27 UTC) #16
melandory
stevenjb@chromium.org: Please review changes in chrome/browser/ui/webui/*
10 months, 4 weeks ago (2016-08-26 18:25:53 UTC) #18
stevenjb
RS lgtm for c/b/ui/webui
10 months, 4 weeks 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
10 months, 4 weeks 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)
10 months, 4 weeks 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
10 months, 3 weeks 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
10 months, 3 weeks ago (2016-08-30 03:45:46 UTC) #28
dsansome
(I unset the commit bit on this change to try fix a stuck CQ - ...
10 months, 3 weeks ago (2016-08-30 03:54:49 UTC) #30
commit-bot: I haz the power
Committed patchset #3 (id:40001)
10 months, 3 weeks 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}
10 months, 3 weeks 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 ...
10 months, 3 weeks 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 ...
10 months, 3 weeks ago (2016-08-30 17:12:44 UTC) #36
maxbogue
10 months, 3 weeks 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973