Re: [UndoManager] Re-introduce DOMTransaction interface?

On Wed, Jul 11, 2012 at 3:23 PM, Ryosuke Niwa <rniwa@webkit.org> wrote:

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

I was specifically talking about WebIDL Dictionaries. In which case,
this.undo would set the undo property on the window. Why would you want to
be able to set the undo function from the execut function? My whole point
has been that we should not keep the actual 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.
>>
>
> Consistency.
>

It's more consistent in some ways and less in others. There aren't many
instances of constructing an object and passing it to a method. There are
increasingly many methods that take Dictionaries in. There are many methods
that take pure callbacks. You have to squint to call this more consistent
IMO.

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

To clarify my position here, I think transact should take in a Dictionary.
Then the useragent converts that into a DOMTransaction and stores it in the
UndoManager's history. So, item would return a DOMTransaction. In that
case, merge could be a value in the Dictionary and readonly on the
DOMTransaction.

The only part we disagree about I think is whether transact should take a
Dictionary or a DOMTransaction. Storing a DOMTransaction and returning it
from item, seems reasonable to me.

Ojan