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

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Adam Barth <abarth at webkit.org>  2012-08-15 13:27:39 PST ---
(From update of attachment 158308)
View in context: https://bugs.webkit.org/attachment.cgi?id=158308&action=review

> Source/WebCore/bindings/v8/DOMTransaction.cpp:61
> +    if (m_undoManager)
> +        m_undoManager->registerRedoStep(this);

Isn't this a use-after-free?  What stops the undo function from destroying |this|.

> Source/WebCore/bindings/v8/DOMTransaction.cpp:84
> +    return !(function.IsEmpty() || !function->IsFunction());

So many negatives!  How about:

return !function.IsEmpty() && function->IsFunction();

> Source/WebCore/bindings/v8/DOMTransaction.cpp:99
> +    if (!context || context->isJSExecutionForbidden())

isJSExecutionForbidden only applies for workers.  In this function only runs on the main thread, so it's not needed.

> Source/WebCore/bindings/v8/DOMTransaction.cpp:102
> +    Frame* frame = static_cast<Document*>(context)->frame();

Why does |context| have the type ScriptExecutionContext if it's always a Document*?  It seems more straightforward to just have

Document* document = m_undoManager->undoScopeHost()->document();

above.

> Source/WebCore/bindings/v8/DOMTransaction.cpp:110
> +    v8::Local<v8::Function> function = v8::Local<v8::Function>::Cast(property);

It seems like getFunction should just return a v8::Handle<v8::Function> and return an empty handle when it can't find one rather than having both a Boolean return value and an out parameter of the wrong type.

> Source/WebCore/bindings/v8/DOMTransaction.cpp:118
> +    frame->script()->proxy()->callFunction(function, receiver, 0, parameters);

We're supposed to call the function with itself as the receiver?  That's a bit odd.  I would have expected the UndoManager or the global object to be the receiver.  Is there a test for this?

> Source/WebCore/bindings/v8/custom/V8DOMTransactionCustom.cpp:41
> +    v8::Persistent<v8::Object> ownerWrapper = store->domObjectMap().get(undoManager);

ownerWrapper -> undoManagerWrapper?

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