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

Issue 3797006: cashew: support additional usage API error result strings (Closed)

Created:
10 years, 2 months ago by Vince Laviano
Modified:
9 years ago
CC:
chromium-os-reviews_chromium.org, Vince Laviano
Visibility:
Public.

Description

cashew: support additional usage API error result strings BUG=chromium-os:7728 TEST=Manual Change-Id: I0cbf2f95605dff132ecb7f776b1d40e1925fbe4f Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=fbf2e43

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -2 lines) Patch
M src/service.h View 1 chunk +6 lines, -0 lines 0 comments Download
M src/service.cc View 3 chunks +28 lines, -2 lines 2 comments Download

Messages

Total messages: 4 (0 generated)
Vince Laviano
This CL enhances cashew to support a wider range of error result strings. I put ...
10 years, 2 months ago (2010-10-15 00:36:05 UTC) #1
Jason Glasgow
LGTM
10 years, 2 months ago (2010-10-15 17:39:09 UTC) #2
Eric Shienbrood
LGTM http://codereview.chromium.org/3797006/diff/1/2 File src/service.cc (right): http://codereview.chromium.org/3797006/diff/1/2#newcode60 src/service.cc:60: static const char *kCrosUsageStatusUnknownDevice = "UNKNOWN DEVICE"; If ...
10 years, 2 months ago (2010-10-15 18:43:18 UTC) #3
Vince Laviano
10 years, 2 months ago (2010-10-15 20:25:12 UTC) #4
http://codereview.chromium.org/3797006/diff/1/2
File src/service.cc (right):

http://codereview.chromium.org/3797006/diff/1/2#newcode60
src/service.cc:60: static const char *kCrosUsageStatusUnknownDevice = "UNKNOWN
DEVICE";
On 2010/10/15 18:43:18, Eric Shienbrood wrote:
> If you add a new status, you have to modify IsValidCrosUsageStatus() below. It
> might be a bit less error prone to define an array of these strings, and have
> IsValid search the array for the presence of the supplied string. Something to
> keep in mind if you're working in this code in the future.

This is a good point.

Composing a rambling reply to your comment that I've since discarded (the gist
of which was "there would still be some duplication, so we should instead
generate code automatically from a single representation of the status codes")
led me to http://json-schema.org/ (xml-style schema validation for json). 

With a parser that supported this, we could get rid of most of the validation
code in Service::OnRequestComplete and in DataPlan::FromDictionaryValue,
including the code that needs IsValidCrosUsageStatus to exist.

Powered by Google App Engine
This is Rietveld 408576698