Add changestamp field to google_apis::DocumentEntry

Add changestamp field to google_apis::DocumentEntry

TEST=existing test, GDataWapiParserTest.*
BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170876

[tzik, 2012-11-28 09:47:31]

Hi, Kochi-san.
We need the changestamp field on the result of change list query.
Can I add it to DocumentEntry, like this?

[kochi, 2012-11-29 07:48:30]

The change looks okay, but
could you add a test (a simple json that contains a
changestamp informatin in a node and its parsing)?

Could you also add someone in Chrome OS team as a reviewer?
(I'd recommend satorux)

[tzik, 2012-12-03 05:53:02]

Thanks for your suggestion. Adding Satoru-san as Reviews.

Could you take a look, Satoru-san?

[satorux1, 2012-12-03 05:59:56]

https://codereview.chromium.org/11428046/diff/7001/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_parser.cc (right):

https://codereview.chromium.org/11428046/diff/7001/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.cc:611: int64* result) {
DCHECK(result);

https://codereview.chromium.org/11428046/diff/7001/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_parser.h (right):

https://codereview.chromium.org/11428046/diff/7001/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:386: // their value nodes.
Please also fix this comment too.

https://codereview.chromium.org/11428046/diff/7001/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:389: // Helper function for
parsing changestamp.
The function comment looks not good. Please revise. What happens if the
changestamp is not present? Returns true on when?

https://codereview.chromium.org/11428046/diff/7001/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:390: static bool
OptionalChangestamp(const base::StringPiece& value,
Function names should usually start with a verb.

[tzik, 2012-12-03 06:16:34]

Thanks! Updated.

https://codereview.chromium.org/11428046/diff/7001/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_parser.cc (right):

https://codereview.chromium.org/11428046/diff/7001/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.cc:611: int64* result) {
On 2012/12/03 05:59:57, satorux1 wrote:
> DCHECK(result);

Done.

https://codereview.chromium.org/11428046/diff/7001/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_parser.h (right):

https://codereview.chromium.org/11428046/diff/7001/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:386: // their value nodes.
On 2012/12/03 05:59:57, satorux1 wrote:
> Please also fix this comment too.

Done.

https://codereview.chromium.org/11428046/diff/7001/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:389: // Helper function for
parsing changestamp.
On 2012/12/03 05:59:57, satorux1 wrote:
> The function comment looks not good. Please revise. What happens if the
> changestamp is not present? Returns true on when?

Done.

https://codereview.chromium.org/11428046/diff/7001/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:390: static bool
OptionalChangestamp(const base::StringPiece& value,
On 2012/12/03 05:59:57, satorux1 wrote:
> Function names should usually start with a verb.

Done.

[satorux1, 2012-12-03 06:38:11]

https://codereview.chromium.org/11428046/diff/4005/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_parser.h (right):

https://codereview.chromium.org/11428046/diff/4005/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:385: // Helper function for
parsing bool fields based on presence of
See below. Please fix the function comment while you are at it.

https://codereview.chromium.org/11428046/diff/4005/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:390: // Helper function for
parsing changestamp. If the field does not exists,
Rewrite "Helper function for parsing changestamp" part. Function comments should
usually start with a verb.

Cannot we drop "Optional" from the function name?

https://codereview.chromium.org/11428046/diff/4005/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:444: // Changestamp (exists only
for change query results).
If it does not exist, what's the value of this? Please describe.

[tzik, 2012-12-03 07:49:10]

I found the test didn't run on my platform...
Changed to run this test other than ChromeOS and fixed for tests.

Could you take another look?

https://codereview.chromium.org/11428046/diff/4005/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_parser.h (right):

https://codereview.chromium.org/11428046/diff/4005/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:385: // Helper function for
parsing bool fields based on presence of
On 2012/12/03 06:38:11, satorux1 wrote:
> See below. Please fix the function comment while you are at it.

Done.

https://codereview.chromium.org/11428046/diff/4005/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:390: // Helper function for
parsing changestamp. If the field does not exists,
On 2012/12/03 06:38:11, satorux1 wrote:
> Rewrite "Helper function for parsing changestamp" part. Function comments
should
> usually start with a verb.
> 
> Cannot we drop "Optional" from the function name?

Done.

https://codereview.chromium.org/11428046/diff/4005/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:444: // Changestamp (exists only
for change query results).
On 2012/12/03 06:38:11, satorux1 wrote:
> If it does not exist, what's the value of this? Please describe.

Done.

[satorux1, 2012-12-03 08:12:12]

LGTM with a suggestion:

https://codereview.chromium.org/11428046/diff/8002/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_parser.h (right):

https://codereview.chromium.org/11428046/diff/8002/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:391: // Returns true if |value|
is NULL or it is parsed as int64 successfully.
Thanks!

https://codereview.chromium.org/11428046/diff/8002/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_parser_unittest.cc (right):

https://codereview.chromium.org/11428046/diff/8002/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser_unittest.cc:59: #if
!defined(OS_ANDROID)
Most of the tests in the file don't run on Android. It'd be simpler to exclude
this in .gyp file.

See chrome_tests_unit.gypi:
            # Test files cannot be opened on Android.                           
  
            'browser/google_apis/gdata_wapi_operations_unittest.cc',

[tzik, 2012-12-03 08:27:25]

https://codereview.chromium.org/11428046/diff/8002/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_parser_unittest.cc (right):

https://codereview.chromium.org/11428046/diff/8002/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser_unittest.cc:59: #if
!defined(OS_ANDROID)
On 2012/12/03 08:12:12, satorux1 wrote:
> Most of the tests in the file don't run on Android. It'd be simpler to exclude
> this in .gyp file.
> 
> See chrome_tests_unit.gypi:
>             # Test files cannot be opened on Android.                         
 
>   
>             'browser/google_apis/gdata_wapi_operations_unittest.cc',
> 

Done. Thanks!

[commit-bot: I haz the power, 2012-12-03 08:27:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/11428046/6007

[commit-bot: I haz the power, 2012-12-03 11:46:51]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-04 01:32:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/11428046/6007

[commit-bot: I haz the power, 2012-12-04 03:55:50]

Change committed as 170876