Re: [UndoManager] Re-introduce DOMTransaction interface?

On Wed, Jul 4, 2012 at 5:39 PM, Ryosuke Niwa <rniwa@webkit.org> wrote:

> On Jul 4, 2012 5:26 PM, "Olli Pettay" <Olli.Pettay@helsinki.fi> wrote:
> >
> > On 07/05/2012 03:11 AM, Ryosuke Niwa wrote:
> >
> >> On Wed, Jul 4, 2012 at 5:00 PM, Olli Pettay <Olli.Pettay@helsinki.fi<mailto:
> Olli.Pettay@helsinki.fi>> wrote:
> >>
> >>     On 07/05/2012 01:38 AM, Ryosuke Niwa wrote:
> >>
> >>         Hi all,
> >>
> >>         Sukolsak has been implementing the Undo Manager API in WebKit
> but the fact undoManager.transact() takes a pure JS object with callback
> >>         functions is
> >>         making it very challenging.  The problem is that this object
> needs to be kept alive by either JS reference or DOM but doesn't have a
> backing C++
> >>         object.  Also, as far as we've looked, there are no other
> specification that uses the same mechanism.
> >>
> >>
> >>     I don't understand what is difficult.
> >>     How is that any different to
> >>     target.addEventListener("foo", { handleEvent: function() {}})
> >>
> >>
> >> It will be very similar to that except this object is going to have 3
> callbacks instead of one.
> >>
> >> The problem is that the event listener is a very special object in
> WebKit for which we have a lot of custom binding code. We don't want to
> implement a
> >> similar behavior for the DOM transaction because it's very error prone.
> >
> >
> > So, it is very much implementation detail.
> > (And I still don't understand how a callback can be so hard in this
> case. There are plenty of different kinds of callback objects.
> >  new MutationObserver(some_callback_function_object) )
>
> Yes. It's an implementation feedback. The mutation observer callback is
> implemented as a special event handler in WebKit.
>
How does this make implementation easier? I'm pretty sure Adam's concern
about increasing the likelihood of memory leaks due to the function objects
that are held on indefinitely by the undomanager. That concern is not
implementation specific and is not addressed by this change. The only thing
that would address Adam's concern that I can think of would be something
that didn't involve registering functions at all. I have trouble thinking
of a useful undomanager API that addresses his memory leak concern though.

With the dictionary API, we just need to spec it so that the input
dictionary is just a convenience instead of needing to pass arguments to
the transact method. We don't need the undomanager to hold on to the actual
object you pass in. If we do that, we don't gain anything by adding back in
DOMTransaction, do we?


> >>         Since I want to make the API consistent with the rest of the
> platform and the implementation maintainable in WebKit, I propose the
> following
> >>         changes:
> >>
> >>            * Re-introduce DOMTransaction interface so that scripts can
> instantiate new DOMTransaction().
> >>            * Introduce AutomaticDOMTransaction that inherits from
> DOMTransaction and has a constructor that takes two arguments: a function
> and an
> >>         optional label
> >>
> >>
> >>         After this change, authors can write:
> >>         scope.undoManager.transact(new
> AutomaticDOMTransaction{__function () {
> >>
> >>               scope.appendChild("foo");
> >>         }, 'append "foo"'));
> >>
> >>
> >>     Looks somewhat odd. DOMTransaction would be just a container for a
> callback?
> >>
> >>
> >> Right. If we wanted, we can make DOMTransaction an event target and
> implement execute, undo, & redo as event listeners to further simplify the
> matter.
> >
> >
> > That could make the code more consistent with rest of the platform, but
> the API would become harder to use.
>
> Why? What's harder in the new syntax?
>
It's more bloated and harder to read IMO.

>  - Ryosuke
>