[SyncFileSystem] Remove existing entry on LocalFileSyncContext::ApplyRemoteChange call.

[SyncFileSystem] Remove existing entry on LocalFileSyncContext::ApplyRemoteChange call.

As a preparation of the directory support of Sync FileSystem, it's convenient that ApplyRemoteChange handles overwrite for different type files.

BUG=231852
TEST="content_unittests --gtest_filter='LocalfileSyncContextTest.*"
NOTRY=true

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

[tzik, 2013-04-30 10:27:59]

PTAL

[kinuko, 2013-04-30 13:31:09]

lgtm

https://codereview.chromium.org/14623008/diff/4001/webkit/fileapi/syncable/lo...
File webkit/fileapi/syncable/local_file_sync_context.cc (right):

https://codereview.chromium.org/14623008/diff/4001/webkit/fileapi/syncable/lo...
webkit/fileapi/syncable/local_file_sync_context.cc:231: base::PlatformFileError
error) {
Can you add a comment about: we intentionally ignore the return code of Remove
here?

https://codereview.chromium.org/14623008/diff/4001/webkit/fileapi/syncable/lo...
File webkit/fileapi/syncable/local_file_sync_context_unittest.cc (right):

https://codereview.chromium.org/14623008/diff/4001/webkit/fileapi/syncable/lo...
webkit/fileapi/syncable/local_file_sync_context_unittest.cc:588:
SYNC_FILE_TYPE_DIRECTORY));
Can we check FileExists(kDir) here?

[tzik, 2013-04-30 13:54:40]

Thanks!

https://codereview.chromium.org/14623008/diff/4001/webkit/fileapi/syncable/lo...
File webkit/fileapi/syncable/local_file_sync_context.cc (right):

https://codereview.chromium.org/14623008/diff/4001/webkit/fileapi/syncable/lo...
webkit/fileapi/syncable/local_file_sync_context.cc:231: base::PlatformFileError
error) {
On 2013/04/30 13:31:09, kinuko wrote:
> Can you add a comment about: we intentionally ignore the return code of Remove
> here?

Done.

https://codereview.chromium.org/14623008/diff/4001/webkit/fileapi/syncable/lo...
File webkit/fileapi/syncable/local_file_sync_context_unittest.cc (right):

https://codereview.chromium.org/14623008/diff/4001/webkit/fileapi/syncable/lo...
webkit/fileapi/syncable/local_file_sync_context_unittest.cc:588:
SYNC_FILE_TYPE_DIRECTORY));
On 2013/04/30 13:31:09, kinuko wrote:
> Can we check FileExists(kDir) here?

Done.

[commit-bot: I haz the power, 2013-04-30 13:54:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/14623008/12001

[commit-bot: I haz the power, 2013-04-30 15:54:10]

Retried try job too often on win_rel for step(s) chrome_frame_net_tests,
chrome_frame_tests, chrome_frame_unittests, content_browsertests,
installer_util_unittests, mini_installer_test, nacl_integration,
sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[kinuko, 2013-05-01 02:22:12]

https://codereview.chromium.org/14623008/diff/21001/webkit/fileapi/syncable/l...
File webkit/fileapi/syncable/local_file_sync_context.cc (right):

https://codereview.chromium.org/14623008/diff/21001/webkit/fileapi/syncable/l...
webkit/fileapi/syncable/local_file_sync_context.cc:231: base::PlatformFileError
error) {
Can you check sync_status() here and return if it's null?

(Currently shutdown handling on non-ui thread is a bit messy in this file,
sorry)

[tzik, 2013-05-01 02:33:39]

Thanks! Done.

[kinuko, 2013-05-01 05:05:41]

lgtm

https://codereview.chromium.org/14623008/diff/31001/webkit/fileapi/syncable/l...
File webkit/fileapi/syncable/local_file_sync_context.cc (right):

https://codereview.chromium.org/14623008/diff/31001/webkit/fileapi/syncable/l...
webkit/fileapi/syncable/local_file_sync_context.cc:232: // Ignore |error| on
this deletion.
nit: It's slightly confusing what this comment is referring (imperative comment
is usually followed by the actual code implementing it), probably better to use
clearer sentence & terms like:

// This method ignores |error| of Remove().

Or more verbosely:

// Remove() may fail if the target entry does not exist (which is ok),
// so we ignore |error| here.

[tzik, 2013-05-01 05:45:21]

https://codereview.chromium.org/14623008/diff/31001/webkit/fileapi/syncable/l...
File webkit/fileapi/syncable/local_file_sync_context.cc (right):

https://codereview.chromium.org/14623008/diff/31001/webkit/fileapi/syncable/l...
webkit/fileapi/syncable/local_file_sync_context.cc:232: // Ignore |error| on
this deletion.
On 2013/05/01 05:05:41, kinuko wrote:
> nit: It's slightly confusing what this comment is referring (imperative
comment
> is usually followed by the actual code implementing it), probably better to
use
> clearer sentence & terms like:
> 
> // This method ignores |error| of Remove().
> 
> Or more verbosely:
> 
> // Remove() may fail if the target entry does not exist (which is ok),
> // so we ignore |error| here.

Done.

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/14623008/37001

[commit-bot: I haz the power, 2013-05-01 06:17:11]

Retried try job too often on linux_chromeos for step(s) ash_unittests,
aura_unittests, base_unittests, browser_tests, cacheinvalidation_unittests,
check_deps, chromeos_unittests, components_unittests, content_browsertests,
content_unittests, crypto_unittests, dbus_unittests, device_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-01 06:20:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/14623008/37001

[commit-bot: I haz the power, 2013-05-01 07:43:01]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-01 08:07:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/14623008/37001

[commit-bot: I haz the power, 2013-05-01 08:07:35]

Change committed as 197587