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

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

Created:
4 years, 4 months ago by melandory
Modified:
4 years, 3 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

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
4 years, 3 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 ...
4 years, 3 months ago (2016-08-26 16:01:10 UTC) #15
Nicolas Zea
lgtm
4 years, 3 months ago (2016-08-26 17:50:27 UTC) #16
melandory
stevenjb@chromium.org: Please review changes in chrome/browser/ui/webui/*
4 years, 3 months ago (2016-08-26 18:25:53 UTC) #18
stevenjb
RS lgtm for c/b/ui/webui
4 years, 3 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
4 years, 3 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)
4 years, 3 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
4 years, 3 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
4 years, 3 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 - ...
4 years, 3 months ago (2016-08-30 03:54:49 UTC) #30
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 3 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}
4 years, 3 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 ...
4 years, 3 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 ...
4 years, 3 months ago (2016-08-30 17:12:44 UTC) #36
maxbogue
4 years, 3 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 408576698