Re: [UndoManager] Re-introduce DOMTransaction interface?

On Wed, Jul 11, 2012 at 11:19 AM, Ryosuke Niwa <rniwa@webkit.org> wrote:

> On Wed, Jul 11, 2012 at 10:36 AM, Ojan Vafai <ojan@chromium.org> wrote:
>
>> Another thing to consider if we add DOMTransaction back in is that you
>> now need to specifiy what happens in more cases, e.g.:
>> -call transact on the same DOMTransaction twice
>> -call transact on a DOMTransaction then modify undo/redo listeners
>>
>
> You mean execute? Assuming that, the first one is a good point. We have
> the second problem already with the dictionary interface because scripts
> can add and or remove execute/executeAutomatic/undo/redo from the
> dictionary within those functions or between transact/undo/redo.
>

We don't have this problem with the dictionary interface because we don't
store the actual dictionary. We take the dictionary and construct a C++
(DOMTransaction?) entry into the undomanager. So, if you reuse the same
dictionary, you get a new C++ object stored for each transact call. If you
modify the dictionary after the transact call, it does not affect the
stored C++ object.

These are solvable problems, but are just more complicated than using a
>> dictionary. I really see no point in adding DOMTransaction back.
>>
>
> The point of adding them back is so that undo/redo are implemented as
> events like any other DOM API we have.
>

I don't see the benefit here. I don't think this is a more author-friendly
API.


> If you want, you could make transact just take the arguments you want
>> DOMTransaction to take. Then of course, you end up in the case of needing a
>> bunch of optional arguments due to automatic vs. manual transactions, which
>> leads us back to using a dictionary.
>>
>> An alternative interface I'd be happy with would be transact(String
>> label, Dictionary optionalArguments) since label is always required.
>>
>
> Well, label isn't always required; e.g. when a transaction is merged with
> previous transaction.
>

Doesn't that make DOMTransaction(label, execute) a problem? I suppose it
could be DOMTransaction(execute, optional label).

Speaking of merge, why is it on transact and not on the transaction
dictionary. The latter makes more sense to me.

Ojan