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

[bugzilla-daemon at webkit.org]

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





--- Comment #2 from Kentaro Hara <haraken at chromium.org>  2012-08-13 18:19:01 PST ---
(From update of attachment 158153)
View in context: https://bugs.webkit.org/attachment.cgi?id=158153&action=review

Added some comments.

Actually I'm not so excited at this patch --- the custom implementations are too dirty.

- If undo, redo, etc were speced as EventListeners, the implementation would become much simpler.

- If you really want to implement undo, redo, etc by the "user object" in the Web IDL spec, we might want to implement the general mechanism of the "user object" to CodeGenerators, instead of adding ad-hoc implementations to custom bindings. But for now I'm not sure how we can generally implement the "user object" in CodeGenerators.

> Source/WebCore/bindings/v8/DOMTransaction.cpp:2
> + * Copyright (C) 2012 Google Inc. All rights reserved.

Use the copyright in WebKit/Source/WebKit/LICENSE

> Source/WebCore/bindings/v8/DOMTransaction.cpp:43
> +    : m_worldContext(worldContext)

I don't fully understand the lifetime of the DOM transaction and the context. Is it guaranteed that m_worldContext is alive while the DOM transaction is alive?

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

Nit: I would prefer !(function.IsEmpty() || !function->IsFunction()) for consistency.

> Source/WebCore/bindings/v8/DOMTransaction.cpp:112
> +    v8::HandleScope handleScope;
> +
> +    v8::Local<v8::Context> v8Context = toV8Context(context, m_worldContext);
> +    if (v8Context.IsEmpty())
> +        return;
> +
> +    v8::Context::Scope scope(v8Context);
> +

I don't fully understand the code around here. Just in case, would you elaborate on why you need to create a new Context and Scope?

> Source/WebCore/bindings/v8/DOMTransaction.h:64
> +    bool getFunction(const char* propertyName, v8::Local<v8::Value>& function);
> +    bool hasFunction(const char* propertyName);
> +    void callFunction(const char* propertyName);

Nit: variable names are not needed.

> Source/WebCore/bindings/v8/custom/V8UndoManagerCustom.cpp:60
> +    transactionWrapper->SetHiddenValue(v8::String::New(DOMTRANSACTION_DATA), dictionary);

v8::String::New() => v8::String::NewSymbol()

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