Re: [UndoManager] Re-introduce DOMTransaction interface?

On Wed, Jul 11, 2012 at 3:12 PM, Ojan Vafai <ojan@chromium.org> wrote:

> 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.
>

I don't follow. Your second point is that it's ambiguous as to what should
happen when undo/redo event listeners are modified. I'm saying that the
same ambiguity rises when the script modifies undo/redo properties of the
pure JS object that implements DOMTransaction because we don't store
undo/redo properties in a separate C++ (or whatever language you're
implementing UA with) object inside transact().

By the way, let us refrain from using the word dictionary here because
there is a specific object named Dictionary in WebIDL and multiple folks on
IRC have told me that our use of the term "dictionary" is confusing. For
further clarify, the "dictionary" in the current specification is an user
object implementing the DOMTransaction callback interface as defined in:
http://www.w3.org/TR/WebIDL/#es-user-objects It can't be a Dictionary
because execute, executeAutomatic, undo, and redo need to be called on the
object; e.g. if we had t = {execute: function () { this.undo = function ()
{ ~}; }}}, then execute will change the "undo" IDL attribute of t.

 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.
>

Consistency.

 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).
>

Yes, DOMTransaction needs to take execute first.

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

Because of the fact DOMTransaction had been a user object. It would be
awfully confusing if the script could override the value of "merge". If we
had re-introduced the DOMTransaction interface, then we can make "merge" a
readonly attribute on the object.

- Ryosuke