Fix deadlock by introducing a new transaction event.

Fix deadlock by introducing a new transaction event.

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

[ncarter (slow), 2010-07-21 03:02:13]

This came out pretty clean!

http://codereview.chromium.org/2805095/diff/1/2
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/2805095/diff/1/2#newcode1647
chrome/browser/sync/engine/syncapi.cc:1647: if (event.todo ==
syncable::DirectoryChangeEvent::TRANSACTION_ENDING) {
Do we even need the new TRANSACTION_ENDING phase, or could we simply call this
during the CALCULATE_CHANGES step, after the existing work we do?

I'm fine either way, just curious if there's something subtle at work (in e.g.
another observer).

[chron, 2010-07-21 05:12:11]

I think in reality we don't need anything but calculate changes. After I
wrote it I think it could be done in calculate changes just fine. The
transaction complete could be useful for UI work. However I think this is
lower risk for M6 merging since it's closest to what it used to do.

On Tue, Jul 20, 2010 at 8:02 PM, <nick@chromium.org> wrote:

> This came out pretty clean!
>
>
> http://codereview.chromium.org/2805095/diff/1/2
> File chrome/browser/sync/engine/syncapi.cc (right):
>
> http://codereview.chromium.org/2805095/diff/1/2#newcode1647
> chrome/browser/sync/engine/syncapi.cc:1647: if (event.todo ==
> syncable::DirectoryChangeEvent::TRANSACTION_ENDING) {
> Do we even need the new TRANSACTION_ENDING phase, or could we simply
> call this during the CALCULATE_CHANGES step, after the existing work we
> do?
>
> I'm fine either way, just curious if there's something subtle at work
> (in e.g. another observer).
>
>
> http://codereview.chromium.org/2805095/show
>

[chron_chromium.org, 2010-07-21 19:08:21]

FYI: ncarter LGTMed this but forgot to include CCs.

On Tue, Jul 20, 2010 at 10:11 PM, Jonathan Huang <chron@google.com> wrote:

> I think in reality we don't need anything but calculate changes. After I
> wrote it I think it could be done in calculate changes just fine. The
> transaction complete could be useful for UI work. However I think this is
> lower risk for M6 merging since it's closest to what it used to do.
>
>
> On Tue, Jul 20, 2010 at 8:02 PM, <nick@chromium.org> wrote:
>
>> This came out pretty clean!
>>
>>
>> http://codereview.chromium.org/2805095/diff/1/2
>> File chrome/browser/sync/engine/syncapi.cc (right):
>>
>> http://codereview.chromium.org/2805095/diff/1/2#newcode1647
>> chrome/browser/sync/engine/syncapi.cc:1647: if (event.todo ==
>> syncable::DirectoryChangeEvent::TRANSACTION_ENDING) {
>> Do we even need the new TRANSACTION_ENDING phase, or could we simply
>> call this during the CALCULATE_CHANGES step, after the existing work we
>> do?
>>
>> I'm fine either way, just curious if there's something subtle at work
>> (in e.g. another observer).
>>
>>
>> http://codereview.chromium.org/2805095/show
>>
>
>

[chron_chromium.org, 2010-07-21 19:08:43]

FYI: This was LGTMed by nick but he forgot to CC reviewers.

On 2010/07/21 05:12:11, chron wrote:
> I think in reality we don't need anything but calculate changes. After I
> wrote it I think it could be done in calculate changes just fine. The
> transaction complete could be useful for UI work. However I think this is
> lower risk for M6 merging since it's closest to what it used to do.
> 
> On Tue, Jul 20, 2010 at 8:02 PM, <mailto:nick@chromium.org> wrote:
> 
> > This came out pretty clean!
> >
> >
> > http://codereview.chromium.org/2805095/diff/1/2
> > File chrome/browser/sync/engine/syncapi.cc (right):
> >
> > http://codereview.chromium.org/2805095/diff/1/2#newcode1647
> > chrome/browser/sync/engine/syncapi.cc:1647: if (event.todo ==
> > syncable::DirectoryChangeEvent::TRANSACTION_ENDING) {
> > Do we even need the new TRANSACTION_ENDING phase, or could we simply
> > call this during the CALCULATE_CHANGES step, after the existing work we
> > do?
> >
> > I'm fine either way, just curious if there's something subtle at work
> > (in e.g. another observer).
> >
> >
> > http://codereview.chromium.org/2805095/show
> >
>

[ncarter (slow), 2010-07-21 19:42:12]

Whoops.  Yes, I LGTMed after seeing chron's justification.