Add request header on gdata operation for testing ETag match.

Add request header on gdata operation for testing ETag match.
The ETag is used to perform revision check and a operation atomically. 

BUG=156037
TEST='existing drive tests'

[tzik, 2012-10-15 13:55:45]

Hi, Kochi-san.
I'd like to use GDataWapiService with ETag match to handle conflict on
upload/download.
Can I add a response header for it, like this? Please take a look to this CL.

[kochi, 2012-10-16 05:04:58]

Initial round of comments.

Could you also update the description to update the
intention of this patch and BUG= & TEST= lines?

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_file_system.cc (right):

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_file_system.cc:1562: std::string(),
Can you add a comment // ETag
at the end of the line?

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_uploader.h (right):

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_uploader.h:162: // ETag to match to the
destination file. Leave this empty if not needed.
nit: "match the destination file"

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/file_system/remove_operation.cc (right):

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/file_system/remove_operation.cc:68: std::string(),
Could you add a comment // ETag
at the end of line?

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_operations.cc (right):

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.cc:774:
headers.push_back(kIfMatchAllHeader);
Even when uploading existing file, is there a possibility
to upload without ETag?

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_operations.h (right):

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.h:356: FilePath virtual_path;
Why unconst for virtual_path?

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_wapi_service_browsertest.cc (right):

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_wapi_service_browsertest.cc:85: std::string(),
Can you add a comment?

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_wapi_service_browsertest.cc:106: std::string(),
Ditto.

[tzik, 2012-10-16 05:48:04]

Thanks, Kochi-san. Updated.

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_file_system.cc (right):

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_file_system.cc:1562: std::string(),
On 2012/10/16 05:04:58, Takayoshi Kochi wrote:
> Can you add a comment // ETag
> at the end of the line?

Done.

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_uploader.h (right):

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_uploader.h:162: // ETag to match to the
destination file. Leave this empty if not needed.
On 2012/10/16 05:04:58, Takayoshi Kochi wrote:
> nit: "match the destination file"

Done.

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/file_system/remove_operation.cc (right):

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/file_system/remove_operation.cc:68: std::string(),
On 2012/10/16 05:04:58, Takayoshi Kochi wrote:
> Could you add a comment // ETag
> at the end of line?

Done.

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_operations.cc (right):

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.cc:774:
headers.push_back(kIfMatchAllHeader);
On 2012/10/16 05:04:58, Takayoshi Kochi wrote:
> Even when uploading existing file, is there a possibility
> to upload without ETag?

I think we can, since existing code do so.
Maybe, we just ignore the conflict and overwrite in the case.

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_operations.h (right):

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.h:356: FilePath virtual_path;
On 2012/10/16 05:04:58, Takayoshi Kochi wrote:
> Why unconst for virtual_path?

Just to keep it consistent with other place in this file.
Anyway, I think it is error to hold this as reference.

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_wapi_service_browsertest.cc (right):

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_wapi_service_browsertest.cc:85: std::string(),
On 2012/10/16 05:04:58, Takayoshi Kochi wrote:
> Can you add a comment?

Done.

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_wapi_service_browsertest.cc:106: std::string(),
On 2012/10/16 05:04:58, Takayoshi Kochi wrote:
> Ditto.

Done.

[kochi, 2012-10-16 06:31:21]

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_operations.h (right):

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.h:356: FilePath virtual_path;
On 2012/10/16 05:48:05, tzik wrote:
> On 2012/10/16 05:04:58, Takayoshi Kochi wrote:
> > Why unconst for virtual_path?
> 
> Just to keep it consistent with other place in this file.
> Anyway, I think it is error to hold this as reference.

Okay, could you do this in a separate CL?

[tzik, 2012-10-16 06:46:17]

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_operations.h (right):

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.h:356: FilePath virtual_path;
On 2012/10/16 06:31:21, Takayoshi Kochi wrote:
> On 2012/10/16 05:48:05, tzik wrote:
> > On 2012/10/16 05:04:58, Takayoshi Kochi wrote:
> > > Why unconst for virtual_path?
> > 
> > Just to keep it consistent with other place in this file.
> > Anyway, I think it is error to hold this as reference.
> 
> Okay, could you do this in a separate CL?

OK, reverted.

http://codereview.chromium.org/11143014/diff/1001/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.h:357: const std::string etag;
This const looks not consistent with others. I'll remove it.

[kochi, 2012-10-16 08:17:00]

LGTM, thanks!

[commit-bot: I haz the power, 2012-10-16 09:00:55]

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

[commit-bot: I haz the power, 2012-10-16 09:01:03]

Presubmit check for 11143014-11017 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/chromeos/extensions

Presubmit checks took 1.9s to calculate.

[tzik, 2012-10-16 09:01:40]

On 2012/10/16 08:17:00, Takayoshi Kochi wrote:
> LGTM, thanks!

Thanks! I pushed it to CQ.

[tzik, 2012-10-16 09:04:29]

Satoru-san, could you take a look as OWNERS of chromeos/extensions?

[satorux1, 2012-10-17 01:00:38]

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
File chrome/browser/google_apis/drive_service_interface.h (right):

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/drive_service_interface.h:149: // Upon completion,
invokes |callback| with results on the calling thread.
Please add some comment about 'etag'.

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/drive_service_interface.h:155: // Upon completion,
invokes |callback| with results on the calling thread.
ditto.

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/drive_service_interface.h:217: const std::string&
etag,
ditto.

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_operations.cc (right):

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.cc:189: etag(etag) {
maybe add something like to ensure that etag is empty for  new file uploads:

DCHECK(upload_mode == existing file ||
       (upload_mode == new file && etag.empty())

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_operations.h (right):

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.h:97: // This class performs the
operation for downloading of a given document/file.
Please add some comment about 'etag'. While you are at it, please add some
comments about other parameters too. :)

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.h:136: // This class performs the
operation for deleting a document.
ditto.

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.h:340: // - |upload_location| should
be the upload_url() of the existing file.
please add some comment about 'etag'

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.h:431: std::string etag;
please add some comment here. it's rather awkward that this does not appear in
the parameters of the constructor.

[tzik, 2012-10-17 02:44:44]

Thanks! Updated!

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
File chrome/browser/google_apis/drive_service_interface.h (right):

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/drive_service_interface.h:149: // Upon completion,
invokes |callback| with results on the calling thread.
On 2012/10/17 01:00:38, satorux1 wrote:
> Please add some comment about 'etag'.

Done.

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/drive_service_interface.h:155: // Upon completion,
invokes |callback| with results on the calling thread.
On 2012/10/17 01:00:38, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/drive_service_interface.h:217: const std::string&
etag,
On 2012/10/17 01:00:38, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_operations.cc (right):

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.cc:189: etag(etag) {
On 2012/10/17 01:00:38, satorux1 wrote:
> maybe add something like to ensure that etag is empty for  new file uploads:
> 
> DCHECK(upload_mode == existing file ||
>        (upload_mode == new file && etag.empty())

Done.

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_operations.h (right):

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.h:97: // This class performs the
operation for downloading of a given document/file.
On 2012/10/17 01:00:38, satorux1 wrote:
> Please add some comment about 'etag'. While you are at it, please add some
> comments about other parameters too. :)

Done.

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.h:136: // This class performs the
operation for deleting a document.
On 2012/10/17 01:00:38, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.h:340: // - |upload_location| should
be the upload_url() of the existing file.
On 2012/10/17 01:00:38, satorux1 wrote:
> please add some comment about 'etag'

Done.

http://codereview.chromium.org/11143014/diff/9002/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_operations.h:431: std::string etag;
On 2012/10/17 01:00:38, satorux1 wrote:
> please add some comment here. it's rather awkward that this does not appear in
> the parameters of the constructor.

Done. Also, add it to constructor.

[satorux1, 2012-10-17 03:14:27]

Thank you for improving the existing poorly written comments!

http://codereview.chromium.org/11143014/diff/22002/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_service_interface.h (right):

http://codereview.chromium.org/11143014/diff/22002/chrome/browser/google_apis...
chrome/browser/google_apis/drive_service_interface.h:149: // If |etag| is set,
checks if it matches the document before deletion.
If etag does not match, what happens? Please document. I guess the server
returns an error. If so, what error?

http://codereview.chromium.org/11143014/diff/22002/chrome/browser/google_apis...
chrome/browser/google_apis/drive_service_interface.h:156: // If |etag| is set,
checks if it matches the document before downloading.
ditto.

http://codereview.chromium.org/11143014/diff/22002/chrome/browser/google_apis...
chrome/browser/google_apis/drive_service_interface.h:210: // If |etag| is set,
checks if it matches the file before downloading.
ditto.

http://codereview.chromium.org/11143014/diff/22002/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_operations.h (right):

http://codereview.chromium.org/11143014/diff/22002/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_operations.h:101: // If |etag| is set, checks
if it matches the file before downloading.
If etag does not match, what happens? Please document.

http://codereview.chromium.org/11143014/diff/22002/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_operations.h:104: // Upon completion, invokes
|download_action_callback|.
Thank you for improving the comment here!

http://codereview.chromium.org/11143014/diff/22002/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_operations.h:148: // If |etag| is set, checks
if it matches the file before deletion.
If etag does not match, what happens? Please document.

http://codereview.chromium.org/11143014/diff/22002/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_operations.h:354: // - |etag| should match the
existing file. If it's empty, matching is skipped.
If etag does not match, what happens? Please document.

[satorux1, 2012-10-24 02:28:58]

ping