AU: FilesystemCopierAction: copy bit-exactly

AU: FilesystemCopierAction: copy bit-exactly

[Daniel Erat, 2010-04-28 14:58:35]

LGTM with some comments

http://codereview.chromium.org/1700018/diff/1/3
File src/platform/update_engine/filesystem_copier_action.cc (right):

http://codereview.chromium.org/1700018/diff/1/3#newcode150
src/platform/update_engine/filesystem_copier_action.cc:150: else
need curly braces for this if-else, since the else statement is more than one
line

http://codereview.chromium.org/1700018/diff/1/4
File src/platform/update_engine/filesystem_copier_action.h (right):

http://codereview.chromium.org/1700018/diff/1/4#newcode82
src/platform/update_engine/filesystem_copier_action.h:82: GCancellable*
cancellable_;
might be better named as 'cancellable_object_' or 'canceller_' or something, to
make it clear from the name that it's not describing whether something is
cancellable or not.

http://codereview.chromium.org/1700018/diff/1/4#newcode85
src/platform/update_engine/filesystem_copier_action.h:85: bool read_in_flight_;
i was tricked by this comment at first.  could you change it to something like
"True if we're waiting on a read to complete; false if we're waiting on a
write."?  (i read the comment as saying that it's true if we're waiting on a
read *or* a write)

http://codereview.chromium.org/1700018/diff/1/4#newcode91
src/platform/update_engine/filesystem_copier_action.h:91:
std::vector<char>::size_type buffer_valid_;
rename to something like 'buffer_valid_size_' or 'num_bytes_in_buffer_'; the
current name makes me think it's going to be a bool.

http://codereview.chromium.org/1700018/diff/1/7
File src/platform/update_engine/update_engine_client.cc (right):

http://codereview.chromium.org/1700018/diff/1/7#newcode44
src/platform/update_engine/update_engine_client.cc:44: <<
chromeos_update_engine::utils::GetGErrorMessage(error);
add a "using chromeos_update_engine::utils::GetGErrorMessage"?

[adlr, 2010-04-28 17:47:55]

thanks for the review!

fixed and submitted.

http://codereview.chromium.org/1700018/diff/1/3
File src/platform/update_engine/filesystem_copier_action.cc (right):

http://codereview.chromium.org/1700018/diff/1/3#newcode150
src/platform/update_engine/filesystem_copier_action.cc:150: else
On 2010/04/28 14:58:35, Daniel Erat wrote:
> need curly braces for this if-else, since the else statement is more than one
> line

Done.

http://codereview.chromium.org/1700018/diff/1/4
File src/platform/update_engine/filesystem_copier_action.h (right):

http://codereview.chromium.org/1700018/diff/1/4#newcode82
src/platform/update_engine/filesystem_copier_action.h:82: GCancellable*
cancellable_;
On 2010/04/28 14:58:35, Daniel Erat wrote:
> might be better named as 'cancellable_object_' or 'canceller_' or something,
to
> make it clear from the name that it's not describing whether something is
> cancellable or not.

Done.

http://codereview.chromium.org/1700018/diff/1/4#newcode85
src/platform/update_engine/filesystem_copier_action.h:85: bool read_in_flight_;
On 2010/04/28 14:58:35, Daniel Erat wrote:
> i was tricked by this comment at first.  could you change it to something like
> "True if we're waiting on a read to complete; false if we're waiting on a
> write."?  (i read the comment as saying that it's true if we're waiting on a
> read *or* a write)

Ooops. fixed.

http://codereview.chromium.org/1700018/diff/1/4#newcode91
src/platform/update_engine/filesystem_copier_action.h:91:
std::vector<char>::size_type buffer_valid_;
On 2010/04/28 14:58:35, Daniel Erat wrote:
> rename to something like 'buffer_valid_size_' or 'num_bytes_in_buffer_'; the
> current name makes me think it's going to be a bool.

Done.

http://codereview.chromium.org/1700018/diff/1/7
File src/platform/update_engine/update_engine_client.cc (right):

http://codereview.chromium.org/1700018/diff/1/7#newcode44
src/platform/update_engine/update_engine_client.cc:44: <<
chromeos_update_engine::utils::GetGErrorMessage(error);
On 2010/04/28 14:58:35, Daniel Erat wrote:
> add a "using chromeos_update_engine::utils::GetGErrorMessage"?

Done.