Move chromeos_mount.cc from libcros to Chrome tree

Move chromeos_mount.cc from libcros to Chrome tree

BUG=chromium-os:16556
TEST=Confirm that removable disk is correctly mounted

[satorux1, 2011-11-07 23:21:55]

This is awesome work, but I think the patch is too large.

Generally, smaller patches are preferable as it's easier to catch mistakes in
smaller patches during code review. I'd suggest you to split this patch into
several patches:

1. Add new code in chrome/browser/chromeos/dbus
2. Change all clients of MountLibrary to use CrosDisksClient
3. Remove MountLibrary code

These are steps I took for LoginLibrary:

http://codereview.chromium.org/8295019
http://codereview.chromium.org/8289021
http://codereview.chromium.org/8305015

I think you can reuse this patch for the step 1


You can also separate clean-up patches like:

- Trivial cleanup like removing unnecessary #includes
- Rework |Disk| class's gigantic constructor (you don't have to do this though)

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/cros_disks_client.cc (right):

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:66: std::string path() const {
return path_; }
dbus_service_path() would be clearer.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:68: std::string mount_path()
const { return mount_path_; }
Let's add a blank line between accessors.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:70: std::string system_path()
const { return system_path_; }
Would be nice to add some example for the three paths (dbus service path, mount
path, and system path), as these are somewhat confusing.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:102: bool
MaybePopValue(dbus::MessageReader* reader, bool* value) {
Function comment is missing. Please fix other places too.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Every function declaration should have comments immediately preceding it that
describe what the function does and how to use it.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:106: return true;
PopBool() may fail, but this function always returns true. Is this intended?
Otherwise, please change this to

  return reader->PopBool(value);

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:109: bool
MaybePopValue(dbus::MessageReader* reader, std::string* value) {
Function overriding like this is prohibited by the style guide. Please give them
unique names, like MaybePopBoolValue, and MaybePopStringValue().

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:130:
ScopedVector<dbus::MessageReader> values_owner;
readers_owner? see below

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:134: dbus::MessageReader*
value = new dbus::MessageReader(response);
value -> reader? This is a MessageReader rather than a value.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:156:
MaybePopValue(properties[cros_disks::kDeviceSize], &total_size_);
As you know, parsing string-to-varint dictionary sucks. We should probably
rework this using Protocol Buffers. Could you add a TODO comment here?

TODO(satorux): Rework this code using Protocol Buffers.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:160: if
(!properties[cros_disks::kDeviceMountPaths]->PopArrayOfStrings(
The map is looked up twice here. You can save one lookup using find(), like:

std::map<std::string, dbus::MessageReader*>::iterator iter =
    properties.find(cros_disks::kDeviceMountPaths]);
if (iter != properties.end()) {
  std::vector<std::string> mount_paths;
  if (iter->second->PopArrayOfStrings(...))

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:171: }
BTW, structs are supposed be just data. Having non-trivial functions in structs
are generally not

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:190: class CrosDisksDBusProxy
{
Please add some class comment.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:195: typedef
base::Callback<void(const std::vector<std::string>)
Can you add '&'?

(const std::vector<std::string>&)

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:328: struct SignalEventTuple{
space before {

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:333: void
OnMount(MountCallback callback, ErrorCallback error_callback,
Move  ErrorCallback error_callback to the next line?

More like a matter of taste, but we often list parameters vertically, when they
cannot fit on one line.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:361: if
(!reader.PopArrayOfStrings(&device_paths)) {
Add something like

LOG(ERROR) << "Invalid response: " << response->ToString()

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:378: if
(!reader.PopBool(&success)) {
ditto.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:435: class CrosDisksClientImpl
: public CrosDisksClient {
Please add some class comment.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:472: base::Bind(&DoNothing),
Instead of this, I think you can pass base::Closure()

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:497: struct statvfs stat;
Maybe zero-clear the stat?

  struct statvfs stat = {};  // Zero-clear

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:519:
dbus_proxy_->FormatDevice(file_path, "vfat",
"vfat" -> make it a constant like const char kFormatVFAT[] = "vfat"

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:571: if
(devices_to_unmount.size() == 0) {
devices_to_unmount.empty() ?

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:725: void
OnGetDeviceProperties(const DiskInfo& disk) {
disk -> disk_info ?

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:891: static void DoNothing() {
I think you don't need this. See my comment above.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:94: bool is_hidden);
Wow, 15 parameters including 5 booleans. I know it's inheriting from the
original code, but you might want to clean them up in a separate patch. I'd do
something like 

 uint64 total_size,
 // The options are OR'ed values of DiskOptions below.
 int options);

enum DiskOptions {
  IS_PARENT = 1 << 0;
  IS_READ_ONLY = 1 << 1;
  HAS_MEDIA = 1 << 2;
  ...
};

You can instead five separate enum types for them.

See also:

http://www.chromium.org/developers/coding-style

Begin to prefer enumerated values (e.g. "DELETE_AFTER_USE/KEEP_ALIVE_AFTER_USE")
to booleans ("bool delete_after_use") as function arguments

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:221: static std::string
MountConditionToString(MountCondition type);
Do we need to expose these methods? Otherwise, you might want to put them in
anonymous namespace in  the.cc file.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:98:
chromeos::CrosDisksClient* lib =
lib -> client ?

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_file_browser_private_api.cc:1250:
chromeos::CrosDisksClient *mount_lib =
mount_lib -> mount_client ?

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_file_browser_private_apitest.cc
(right):

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_file_browser_private_apitest.cc:83:
false)));
Gee. Five boolean parameters are super cryptic...

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_init.cc (left):

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:96: #include
"chrome/browser/chromeos/cros/mount_library.h"
Nice. You might want to do this in a separate patch.

[hashimoto, 2011-11-08 01:15:28]

On 2011/11/07 23:21:55, satorux1 wrote:
> This is awesome work, but I think the patch is too large.
> 
> Generally, smaller patches are preferable as it's easier to catch mistakes in
> smaller patches during code review. I'd suggest you to split this patch into
> several patches:
> 
> 1. Add new code in chrome/browser/chromeos/dbus
> 2. Change all clients of MountLibrary to use CrosDisksClient
> 3. Remove MountLibrary code
> 

Sorry for this huge and hard-to-review patch.
Maybe I should only do copying-and-pasting the libcros code and minimum
refactoring at first so that the review system can know that
cros_disks_client.cc is a copy of mount_library.cc, then do more aggressive
refactoring to replace raw function pointers with base::Bind.

[satorux1, 2011-11-08 02:02:37]

On 2011/11/08 01:15:28, hashimoto wrote:
> On 2011/11/07 23:21:55, satorux1 wrote:
> > This is awesome work, but I think the patch is too large.
> > 
> > Generally, smaller patches are preferable as it's easier to catch mistakes
in
> > smaller patches during code review. I'd suggest you to split this patch into
> > several patches:
> > 
> > 1. Add new code in chrome/browser/chromeos/dbus
> > 2. Change all clients of MountLibrary to use CrosDisksClient
> > 3. Remove MountLibrary code
> > 
> 
> Sorry for this huge and hard-to-review patch.
> Maybe I should only do copying-and-pasting the libcros code and minimum
> refactoring at first so that the review system can know that
> cros_disks_client.cc is a copy of mount_library.cc, then do more aggressive
> refactoring to replace raw function pointers with base::Bind.

Never mind. As long as you split this into three patches or so, it'll be fine.

[hashimoto, 2011-11-08 07:31:04]

Thank you for reviewing this huge patch set.

This is a new patch set and I am going to split this into three patches.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/cros_disks_client.cc (right):

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:66: std::string path() const {
return path_; }
On 2011/11/07 23:21:55, satorux1 wrote:
> dbus_service_path() would be clearer. 
Looking at cros-disks code, device_path() is more suitable name for this
property.  I cannot imagine why the comment says "DBus service path".
http://git.chromium.org/gitweb/?p=chromiumos/platform/cros-disks.git;a=blob;f...

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:68: std::string mount_path()
const { return mount_path_; }
On 2011/11/07 23:21:55, satorux1 wrote:
> Let's add a blank line between accessors.

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:70: std::string system_path()
const { return system_path_; }
On 2011/11/07 23:21:55, satorux1 wrote:
> Would be nice to add some example for the three paths (dbus service path,
mount
> path, and system path), as these are somewhat confusing.

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:102: bool
MaybePopValue(dbus::MessageReader* reader, bool* value) {
On 2011/11/07 23:21:55, satorux1 wrote:
> Function comment is missing. Please fix other places too.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> 
> Every function declaration should have comments immediately preceding it that
> describe what the function does and how to use it. 

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:106: return true;
On 2011/11/07 23:21:55, satorux1 wrote:
> PopBool() may fail, but this function always returns true. Is this intended?
> Otherwise, please change this to
> 
>   return reader->PopBool(value);

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:109: bool
MaybePopValue(dbus::MessageReader* reader, std::string* value) {
On 2011/11/07 23:21:55, satorux1 wrote:
> Function overriding like this is prohibited by the style guide. Please give
them
> unique names, like MaybePopBoolValue, and MaybePopStringValue().

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:130:
ScopedVector<dbus::MessageReader> values_owner;
On 2011/11/07 23:21:55, satorux1 wrote:
> readers_owner? see below

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:134: dbus::MessageReader*
value = new dbus::MessageReader(response);
On 2011/11/07 23:21:55, satorux1 wrote:
> value -> reader? This is a MessageReader rather than a value.

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:156:
MaybePopValue(properties[cros_disks::kDeviceSize], &total_size_);
On 2011/11/07 23:21:55, satorux1 wrote:
> As you know, parsing string-to-varint dictionary sucks. We should probably
> rework this using Protocol Buffers. Could you add a TODO comment here?
> 
> TODO(satorux): Rework this code using Protocol Buffers.

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:160: if
(!properties[cros_disks::kDeviceMountPaths]->PopArrayOfStrings(
On 2011/11/07 23:21:55, satorux1 wrote:
> The map is looked up twice here. You can save one lookup using find(), like:
> 
> std::map<std::string, dbus::MessageReader*>::iterator iter =
>     properties.find(cros_disks::kDeviceMountPaths]);
> if (iter != properties.end()) {
>   std::vector<std::string> mount_paths;
>   if (iter->second->PopArrayOfStrings(...))

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:171: }
On 2011/11/07 23:21:55, satorux1 wrote:
> BTW, structs are supposed be just data. Having non-trivial functions in
structs
> are generally not 

Changed it to a class.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:190: class CrosDisksDBusProxy
{
On 2011/11/07 23:21:55, satorux1 wrote:
> Please add some class comment.

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:195: typedef
base::Callback<void(const std::vector<std::string>)
On 2011/11/07 23:21:55, satorux1 wrote:
> Can you add '&'?
> 
> (const std::vector<std::string>&)

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:328: struct SignalEventTuple{
On 2011/11/07 23:21:55, satorux1 wrote:
> space before {

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:333: void
OnMount(MountCallback callback, ErrorCallback error_callback,
On 2011/11/07 23:21:55, satorux1 wrote:
> Move  ErrorCallback error_callback to the next line?
> 
> More like a matter of taste, but we often list parameters vertically, when
they
> cannot fit on one line.

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:361: if
(!reader.PopArrayOfStrings(&device_paths)) {
On 2011/11/07 23:21:55, satorux1 wrote:
> Add something like
> 
> LOG(ERROR) << "Invalid response: " << response->ToString()

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:378: if
(!reader.PopBool(&success)) {
On 2011/11/07 23:21:55, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:435: class CrosDisksClientImpl
: public CrosDisksClient {
On 2011/11/07 23:21:55, satorux1 wrote:
> Please add some class comment.

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:472: base::Bind(&DoNothing),
On 2011/11/07 23:21:55, satorux1 wrote:
> Instead of this, I think you can pass base::Closure()

Passing base::Closure() results in calling NULL function pointer and crash.
I know having a function like DoNothing is an ugly way.
But I prefer it rather than checking callback.is_null() every time before
function call.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:497: struct statvfs stat;
On 2011/11/07 23:21:55, satorux1 wrote:
> Maybe zero-clear the stat?
> 
>   struct statvfs stat = {};  // Zero-clear

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:519:
dbus_proxy_->FormatDevice(file_path, "vfat",
On 2011/11/07 23:21:55, satorux1 wrote:
> "vfat" -> make it a constant like const char kFormatVFAT[] = "vfat"

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:571: if
(devices_to_unmount.size() == 0) {
On 2011/11/07 23:21:55, satorux1 wrote:
> devices_to_unmount.empty() ?

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:725: void
OnGetDeviceProperties(const DiskInfo& disk) {
On 2011/11/07 23:21:55, satorux1 wrote:
> disk -> disk_info ?

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:94: bool is_hidden);
On 2011/11/07 23:21:55, satorux1 wrote:
> Wow, 15 parameters including 5 booleans. I know it's inheriting from the
> original code, but you might want to clean them up in a separate patch. I'd do
> something like 
> 
>  uint64 total_size,
>  // The options are OR'ed values of DiskOptions below.
>  int options);
> 
> enum DiskOptions {
>   IS_PARENT = 1 << 0;
>   IS_READ_ONLY = 1 << 1;
>   HAS_MEDIA = 1 << 2;
>   ...
> };
> 
> You can instead five separate enum types for them.
> 
> See also:
> 
> http://www.chromium.org/developers/coding-style
> 
> Begin to prefer enumerated values (e.g.
"DELETE_AFTER_USE/KEEP_ALIVE_AFTER_USE")
> to booleans ("bool delete_after_use") as function arguments 

Sounds good.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:221: static std::string
MountConditionToString(MountCondition type);
On 2011/11/07 23:21:55, satorux1 wrote:
> Do we need to expose these methods? Otherwise, you might want to put them in
> anonymous namespace in  the.cc file.

They are used by extension_file_browser_private_api.cc and
file_browser_event_router.cc

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:98:
chromeos::CrosDisksClient* lib =
On 2011/11/07 23:21:55, satorux1 wrote:
> lib -> client ?

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_file_browser_private_api.cc:1250:
chromeos::CrosDisksClient *mount_lib =
On 2011/11/07 23:21:55, satorux1 wrote:
> mount_lib -> mount_client ?

Done.

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_file_browser_private_apitest.cc
(right):

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_file_browser_private_apitest.cc:83:
false)));
On 2011/11/07 23:21:55, satorux1 wrote:
> Gee. Five boolean parameters are super cryptic...

I agree, they are too ugly…

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_init.cc (left):

http://codereview.chromium.org/8386031/diff/5002/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:96: #include
"chrome/browser/chromeos/cros/mount_library.h"
On 2011/11/07 23:21:55, satorux1 wrote:
> Nice. You might want to do this in a separate patch.

OK, I will do this later.

[hashimoto, 2011-11-17 11:48:33]

The content of this patch is divided into three patches.
http://src.chromium.org/viewvc/chrome?view=rev&revision=110045
http://src.chromium.org/viewvc/chrome?view=rev&revision=110486
http://src.chromium.org/viewvc/chrome?view=rev&revision=110469
Abandon this patch.