Re: [UndoManager] Re-introduce DOMTransaction interface?

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

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

Oh, then you're proposing something different here. There is a use case for
being able to add custom properties (for extra information used by library,
etc...) and store that in the undo manager. If we had used Dictionary for,
say, the constructor of DOMTransaction, then you'd have to add those
properties after creating DOMTransaction. If you're proposing to use
Dictionary for transact() so that we don't keep any objects, then that just
doesn't work.

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

So one big use pattern we're predicting is something along the line of:

undoManager.transact(transactionFactory(~~));

where transactionFactory can create a custom DOM transaction object. Now
all properties set on the object returned by transactionFactory will be
lost when execute/executeAutomatic/undo/redo are called in your proposal,
and that's very counter intuitive and weird in my opinion.

- Ryosuke