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

Issue 2825943002: Removed translate MetricsNameIndex. (Closed)

Created:
3 years, 8 months ago by martis
Modified:
3 years, 8 months ago
Reviewers:
droger
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Removed translate MetricsNameIndex. The enum is unnecessary and is only used in tests. I've introduced an internal namespace, but I'm happy to change if there's a more idiomatic approach. BUG=712922 Review-Url: https://codereview.chromium.org/2825943002 Cr-Commit-Position: refs/heads/master@{#467276} Committed: https://chromium.googlesource.com/chromium/src/+/40c92451bdce5b9b9fae80f7bdd87ffc9b74e16e

Patch Set 1 #

Patch Set 2 : Removed duplicate comment #

Total comments: 7

Patch Set 3 : Changed constant declaration style. #

Patch Set 4 : Fixed string declarations. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -93 lines) Patch
M components/translate/core/common/translate_metrics.h View 1 2 3 2 chunks +18 lines, -19 lines 0 comments Download
M components/translate/core/common/translate_metrics.cc View 1 2 3 5 chunks +20 lines, -46 lines 0 comments Download
M components/translate/core/common/translate_metrics_unittest.cc View 13 chunks +33 lines, -28 lines 0 comments Download

Messages

Total messages: 13 (5 generated)
martis
3 years, 8 months ago (2017-04-19 03:41:43 UTC) #2
droger
Thanks! LGTM once the non-optional comments are addressed. https://codereview.chromium.org/2825943002/diff/20001/components/translate/core/common/translate_metrics.cc File components/translate/core/common/translate_metrics.cc (right): https://codereview.chromium.org/2825943002/diff/20001/components/translate/core/common/translate_metrics.cc#newcode20 components/translate/core/common/translate_metrics.cc:20: const ...
3 years, 8 months ago (2017-04-19 08:26:50 UTC) #3
martis
Thanks for the review, I think I've addressed the non-optional comments. https://codereview.chromium.org/2825943002/diff/20001/components/translate/core/common/translate_metrics.cc File components/translate/core/common/translate_metrics.cc (right): ...
3 years, 8 months ago (2017-04-20 02:57:29 UTC) #4
droger
https://codereview.chromium.org/2825943002/diff/20001/components/translate/core/common/translate_metrics.h File components/translate/core/common/translate_metrics.h (right): https://codereview.chromium.org/2825943002/diff/20001/components/translate/core/common/translate_metrics.h#newcode19 components/translate/core/common/translate_metrics.h:19: extern const char* kRenderer4LanguageDetection; On 2017/04/20 02:57:29, martis wrote: ...
3 years, 8 months ago (2017-04-20 07:30:30 UTC) #5
martis
On 2017/04/20 07:30:30, droger wrote: > https://codereview.chromium.org/2825943002/diff/20001/components/translate/core/common/translate_metrics.h > File components/translate/core/common/translate_metrics.h (right): > > https://codereview.chromium.org/2825943002/diff/20001/components/translate/core/common/translate_metrics.h#newcode19 > ...
3 years, 8 months ago (2017-04-26 08:15:08 UTC) #6
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/2825943002/60001
3 years, 8 months ago (2017-04-26 08:16:53 UTC) #9
droger
Thanks for fixing this bug, that's really appreciated!
3 years, 8 months ago (2017-04-26 08:40:33 UTC) #10
commit-bot: I haz the power
3 years, 8 months ago (2017-04-26 09:06:56 UTC) #13
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/40c92451bdce5b9b9fae80f7bdd8...

Powered by Google App Engine
This is Rietveld 408576698