[whatwg] Feedback on UndoManager spec

[Ojan Vafai]

On Thu, Oct 27, 2011 at 10:48 AM, Aryeh Gregor <ayg at aryeh.name> wrote:

> On Wed, Oct 26, 2011 at 2:39 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
>
> The same is true for having apply and reapply. Jonas wanted to get rid of
> > reapply altogether and just have
> > void apply(in boolean isReapply)
> > In this world, you could do
> > { apply: function(isReapply) { return isReapply ? this.doApply() :
> > this.doReapply(); } }.
> > I didn't want this API because I'd expect apply and reapply to be
> > substantially different.
>
> I think either one of those two APIs would be fine.  I don't think the
> compromise is good, because it gives authors two ways to do the same
> thing, which is confusing.  I don't know which API is better without
> knowing the use-cases for manual transactions.  But Jonas' version is
> more flexible, because if the two are substantially different you can
> always just do
>
>  { apply: function(isReapply) {
>      if (isReapply) {
>          // reapply logic
>      } else {
>          // totally separate apply logic
>      }
>  }, . . . }
>
> which is really no harder to write than
>
>  { apply: function() {
>      // apply logic
>  }, reapply: function() {
>      // totally separate reapply logic
>  }, . . . }
>
> It's only one or two lines longer, and one level of indent greater.
> So I don't think a separate reapply member of the dictionary is
> useful.  It just makes things more complicated, even if most of the
> time the logic will be totally separate.


I disagree. I think the boolean makes things more complicated. Boolean
arguments stink. Every time you want to use this API you need to go look up
the documentation to remember whether the boolean is "isReapply" or
"isApply". There's no such confusion if you have a separate method.

You can easily get the same semantics and code reuse with a simple helper
function. I clearly have a preference for having an *optional* reapply
method, but I agree that the current compromise is worse than either
individual option.

function myApply(isReapply) { }

{
  apply: function() { myApply(false); },
  reapply: function() { myApply(true); }
}


> On Thu, Oct 27, 2011 at 2:44 AM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> > Interesting. I've done some quick testing but maybe problems I had in
> mind
> > no longer exist in WebKit. We do a poor job on node adoption and lifetime
> > control so this might be a good opportunity to sort them out anyway.
>
> FWIW, I've noticed WebKit doesn't always do adoptions correctly.  For
> instance, Range.setStart() will throw if the node you pass is from a
> different document than the range, when the spec says it should
> succeed.  I think I've noticed this for some Node methods too.  The
> general rule in the specs these days is anytime you reparent a node to
> another document, it gets silently adopted.
>

This is a known bug. I just haven't gotten around to fixing it. Except for a
bug in our nascent shadow dom implementation, this is the only case where
WebKit still throws a wrong document error.
http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=WRONG_DOCUMENT_ERR%20file:(%5C.h%7C%5C.cpp)$&type=cs