Move Drive API files to google_apis directory

Move Drive API files from chrome/browser/chromeos/gdata to chrome/browser/google_apis

Remaining files will be moved in later CLs.

BUG=127855
TEST=pass all existing tests
TBR=estade@chromium.org, ben@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157086

[nhiroki, 2012-09-05 13:23:38]

Hi, kochi-san and kinuko-san

Could you review this? Thanks!

[kochi, 2012-09-05 23:08:37]

Adding Satoru and Daniel to cc.

[kochi, 2012-09-06 05:19:50]

You should also move
* gdata_wapi_parser.{h,cc} + its unittests
* gdata_wapi_service.{h,cc} + its unittests
* gdata_operations.{h,cc}
* operations_base.{h,cc}
* operations_runner.{h,cc}
* operations_registry.{h,cc}

https://chromiumcodereview.appspot.com/10920091/diff/1/chrome/browser/google_...
File chrome/browser/google_api/OWNERS (right):

https://chromiumcodereview.appspot.com/10920091/diff/1/chrome/browser/google_...
chrome/browser/google_api/OWNERS:1: achuith@chromium.org
Could you add a comment on the top that this list matches
chrome/browser/chromeos/gdata?
e.g.

# This list should match chrome/browser/chromeos/gdata

and also could you add the opposite side for
chrome/browser/chromeos/gdata/OWNERS?

[nhiroki, 2012-09-06 06:13:39]

Thanks for comments!

> You should also move
> * gdata_wapi_parser.{h,cc} + its unittests
> * gdata_wapi_service.{h,cc} + its unittests
> * gdata_operations.{h,cc}
> * operations_base.{h,cc}
> * operations_runner.{h,cc}
> * operations_registry.{h,cc}

Yes, I know. We are also planing to move:

* gdata_errorcode
* gdata_util
* gdata_wapi_feed_loader
* gdata_wapi_feed_processor

Do you prefer to add those remaining files to this CL?
I think I move the files in the next CL.

[kochi, 2012-09-06 06:23:38]

On 2012/09/06 06:13:39, nhiroki wrote:
> Thanks for comments!
> 
> > You should also move
> > * gdata_wapi_parser.{h,cc} + its unittests
> > * gdata_wapi_service.{h,cc} + its unittests
> > * gdata_operations.{h,cc}
> > * operations_base.{h,cc}
> > * operations_runner.{h,cc}
> > * operations_registry.{h,cc}
> 
> Yes, I know. We are also planing to move:
> 
> * gdata_errorcode
> * gdata_util
> * gdata_wapi_feed_loader
> * gdata_wapi_feed_processor
> 
> Do you prefer to add those remaining files to this CL?
> I think I move the files in the next CL.

As it is preferable to move files atomically, I'd prefer so.
But you can split the CL if it makes it faster to complete the move.

[kinuko, 2012-09-06 06:26:56]

https://chromiumcodereview.appspot.com/10920091/diff/1/chrome/browser/google_...
File chrome/browser/google_api/OWNERS (right):

https://chromiumcodereview.appspot.com/10920091/diff/1/chrome/browser/google_...
chrome/browser/google_api/OWNERS:1: achuith@chromium.org
On 2012/09/06 05:19:50, Takayoshi Kochi wrote:
> Could you add a comment on the top that this list matches
> chrome/browser/chromeos/gdata?
> e.g.
> 
> # This list should match chrome/browser/chromeos/gdata
> 
> and also could you add the opposite side for
> chrome/browser/chromeos/gdata/OWNERS?

How about

chrome/browser/google_api/OWNERS:
# The list below is originally copied from chrome/browser/chromeos/gdata
# Make sure to update chrome/browser/chromeos/gdata/OWNERS if necessary

chrome/browser/chromeos/gdata/OWNERS:
# Make sure to update chrome/browser/google_api/OWNERS if necessary

or something like that?

[nhiroki, 2012-09-06 06:44:04]

> * gdata_errorcode
> * gdata_util
> * gdata_wapi_feed_loader
> * gdata_wapi_feed_processor

Sorry, I take back gdata_wapi_feed_loader and gdata_wapi_feed_processor.

> As it is preferable to move files atomically, I'd prefer so.
> But you can split the CL if it makes it faster to complete the move.

All right. I will split the files to 2 CLs.

[kochi, 2012-09-06 10:39:58]

lgtm

[nhiroki, 2012-09-06 10:49:05]

Thanks, but I found some dependency problems.
I have to work more in this CL.

[nhiroki, 2012-09-06 14:07:54]

kochi-san,

I have a trouble with compiling Chrome on Windows.

"enum LinkType" (in gdata_wapi_parser.h) defines "ALTERNATE" element.
I found this element fails to compile on Windows buildbot because "ALTERNATE"
may be reserved keyword in VC++.
In fact, I can compile successfully after renaming "ALTERNATE" to "kALTERNATE".

Can I rename those elements? For example, TYPE_ALTERNATE or kALTERNATE.
If it's ok, I will make anther CL.

Thanks!

[kinuko, 2012-09-06 14:18:03]

On 2012/09/06 14:07:54, nhiroki wrote:
> kochi-san,
> 
> I have a trouble with compiling Chrome on Windows.
> 
> "enum LinkType" (in gdata_wapi_parser.h) defines "ALTERNATE" element.
> I found this element fails to compile on Windows buildbot because "ALTERNATE"
> may be reserved keyword in VC++.
> In fact, I can compile successfully after renaming "ALTERNATE" to
"kALTERNATE".
> 
> Can I rename those elements? For example, TYPE_ALTERNATE or kALTERNATE.

nit: kAlternate to keep consistent with the coding style :)

> If it's ok, I will make anther CL.
> 
> Thanks!

[kochi, 2012-09-06 15:20:35]

If ALTERNATE is #define'd in some windows header, it's inevitable to rename
it...

Could you make a separate CL that does only rename the LinkType enum to all
"kSomeThing"?

On 2012/09/06 14:18:03, kinuko wrote:
> On 2012/09/06 14:07:54, nhiroki wrote:
> > kochi-san,
> > 
> > I have a trouble with compiling Chrome on Windows.
> > 
> > "enum LinkType" (in gdata_wapi_parser.h) defines "ALTERNATE" element.
> > I found this element fails to compile on Windows buildbot because
"ALTERNATE"
> > may be reserved keyword in VC++.
> > In fact, I can compile successfully after renaming "ALTERNATE" to
> "kALTERNATE".
> > 
> > Can I rename those elements? For example, TYPE_ALTERNATE or kALTERNATE.
> 
> nit: kAlternate to keep consistent with the coding style :)
> 
> > If it's ok, I will make anther CL.
> > 
> > Thanks!

[kochi, 2012-09-06 15:21:21]

On 2012/09/06 15:20:35, Takayoshi Kochi wrote:
> If ALTERNATE is #define'd in some windows header, it's inevitable to rename
> it...
> 
> Could you make a separate CL that does only rename the LinkType enum to all
> "kSomeThing"?

I mean, renaming not only ALTERNATE but also all the enums in LinkType.

> On 2012/09/06 14:18:03, kinuko wrote:
> > On 2012/09/06 14:07:54, nhiroki wrote:
> > > kochi-san,
> > > 
> > > I have a trouble with compiling Chrome on Windows.
> > > 
> > > "enum LinkType" (in gdata_wapi_parser.h) defines "ALTERNATE" element.
> > > I found this element fails to compile on Windows buildbot because
> "ALTERNATE"
> > > may be reserved keyword in VC++.
> > > In fact, I can compile successfully after renaming "ALTERNATE" to
> > "kALTERNATE".
> > > 
> > > Can I rename those elements? For example, TYPE_ALTERNATE or kALTERNATE.
> > 
> > nit: kAlternate to keep consistent with the coding style :)
> > 
> > > If it's ok, I will make anther CL.
> > > 
> > > Thanks!

[nhiroki, 2012-09-06 15:34:21]

On 2012/09/06 15:21:21, Takayoshi Kochi wrote:
> On 2012/09/06 15:20:35, Takayoshi Kochi wrote:
> > If ALTERNATE is #define'd in some windows header, it's inevitable to rename
> > it...
> > 
> > Could you make a separate CL that does only rename the LinkType enum to all
> > "kSomeThing"?
> 
> I mean, renaming not only ALTERNATE but also all the enums in LinkType.

Okay, I'll make it. Thanks

[satorux1, 2012-09-06 20:08:52]

On 2012/09/06 15:34:21, nhiroki wrote:
> On 2012/09/06 15:21:21, Takayoshi Kochi wrote:
> > On 2012/09/06 15:20:35, Takayoshi Kochi wrote:
> > > If ALTERNATE is #define'd in some windows header, it's inevitable to
rename
> > > it...
> > > 
> > > Could you make a separate CL that does only rename the LinkType enum to
all
> > > "kSomeThing"?
> > 
> > I mean, renaming not only ALTERNATE but also all the enums in LinkType.
> 
> Okay, I'll make it. Thanks

please do it in a separate patch. this patch is already very big.

[nhiroki, 2012-09-07 02:43:25]

I updated the CL.

Could you take another look? Sorry for the huge patch.

- Add some depending files
- Modify *.gypi files 
- Surround ChromeOS specific feature and tests by "#if defined(CHROMEOS) ~
#endif" (gdata_util.cc, gdata_util_unittest.cc)

BTW, I made a separate patch for renaming issue
(http://codereview.chromium.org/10918101/).
This CL should be commited after the renaming patch is landed.

Thanks!

http://codereview.chromium.org/10920091/diff/1/chrome/browser/google_api/OWNERS
File chrome/browser/google_api/OWNERS (right):

http://codereview.chromium.org/10920091/diff/1/chrome/browser/google_api/OWNE...
chrome/browser/google_api/OWNERS:1: achuith@chromium.org
On 2012/09/06 06:26:57, kinuko wrote:
> On 2012/09/06 05:19:50, Takayoshi Kochi wrote:
> > Could you add a comment on the top that this list matches
> > chrome/browser/chromeos/gdata?
> > e.g.
> > 
> > # This list should match chrome/browser/chromeos/gdata
> > 
> > and also could you add the opposite side for
> > chrome/browser/chromeos/gdata/OWNERS?
> 
> How about
> 
> chrome/browser/google_api/OWNERS:
> # The list below is originally copied from chrome/browser/chromeos/gdata
> # Make sure to update chrome/browser/chromeos/gdata/OWNERS if necessary
> 
> chrome/browser/chromeos/gdata/OWNERS:
> # Make sure to update chrome/browser/google_api/OWNERS if necessary
> 
> or something like that?
> 

Done.

[kochi, 2012-09-07 04:57:39]

please rebase, then git cl upload, fix, then git cl upload again so that changes
after rebase are clear.

https://chromiumcodereview.appspot.com/10920091/diff/13011/chrome/browser/chr...
File chrome/browser/chromeos/gdata/mock_drive_web_apps_registry.h (right):

https://chromiumcodereview.appspot.com/10920091/diff/13011/chrome/browser/chr...
chrome/browser/chromeos/gdata/mock_drive_web_apps_registry.h:6: #define
CHROME_BROWSER_CHROMEOS_GDATA_MOCK_DRIVE_WEB_APPS_REGISTRY_H_
Good catch!

[nhiroki, 2012-09-07 05:03:30]

On 2012/09/07 04:57:39, Takayoshi Kochi wrote:
> please rebase, then git cl upload, fix, then git cl upload again so that
changes
> after rebase are clear.

I see. I'll keep that in mind.

[kochi, 2012-09-07 06:56:21]

https://chromiumcodereview.appspot.com/10920091/diff/10056/chrome/browser/goo...
File chrome/browser/google_apis/gdata_util.cc (right):

https://chromiumcodereview.appspot.com/10920091/diff/10056/chrome/browser/goo...
chrome/browser/google_apis/gdata_util.cc:86: return false;
Can you add a TODO() here for what's necessary for platforms
other than Chrome OS unless you won't use this on other
platforms.

https://chromiumcodereview.appspot.com/10920091/diff/10056/chrome/browser/goo...
File chrome/browser/google_apis/gdata_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/10920091/diff/10056/chrome/browser/goo...
chrome/browser/google_apis/gdata_util_unittest.cc:26: #if defined(CHROMEOS)
Please add
TODO(yoshiki): Find platform independent way to get/set local timezone.

[nhiroki, 2012-09-07 09:06:26]

Reflected the comments.
Now I'm working on rebase.

http://codereview.chromium.org/10920091/diff/10056/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_util.cc (right):

http://codereview.chromium.org/10920091/diff/10056/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_util.cc:86: return false;
On 2012/09/07 06:56:21, Takayoshi Kochi wrote:
> Can you add a TODO() here for what's necessary for platforms
> other than Chrome OS unless you won't use this on other
> platforms.

Done.

http://codereview.chromium.org/10920091/diff/10056/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_util_unittest.cc (right):

http://codereview.chromium.org/10920091/diff/10056/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_util_unittest.cc:26: #if defined(CHROMEOS)
On 2012/09/07 06:56:21, Takayoshi Kochi wrote:
> Please add
> TODO(yoshiki): Find platform independent way to get/set local timezone.

Done.

[kochi, 2012-09-07 09:19:05]

lgtm

[commit-bot: I haz the power, 2012-09-07 09:38:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/10920091/13109

[commit-bot: I haz the power, 2012-09-07 09:39:00]

Presubmit check for 10920091-13109 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/chromeos/gdata/gdata_contacts_service.cc:484
    operation->set_feed_url_for_testing( \
  chrome/browser/chromeos/gdata/gdata_contacts_service.cc:593
    operation->set_feed_url_for_testing(

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

Presubmit checks took 5.0s to calculate.

[kochi, 2012-09-07 09:52:32]

Satoru, could you also take a look at this?

Adding TBR= estade@ for ui/webui, ben@ for gyp.

[Daniel Erat, 2012-09-07 13:44:29]

LGTM for *contacts*

[Jói, 2012-09-07 17:05:18]

Please a chrome/browser/google_apis/DEPS file with include_rules that are as
tight as possible to avoid dependencies getting added that will make it harder
to move this to src/google_apis/, something like:

include_rules = [
  "-chrome",
  "-content",
  "+chrome/browser/google_apis",

  # Temporarily needed, see http://crbug.com/146989
  "!chrome/stuff/needed/in/production/code.h",
]

specific_include_rules = {
  ".*_[a-z]+test\.cc": [
    # Temporarily needed, see http://crbug.com/146989
    "!chrome/stuff/needed/in/tests.h",
  ],
}

Also note that once you move this to src/google_apis as per bug 146989, the
location for the gdata code would be src/google_apis/gdata/, not
src/google_apis/.  Up to you whether to match that in this change (i.e.
src/chrome/browser/google_apis/gdata/), it doesn't really matter since it will
be moved anyway.

Cheers,
Jói

[satorux1, 2012-09-07 17:22:32]

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/OWNERS (right):

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/OWNERS:1: # Make sure to update
chrome/browser/google_apis/OWNERS if necessary.
looks unnecessary. please remove.

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_wapi_parser.h (right):

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_wapi_parser.h:275: int icon_side_length()
const { return icon_side_length_; }
I'd suggest to make changes like this in a separate patch. This patch is huge.

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
File chrome/browser/google_apis/OWNERS (right):

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
chrome/browser/google_apis/OWNERS:2: # Make sure to update
chrome/browser/chromeos/gdata/OWNERS if necessary.
don't need. please remove the comment.

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_api_parser.h (right):

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
chrome/browser/google_apis/drive_api_parser.h:101: int icon_side_length() const
{ return icon_side_length_; }
I'd suggest to make changes like this in a separate patch. This patch is huge.

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_util.cc (right):

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_util.cc:67: bool IsGDataAvailable(Profile*
profile) {
Seems to me that this is IsDriveAvailable() rather than gdata.

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_util.cc:87: // way.
Please file a bug and add a URL to it. s/GData/Drive/

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_util.cc:90: }
This patch is huge. I'd suggest to make changes like this in a separate patch.

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_util_unittest.cc (right):

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_util_unittest.cc:14: #endif
This patch is huge. I'd suggest to make changes like this in a separate patch.

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_util_unittest.cc:26: // TODO(yoshiki): Find
platform independent way to get/set local timezone.
Please file a bug and add a URL to it.

[yoshiki, 2012-09-10 05:34:50]

http://codereview.chromium.org/10920091/diff/9088/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_util_unittest.cc (right):

http://codereview.chromium.org/10920091/diff/9088/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_util_unittest.cc:12: #if defined(CHROMEOS)
It is better to use "OS_CHROMEOS" rather than "CHROMEOS". I think OS_CHROMEOS is
common.

http://codereview.chromium.org/10920091/diff/9088/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_util_unittest.cc:27: #if defined(CHROMEOS)
ditto

[yoshiki, 2012-09-10 05:43:43]

http://codereview.chromium.org/10920091/diff/9088/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_util.cc (right):

http://codereview.chromium.org/10920091/diff/9088/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_util.cc:22: #include
"chrome/browser/chromeos/login/user_manager.h"
Don't include files under chrome/browser/chromeos/ from non-chromeos code.
Please put this line into between "#if defined(OS_CHROMEOS)" and "#endif".

[nhiroki, 2012-09-10 07:17:38]

Thanks for reviewing!

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_wapi_parser.h (right):

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_wapi_parser.h:275: int icon_side_length()
const { return icon_side_length_; }
On 2012/09/07 17:22:32, satorux1 wrote:
> I'd suggest to make changes like this in a separate patch. This patch is huge.

I made a separate patch (http://codereview.chromium.org/10911174/).

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_api_parser.h (right):

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
chrome/browser/google_apis/drive_api_parser.h:101: int icon_side_length() const
{ return icon_side_length_; }
On 2012/09/07 17:22:32, satorux1 wrote:
> I'd suggest to make changes like this in a separate patch. This patch is huge.

I made the separate patch (http://codereview.chromium.org/10911174/).

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_util.cc (right):

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_util.cc:67: bool IsGDataAvailable(Profile*
profile) {
On 2012/09/07 17:22:32, satorux1 wrote:
> Seems to me that this is IsDriveAvailable() rather than gdata.

Renaming is not urgent. I will do in another CL.

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_util.cc:87: // way.
On 2012/09/07 17:22:32, satorux1 wrote:
> Please file a bug and add a URL to it. s/GData/Drive/

Ditto.

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_util.cc:90: }
On 2012/09/07 17:22:32, satorux1 wrote:
> This patch is huge. I'd suggest to make changes like this in a separate patch.

I made a separate patch to wrap with OS_CHROMEOS macro
(http://codereview.chromium.org/10914173/).

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_util_unittest.cc (right):

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_util_unittest.cc:14: #endif
On 2012/09/07 17:22:32, satorux1 wrote:
> This patch is huge. I'd suggest to make changes like this in a separate patch.

Done in the separate patch (http://codereview.chromium.org/10914173/).

http://codereview.chromium.org/10920091/diff/13109/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_util_unittest.cc:26: // TODO(yoshiki): Find
platform independent way to get/set local timezone.
On 2012/09/07 17:22:32, satorux1 wrote:
> Please file a bug and add a URL to it.

Done in the separate patch (http://codereview.chromium.org/10914173/).

http://codereview.chromium.org/10920091/diff/9088/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_util.cc (right):

http://codereview.chromium.org/10920091/diff/9088/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_util.cc:22: #include
"chrome/browser/chromeos/login/user_manager.h"
On 2012/09/10 05:43:43, yoshiki wrote:
> Don't include files under chrome/browser/chromeos/ from non-chromeos code.
> Please put this line into between "#if defined(OS_CHROMEOS)" and "#endif".

Done in the separate patch (http://codereview.chromium.org/10914173/).

http://codereview.chromium.org/10920091/diff/9088/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_util_unittest.cc (right):

http://codereview.chromium.org/10920091/diff/9088/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_util_unittest.cc:12: #if defined(CHROMEOS)
On 2012/09/10 05:34:50, yoshiki wrote:
> It is better to use "OS_CHROMEOS" rather than "CHROMEOS". I think OS_CHROMEOS
is
> common.

Done in the separate patch (http://codereview.chromium.org/10914173/).

http://codereview.chromium.org/10920091/diff/9088/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_util_unittest.cc:27: #if defined(CHROMEOS)
On 2012/09/10 05:34:50, yoshiki wrote:
> ditto

Done in the separate patch (http://codereview.chromium.org/10914173/).

[yoshiki, 2012-09-10 07:35:33]

Some files under chrome/browser/google_apis/ still include chromeos-specific
headers. 

For example,
- chrome/browser/google_apis/drive_api_operations.h
- chrome/browser/google_apis/auth_service.cc
- chrome/browser/google_apis/drive_api_service.cc
include some header files under chrome/browser/chromeos/.

Could you add "defined(OS_CHEROMEOS)" as well? Or if impossible, please add TODO
comment.

[nhiroki, 2012-09-10 09:29:30]

On 2012/09/07 17:05:18, Jói wrote:
> Please a chrome/browser/google_apis/DEPS file with include_rules that are as
> tight as possible to avoid dependencies getting added that will make it harder
> to move this to src/google_apis/, something like:
> 
> include_rules = [
>   "-chrome",
>   "-content",
>   "+chrome/browser/google_apis",
> 
>   # Temporarily needed, see http://crbug.com/146989
>   "!chrome/stuff/needed/in/production/code.h",
> ]
> 
> specific_include_rules = {
>   ".*_[a-z]+test\.cc": [
>     # Temporarily needed, see http://crbug.com/146989
>     "!chrome/stuff/needed/in/tests.h",
>   ],
> }
> 
> Also note that once you move this to src/google_apis as per bug 146989, the
> location for the gdata code would be src/google_apis/gdata/, not
> src/google_apis/.  Up to you whether to match that in this change (i.e.
> src/chrome/browser/google_apis/gdata/), it doesn't really matter since it will
> be moved anyway.
> 
> Cheers,
> Jói

Hi, Jói

Thank you for the suggestion. I added DEPS file. Can you take a look?

I guess some files will still remain in src/chrome/browser/google_apis directory
after the moving to src/google_apis, so some dependencies may not be deleted.

Thanks

[Jói, 2012-09-10 11:23:01]

Thanks for adding the DEPS file, it looks good, although that's a lot
of dependencies :(  It would be nice to see this more modularized; it
doesn't seem like a GData protocol implementation should need so many
things from Chrome.

Are any of the dependencies only for test files?  If so you can
extract them into a specific_include_rules section as I showed in my
last email, repeated here:

specific_include_rules = {
  ".*_[a-z]+test\.cc": [
    # Temporarily needed, see http://crbug.com/146989
    "!chrome/stuff/needed/in/tests.h",
  ],
}

Cheers,
Jói


On Mon, Sep 10, 2012 at 9:29 AM,  <nhiroki@chromium.org> wrote:
> On 2012/09/07 17:05:18, Jói wrote:
>>
>> Please a chrome/browser/google_apis/DEPS file with include_rules that are
>> as
>> tight as possible to avoid dependencies getting added that will make it
>> harder
>> to move this to src/google_apis/, something like:
>
>
>> include_rules = [
>>    "-chrome",
>>    "-content",
>>    "+chrome/browser/google_apis",
>
>
>>    # Temporarily needed, see http://crbug.com/146989
>>    "!chrome/stuff/needed/in/production/code.h",
>> ]
>
>
>> specific_include_rules = {
>>    ".*_[a-z]+test\.cc": [
>>      # Temporarily needed, see http://crbug.com/146989
>>      "!chrome/stuff/needed/in/tests.h",
>>    ],
>> }
>
>
>> Also note that once you move this to src/google_apis as per bug 146989,
>> the
>> location for the gdata code would be src/google_apis/gdata/, not
>> src/google_apis/.  Up to you whether to match that in this change (i.e.
>> src/chrome/browser/google_apis/gdata/), it doesn't really matter since it
>> will
>> be moved anyway.
>
>
>> Cheers,
>> Jói
>
>
> Hi, Jói
>
> Thank you for the suggestion. I added DEPS file. Can you take a look?
>
> I guess some files will still remain in src/chrome/browser/google_apis
> directory
> after the moving to src/google_apis, so some dependencies may not be
> deleted.
>
> Thanks
>
> http://codereview.chromium.org/10920091/

[nhiroki, 2012-09-10 11:57:57]

On 2012/09/10 11:23:01, Jói wrote:
> Thanks for adding the DEPS file, it looks good, although that's a lot
> of dependencies :(  It would be nice to see this more modularized; it
> doesn't seem like a GData protocol implementation should need so many
> things from Chrome.
> 
> Are any of the dependencies only for test files?  If so you can
> extract them into a specific_include_rules section as I showed in my
> last email, repeated here:
> 
> specific_include_rules = {
>   ".*_[a-z]+test\.cc": [
>     # Temporarily needed, see http://crbug.com/146989
>     "!chrome/stuff/needed/in/tests.h",
>   ],
> }
> 
> Cheers,
> Jói

Thanks for pointing out. I found a few dependencies only for tests files, and
moved them to specific_include_rules.


On 2012/09/10 07:35:33, yoshiki wrote:
> Some files under chrome/browser/google_apis/ still include chromeos-specific
> headers. 
> 
> For example,
> - chrome/browser/google_apis/drive_api_operations.h
> - chrome/browser/google_apis/auth_service.cc
> - chrome/browser/google_apis/drive_api_service.cc
> include some header files under chrome/browser/chromeos/.
> 
> Could you add "defined(OS_CHEROMEOS)" as well? Or if impossible, please add
TODO
> comment.

I added comments. I think most of them will be resolved in the next moving
patch.

[Jói, 2012-09-10 12:17:56]

LGTM as an initial thing to land.  I will continue pestering you to
move the core protocol stuff to src/google_apis as per
http://crbug.com/146989 :-)

Cheers,
Jói


On Mon, Sep 10, 2012 at 11:57 AM,  <nhiroki@chromium.org> wrote:
> On 2012/09/10 11:23:01, Jói wrote:
>>
>> Thanks for adding the DEPS file, it looks good, although that's a lot
>> of dependencies :(  It would be nice to see this more modularized; it
>> doesn't seem like a GData protocol implementation should need so many
>> things from Chrome.
>
>
>> Are any of the dependencies only for test files?  If so you can
>> extract them into a specific_include_rules section as I showed in my
>> last email, repeated here:
>
>
>> specific_include_rules = {
>>    ".*_[a-z]+test\.cc": [
>>      # Temporarily needed, see http://crbug.com/146989
>>      "!chrome/stuff/needed/in/tests.h",
>>    ],
>> }
>
>
>> Cheers,
>> Jói
>
>
> Thanks for pointing out. I found a few dependencies only for tests files,
> and
> moved them to specific_include_rules.
>
>
>
> On 2012/09/10 07:35:33, yoshiki wrote:
>>
>> Some files under chrome/browser/google_apis/ still include
>> chromeos-specific
>> headers.
>
>
>> For example,
>> - chrome/browser/google_apis/drive_api_operations.h
>> - chrome/browser/google_apis/auth_service.cc
>> - chrome/browser/google_apis/drive_api_service.cc
>> include some header files under chrome/browser/chromeos/.
>
>
>> Could you add "defined(OS_CHEROMEOS)" as well? Or if impossible, please
>> add
>
> TODO
>>
>> comment.
>
>
> I added comments. I think most of them will be resolved in the next moving
> patch.
>
> http://codereview.chromium.org/10920091/

[satorux1, 2012-09-10 16:44:29]

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
File chrome/browser/google_apis/auth_service.cc (right):

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
chrome/browser/google_apis/auth_service.cc:14: #include
"chrome/browser/chromeos/gdata/task_util.h"
ditto. could you move these files before this patch?

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
File chrome/browser/google_apis/auth_service.h (right):

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
chrome/browser/google_apis/auth_service.h:13: // TODO(nhiroki): Remove a
dependency on ChromeOS files.
I think it's wrong to depend on /chromeos/ files from non-chromeos platforms.
Can you move operations_base.h and related files before this patch?

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_api_operations.h (right):

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
chrome/browser/google_apis/drive_api_operations.h:10: // TODO(nhiroki): Remove a
dependency on ChromeOS files.
ditto

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_api_service.h (right):

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
chrome/browser/google_apis/drive_api_service.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
I think we don't want to move this file. See

http://code.google.com/p/chromium/issues/detail?id=127855#c18

For now, could you keep this in the original place? If it becomes necessary, we
can move this at a later time.

[nhiroki, 2012-09-11 00:44:56]

Thank you for reviewing.

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
File chrome/browser/google_apis/auth_service.cc (right):

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
chrome/browser/google_apis/auth_service.cc:14: #include
"chrome/browser/chromeos/gdata/task_util.h"
On 2012/09/10 16:44:29, satorux1 wrote:
> ditto. could you move these files before this patch?

Done in the separate CL (see http://codereview.chromium.org/10913184/).

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
File chrome/browser/google_apis/auth_service.h (right):

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
chrome/browser/google_apis/auth_service.h:13: // TODO(nhiroki): Remove a
dependency on ChromeOS files.
On 2012/09/10 16:44:29, satorux1 wrote:
> I think it's wrong to depend on /chromeos/ files from non-chromeos platforms.
> Can you move operations_base.h and related files before this patch?

I made a separate CL (see http://codereview.chromium.org/10913184/).

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_api_operations.h (right):

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
chrome/browser/google_apis/drive_api_operations.h:10: // TODO(nhiroki): Remove a
dependency on ChromeOS files.
On 2012/09/10 16:44:29, satorux1 wrote:
> ditto

Done in the separate CL (see http://codereview.chromium.org/10913184/).

[nhiroki, 2012-09-11 01:40:17]

I moved gdata_wapi_parser to avoid the dependency on ChromeOS files.

I think we can completely remove those dependencies except wrapped with
OS_CHROMEOS macros after the separate CL
(http://codereview.chromium.org/10913184/) is landed.

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_api_service.h (right):

http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
chrome/browser/google_apis/drive_api_service.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/09/10 16:44:29, satorux1 wrote:
> I think we don't want to move this file. See
> 
> http://code.google.com/p/chromium/issues/detail?id=127855#c18
> 
> For now, could you keep this in the original place? If it becomes necessary,
we
> can move this at a later time.

I see. I moved drive_api_service.{cc,h} to the original place.

[Ben Goodger (Google), 2012-09-11 17:19:27]

lgtm

[nhiroki, 2012-09-12 10:25:48]

On 2012/09/11 01:40:17, nhiroki wrote:
> I moved gdata_wapi_parser to avoid the dependency on ChromeOS files.
> 
> I think we can completely remove those dependencies except wrapped with
> OS_CHROMEOS macros after the separate CL
> (http://codereview.chromium.org/10913184/) is landed.

After rebase, I found new dependency on ChromeOS files. gdata_wapi_parser.h
dependes on browser/chromeos/gdata/drive.pb.h, which is automatically generated
by the protobuf compiler.

I made a separate CL to move drive.proto to browser/google_apis
(http://codereview.chromium.org/10915226/).

[satorux1, 2012-09-12 14:57:56]

On 2012/09/12 10:25:48, nhiroki wrote:
> On 2012/09/11 01:40:17, nhiroki wrote:
> > I moved gdata_wapi_parser to avoid the dependency on ChromeOS files.
> > 
> > I think we can completely remove those dependencies except wrapped with
> > OS_CHROMEOS macros after the separate CL
> > (http://codereview.chromium.org/10913184/) is landed.
> 
> After rebase, I found new dependency on ChromeOS files. gdata_wapi_parser.h
> dependes on browser/chromeos/gdata/drive.pb.h, which is automatically
generated
> by the protobuf compiler.
> 
> I made a separate CL to move drive.proto to browser/google_apis
> (http://codereview.chromium.org/10915226/).

I think the dependency is only for "DriveEntryKind" enum. 
Rather than moving drive.proto, you might want to create a new .proto file in
google_apis directory that just contains the enum, and include it from
drive.proto.

[nhiroki, 2012-09-13 07:02:06]

On 2012/09/12 14:57:56, satorux1 wrote:
> On 2012/09/12 10:25:48, nhiroki wrote:
> > On 2012/09/11 01:40:17, nhiroki wrote:
> > > I moved gdata_wapi_parser to avoid the dependency on ChromeOS files.
> > > 
> > > I think we can completely remove those dependencies except wrapped with
> > > OS_CHROMEOS macros after the separate CL
> > > (http://codereview.chromium.org/10913184/) is landed.
> > 
> > After rebase, I found new dependency on ChromeOS files. gdata_wapi_parser.h
> > dependes on browser/chromeos/gdata/drive.pb.h, which is automatically
> generated
> > by the protobuf compiler.
> > 
> > I made a separate CL to move drive.proto to browser/google_apis
> > (http://codereview.chromium.org/10915226/).
> 
> I think the dependency is only for "DriveEntryKind" enum. 
> Rather than moving drive.proto, you might want to create a new .proto file in
> google_apis directory that just contains the enum, and include it from
> drive.proto.

It's good idea, but I found it's difficult to import
google_apis/separate_drive.proto from chromeos/gdata/drive.proto since current
protoc.gypi doesn't support importing proto files from different directories. I
filed this (http://crbug.com/148889).

As for now, how about pending to move the proto file? If it's OK with you, I'll
file an issue and add TODO comments.

[nhiroki, 2012-09-13 17:28:02]

On 2012/09/13 07:02:06, nhiroki wrote:
> As for now, how about pending to move the proto file? If it's OK with you,
I'll
> file an issue and add TODO comments.

Sorry, it might be confusing description.

How about creating a new proto file (drive_api.proto) in google_apis/ that just
contains "DriveEntryKind" enum and NOT deleting the enum from drive.proto? I
know duplicate code is totally unfavorable, but it seems a simple workaround. I
mean to add TODO comment on the duplicate.

[satorux1, 2012-09-13 20:05:26]

On 2012/09/13 17:28:02, nhiroki wrote:
> On 2012/09/13 07:02:06, nhiroki wrote:
> > As for now, how about pending to move the proto file? If it's OK with you,
> I'll
> > file an issue and add TODO comments.
> 
> Sorry, it might be confusing description.
> 
> How about creating a new proto file (drive_api.proto) in google_apis/ that
just
> contains "DriveEntryKind" enum and NOT deleting the enum from drive.proto? I
> know duplicate code is totally unfavorable, but it seems a simple workaround.
I
> mean to add TODO comment on the duplicate.

I think we can move the enum from .proto to a .h file. In proto, we can store
the enum as 'int32'. This is less type-safe, but not a big deal. I'll create a
patch.

[nhiroki, 2012-09-14 14:56:06]

On 2012/09/13 20:05:26, satorux1 wrote:
> On 2012/09/13 17:28:02, nhiroki wrote:
> > On 2012/09/13 07:02:06, nhiroki wrote:
> > > As for now, how about pending to move the proto file? If it's OK with you,
> > I'll
> > > file an issue and add TODO comments.
> > 
> > Sorry, it might be confusing description.
> > 
> > How about creating a new proto file (drive_api.proto) in google_apis/ that
> just
> > contains "DriveEntryKind" enum and NOT deleting the enum from drive.proto? I
> > know duplicate code is totally unfavorable, but it seems a simple
workaround.
> I
> > mean to add TODO comment on the duplicate.
> 
> I think we can move the enum from .proto to a .h file. In proto, we can store
> the enum as 'int32'. This is less type-safe, but not a big deal. I'll create a
> patch.

Thanks for making patch! This CL grabbed it on PatchSet 13.
Now I'm expecting a patch(*) which resolves dependency problems to be landed.

* http://codereview.chromium.org/10911306/

[satorux1, 2012-09-15 05:02:22]

http://codereview.chromium.org/10920091/diff/1216/chrome/browser/google_apis/...
File chrome/browser/google_apis/drive_api_parser_unittest.cc (right):

http://codereview.chromium.org/10920091/diff/1216/chrome/browser/google_apis/...
chrome/browser/google_apis/drive_api_parser_unittest.cc:26: // TODO(satorux):
Make it possible to run these tests on any platforms after
TODO(nhiroki)

http://codereview.chromium.org/10920091/diff/1216/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_wapi_parser_unittest.cc (right):

http://codereview.chromium.org/10920091/diff/1216/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_wapi_parser_unittest.cc:57: // TODO(satorux):
Make it possible to run these tests on any platforms after
TODO(nhiroki)

[satorux1, 2012-09-15 05:18:59]

http://codereview.chromium.org/10920091/diff/1216/chrome/browser/google_apis/...
File chrome/browser/google_apis/drive_api_parser_unittest.cc (right):

http://codereview.chromium.org/10920091/diff/1216/chrome/browser/google_apis/...
chrome/browser/google_apis/drive_api_parser_unittest.cc:26: // TODO(satorux):
Make it possible to run these tests on any platforms after
On 2012/09/15 05:02:22, satorux1 wrote:
> TODO(nhiroki)

Please make this change in a separate patch rather than slip this into the
gigantic patch. In fact, it was hard to find out this change from this gigantic
patch.

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

http://codereview.chromium.org/10920091/diff/1216/chrome/browser/google_apis/...
chrome/browser/google_apis/drive_service_interface.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
I thought we didn't want to move this file. Could you keep this in the  original
directory?


See also:
http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...

http://codereview.chromium.org/10920091/diff/1216/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_wapi_parser_unittest.cc (right):

http://codereview.chromium.org/10920091/diff/1216/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_wapi_parser_unittest.cc:57: // TODO(satorux):
Make it possible to run these tests on any platforms after
On 2012/09/15 05:02:22, satorux1 wrote:
> TODO(nhiroki)

ditto. please do this in a separate patch.

[nhiroki, 2012-09-15 07:21:39]

Thank you for reviewing.

I think (hope) platform-specific problems have been completely resolved at this
point.

http://codereview.chromium.org/10920091/diff/1216/chrome/browser/google_apis/...
File chrome/browser/google_apis/drive_api_parser_unittest.cc (right):

http://codereview.chromium.org/10920091/diff/1216/chrome/browser/google_apis/...
chrome/browser/google_apis/drive_api_parser_unittest.cc:26: // TODO(satorux):
Make it possible to run these tests on any platforms after
On 2012/09/15 05:18:59, satorux1 wrote:
> On 2012/09/15 05:02:22, satorux1 wrote:
> > TODO(nhiroki)
> 
> Please make this change in a separate patch rather than slip this into the
> gigantic patch. In fact, it was hard to find out this change from this
gigantic
> patch.

I made a separate CL (http://codereview.chromium.org/10910299/) and have already
rebased on it.

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

http://codereview.chromium.org/10920091/diff/1216/chrome/browser/google_apis/...
chrome/browser/google_apis/drive_service_interface.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2012/09/15 05:18:59, satorux1 wrote:
> I thought we didn't want to move this file. Could you keep this in the 
original
> directory?
> 
> 
> See also:
>
http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...

I see what you mean, but could I move drive_service_interface.h this time?
drive_uploader.cc depends on drive_service_interface.h, and we'd like to use
drive_uploader.cc if possible.

http://codereview.chromium.org/10920091/diff/1216/chrome/browser/google_apis/...
File chrome/browser/google_apis/gdata_wapi_parser_unittest.cc (right):

http://codereview.chromium.org/10920091/diff/1216/chrome/browser/google_apis/...
chrome/browser/google_apis/gdata_wapi_parser_unittest.cc:57: // TODO(satorux):
Make it possible to run these tests on any platforms after
On 2012/09/15 05:18:59, satorux1 wrote:
> On 2012/09/15 05:02:22, satorux1 wrote:
> > TODO(nhiroki)
> 
> ditto. please do this in a separate patch.

I made a separate CL (http://codereview.chromium.org/10910299/) and have already
rebased on it.

[satorux1, 2012-09-15 12:34:48]

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

http://codereview.chromium.org/10920091/diff/1216/chrome/browser/google_apis/...
chrome/browser/google_apis/drive_service_interface.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2012/09/15 07:21:39, nhiroki wrote:
> On 2012/09/15 05:18:59, satorux1 wrote:
> > I thought we didn't want to move this file. Could you keep this in the 
> original
> > directory?
> > 
> > 
> > See also:
> >
>
http://codereview.chromium.org/10920091/diff/15266/chrome/browser/google_apis...
> 
> I see what you mean, but could I move drive_service_interface.h this time?
> drive_uploader.cc depends on drive_service_interface.h, and we'd like to use
> drive_uploader.cc if possible. 

I think moving the interface class alone doesn't make drive_uploader.cc usable.
You need an object of DriveServiceInterface to use DriveUploader, right?

For now, please also keep the drive_uploader.cc in the original place. I think
we should resolve the dependency problem first.

Besides, please don't try to move many files at once. In retrospect, it would
have been easier if you moved one set of .h/.cc/_unittest.cc files at a time...

[nhiroki, 2012-09-16 05:26:48]

On 2012/09/15 12:34:48, satorux1 wrote:
> I think moving the interface class alone doesn't make drive_uploader.cc
usable.
> You need an object of DriveServiceInterface to use DriveUploader, right?
> 
> For now, please also keep the drive_uploader.cc in the original place. I think
> we should resolve the dependency problem first.
> 
> Besides, please don't try to move many files at once. In retrospect, it would
> have been easier if you moved one set of .h/.cc/_unittest.cc files at a
time...

Okay, I stopped moving drive_service_interface.h and drive_uploader.{cc, h} in
this patch. Then, I moved these files in the original place.

Could you take another look? Thanks

[satorux1, 2012-09-16 15:46:33]

LGTM with nits. Please submit with the nits fixed.

http://codereview.chromium.org/10920091/diff/11450/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_test_util.cc (left):

http://codereview.chromium.org/10920091/diff/11450/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_test_util.cc:15: 
Please refrain from making unnecessary changes like this when moving files,
especially when a patch is as big as this. Even though it's well intended,
changes like this cause unnecessary noise in the diff and the revision history.
Please rever this.

http://codereview.chromium.org/10920091/diff/11450/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_test_util.cc:63: 
ditto

http://codereview.chromium.org/10920091/diff/11450/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_parser_unittest.cc (right):

http://codereview.chromium.org/10920091/diff/11450/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser_unittest.cc:10: #include
"base/logging.h"
why did you add this? when moving files, please refrain from modying files.
please remove if unnecessary.

[nhiroki, 2012-09-16 16:12:55]

http://codereview.chromium.org/10920091/diff/11450/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_parser_unittest.cc (right):

http://codereview.chromium.org/10920091/diff/11450/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser_unittest.cc:10: #include
"base/logging.h"
On 2012/09/16 15:46:33, satorux1 wrote:
> why did you add this? when moving files, please refrain from modying files.
> please remove if unnecessary.

NOTREACHED macro (line 43) is defined in logging.h.
gdata_wapi_parser_unittest.cc includes it via file_util.h only if
defined(OS_POSIX). As a result, Windows build doesn't include it and fails.

Sorry, I'll create a separate patch soon.

[nhiroki, 2012-09-16 16:42:02]

http://codereview.chromium.org/10920091/diff/11450/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_test_util.cc (left):

http://codereview.chromium.org/10920091/diff/11450/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_test_util.cc:15: 
On 2012/09/16 15:46:33, satorux1 wrote:
> Please refrain from making unnecessary changes like this when moving files,
> especially when a patch is as big as this. Even though it's well intended,
> changes like this cause unnecessary noise in the diff and the revision
history.
> Please rever this.

Done.

http://codereview.chromium.org/10920091/diff/11450/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_test_util.cc:63: 
On 2012/09/16 15:46:33, satorux1 wrote:
> ditto

Done.

http://codereview.chromium.org/10920091/diff/11450/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_parser_unittest.cc (right):

http://codereview.chromium.org/10920091/diff/11450/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser_unittest.cc:10: #include
"base/logging.h"
On 2012/09/16 16:12:56, nhiroki wrote:
> On 2012/09/16 15:46:33, satorux1 wrote:
> > why did you add this? when moving files, please refrain from modying files.
> > please remove if unnecessary.
> 
> NOTREACHED macro (line 43) is defined in logging.h.
> gdata_wapi_parser_unittest.cc includes it via file_util.h only if
> defined(OS_POSIX). As a result, Windows build doesn't include it and fails.
> 
> Sorry, I'll create a separate patch soon.

I made the patch http://codereview.chromium.org/10914309/

[commit-bot: I haz the power, 2012-09-17 04:54:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/10920091/13612

[commit-bot: I haz the power, 2012-09-17 06:54:26]

Change committed as 157086