Add OS_CHROMEOS case in file_util::GetHomeDir()

Add OS_CHROMEOS case in file_util::GetHomeDir()

BUG=139611
TEST=unit_tests

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

[satorux1, 2012-08-02 06:09:57]

http://codereview.chromium.org/10843041/diff/2002/base/chromeos/chromeos_vers...
File base/chromeos/chromeos_version.cc (right):

http://codereview.chromium.org/10843041/diff/2002/base/chromeos/chromeos_vers...
base/chromeos/chromeos_version.cc:24: // is not found.
Please remove the comment. we shouldn't repeat here.

http://codereview.chromium.org/10843041/diff/2002/base/chromeos/chromeos_vers...
base/chromeos/chromeos_version.cc:26: if (base::chromeos::IsRunningOnChromeOS())
remove base::chromeos::

[Haruki Sato, 2012-08-02 06:44:13]

Darin, could you OWNER review for base/ directory?

Thanks.

http://codereview.chromium.org/10843041/diff/2002/base/chromeos/chromeos_vers...
File base/chromeos/chromeos_version.cc (right):

http://codereview.chromium.org/10843041/diff/2002/base/chromeos/chromeos_vers...
base/chromeos/chromeos_version.cc:24: // is not found.
On 2012/08/02 06:09:57, satorux1 wrote:
> Please remove the comment. we shouldn't repeat here.

Done.
Thanks.

http://codereview.chromium.org/10843041/diff/2002/base/chromeos/chromeos_vers...
base/chromeos/chromeos_version.cc:26: if (base::chromeos::IsRunningOnChromeOS())
On 2012/08/02 06:09:57, satorux1 wrote:
> remove base::chromeos::

Done.
Thanks.

[darin (slow to review), 2012-08-02 06:55:35]

http://codereview.chromium.org/10843041/diff/1005/base/chromeos/chromeos_vers...
File base/chromeos/chromeos_version.h (right):

http://codereview.chromium.org/10843041/diff/1005/base/chromeos/chromeos_vers...
base/chromeos/chromeos_version.h:21: BASE_EXPORT std::string GetHomeDirectory();
Why isn't this just exposed as PathService::Get(base::DIR_HOME)?

[satorux1, 2012-08-02 07:02:31]

http://codereview.chromium.org/10843041/diff/1005/base/chromeos/chromeos_vers...
File base/chromeos/chromeos_version.h (right):

http://codereview.chromium.org/10843041/diff/1005/base/chromeos/chromeos_vers...
base/chromeos/chromeos_version.h:21: BASE_EXPORT std::string GetHomeDirectory();
On 2012/08/02 06:55:35, darin wrote:
> Why isn't this just exposed as PathService::Get(base::DIR_HOME)?

That's a great great idea. haruki@, please revise this patch per the suggestion.

[Haruki Sato, 2012-08-03 06:50:01]

Fixed file_util::GetHomeDir() which is already there and used.
This can remove the hack-ish GetHomeDirectory now.

Could you take another look please?

Thanks.

http://codereview.chromium.org/10843041/diff/1005/base/chromeos/chromeos_vers...
File base/chromeos/chromeos_version.h (right):

http://codereview.chromium.org/10843041/diff/1005/base/chromeos/chromeos_vers...
base/chromeos/chromeos_version.h:21: BASE_EXPORT std::string GetHomeDirectory();
Thank you for the helpful advice!

Also talked to kochi@, and I agree that this kind of modification should go into
some generic utility.
I found that PathService already has DIR_HOME and actually calls
file_util::GetHomeDir() for it.
Reverted the changes in this file and fixed it in file_util.


On 2012/08/02 07:02:32, satorux1 wrote:
> On 2012/08/02 06:55:35, darin wrote:
> > Why isn't this just exposed as PathService::Get(base::DIR_HOME)?
> 
> That's a great great idea. haruki@, please revise this patch per the
suggestion.
>

[kochi, 2012-08-03 14:35:58]

lgtm with style nits

http://codereview.chromium.org/10843041/diff/9004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache.cc (right):

http://codereview.chromium.org/10843041/diff/9004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache.cc:54: }
You don't need braces for this if (do as line 48)

http://codereview.chromium.org/10843041/diff/9004/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/10843041/diff/9004/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:58: }
You can remove this braces as the statement fits in one line.

[satorux1, 2012-08-03 16:46:00]

chromeos part lgtm with a nit

http://codereview.chromium.org/10843041/diff/9004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache.cc (right):

http://codereview.chromium.org/10843041/diff/9004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache.cc:52: if
(!PathService::Get(base::DIR_HOME, &path)) {
This should not happen, but let's add:

LOG(ERROR) << "Home directory not found";

[darin (slow to review), 2012-08-03 16:53:11]

LGTM for base/ change.

[Haruki Sato, 2012-08-06 02:37:48]

Thank you all for the review!
Adding to the CQ.

Thanks.

http://codereview.chromium.org/10843041/diff/9004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache.cc (right):

http://codereview.chromium.org/10843041/diff/9004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache.cc:52: if
(!PathService::Get(base::DIR_HOME, &path)) {
On 2012/08/03 16:46:01, satorux1 wrote:
> This should not happen, but let's add:
> 
> LOG(ERROR) << "Home directory not found";

Done.
Thanks.

http://codereview.chromium.org/10843041/diff/9004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache.cc:54: }
On 2012/08/03 14:35:59, Takayoshi Kochi wrote:
> You don't need braces for this if (do as line 48)

As I added another line in the clause, I'm keeping the braces.
Thank you for the advice.

http://codereview.chromium.org/10843041/diff/9004/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/10843041/diff/9004/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:58: }
On 2012/08/03 14:35:59, Takayoshi Kochi wrote:
> You can remove this braces as the statement fits in one line.

Done.
Thanks.

[commit-bot: I haz the power, 2012-08-06 02:38:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/10843041/1010

[commit-bot: I haz the power, 2012-08-06 02:53:15]

Try job failure for 10843041-1010 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-06 08:20:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/10843041/1010

[commit-bot: I haz the power, 2012-08-06 09:14:46]

Change committed as 150074