Bug 91812 - Implement UndoManager's automatic DOM transactions
Summary: Implement UndoManager's automatic DOM transactions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 89722
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-19 21:24 PDT by Sukolsak Sakshuwong
Modified: 2012-08-21 20:56 PDT (History)
14 users (show)

See Also:


Attachments
Patch (114.92 KB, patch)
2012-07-19 21:29 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (126.34 KB, patch)
2012-07-20 13:46 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (47.90 KB, patch)
2012-08-17 21:35 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (53.06 KB, patch)
2012-08-20 19:02 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (49.04 KB, patch)
2012-08-21 00:00 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (49.98 KB, patch)
2012-08-21 00:12 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (49.20 KB, patch)
2012-08-21 20:04 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sukolsak Sakshuwong 2012-07-19 21:24:13 PDT
Implement UndoManager's automatic DOM transactions
Comment 1 Sukolsak Sakshuwong 2012-07-19 21:29:37 PDT
Created attachment 153399 [details]
Patch
Comment 2 Kentaro Hara 2012-07-20 00:08:54 PDT
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.
Comment 3 Sukolsak Sakshuwong 2012-07-20 00:37:01 PDT
(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.
Comment 4 Kentaro Hara 2012-07-20 00:46:05 PDT
> 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.)
Comment 5 Ryosuke Niwa 2012-07-20 00:52:29 PDT
This bug depends on 89722, not the other way around. Automatic transaction is a natural addition to manual transaction.
Comment 6 Kentaro Hara 2012-07-20 01:41:57 PDT
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".
Comment 7 Sukolsak Sakshuwong 2012-07-20 13:46:02 PDT
Created attachment 153589 [details]
Patch
Comment 8 Sukolsak Sakshuwong 2012-08-17 21:35:24 PDT
Created attachment 159259 [details]
Patch
Comment 9 Ryosuke Niwa 2012-08-17 22:28:39 PDT
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?
Comment 10 Sukolsak Sakshuwong 2012-08-20 19:02:15 PDT
Created attachment 159588 [details]
Patch
Comment 11 Ryosuke Niwa 2012-08-20 20:54:50 PDT
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.
Comment 12 Sukolsak Sakshuwong 2012-08-21 00:00:59 PDT
Created attachment 159628 [details]
Patch
Comment 13 Sukolsak Sakshuwong 2012-08-21 00:12:12 PDT
Created attachment 159629 [details]
Patch
Comment 14 Ryosuke Niwa 2012-08-21 09:54:01 PDT
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?
Comment 15 Sukolsak Sakshuwong 2012-08-21 10:19:36 PDT
(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.
Comment 16 Sukolsak Sakshuwong 2012-08-21 20:04:36 PDT
Created attachment 159848 [details]
Patch
Comment 17 Ryosuke Niwa 2012-08-21 20:37:39 PDT
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.
Comment 18 WebKit Review Bot 2012-08-21 20:56:08 PDT
Comment on attachment 159848 [details]
Patch

Clearing flags on attachment: 159848

Committed r126258: <http://trac.webkit.org/changeset/126258>
Comment 19 WebKit Review Bot 2012-08-21 20:56:14 PDT
All reviewed patches have been landed.  Closing bug.