Add DriveApiUrlGenerator.

Add DriveApiUrlGenerator.

This CL is preparation for server path injection, especially for testing.
It Extracts URL constructing logic from GetURL methods of the classes in
drive_api_operations.{h,cc}, and puts them into a new class
DriveApiUrlGenerator.
DriveApiUrlGenerator is given via operation classes' constructors from their
client (DriveApiService).
The concept of this CL is very similar to the following CLs:
https://codereview.chromium.org/11411086
https://codereview.chromium.org/11418084
https://codereview.chromium.org/11417108
https://codereview.chromium.org/11417109

BUG=162155
TEST=ran unit_tests and chromeos_unittests


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

[hidehiko, 2013-01-07 15:55:49]

Thank you for your review in advance,
- hidehiko

[satorux1, 2013-01-08 03:29:12]

LGTM with nits:

https://codereview.chromium.org/11799003/diff/1/chrome/browser/google_apis/dr...
File chrome/browser/google_apis/drive_api_operations.h (right):

https://codereview.chromium.org/11799003/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_operations.h:35: DriveApiUrlGenerator
url_generator_;
const?

https://codereview.chromium.org/11799003/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_operations.h:56: DriveApiUrlGenerator
url_generator_;
ditto. please check other places too

[kinaba, 2013-01-08 03:33:34]

lgtm

[hidehiko, 2013-01-08 03:54:48]

Thank you for your review. Updated.

https://codereview.chromium.org/11799003/diff/1/chrome/browser/google_apis/dr...
File chrome/browser/google_apis/drive_api_operations.h (right):

https://codereview.chromium.org/11799003/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_operations.h:35: DriveApiUrlGenerator
url_generator_;
On 2013/01/08 03:29:13, satorux1 wrote:
> const?

Done.

https://codereview.chromium.org/11799003/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_operations.h:56: DriveApiUrlGenerator
url_generator_;
On 2013/01/08 03:29:13, satorux1 wrote:
> ditto. please check other places too

Done. Ditto for below similar ones.

[satorux1, 2013-01-08 03:56:33]

LGTM

[Haruki Sato, 2013-01-08 03:58:35]

lgtm

[commit-bot: I haz the power, 2013-01-08 04:23:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/11799003/8001

[commit-bot: I haz the power, 2013-01-08 04:24:05]

Presubmit check for 11799003-8001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You added one or more #includes of files that are temporarily
allowed but being removed. Can you avoid introducing the
#include? See relevant DEPS file(s) for details and contacts.
  chrome/browser/google_apis/drive_api_url_generator.cc
      Illegal include: "chrome/common/net/url_util.h"
    Because of "!chrome/common/net/url_util.h" from chrome/browser/google_apis's
include_rules.

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.4s to calculate.

[hidehiko, 2013-01-08 04:30:47]

Hello Lei,

I'm trying to add a file drive_api_url_generator{.cc,.h,_unittest.cc} for the
refactoring, so I needed to edit .gyp file in chrome directory. Could you kindly
review this issue?

I'd appreciate your help in advance,
- hidehiko

[Lei Zhang, 2013-01-08 04:42:50]

.gyp files lgtm

https://codereview.chromium.org/11799003/diff/8001/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_api_url_generator.h (right):

https://codereview.chromium.org/11799003/diff/8001/chrome/browser/google_apis...
chrome/browser/google_apis/drive_api_url_generator.h:47: };
nit: add DISALLOW_COPY_AND_ASSIGN.

[hidehiko, 2013-01-08 04:53:19]

Thank you for your quick review!

https://codereview.chromium.org/11799003/diff/8001/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_api_url_generator.h (right):

https://codereview.chromium.org/11799003/diff/8001/chrome/browser/google_apis...
chrome/browser/google_apis/drive_api_url_generator.h:47: };
On 2013/01/08 04:42:50, Lei Zhang wrote:
> nit: add DISALLOW_COPY_AND_ASSIGN.

It is necessary to be copy-able for this class, because the instance is copied
into the operation classes' members (classes in drive_api_operations.{cc,h}).

Satoru, could you double check?

[satorux1, 2013-01-08 04:55:00]

https://codereview.chromium.org/11799003/diff/8001/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_api_url_generator.h (right):

https://codereview.chromium.org/11799003/diff/8001/chrome/browser/google_apis...
chrome/browser/google_apis/drive_api_url_generator.h:47: };
On 2013/01/08 04:53:19, hidehiko wrote:
> On 2013/01/08 04:42:50, Lei Zhang wrote:
> > nit: add DISALLOW_COPY_AND_ASSIGN.
> 
> It is necessary to be copy-able for this class, because the instance is copied
> into the operation classes' members (classes in drive_api_operations.{cc,h}).
> 
> Satoru, could you double check?

This class has to be copyable. You might want to add something like:

// This class is copyable hence no DISALLOW_COPY_AND_ASSIGN here.

[hidehiko, 2013-01-08 05:19:28]

Thank you for your help.

Rebased(for gyp files) and updated.

https://codereview.chromium.org/11799003/diff/8001/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_api_url_generator.h (right):

https://codereview.chromium.org/11799003/diff/8001/chrome/browser/google_apis...
chrome/browser/google_apis/drive_api_url_generator.h:47: };
On 2013/01/08 04:55:00, satorux1 wrote:
> On 2013/01/08 04:53:19, hidehiko wrote:
> > On 2013/01/08 04:42:50, Lei Zhang wrote:
> > > nit: add DISALLOW_COPY_AND_ASSIGN.
> > 
> > It is necessary to be copy-able for this class, because the instance is
copied
> > into the operation classes' members (classes in
drive_api_operations.{cc,h}).
> > 
> > Satoru, could you double check?
> 
> This class has to be copyable. You might want to add something like:
> 
> // This class is copyable hence no DISALLOW_COPY_AND_ASSIGN here.

Done.

[commit-bot: I haz the power, 2013-01-08 05:36:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/11799003/15003

[commit-bot: I haz the power, 2013-01-08 09:00:27]

Change committed as 175504