[SyncFS] Force activate folder tracker on folder creation conflict case

[SyncFS] Force activate folder tracker on folder creation conflict case

If a folder activation failed due to existing item, the folder is eventually deleted.
This can be happen in some edge case.

This CL should fix it.

BUG=331988
NOTRY=true

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

[tzik, 2014-02-06 08:59:40]

PTL

[kinuko, 2014-02-06 09:48:01]

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc
(right):

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc:995:
NULL));
nit: can we match the style for line 984 and 990?

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1044: if
(same_title.has_active()) {
Does it make sense to have some comment here? It's cryptic why we're doing this
without context. Something like

'This tracker's status should be resolved when next remote change's fetched and
synced. Mark this inactive for now so that the new one can become active'

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.h (right):

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:307: //
without deactivating any other path trackers.  In this case, tries to
We never used the term 'path tracker', it's bit unclear

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:311: // In the
FAILED case, |callback| is not invoked.
nit/suggestion: remove the line-break at the end of line 310, and change the
line 311 to just tell: '|callback| is not invoked in this case'

[nhiroki, 2014-02-06 10:02:42]

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.h (right):

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:312:
ActivationStatus TryNoSideEffectActivation(
Can we reconsider this function name? In a specific case this deactivates a
tracker (that is "SideEffect)".

[tzik, 2014-02-07 04:43:53]

Updated!

I added some comment, and renamed the function.
PTAL.

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc
(right):

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc:995:
NULL));
On 2014/02/06 09:48:01, kinuko wrote:
> nit: can we match the style for line 984 and 990?

Done.

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1044: if
(same_title.has_active()) {
On 2014/02/06 09:48:01, kinuko wrote:
> Does it make sense to have some comment here? It's cryptic why we're doing
this
> without context. Something like
> 
> 'This tracker's status should be resolved when next remote change's fetched
and
> synced. Mark this inactive for now so that the new one can become active'

Added some comment around here.

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.h (right):

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:307: //
without deactivating any other path trackers.  In this case, tries to
On 2014/02/06 09:48:01, kinuko wrote:
> We never used the term 'path tracker', it's bit unclear

Revised these comment.

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:311: // In the
FAILED case, |callback| is not invoked.
On 2014/02/06 09:48:01, kinuko wrote:
> nit/suggestion: remove the line-break at the end of line 310, and change the
> line 311 to just tell: '|callback| is not invoked in this case'

Done.

https://codereview.chromium.org/148483009/diff/100001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:312:
ActivationStatus TryNoSideEffectActivation(
On 2014/02/06 10:02:42, nhiroki wrote:
> Can we reconsider this function name? In a specific case this deactivates a
> tracker (that is "SideEffect)".

Done.

[nhiroki, 2014-02-07 05:24:30]

Nitpick comments only.

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1023: //  -
|file_id| as the tracker |file_id|.
nit: as the tracker |file_id| -> as the tracker's |file_id|

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1041: // If
this case, report the failure to the caller.
nit: multiple parent -> multiple parents

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.h (right):

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:308: //
different path to the tracker to be activated.
nit: "... different path to the tracker to be activated." sounds a bit wordy to
me. "... other trackers that have same file_id but different path." would be
clearer...? wdyt?

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:310: // be
activated, the function returns ACTIVATION_FAILED_ANOTHER_ACTIVE_TRACKER
"another" and "different" seems the same meaning. How about "If |file_id| has
another active tracker, the function ..."?

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:312: // If
there is another active tracker that has the same path to the tracker to
"... that has the same path but different file_id, ..." ?

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:313: // be
activated, inactivates it.
Can we use "deactivates" instead of "inactivates" (or vice verse) for
consistency?

[tzik, 2014-02-07 05:43:41]

Updated.

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1023: //  -
|file_id| as the tracker |file_id|.
On 2014/02/07 05:24:30, nhiroki wrote:
> nit: as the tracker |file_id| -> as the tracker's |file_id|

Done.

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1041: // If
this case, report the failure to the caller.
On 2014/02/07 05:24:30, nhiroki wrote:
> nit: multiple parent -> multiple parents

Done.

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.h (right):

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:308: //
different path to the tracker to be activated.
On 2014/02/07 05:24:30, nhiroki wrote:
> nit: "... different path to the tracker to be activated." sounds a bit wordy
to
> me. "... other trackers that have same file_id but different path." would be
> clearer...? wdyt?

Looks nice. Done.

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:310: // be
activated, the function returns ACTIVATION_FAILED_ANOTHER_ACTIVE_TRACKER
On 2014/02/07 05:24:30, nhiroki wrote:
> "another" and "different" seems the same meaning. How about "If |file_id| has
> another active tracker, the function ..."?

Done.

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:312: // If
there is another active tracker that has the same path to the tracker to
On 2014/02/07 05:24:30, nhiroki wrote:
> "... that has the same path but different file_id, ..." ?

Done.

https://codereview.chromium.org/148483009/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:313: // be
activated, inactivates it.
On 2014/02/07 05:24:30, nhiroki wrote:
> Can we use "deactivates" instead of "inactivates" (or vice verse) for
> consistency?

Done.

[kinuko, 2014-02-07 08:00:21]

lgtm

https://codereview.chromium.org/148483009/diff/270001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.h (right):

https://codereview.chromium.org/148483009/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:307: //
tracker can be activated without inactivating other trackers that has the
nit: has -> have

https://codereview.chromium.org/148483009/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:308: // same
|file_id| but a different path.
a different path -> different paths

[nhiroki, 2014-02-07 10:13:02]

lgtm

[tzik, 2014-02-10 02:38:58]

https://codereview.chromium.org/148483009/diff/270001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.h (right):

https://codereview.chromium.org/148483009/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:307: //
tracker can be activated without inactivating other trackers that has the
On 2014/02/07 08:00:21, kinuko wrote:
> nit: has -> have

Done.

https://codereview.chromium.org/148483009/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:308: // same
|file_id| but a different path.
On 2014/02/07 08:00:21, kinuko wrote:
> a different path -> different paths

Done.

[tzik, 2014-02-10 02:39:04]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2014-02-10 02:39:15]

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

[commit-bot: I haz the power, 2014-02-10 04:49:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-10 04:49:20]

Retried try job too often on win_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[tzik, 2014-02-10 05:03:52]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2014-02-10 05:04:00]

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

[commit-bot: I haz the power, 2014-02-10 05:04:47]

Change committed as 250049