On Wed, Jul 11, 2012 at 11:19 AM, Ryosuke Niwa <rn...@webkit.org> wrote:
> On Wed, Jul 11, 2012 at 10:36 AM, Ojan Vafai <o...@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
