Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2385913002:
Define canonical enum for "HeliumResult" status values. (Closed)
|
Created:
4 years, 2 months ago by Kevin M Modified:
4 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, bgoldman+watch-blimp_chromium.org, steimel+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, scf+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDefine canonical enum for "HeliumResult" status values.
Add HeliumResult-to-string mapping for easier logging.
Add unit tests for string conversion functions.
R=lethalantidote@chromium.org,gcasto@chromium.org
BUG=
Committed: https://crrev.com/1ce8eb06b7f7ffc4adae084273aea6c1e9d7aef5
Cr-Commit-Position: refs/heads/master@{#422683}
Patch Set 1 #
Total comments: 10
Patch Set 2 : feedbacks #Patch Set 3 : the android compiler is dumb #
Total comments: 3
Messages
Total messages: 28 (13 generated)
Description was changed from ========== Define canonical enum for "HeliumResult" status values. Add HeliumResult-to-string mapping for easier logging. Add unit tests for string conversion functions. R=lethalantidote@chromium.org,gcasto@chromium.org BUG= ========== to ========== Define canonical enum for "HeliumResult" status values. Add HeliumResult-to-string mapping for easier logging. Add unit tests for string conversion functions. R=lethalantidote@chromium.org,gcasto@chromium.org BUG= ==========
kmarshall@chromium.org changed reviewers: + wez@chromium.org
+wez
The CQ bit was checked by scf@chromium.org
The CQ bit was unchecked by scf@chromium.org
lgtm https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_err... File blimp/net/helium/helium_errors.h (right): https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_err... blimp/net/helium/helium_errors.h:15: HELIUM_ERROR(CONNECTION_LOST, 2) move the comment into a string? and use this for the |ToString|? i.e. HELIUM_ERROR(FAILED, 1, "Generic failure occured") https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_res... File blimp/net/helium/helium_result.h (right): https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_res... blimp/net/helium/helium_result.h:21: #include "blimp/net/helium/helium_errors.h" this is really neat
lgtm
lgtm https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_err... File blimp/net/helium/helium_errors.h (right): https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_err... blimp/net/helium/helium_errors.h:11: HELIUM_ERROR(FAILED, 1) Nit: OK, FAILED, and CONNECTION_LOST were spelled SUCCESS, INTERNAL_ERROR, and DISCONNECTED in the doc. I don't want to hold this up nit-picking the names (I haven't thought about them enough to know for certain which I think are better) but we might want to spend 15 minutes talking this out at some point. Getting good names is useful as they can be quite sticky. https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_res... File blimp/net/helium/helium_result.h (right): https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_res... blimp/net/helium/helium_result.h:19: OK, Any particular reason why OK is special cased?
lgtm https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_res... File blimp/net/helium/helium_result.cc (right): https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_res... blimp/net/helium/helium_result.cc:18: #include "blimp/net/helium/helium_errors.h" Just for my own knowledge, why does the include come last in this case?
https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_err... File blimp/net/helium/helium_errors.h (right): https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_err... blimp/net/helium/helium_errors.h:11: HELIUM_ERROR(FAILED, 1) On 2016/10/03 18:40:47, Garrett Casto wrote: > Nit: OK, FAILED, and CONNECTION_LOST were spelled SUCCESS, INTERNAL_ERROR, and > DISCONNECTED in the doc. I don't want to hold this up nit-picking the names (I > haven't thought about them enough to know for certain which I think are better) > but we might want to spend 15 minutes talking this out at some point. Getting > good names is useful as they can be quite sticky. OK, I'll add those. This CL is primarily about establishing a container for where to put the codes, and logic for stringifying the codes. Discussion about what might go in our error space could be a good teamwide topic. https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_err... blimp/net/helium/helium_errors.h:15: HELIUM_ERROR(CONNECTION_LOST, 2) On 2016/10/03 17:57:44, scf wrote: > move the comment into a string? and use this for the |ToString|? > > i.e. > HELIUM_ERROR(FAILED, 1, "Generic failure occured") That could be nice for logging, but I am a little wary that it could be used a crutch for half-assing enum value naming. And these are inherently not localizable, so we can't put them on the UI. https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_res... File blimp/net/helium/helium_result.cc (right): https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_res... blimp/net/helium/helium_result.cc:18: #include "blimp/net/helium/helium_errors.h" On 2016/10/03 20:47:33, CJ wrote: > Just for my own knowledge, why does the include come last in this case? This is an unconventional use of preprocessor includes which basically generates a bunch of inline code using the HELIUM_ERROR defines. It's not actually *last* here; it's surrounded by a switch block. https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_res... File blimp/net/helium/helium_result.h (right): https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_res... blimp/net/helium/helium_result.h:19: OK, On 2016/10/03 18:40:47, Garrett Casto wrote: > Any particular reason why OK is special cased? The enum name generation step adds an ERR prefix which I rather like for distinguishing errors from successes. OK is special cased so that it doesn't inherit that token pasting.
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_res... File blimp/net/helium/helium_result.cc (right): https://codereview.chromium.org/2385913002/diff/1/blimp/net/helium/helium_res... blimp/net/helium/helium_result.cc:18: #include "blimp/net/helium/helium_errors.h" On 2016/10/03 22:32:19, Kevin M wrote: > On 2016/10/03 20:47:33, CJ wrote: > > Just for my own knowledge, why does the include come last in this case? > > This is an unconventional use of preprocessor includes which basically generates > a bunch of inline code using the HELIUM_ERROR defines. It's not actually *last* > here; it's surrounded by a switch block I get that a switch statement is being created, I was just curious if the include has to be within the macro. (still learning macros, so sorry if your answer already covered this, and I missed it).
The CQ bit was unchecked by kmarshall@chromium.org
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gcasto@chromium.org, scf@chromium.org, lethalantidote@chromium.org Link to the patchset: https://codereview.chromium.org/2385913002/#ps20001 (title: "feedbacks")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gcasto@chromium.org, scf@chromium.org, lethalantidote@chromium.org Link to the patchset: https://codereview.chromium.org/2385913002/#ps40001 (title: "the android compiler is dumb")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Define canonical enum for "HeliumResult" status values. Add HeliumResult-to-string mapping for easier logging. Add unit tests for string conversion functions. R=lethalantidote@chromium.org,gcasto@chromium.org BUG= ========== to ========== Define canonical enum for "HeliumResult" status values. Add HeliumResult-to-string mapping for easier logging. Add unit tests for string conversion functions. R=lethalantidote@chromium.org,gcasto@chromium.org BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Define canonical enum for "HeliumResult" status values. Add HeliumResult-to-string mapping for easier logging. Add unit tests for string conversion functions. R=lethalantidote@chromium.org,gcasto@chromium.org BUG= ========== to ========== Define canonical enum for "HeliumResult" status values. Add HeliumResult-to-string mapping for easier logging. Add unit tests for string conversion functions. R=lethalantidote@chromium.org,gcasto@chromium.org BUG= Committed: https://crrev.com/1ce8eb06b7f7ffc4adae084273aea6c1e9d7aef5 Cr-Commit-Position: refs/heads/master@{#422683} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1ce8eb06b7f7ffc4adae084273aea6c1e9d7aef5 Cr-Commit-Position: refs/heads/master@{#422683}
Message was sent while issue was closed.
https://codereview.chromium.org/2385913002/diff/40001/blimp/net/helium/helium... File blimp/net/helium/helium_errors.h (right): https://codereview.chromium.org/2385913002/diff/40001/blimp/net/helium/helium... blimp/net/helium/helium_errors.h:10: HELIUM_ERROR(INTERNAL_ERROR, 1) nit: Should this be UNKNOWN_ERROR? i.e. it indicates some condition that we really never expected could possibly occur? https://codereview.chromium.org/2385913002/diff/40001/blimp/net/helium/helium... File blimp/net/helium/helium_result.cc (right): https://codereview.chromium.org/2385913002/diff/40001/blimp/net/helium/helium... blimp/net/helium/helium_result.cc:19: break; This break looks mis-placed, or unnecessary? https://codereview.chromium.org/2385913002/diff/40001/blimp/net/helium/helium... File blimp/net/helium/helium_result_unittest.cc (right): https://codereview.chromium.org/2385913002/diff/40001/blimp/net/helium/helium... blimp/net/helium/helium_result_unittest.cc:16: ~HeliumResultTest() override {} Do we need a test fixture base, given that it doesn't actually do anything, and has no members? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
