Implement UndoManager's automatic DOM transactions

• Component: HTML Editing
• Status: RESOLVED FIXED

[Sukolsak Sakshuwong]

Implement UndoManager's automatic DOM transactions

[Sukolsak Sakshuwong]

Created attachment 153399 [details]
Patch

[Kentaro Hara]

What's the difference between this bug and bug 89722? As you got a lot of feedback in bug 89722, you might want to keep working on bug 89722 if you don't have a strong reason to create a new bug.

[Sukolsak Sakshuwong]

(In reply to comment #2)
> What's the difference between this bug and bug 89722? As you got a lot of feedback in bug 89722, you might want to keep working on bug 89722 if you don't have a strong reason to create a new bug.

The patch for bug 89722 is getting too large so I figure it's better to create a separate patch that focuses on only one feature. I will upload a new patch for bug 89722 soon. It will contain all the changes made in this patch, excluding the automatic DOM transaction part. I will keep working on bug 89722 and will merge all the work on that bug with this bug. I'm not sure if there is a better way to do this though.

[Kentaro Hara]

> The patch for bug 89722 is getting too large so I figure it's better to create a separate patch that focuses on only one feature.

Makes sense. Then you can create a new bug blocking the original bug, to clarify the dependency. (Done for this bug.)

[Ryosuke Niwa]

This bug depends on 89722, not the other way around. Automatic transaction is a natural addition to manual transaction.

[Kentaro Hara]

Comment on attachment 153399 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153399&action=review

V8 change LGTM.

> Source/WebCore/bindings/js/JSUndoManagerCustom.cpp:90
> +        return throwTypeError(exec);

Let's make this message the same as the V8's one.

> Source/WebCore/bindings/js/JSUndoManagerCustom.cpp:104
> +    setDOMException(exec, ec);

if (ec) setDOMException(exec, ec);

> Source/WebCore/bindings/v8/custom/V8UndoManagerCustom.cpp:80
> +    ExceptionCode ec = 0;

Nit: This line should be just before imp->transact().

> Source/WebCore/bindings/v8/custom/V8UndoManagerCustom.cpp:84
> +        return V8Proxy::throwTypeError("Not an object.", args.GetIsolate());

Nit: I expected more descriptive message than "Not an object".

[Sukolsak Sakshuwong]

Created attachment 153589 [details]
Patch

[Sukolsak Sakshuwong]

Created attachment 159259 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 159259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159259&action=review

> Source/WebCore/bindings/v8/DOMTransaction.cpp:38
> +DOMTransaction* DOMTransaction::s_recordingDOMTransaction = 0;

It's probably better to put this on undoManager so that we can share code with JSC.

> Source/WebCore/bindings/v8/DOMTransaction.cpp:58
> +        m_transactionSteps = adoptPtr(new Vector<RefPtr<DOMTransactionStep> >);

We can't just have Vector<RefPtr<DOMTransactionStep> > as a member?

> Source/WebCore/editing/DOMTransactionStep.h:45
> +class DOMTransactionStep : public RefCounted<DOMTransactionStep> {
> +public:
> +    virtual ~DOMTransactionStep() { }

You need to make the constructor private and add a static create() function. See other RefCounted classes for an example.

> Source/WebCore/editing/DOMTransactionStep.h:62
> +    RefPtr<Node> m_parent;
> +    RefPtr<Node> m_refChild;
> +    RefPtr<Node> m_child;

Don't we have that cycle problem?

[Sukolsak Sakshuwong]

Created attachment 159588 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 159588 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159588&action=review

> Source/WebCore/bindings/v8/DOMTransaction.cpp:57
> +        UndoManager::setRecordingDOMTransaction(this);
> +        callFunction("executeAutomatic");
> +        UndoManager::setRecordingDOMTransaction(0);

These calls to setRecordingDOMTransaction should be done a RAII object.

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:62
> +            AtomicString oldValue = m_mutationRecipients->isOldValueRequested() ? s_currentDecl->parentElement()->getAttribute(HTMLNames::styleAttr) : nullAtom;

getAttribute is an expensive operation. We should probably not compute it twice for mutation observers & undo manager.
Instead, share the code that it's only computed once.

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:69
> +        if (!s_currentDecl->parentElement())
> +            return;

We already have this early exit in lines 58-59

> Source/WebCore/dom/ContainerNode.cpp:353
> +#if ENABLE(UNDO_MANAGER)
> +    bool isRecordingAutomaticTransaction = UndoManager::isRecordingAutomaticTransaction(container);
> +#endif

What's the point of this local variable? I'm pretty certain the value of isRecordingAutomaticTransaction could change.
e.g. its undo manager can get disconnected in the middle of a DOM transaction.

> Source/WebCore/dom/Document.cpp:6162
> +PassRefPtr<UndoManager> Document::getUndoManager(Node* node)

We prefix a function name with "get" when the function has an out argument.
Since we don't have an out argument, we shouldn't prefix it with "get".
How about undoManagerForNode?

> Source/WebCore/editing/UndoManager.cpp:219
> +    if (!s_recordingDOMTransaction || !s_recordingDOMTransaction->undoManager())

I don't think we can ever have a recording DOM transaction and then its undo manager being null.
We should assert that condition instead.

[Sukolsak Sakshuwong]

Created attachment 159628 [details]
Patch

[Sukolsak Sakshuwong]

Created attachment 159629 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 159629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159629&action=review

> Source/WebCore/dom/CharacterData.cpp:193
> +#if ENABLE(UNDO_MANAGER)
> +    if (UndoManager::isRecordingAutomaticTransaction(this)) {
> +        const String& replacingData = newData.substring(offsetOfReplacedData, newLength);
> +        const String& replacedData = m_data.substring(offsetOfReplacedData, oldLength);
> +        UndoManager::addTransactionStep(DataReplacingDOMTransactionStep::create(this, offsetOfReplacedData, oldLength, replacingData, replacedData));
> +    }
> +#endif

Why don't we do this in dispatchModifiedEvent to follow the pattern?

[Sukolsak Sakshuwong]

(In reply to comment #14)
> (From update of attachment 159629 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159629&action=review
> 
> > Source/WebCore/dom/CharacterData.cpp:193
> > +#if ENABLE(UNDO_MANAGER)
> > +    if (UndoManager::isRecordingAutomaticTransaction(this)) {
> > +        const String& replacingData = newData.substring(offsetOfReplacedData, newLength);
> > +        const String& replacedData = m_data.substring(offsetOfReplacedData, oldLength);
> > +        UndoManager::addTransactionStep(DataReplacingDOMTransactionStep::create(this, offsetOfReplacedData, oldLength, replacingData, replacedData));
> > +    }
> > +#endif
> 
> Why don't we do this in dispatchModifiedEvent to follow the pattern?

Because we need to record the offset and the length of replaced data to be able to unapply/reapply change.

[Sukolsak Sakshuwong]

Created attachment 159848 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 159848 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159848&action=review

> Source/WebCore/bindings/v8/DOMTransaction.cpp:80
> +    else {
> +        for (size_t i = m_transactionSteps.size(); i > 0; --i)
> +            m_transactionSteps[i - 1]->unapply();
> +    }

It'll be nice if we can share this code with JSC implementation.

[WebKit Review Bot]

Comment on attachment 159848 [details]
Patch

Clearing flags on attachment: 159848

Committed r126258: <http://trac.webkit.org/changeset/126258>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.