[Webkit-unassigned] [Bug 93912] Implement UndoManager's V8 bindings

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=93912





--- Comment #15 from Ryosuke Niwa <rniwa at webkit.org>  2012-08-16 12:43:06 PST ---
(From update of attachment 158308)
View in context: https://bugs.webkit.org/attachment.cgi?id=158308&action=review

> Source/WebCore/ChangeLog:7
> +

You should describe what kind of change you're making here.

> Source/WebCore/bindings/js/DOMTransaction.h:46
> +    EditAction editingAction() const OVERRIDE { return EditActionUnspecified; }
> +    bool isDOMTransaction() const OVERRIDE { return true; }

Missing virtual.

> Source/WebCore/bindings/v8/DOMTransaction.cpp:59
> +

Assert that m_undoManager->refCount() is non-zero here so as to address Adam's concern?

>>> Source/WebCore/bindings/v8/DOMTransaction.cpp:84
>>> +    return !(function.IsEmpty() || !function->IsFunction());
>> 
>> So many negatives!  How about:
>> 
>> return !function.IsEmpty() && function->IsFunction();
> 
> I previously used that but changed it to the current code as suggested by haraken's comment #2.

Maybe you can create a local variable to make the semantic clear.

> Source/WebCore/bindings/v8/DOMTransaction.h:47
> +    EditAction editingAction() const OVERRIDE { return EditActionUnspecified; }
> +    bool isDOMTransaction() const OVERRIDE { return true; }

Missing virtual. I'm surprised it works.

> Source/WebCore/editing/UndoManager.cpp:61
> +void UndoManager::stop()

You can define stop and clearStack after transact to make the diff look saner.

> Source/WebCore/editing/UndoManager.cpp:91
> +    if (m_isInProgress || !isConnected()) {

Eventually, this m_inInProgress flag needs to be global because we disallow execution of any transaction while we're applying, unapplying, or reapplying a transaction anywhere.
This can be done a separate patch of course.

> Source/WebCore/editing/UndoManager.cpp:104
> +        m_undoStack.append(adoptPtr(new UndoManagerEntry));

Please add static create function to UndoManagerEntry and adoptPtr there.

> Source/WebCore/editing/UndoManager.cpp:116
> +    m_inProgressEntry = adoptPtr(new UndoManagerEntry);

Ditto.

> Source/WebCore/editing/UndoManager.cpp:118
> +    m_isInProgress = true;

Once we make isInProgress global, we can just turn this into a RAII.

> LayoutTests/editing/undomanager/domtransaction-survives-gc.html:21
> +	"execute": function() {
> +		log.innerText += ("execute\n");
> +	},

You can put the whole thing in one line.

> LayoutTests/editing/undomanager/domtransaction-survives-gc.html:40
> +	if (window.GCController)
> +		GCController.collect();

I don't think we should be using testharness.js in the test that uses GCController since we can't export to W3C anyway.
Also, there's a tab character here.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.